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
readBillAmount()
|> calculateTaxAmount
|> printResult
end
def readFloatInput(message) do
IO.gets("#{message}\n")
|> Float.parse
|> handleFloatParsing(&TaxCalculator.readFloatInput/1, message)
end
def handleFloatParsing(result, func, message) do
case result do
:error ->
func.(message)
{float, _} -> float
end
end
def readBillAmount do
billAmount = readFloatInput("What is the bill amount?")
tipRate = readFloatInput("What is the tip rate?")
%Input{billAmount: billAmount, tipRate: tipRate}
end
def calculateTaxAmount(input) do
tip = input.billAmount * (input.tipRate/100)
total = input.billAmount + tip
%Output{total: total, tip: tip}
end
def printResult(output) do
IO.puts "Tip: $#{output.tip}"
IO.puts "Total: $#{output.total}"
end
When you are putting your code on forum please use this syntax:
```elixir
# your code here
```
Do not use case if you can use pattern match in parameters
Change function and variable names to snake_case
Use exactly 2 spaces for indention
Use defp instead of def for all functions that should not be accessible from other modules
Always use parentheses () when calling local function (in same module)
Always try to name well all unbound variables (like: _name_here)
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).
Optionally use better order of methods to find functions more quickly
Example code:
def main(_argv) do
read_bill_amount()
|> calculate_tax_amount()
|> print_result()
end
defp read_bill_amount do
%Input{
bill_amount: read_float_input(:question_bill),
tip_rate: read_float_input(:question_rate)
}
end
defp read_float_input(message_code) do
message_code
|> get_message()
|> Kernel.<>("\n")
|> IO.gets
|> Float.parse
|> handle_float_parsing(&TaxCalculator.read_float_input/1, message)
end
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
%Output{
tip: bill_amount * (tip_rate / 100),
total: bill_amount + tip,
}
end
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
end
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)
end
end
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}
end
defp print_result(output) do
IO.puts "Tip: $#{output.tip}"
IO.puts "Total: $#{output.total}"
end
end
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.
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.
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.
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.
