Looking for feedback on code

Hi all,
I am a beginner in Elixir trying to write some code and learn about the language. Not sure if this is a good channel to ask for code review. Do direct me elsewhere or let me know if this is inappropriate.

I am just writing a simple TaxCalculator, below highlighting the main functions. I like the fact that through the use of the pipe operator, i am “forced” to breakdown the program into small functions.

Specifically regarding handleFloatParsing function, i am wondering if i am leaving the current thread of execution hanging and creating more and more execution stack if user keep inputing some non-float string. handleFloatParsing -> :error -> handleFloatParsing -> :error -> ...
Is there a better way of doing this?

def main(_argv) do
    |> calculateTaxAmount
    |> printResult

  def readFloatInput(message) do
    |> Float.parse
    |> handleFloatParsing(&TaxCalculator.readFloatInput/1, message) 

  def handleFloatParsing(result, func, message) do
    case result do
      :error ->
      {float, _} -> float

  def readBillAmount do
    billAmount = readFloatInput("What is the bill amount?")
    tipRate = readFloatInput("What is the tip rate?")
    %Input{billAmount: billAmount, tipRate: tipRate}

  def calculateTaxAmount(input) do
    tip = input.billAmount * (input.tipRate/100)
    total = input.billAmount + tip
    %Output{total: total, tip: tip}

  def printResult(output) do
    IO.puts "Tip: $#{output.tip}"
    IO.puts "Total: $#{output.total}"

@laiboonh: You should correct some things:

  1. When you are putting your code on forum please use this syntax:
    # your code here
  2. Do not use case if you can use pattern match in parameters
  3. Change function and variable names to snake_case
  4. Use exactly 2 spaces for indention
  5. Use defp instead of def for all functions that should not be accessible from other modules
  6. Always use parentheses () when calling local function (in same module)
  7. Always try to name well all unbound variables (like: _name_here)
  8. Optionally create one more method that returns your messages (usefull if you will want to support other languages in future - easier to find and change code).
  9. Optionally use better order of methods to find functions more quickly

Example code:

def main(_argv) do
  |> calculate_tax_amount()
  |> print_result()

defp read_bill_amount do
    bill_amount: read_float_input(:question_bill),
    tip_rate: read_float_input(:question_rate)

defp read_float_input(message_code) do
  |> get_message()
  |> Kernel.<>("\n")
  |> IO.gets
  |> Float.parse
  |> handle_float_parsing(&TaxCalculator.read_float_input/1, message)

defp get_message(:question_bill), do: "What is the bill amount?"
defp get_message(:question_rate), do: "What is the tip rate?"

defp handle_float_parsing(:error, func, message), do: func.(message)
defp handle_float_parsing({result, _rest}, _func, _message), do: result

defp calculate_tax_amount(%{bill_amount: bill_amount, tip_rate: tip_rate}) do
    tip: bill_amount * (tip_rate / 100),
    total: bill_amount + tip,

defp print_result(%{tip: tip, total: total}),
     do: IO.puts "Tip: $#{tip}" <> "\n" <> "Total: $#{total}"

Absolutely NEVER use Float if you are working with moneys. See decimal hex package.


That’s better that some of my early Elixir. In addition to @Eiji 's points about syntax, I’ll offer my suggested refactoring that removes a bit of the indirection

defmodule TaxCalculator do
  def main(_argv) do
    |> read_float_input("What is the bill amount?", :bill_amount)
    |> read_float_input("What is the tip rate?", :tip_rate)
    |> calculate_tax_amount
    |> print_result

  defp read_float_input(result, message, key) do
    raw_input = IO.gets("#{message}\n")
    case Float.parse(raw_input) do
      {float, _} -> Map.put(result, key, float)
      :error     -> read_float_input(result, message, key)

  defp calculate_tax_amount(%{bill_amount: bill_amount, tip_rate: tip_rate}) do
    tip = bill_amount * (tip_rate/100)
    total = bill_amount + tip
    %{total: total, tip: tip}

  defp print_result(output) do
    IO.puts "Tip: $#{output.tip}"
    IO.puts "Total: $#{output.total}"

Primarily I removed the 2 structs in favor of maps, but I also made your final question about overflowing the stack more explicit. Notice how read_float_input/3, in the error case, calls itself as the very last step of the function? BEAM does TCO (tail call optimization) and will reuse the stack frame instead of creating a new one, so a persistently rebellious user that never gave you a float would not cause any stack overflow problems. As you had it there were both readFloatInput/1 and handleFloatParsing/3 involved in that potential infinite recursion, and BEAM will handle that case as well, so your solution would not be a problem, but stylistically, I prefer making it more explicit.



Thank you for your input. May i know how can i write a test to test read_float_input especially for the :error case

Ah, yes, testability is a useful metric for these refactorings. I haven’t actually used it myself, but ExUnit has a capture_io function that is useful. I believe this forum post and the StackOverflow answer it points to will help you. I don’t have time to put the pieces together right now, but if you have struggles post back and I’ll take a look at the details later today.

Thank you for pointing me in the right direction I will give it a go and
post my efforts here. Awesome community :slight_smile:

I had a few minutes and had some fun writing this up. Don’t read it yet if you want to work it out for yourself first.

defmodule TaxCalculatorTest do
  use ExUnit.Case
  import ExUnit.CaptureIO

  test "handles error float parsing" do
    user_input = ["iamnotafloat", "still no float", "😎", "1.1"]

    prompts = capture_io(Enum.join(user_input, "\n"), fn ->
      send(self(), TaxCalculator.read_float_input(%{}, "prompt", :float_key))

    assert_received %{float_key: 1.1}

    prompt_list = String.split(prompts)
    assert length(prompt_list) == length(user_input)
    assert ["prompt"] == Enum.uniq(prompt_list)
1 Like

Nice one. It is also interesting to note that user_input have to end with a successful case last.

This will lead to a runtime error

capture_io([input: "foo"], fn -> TaxCalculator.read_float_input(%{}, "prompt", :float_key) end)

A followup question on testing:

I noticed this when the function that i want to test on is a private function

** (UndefinedFunctionError) function TipCalculator.read_float_input/3 is undefined or private

I would like to test all functions in my program and at the same time keep some of these functions private as they need not be visible to the end user directly. Why does Elixir or ExUnit put such constraints?

Thanks to capture_io i was able to fully achieve 100% test coverage.
Question: Does anyone know what does the small number on the second left hand side column represent?

great, think this example with user input and calculations on user input is a very good case for adding property based testing:

Property-based tests make statements about the output of your code based on the input, and these statements are verified for many different possible inputs.

especially since you are using float calculations, and property based testing should hopefully bring forward issues with that(as @Eiji pointed out above ), as well as inputting negative numbers etc.

iex> 0.1 + 0.02 == 0.12                                                           
iex> Decimal.add(Decimal.new(0.1), Decimal.new(0.02) ) == Decimal.new(0.12)
1 Like

I caution having the goal of 100% test coverage. It’s not enough to simply encounter every line of code if the tests do not validate each line’s behavior. It’s several years old and based in Ruby, but much of my underlying test philosophy agrees with this series of blogs: http://jasonrudolph.com/blog/testing-anti-patterns-how-to-fail-with-100-test-coverage/

With that out of the way, yes choosing to make a function private removes the ability to directly test it. But you can still exercise its behavior through a public function. You could use capture_io to test your code only from the main function.

Also don’t worry too much about what is public/private in a single module educational exercise. In a larger production application, it’s usually clearer what should be public/private, in my experience.

Thanks for the advice. I agree that quality of tests is more important than