Skip to content

423 triangle generator #458

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

Merged
merged 4 commits into from
Oct 4, 2017

Conversation

bmeverett
Copy link
Contributor

Adds the triangle test generator. I modified the class to better take into account the canonical data as suggested by @ErikSchierboom here

Closes #423

@@ -9,7 +9,22 @@ public enum TriangleKind

public static class Triangle
{
public static TriangleKind Kind(decimal side1, decimal side2, decimal side3)
public static bool IsScalene(double side1, double side2, double side3)
Copy link
Member

Choose a reason for hiding this comment

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

Just to be consistent, we should keep the types used here and in Example.cs the same.

@ErikSchierboom
Copy link
Member

Just one small nit, it already looks great!

@bmeverett
Copy link
Contributor Author

@ErikSchierboom fixed! However, it doesn't seem to like the double as parameters. It complained when I ran it locally but I think the actual test is different, so I should probably change back to decimal. Is there way to run tests locally or can I just run the build.ps1 script?

@ErikSchierboom
Copy link
Member

@bmeverett You should use the build.ps1 script, but you can speed things up significantly by specifying the exercise you want to build/test: build.ps1 --exercise=triangle.

@ErikSchierboom ErikSchierboom merged commit 3110183 into exercism:master Oct 4, 2017
@ErikSchierboom
Copy link
Member

Merged. Great work @bmeverett! Thanks. 🎉

@bmeverett bmeverett deleted the 423-triangle-generator branch October 4, 2017 14:30
@robkeim
Copy link
Contributor

robkeim commented Oct 7, 2017

Thanks @bmeverett for taking the time to do this, much appreciated!

I know I'm late here, but I have a couple of comments (@ErikSchierboom feel free to chime in here too since you reviewed this originally):

  • It would be more natural C# to have spaces between function arguments IsScalene(1, 2, 3) instead of IsScalene(1,2,3). That seems like a one line fix
  • In Triangle.cs shouldn't we remove the Kind method which is leading the students to a solution in the same direction? I'd personally be in favor of dropping that and enum in that file and letting students chose if they wanted to solve the problem that way or not.

@ErikSchierboom
Copy link
Member

Two excellent points @robkeim. The first issue was one that I felt could easily be corrected later, the second one I missed.

@robkeim
Copy link
Contributor

robkeim commented Oct 8, 2017

Thanks @ErikSchierboom for the input. @bmeverett would you like to make a new PR to fix these issues? If not, no worries I can do it.

@bmeverett
Copy link
Contributor Author

@robkeim @ErikSchierboom I can take a look at it. Do you want the Kind method removed from Triangle.cs and Example.cs or just Triangle.cs?

@ErikSchierboom
Copy link
Member

@bmeverett It's fine in Example.cs, as it is actually used there. It only needs to be removed from Triangle.cs.

@robkeim
Copy link
Contributor

robkeim commented Oct 9, 2017

Changes look good, thanks @bmeverett 🎉

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.

3 participants