Code Review for a sales tax problem solved using Elixir

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.

You’re not specifying the types and specs correctly. See how you should be doing that here.

For example, in lib/receipt_csv_parser.ex the spec for read_line_items/1 should be @spec read_line_items(String.t) :: list(String.t). The main error of the spec you have is String doesn’t mean anything. The string type is given by String.t/0

In the same vein, Item isn’t a type. You need to define it’s type like this (e.g.):

# in item.ex

@type t :: %__MODULE__{}

Then, back in lib/receipt_csv_parser.ex you can fix the spec for imported?/1 by making it @spec imported?(Item.t) :: boolean.

Thank you @hlx, I didn’t know about the Decimal Package in Elixir until now . I’m familiar that in Ruby we have something called as Big Decimal Class to deal with such things related to Money.

I’ll definitely take a look at this package in more detail, thank you for bringing that up

Thank you @david_ex , I’ve attempted to fix the typespecs in this PR. It would be great if you could review that PR and share your thoughts.

I’m currently still getting 2 more errors with regard to the type specs in shopping_cart.ex which I’m finding it difficult to debug. I’m getting the below errors with regard to that file when running the updated type specs with the dialyzer package. Those errors are:

lib/sales_tax.ex:54:no_return
The created fun has no local return.

---

lib/sales_tax.ex:56:call
The call:
ShoppingCart.initialize_cart_product(_total_sales_tax_from_one_item :: float(), _product :: Item)

breaks the contract
(number(), input_item()) :: output_item()

---

Any ideas on what I could be doing wrong here?

Also it feels like there isn’t much detailed documentation how to use typespecs with custom types. Any suggestions on where I could learn more on the best practices to adhere to when using typespecs with custom types would be really helpful. Thank you!

For the issue on line 56, dialzyer is telling you what the problem is: ShoppingCart.initialize_cart_product/2 takes a number/0 as the first argument, but you’re giving it the return value of SalesTaxCalculator.calculate_total_sales_tax/1 which is a float/0 per the specs you defined.

Regarding the “no local return” error, this often means that the body of the function can raise an error (in which case no return value would be provided). To find out what the issue is, try commenting lines (or replacing them with dummy lines that ensure the types work in your code) to find the line causing triggering the dialyzer issue. Then, either fix your code/specs, or add no_return as a possible return value.

Thank you for taking the time out to answer my question @david_ex. It’s an interesting thing what you’ve brought up and I didn’t think that dialyzer would take in to account what we’re returning from SalesTaxCalculator.calculate_total_sales_tax/1

The code for that method looks like below:

def calculate_total_sales_tax(product) do
total_amount =
calculate_basic_sales_tax(product.basic_sales_tax_applicable, product.price) +
calculate_import_duty_sales_tax(product)

if total_amount != 0 do
  Float.round(total_amount, 2)
else
  total_amount
end

end

This way it could return a Float or a number(i.e., it would return 0). I tried changing the type spec for the above method to: @spec calculate_total_sales_tax(Item) :: number in this commit and In the ShoppingCart Module I retained things like below:

  @spec initialize_cart_product(number, input_item ) :: output_item
  def initialize_cart_product(total_sales_tax_from_one_item, product) do
    %Item{
      price: Float.round(product.price + total_sales_tax_from_one_item, 2) * product.quantity,
      quantity: product.quantity,
      name: product.name,
      basic_sales_tax_applicable: product.basic_sales_tax_applicable,
      imported: product.imported
    }
  end

Now both of them have the same type for total_sales_tax_from_one_item which is number. With this, I’m still getting the below error:

lib/sales_tax.ex:56:call
The call:
ShoppingCart.initialize_cart_product(_total_sales_tax_from_one_item :: number(), _product :: Item)

breaks the contract
(number(), input_item()) :: output_item()
________________________________________________________________________________
done (warnings were emitted)

Any thoughts on what I could be potentially missing? Thank you.

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
}

Hi @david_ex ,

Firstly, please accept my sincere apologies for a late response, I got caught up with a few things of late.

Thanks a lot for your time and valuable suggestions . I finally got a passed successfully message from dialyzer thanks to your inputs. I’ve made the related changes to typespecs as part of this commit

I’m completely with you on the naming conventions in terms of adding a top-level namespace and that would definitely prevent conflicts in the future.

It’s a good point that using String.t() instead of binary would make things more obvious.

With regard to your point on sprinkling typing information all over the code. The intention of using typespecs is to make things easier for readers to understand the parameter types and the return types with regard to each function prototype. Do you see potential areas where I could present relevant information with regard to using typespecs without over using it ? I’d really appreciate any specific examples where I could the code more concise on this regard to understand your point more clearly.

Thank you once again for your valuable time and feedback It definitely helped me to improve my code further.

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.

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