Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raindrops 48in24 approaches #1422

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

michalporeba
Copy link
Contributor

Resolves #1420

I'm trying a different approach to approaches than the one I used in the leap.
Rather than focusing on the features of the language I'm focusing on two logical approaches and showing how they can be implemented differently using various features of the language. Let me know what you think.

Copy link
Contributor

Thank you for contributing to exercism/elixir 💜 🎉. This is an automated PR comment 🤖 for the maintainers of this repository that helps with the PR review process. You can safely ignore it and wait for a maintainer to review your changes.

Based on the files changed in this PR, it would be good to pay attention to the following details when reviewing the PR:

  • General steps

    • 🏆 Does this PR need to receive a label with a reputation modifier (x:size/{tiny,small,medium,large,massive})? (A medium reputation amount is awarded by default, see docs)
  • Any exercise changed

    • 👤 Does the author of the PR need to be added as an author or contributor in <exercise>/.meta/config.json (see docs)?
    • 🔬 Do the analyzer and the analyzer comments exist for this exercise? Do they need to be changed?
    • 📜 Does the design file (<exercise>/.meta/design.md) need to be updated to document new implementation decisions?
  • Practice exercise changed

    • 🌲 Do prerequisites, practices, and difficulty in config.json need to be updated?
    • 🧑‍🏫 Are the changes in accordance with the community-wide problem specifiations?

Automated comment created by PR Commenter 🤖.

@michalporeba
Copy link
Contributor Author

Link checkers will fail until the first deployment, as this PR will create the missing pages.

@@ -0,0 +1,49 @@
# Introduction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my preference would be to try to start the approaches documents with the "most reasonable" solution first, and explore the more exotic solutions further down. I know that's very subjective, but I hope we can agree on which one is the best 😁

I have a strong opinion for this exercise that the best solution uses a list that pairs divisors to sounds, and then iterates over them like this one:

https://exercism.org/tracks/elixir/exercises/raindrops/solutions/SaberCon

I don't know if you would qualify that as a variant of a "step by step" approach? You didn't mention anything with Enum yet. Maybe it deserves to be its own approach.

Btw. In this exercise it's important to note that maps in Elixir are not sorted. This solution for example https://exercism.org/tracks/elixir/exercises/raindrops/solutions/alkhulaifi has to sort the map first before iterating, which clearly suggests a list of two-tuples is a better data structure here.

Comment on lines +10 to +19
case {rem(number, 3), rem(number, 5), rem(number, 7)} do
{0, 0, 0} -> "PlingPlangPlong"
{0, 0, _} -> "PlingPlang"
{0, _, 0} -> "PlingPlong"
{_, 0, 0} -> "PlangPlong"
{0, _, _} -> "Pling"
{_, 0, _} -> "Plang"
{_, _, 0} -> "Plong"
_ -> Integer.to_string(number)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


All the examples above, at their core, represent the same approach of doing the check step by step.

[pattern-matching-approach]: https://exercism.org/tracks/elixir/exercises/raindrops/approaches/pattern-matching
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the pattern matching approach for this exercise?

@@ -0,0 +1,75 @@
# Pattern Matching
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong heading

exercises/practice/raindrops/.approaches/config.json Outdated Show resolved Hide resolved
Comment on lines +1 to +47
# Step By Step

```elixir
defmodule Raindrops do
@spec convert(pos_integer) :: String.t()
def convert(number) do
pling = if rem(number, 3) == 0, do: "Pling", else: ""
plang = if rem(number, 5) == 0, do: "Plang", else: ""
plong = if rem(number, 7) == 0, do: "Plong", else: ""
sound = pling <> plang <> plong

if sound == "" do
Integer.to_string(number)
else
sound
end
end
end
```

In this approach, we test each condition only once, similar to using the `case` on a tuple in the [pattern matching approach][pattern-matching-approach].
However, this time, if a condition is true, we capture the sound component, and if it is not true, we capture the sound as an empty string.

Once this is done, we can concatenate all three conditions to get the full sound.
Finally, if the `sound` is empty, we can return the number or, alternatively, the calculated `sound`.

## Functions

We can create private functions to test for component sounds.

```elixir
defp pling(n) when rem(n, 3) == 0, do: "Pling"
defp pling(_), do: ""
defp plang(n) when rem(n, 5) == 0, do: "Plang"
defp plang(_), do: ""
defp plong(n) when rem(n, 7) == 0, do: "Plong"
defp plong(_), do: ""
defp sound(sound, number) when sound == "", do: Integer.to_string(number)
defp sound(sound, _number), do: sound
```

Now the solution can look like this:
```elixir
def convert(number) do
sound(pling(number) <> plang(number) <> plong(number), number)
end
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String concatenation isn't used by Elixir devs that often. It has a big disadvantage - it crashes on nils. Elixir devs usually always use string interpolation.

Using string interpolation in both of those solutions would make them slightly shorter:

defmodule Raindrops do
  @spec convert(pos_integer) :: String.t()
  def convert(number) do
    pling = if rem(number, 3) == 0, do: "Pling"
    plang = if rem(number, 5) == 0, do: "Plang"
    plong = if rem(number, 7) == 0, do: "Plong"
    sound = "#{pling}#{plang}#{plong}"

    if sound == "" do
      Integer.to_string(number)
    else
      sound
    end
  end
end
defp pling(n), do: if(rem(n, 3) == 0, do: "Pling")
defp plang(n), do: if(rem(n, 5) == 0, do: "Plang")
defp plong(n), do: if(rem(n, 7) == 0, do: "Plong")
defp sound(sound, number) when sound == "", do: Integer.to_string(number)
defp sound(sound, _number), do: sound

def convert(number) do
  sound("#{pling(number)}#{plang(number)}#{plong(number)}", number)
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

48in24 Approaches for Raindrops
2 participants