-
-
Notifications
You must be signed in to change notification settings - Fork 398
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
New practice exercise square-root
#797
Conversation
Thank you for contributing to Based on the files changed in this PR, it would be good to pay attention to the following details when reviewing the PR:
Automated comment created by PR Commenter 🤖. |
|
||
Given a natural radicand, return its square root. | ||
|
||
Note that the term "radicand" refers to the number for which the root is to be determined. That is, it is the number under the root symbol. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may have to add that this is a simplified exercise where only perfect squares are tested.
This way, we may avoid questions where the student asks what happens if function takes, for example, 3
.
We may also add an anylizer ensuring that :math.sqrt(x)
is not used. Note that maybe just a hint is ok here, what do you think @angelikatyborska ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we definitely need to tell students not to use :math.sqrt()
. I would recommend:
- Adding an
instructions.append.md
file to tell students that they need to implement an algorithm on their own and not use:math.sqrt()
orFloat.pow
- Adding an issue to the analyzer repo so that somebody works on it at some point, an issue similar to Write a basic
leap
analysis elixir-analyzer#42 for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may have to add that this is a simplified exercise where only perfect squares are tested.
That will be apparent looking at the tests, I think. It's not uncommon for tests to have more information than the description :)
|
||
Given a natural radicand, return its square root. | ||
|
||
Note that the term "radicand" refers to the number for which the root is to be determined. That is, it is the number under the root symbol. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we definitely need to tell students not to use :math.sqrt()
. I would recommend:
- Adding an
instructions.append.md
file to tell students that they need to implement an algorithm on their own and not use:math.sqrt()
orFloat.pow
- Adding an issue to the analyzer repo so that somebody works on it at some point, an issue similar to Write a basic
leap
analysis elixir-analyzer#42 for example
# @tag :pending | ||
test "root of 1" do | ||
radicand = 1 | ||
output = SquareRoot.square_root(radicand) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"square root square root" :D sounds a bit silly. Maybe we can deviate from problem specs in this case and do, let's say SquareRoot.calculate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might look better in the tests, but it will look weird in the lib file. How about sqrt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My problem with SquareRoot.square_root
and SquareRoot.sqrt
is that in both cases both the module and the function name are exactly the same noun, shortened or not. I don't really know why, but it feels more correct to me if function name is either an action that gets executed on the "subject" indicated by the module name, or if the function name is a noun that's a specialization of the module name. I wonder if there's an article about this 🤔 I would like to know where my feeling comes from, maybe it's completely unjustified?
Here's more proposals. They all look ok to me 🤷
defmodule SquareRoot do
@doc """
Calculate the integer square root of a positive integer
"""
@spec calculate(radicand :: pos_integer) :: pos_integer
def calculate(radicand) do
end
end
defmodule Root do
@doc """
Calculate the integer square root of a positive integer
"""
@spec square(radicand :: pos_integer) :: pos_integer
def square(radicand) do
end
end
defmodule Math do
@doc """
Calculate the integer square root of a positive integer
"""
@spec sqrt(radicand :: pos_integer) :: pos_integer
def sqrt(radicand) do
end
end
defmodule Calculator do
@doc """
Calculate the integer square root of a positive integer
"""
@spec sqrt(radicand :: pos_integer) :: pos_integer
def sqrt(radicand) do
end
end
defmodule PosInteger do
@doc """
Calculate the integer square root of a positive integer
"""
@spec sqrt(radicand :: pos_integer) :: pos_integer
def sqrt(radicand) do
end
end
}, | ||
"blurb": "Given a natural radicand, return its square root.", | ||
"source": "wolf99", | ||
"source_url": "https://github.com/exercism/problem-specifications/pull/1582" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How meta 😂 I suspect this isn't really a "good" value for source URL but meh 🤷 we're just copying problem specifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hahaha I didn't notice that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
I did this one in 20 minutes (after thinking researching the algorithm), pretty good feeling :)