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. 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
}
david_ex

david_ex

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.

david_ex

david_ex

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.

Where Next?

Popular in Questions Top

Kagamiiiii
Student & New to elixir. Nice language. I want to convert a english character, e.g. “a”, which is stored in a variable, to it’s asci...
New
fireproofsocks
I’m working on defining a simple Ecto schema for a table (in PostGres), but I don’t see where I can define a column as NOT NULL. Conside...
New
vac
Hi, I'm quite new in Elixir and I'm trying to format a string to a PEM format. I have the certificate value like MIIDBTCCAe2...... and ...
New
mgjohns61585
Could someone help me? I'm making my first elixir program, number guessing game. I can't figure out how to convert the user's guess from ...
New
gonzofish
I’m currently trying to understand how to join three tables using Ecto. All the examples I’ve seen use 2, so maybe I’m just missing somet...
New
LegitStack
I’m trying to make a websocket server in Phoenix or raw Elixir. I heard about gun, I think I could use cowboy, but since I’m not that sma...
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
Exadra37
Sometimes I want to check if the input into a function is not a blank string. My first approach: defmodule Example do def do_stuff(s...
New
belgoros
I’m not a pro in using Regex and can’t figure out why the following behaviour happens, especially if we take into account the difference ...
New
wernerlaude
In AR this is so simple @articles = current_user.articles How to do in Ecto? def index(conn, _params) do current_user = conn.assig...
New

Other popular topics Top

JDanielMartinez
Hi! May someone helps me, please! I have two apps into an umbrella project: the first one is Database, which manages queries, and the se...
New
shahryarjb
Hello, I get Persian date from my client and convert it to normal calendar like this: def jalali_string_to_miladi_english_number(persi...
New
sorentwo
Hello! tl;dr Announcing Oban, an Ecto based job processing library with a focus on reliability and historical observability. After spen...
977 41022 311
New
srinivasu
How to handle excepions in elixir? Suppose i have A, B, C ,D, E modules. and each module has get() function. A.get() method will call th...
New
dotdotdotPaul
Okay, I'm having a heck of a time trying to figure out how to best handle the validation of belongs_to associations in Ecto. I'm sure I'...
New
AstonJ
You’re a programmer, so you don’t need spoon feeding with the conventional drivel about “this is an integer.” No. You need to know what’s...
New
freewebwithme
Using vs code and installed ElixirLS: support and debugger. And I got an error popped up on start up says Failed to run ‘elixir’ comma...
New
myronmarston
The Elixir Typespec docs show the following syntax for keyword lists in typespecs: # ... | [key: type] # keyword lis...
New
fireproofsocks
Forgive me if this is obvious, but how does one delete a database record WITHOUT selecting it first? https://hexdocs.pm/ecto/Ecto.Repo.h...
New
msaraiva
Surface is an experimental library built on top of Phoenix LiveView and its new LiveComponent API that aims to provide a more declarative...
564 42633 214
New

We're in Beta

About us Mission Statement