boddhisattva

boddhisattva

Code Review for a sales tax problem solved using Elixir

Dear Reader,

Greetings! I’ve implemented a solution to a Sales Tax problem using Elixir via this github repo: GitHub - boddhisattva/sales_tax: This is a program that calculates sales tax and prints receipt details as part of a purchase of a set of Items · GitHub. Kindly follow the instructions in the README to get up and running with the app if you’re trying to set this up in your local computer.

I’d consider myself as a beginner level Elixir developer and thereby I’d really appreciate any feedback on how I could improve my solution further.

I’ve attempted to share some of my code related design decisions through this section. It would be great to hear your thoughts on how this code can be further improved in.

I’ve also attempted to add Dialyzer hex package to my code and I get the below results in the output. I’m currently also attempting to understand the Dialyzer output in some more detail as well. If there are any thoughts that you’d like to share based on the below output from the dialyzer hex package with reference to the code on github, I’d be glad to hear your thoughts on this as well.

lib/receipt_csv_parser.ex:15: Invalid type specification for function ‘Elixir.ReceiptCsvParser’:read_line_items/1. The success typing is (binary() | maybe_improper_list(binary() | maybe_improper_list(any(),binary() | ) | char(),binary() | )) -> [any()] lib/receipt_csv_parser.ex:38: Function parse_item/1 has no local return lib/receipt_csv_parser.ex:56: Function update_other_item_details/1 has no local return lib/receipt_csv_parser.ex:59: The call ‘Elixir.Item’:‘imported?’(Vitem@1::#{’ struct ':=‘Elixir.Item’, ‘basic_sales_tax_applicable’:=‘true’, ‘imported’:=‘false’, ‘name’:= , ‘price’:=float(), ‘quantity’:=integer()}) breaks the contract (‘Elixir.Item’) -> boolean() lib/receipt_generator.ex:46: Invalid type specification for function ‘Elixir.ReceiptGenerator’:generate_details/1. The success typing is (atom() | #{‘items’:= , ‘sales_tax’:=float(), ‘total’:=float(), => }) -> ‘ok’ lib/sales_tax.ex:20: Invalid type specification for function ‘Elixir.SalesTax’:main/1. The success typing is ([binary()]) -> ‘ok’ lib/sales_tax.ex:48: The created fun has no local return lib/shopping_cart.ex:33: Invalid type specification for function ‘Elixir.ShoppingCart’:initialize_cart_product/2. The success typing is (number(),atom() | #{‘basic_sales_tax_applicable’:= , ‘imported’:= , ‘name’:= , ‘price’:=number(), ‘quantity’:=number(), => }) -> #{’ struct ':=‘Elixir.Item’, ‘basic_sales_tax_applicable’:= , ‘imported’:= , ‘name’:= , ‘price’:=float(), ‘quantity’:=_} lib/shopping_cart.ex:76: Invalid type specification for function ‘Elixir.ShoppingCart’:update/3. The success typing is (#{‘items’:=[any()], ‘sales_tax’:=number(), ‘total’:=number(), => },atom() | #{‘price’:=float(), ‘quantity’:=number(), => },number()) -> #{‘items’:=[any(),…], ‘sales_tax’:=number(), ‘total’:=float(), => }

Thank you for your time.

Most Liked

david_ex

david_ex

To be clear, I don’t think you can “overuse” types. The problem with your code is that there were several instances of types with the same/similar names and they weren’t properly documented. In fact, the situation was confusing enough that you made mistakes yourself when writing the specs, so it will definitely be confusing for any other programmers.

What I mean is that types are like code. They should ideally mean one specific thing, should exist in one place, and should be documented clearly. So for a given “shape” of data, define a typespec for it in one single place, and have all specs that work with that data “reuse” the typespec by referring to it: don’t declare it again in the same module.

If you need different typespecs for similar but different data (e.g. item before processing VS item after processing), then absolutely declare 2 different types but make sure their names are clear, and they’re properly documented.

hlx

hlx

After taking a quick look at your code I would suggest you take a look at Decimal for your calculations. You can find many articles on why you should not use Float.

david_ex

david_ex

Your problem is what dialyzer is telling you: you’re not allowed to call initialize_cart_product/2 with (number(), Item) because the spec says it accepts (number(), input_item()).

Indeed, if you look at the definition of input_item/0 in shopping_cart.ex and compare to the item/0 in receipt_csv_parser.ex (which is what you’re passing to initialize the cart product), you’ll see they don’t match (their price and quantity types differ).

Some general tips:

  • you should put all of your modules within a top-level namespace unless you have a good reason not to do so (e.g. Item should be SalesTax.Item). Alias them where they’re used if you’re worried about long module names. Otherwise, any project using your code is going to have a conflict if they (e.g.) also define an Item struct. See here. Note that nothing forces you to do it, but it’s a good convention to follow.

  • you should probably use String.t instead of binary: they’re the same to analysis tools, but String.t makes it more obvious what you’re working with.

  • you’re sprinkling typing information all over your code. This makes it hard to understand and (as you’re finding out) hard to maintain.

For your specific case, I would suggest doing this:

# item.ex
@type t :: %__MODULE__{
    :basic_sales_tax_applicable => boolean(),
    :imported => boolean(),
    :name => binary(),
    :price => number(),
    :quantity => integer()
}

Then, everywhere else (e.g. here) use that value in your specs:

  @spec initialize_cart_product(number, Item.t()) :: Item.t()
  def initialize_cart_product(total_sales_tax_from_one_item, product) do

If you want to differentiate items before/after processing, you can declare additional “sub-types” in item.ex, e.g.:

# item.ex
@type t :: before_processing | after_processing

@type before_processing :: %__MODULE__{
    # type info
}

@type after_processing :: %__MODULE__{
    # type info
}

Last Post!

boddhisattva

boddhisattva

Thanks to your wonderful suggestions @david_ex I’ve been able to gain valuable insights on how do I write better typespecs with regard to my code. I’ve attempted to add all possible changes in this PR based on the discussions that we’ve had through some of the earlier threads on this post.

If you think that there’s still scope for any further improvement to the updated version of how I’ve used typespecs, please do let me know.

Thank you for sharing your experience of how one could use typespecs more efficiently :slight_smile:

Where Next?

Popular in Questions Top

vegabook
I’m brand new to Phoenix and I have stripped one of the demo applications to the bone. I just want to get an svg up on the screen. Here i...
New
ashish173
I am using Ecto timestamps with postgres, I can see the timestamps() use the :naive_dateime but for my use case I wanted to store the ti...
New
Fl4m3Ph03n1x
About me? ( if you have nothing better to do than reading about some random guy in the internet :stuck_out_tongue: ) Hello all, this is ...
New
bsollish-terakeet
Credo is smart enough to check for (something like) this: assert length(the_list) == 0 with this response: Checking if an enum is empt...
New
jerry
Good day to you all. I have been struggling to get a query involving like and ilike to work. Can anyone assist me on this, please? pro...
New
siddhant3030
Hi, I have to write a raw query for one of my project. But till now I have used ecto queries and don’t have much experience writing raw ...
New
marius95
Hello everyone, I try to use an Javascript Event Handler in my root.html.leex file. Therefore I created a function in the app.js file: ...
New

Other popular topics Top

JeremM34
Hello, how can I check the Phoenix version ? Thanks !
New
grych
Hi folks, Few months ago I have announced the proof-of-concept of the library to manipulate the browsers DOM objects directly from Elixi...
639 54260 488
New
hariharasudhan94
Lets say I have map like this fetching from my database %{"_id" => #BSON.ObjectId<58eb1a7a9ad169198c3dXXXX>, "email" => ...
New
jononomo
For some reason my phoenix channels are working for me in my local dev environment, but as soon as I deploy via Docker, I get a 403 error...
New
Patoshizzle
After calling mix ecto.create I get this error: 17:00:32.162 [error] GenServer #PID<0.412.0> terminating ** (Postgrex.Error) FATAL...
New
senggen
Erlang/OTP 25 [erts-13.2.2] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] 15:22:35.803 [error] gen_event {lager_file_backend...
New

We're in Beta

About us Mission Statement