Handle invalid options #8

tteerawat · 2020-01-14T14:49:39Z

What i did:

Extract parsing functions from Split module to a new module called Parser.
Handle invalid options and inputs.
Refactor/edit Router and Split modules.

Related issue: #7

tteerawat · 2020-01-14T15:50:30Z

lib/chopperbot/split.ex

-
+  defp apply_multiplier(orders, multiplier) do
+    Enum.map(orders, fn {name, amount} ->
+      new_amount = Float.round(amount * multiplier, 15)


15 is the maximum number of fractional digits for Float.round/2.

tteerawat · 2020-01-14T15:51:49Z

lib/chopperbot/split.ex

@@ -3,32 +3,46 @@ defmodule Chopperbot.Split do

  TODO:
  [ ] add support for LINE format
-  [ ] add flexible discount ex. -10%
  [ ] [Bug] String.split wrong on copy & paste the command in Slack


Should we remove this line too 😆 ?

turboza

@teerawat1992 nice job. this is quite neat refactoring!

I felt like transform_to_orders is still a little hard to read. Let me come back to think again tomorrow!

turboza · 2020-01-16T11:03:34Z

lib/chopperbot/split/input_transformer.ex

+      ** (FunctionClauseError) no function clause matching in anonymous fn/1 in Chopperbot.Split.InputTransformer.transform!/1
+  """
+  @spec transform!([String.t()]) :: [Order.t()]
+  def transform!(inputs) do


Just curious, why InputTransformer.transform! while we use OptionTransformer.transform (no bang)?

I think you mentioned on the outdated code 😆

Oh right! 😂

turboza · 2020-01-16T11:12:27Z

lib/chopperbot/split/option_transformer.ex

+
+  defp accumulate_multipliers(multipliers) do
+    multipliers
+    |> Enum.reduce(1.0, &(&1 * &2))


nice way to write reduce! 👍

turboza · 2020-01-16T11:15:03Z

lib/chopperbot/split/option_transformer.ex

+  end
+
+  defp transform_to_multipliers(multipliers, invalid_options, []) do
+    {multipliers, Enum.reverse(invalid_options)}


Oh, your reverse in order to display the same input order? quite neat.

turboza · 2020-01-16T11:18:19Z

lib/chopperbot/router.ex

+          build_response(input)
+
+        _ ->
+          build_line_suggestion_response()


nice for returning help here.

turboza · 2020-01-16T11:18:44Z

lib/chopperbot/split.ex

@@ -3,32 +3,46 @@ defmodule Chopperbot.Split do

  TODO:
  [ ] add support for LINE format
-  [ ] add flexible discount ex. -10%
  [ ] [Bug] String.split wrong on copy & paste the command in Slack


turboza · 2020-01-16T15:35:09Z

lib/chopperbot/split/parser.ex

+      {:error, :invalid_input, ["one-hundred", "ten", "baht"]}
+  """
+  @spec parse(String.t()) ::
+          {:ok, parsed()}


I am thinking about parsed not sure if abstracting the type into parsed worth sacrificing the type reading, since parsed is not reused that much too.

it's used only once tho lol, can be replaced with just %{orders: [Order.t()], multiplier: float()} 🙇

turboza · 2020-01-16T15:37:09Z

lib/chopperbot/router.ex

+      "2️⃣",
+      "split alice 100 bob 200 +v",
+      "3️⃣",
+      "split alice 100 bob 200 share 100"


Oh, out of the scope, but maybe we can add the discount use case as well. (maybe later)

turboza · 2020-01-16T15:38:02Z

lib/chopperbot/router.ex

+      |> String.trim()
+      |> String.downcase()
+      |> case do
+        "split " <> input ->


I like this way of matching 👍

tteerawat · 2020-01-16T16:51:34Z

I felt like transform_to_orders is still a little hard to read. Let me come back to think again tomorrow!

🙇

tteerawat · 2020-01-16T17:01:35Z

I felt like transform_to_orders is still a little hard to read. Let me come back to think again tomorrow!

Another candidate for transform_to_orders function (looks more complex IMO 😅 ),
Looking forward to yours 😉

  defp transform_to_orders(input_pairs, orders \\ [], invalid_inputs \\ [])

  defp transform_to_orders([input_pair | rest_input_pairs], orders, invalid_inputs) do
    with {:ok, name, amount} <- validate_input_pair(input_pair),
         {:ok, float_amount} <- validate_amount_string(amount) do
      order = {String.downcase(name), float_amount}

      transform_to_orders(
        rest_input_pairs,
        [order | orders],
        invalid_inputs
      )
    else
      {:error, invalid_input} ->
        transform_to_orders(
          rest_input_pairs,
          orders,
          [invalid_input | invalid_inputs]
        )
    end
  end

  defp transform_to_orders([], orders, invalid_inputs) do
    {Enum.reverse(orders), Enum.reverse(invalid_inputs)}
  end

  defp validate_input_pair(input_pair) do
    case input_pair do
      [name, amount] -> {:ok, name, amount}
      [invalid_input] -> {:error, invalid_input}
    end
  end

  defp validate_amount_string(string) do
    case Float.parse(string) do
      {float_number, ""} -> {:ok, float_number}
      _ -> {:error, string}
    end
  end

turboza · 2020-01-17T09:27:09Z

Ohhh. thank you for the alternative. I couldn't think of the solution better than these 2 yet 😂

I think the new one you provide looks cleaner and easier to understand. Although the previous one is actually smarter and shorter. So I would vote with the new one.

turboza

@teerawat1992 Awesome job! thanks for the contribution. 👍 💯
This is a great improvement. Feel free to ping me to merge when you are ready.

(I am not sure how to add you as a collaborator without paying more to Github 😅 )

turboza · 2020-01-17T09:22:02Z

lib/chopperbot/split/input_transformer.ex

+      iex> transform(["ant", "200", "pipe", "100", "share", "-30"])
+      {:ok, [{"ant", 200.0}, {"pipe", 100.0}, {"share", -30.0}]}
+
+      iex> transform(["Satoshi", "10.9", "Takeshi", "390.13", "satoshi", "112.50",])


excessive comma , at the end?

Good catch! 🙇

turboza · 2020-01-17T09:25:00Z

lib/chopperbot/split/input_transformer.ex

+
+  defp transform_to_orders([[no_pair] | rest_input_pairs], orders, invalid_inputs) do
+    transform_to_orders(rest_input_pairs, orders, [no_pair | invalid_inputs])
+  end


I don't see the case for having rest_input_pairs here.
How about this?

defp transform_to_orders([[no_pair]], orders, invalid_inputs) do transform_to_orders([], orders, [no_pair | invalid_inputs]) end

This will disappear in the new version 🙇

tteerawat · 2020-01-17T13:08:02Z

@turboza Refactored as suggested 🙇

tteerawat added 4 commits January 14, 2020 21:47

✨ Implement Split.Parser and some helper modules

e5334ca

♻️ Refactor Router module

c1259c7

🎨 Apply Parser to Split

1c7a091

🔥 Remove unused functions from Split module

e27bc4b

tteerawat commented Jan 14, 2020

View reviewed changes

♻️ Change apply_multiplier/2 to public function with doc test

dbd41b7

tteerawat force-pushed the validate-options branch from ca333b0 to dbd41b7 Compare January 14, 2020 16:07

tteerawat added 2 commits January 15, 2020 16:07

♻️ Refactor OptionTransformer

9580a72

🎨 Edit InputTransformer to handle invalid inputs

1fc3f10

turboza reviewed Jan 16, 2020

View reviewed changes

turboza approved these changes Jan 17, 2020

View reviewed changes

tteerawat added 3 commits January 17, 2020 19:58

♻️ Refactor InputTransformer.transform_to_orders/3

e79647d

🔥 Remove todo

e1ebbe0

🔥 Remove parsed() type in Parser module

bd37f5c

Merge branch 'master' into validate-options

377bc2a

turboza merged commit cbeb8bf into flipay:master Jan 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle invalid options #8

Handle invalid options #8

tteerawat commented Jan 14, 2020 •

edited

Loading

tteerawat Jan 14, 2020

tteerawat Jan 14, 2020

turboza Jan 16, 2020

turboza left a comment

turboza Jan 16, 2020

tteerawat Jan 16, 2020

turboza Jan 17, 2020

turboza Jan 16, 2020

turboza Jan 16, 2020

turboza Jan 16, 2020

turboza Jan 16, 2020

turboza Jan 16, 2020

tteerawat Jan 16, 2020

turboza Jan 16, 2020

turboza Jan 16, 2020

tteerawat commented Jan 16, 2020

tteerawat commented Jan 16, 2020 •

edited

Loading

turboza commented Jan 17, 2020

turboza left a comment •

edited

Loading

turboza Jan 17, 2020

tteerawat Jan 17, 2020

turboza Jan 17, 2020

tteerawat Jan 17, 2020

tteerawat commented Jan 17, 2020

Handle invalid options #8

Handle invalid options #8

Conversation

tteerawat commented Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turboza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tteerawat commented Jan 16, 2020

tteerawat commented Jan 16, 2020 • edited Loading

turboza commented Jan 17, 2020

turboza left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tteerawat commented Jan 17, 2020

tteerawat commented Jan 14, 2020 •

edited

Loading

tteerawat commented Jan 16, 2020 •

edited

Loading

turboza left a comment •

edited

Loading