Skip to content

resistor-color-trio: create exercise#1772

Closed
HuskyNator wants to merge 10 commits intoexercism:masterfrom
HuskyNator:risistor-color-trio(#1750)
Closed

resistor-color-trio: create exercise#1772
HuskyNator wants to merge 10 commits intoexercism:masterfrom
HuskyNator:risistor-color-trio(#1750)

Conversation

@HuskyNator
Copy link
Copy Markdown
Contributor

Closes #1750

Reviewer Resources:

Track Policies

@HuskyNator
Copy link
Copy Markdown
Contributor Author

Not sure why these are conflicts...

@lemoncurry
Copy link
Copy Markdown
Contributor

Not sure why these are conflicts...

It seems that you also added some changes for the Resistor Color Duo exercise, which has already been merged.

Comment on lines +2 to +4
private enum Color {
black, brown, red, orange, yellow, green, blue, violet, grey, white;
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Java enums should generally be in UPPER_SNAKE_CASE.

I think the API should actually ingest the enum which should be a public file of its own (Color.java).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does the Java track exercise a policy or a preference wrt. multiple files handed out?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do not know of a specific policy, but I know that there are exercises which do this: https://github.com/exercism/java/tree/master/exercises/dominoes/src/main/java

There is one reference to multiple file submissions in the POLICIES file though (which implies it is totally fine):

The first exercise in the track whose test suite mandates multiple solution files should be accompanied by a hints.md file reminding the practitioner that the CLI supports multiple file submissions.

@@ -0,0 +1,5 @@
class ResistorColorTrio {
String value(String[] colors) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
String value(String[] colors) {
String value(Color... colors) {

This should really take the Color enum as input. It's a good setup for varargs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

From the exercise I thought the idea behind the exercise was applying knowledge about enums. Thus predefining them might defeat the purpose.

Fairly honestly however, I have since almost fully forgotten how exercism works and/or why the entire project is set up like it is. My apologies.

Comment on lines +17 to +19
String[] input = { "orange", "orange", "black" };
String expected = "33 ohms";
String actual = resistorColorTrio.value(input);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
String[] input = { "orange", "orange", "black" };
String expected = "33 ohms";
String actual = resistorColorTrio.value(input);
String expected = "33 ohms";
String actual = resistorColorTrio.value(Color.ORANGE, Color.ORANGE, Color.BLACK);

Same throughout.

Comment thread exercises/resistor-color-trio/src/test/java/ResistorColorTrioTest.java Outdated
Comment thread exercises/resistor-color-trio/src/test/java/ResistorColorTrioTest.java Outdated
Comment on lines +7 to +15
int intValue = (10 * Color.valueOf(colors[0]).ordinal() + Color.valueOf(colors[1]).ordinal())
* (int) Math.pow(10, Color.valueOf(colors[2]).ordinal());
String stringValue;
if (intValue >= 1000) {
stringValue = String.valueOf(intValue / 1000) + " kiloohms";
} else {
stringValue = String.valueOf(intValue) + " ohms";
}
return stringValue;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The canonical implementation should probably include encoding more information into the Color enum. Might also want to consider another enum for the powers (like OHMS, KILOOHMS, ...). This solution only adds support for kiloohms and we can do better (I know the Kotlin version goes up to something like exaohms).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The Haskell solution only goes to giga, but adds optional edge cases that the original exercise does not address, because functional programmers have an obsession with totality. The point is that while you could extend the range beyond giga, you could also do a lot of other things, or not.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suppose I am biased by having seen the Kotlin implementation of this exercise. IMO: we should go up to giga specifically since the exercise naturally produces gigaohms.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Keeping it simple and as specific as possible seemed like a good option.
However, as per my other comment, i do not know if this is preferrable or not.

@sshine sshine changed the title Risistor color trio resistor-color-trio: create exercise Apr 13, 2020
Copy link
Copy Markdown
Contributor

@sshine sshine left a comment

Choose a reason for hiding this comment

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

Thanks for all your reviews, @jmrunkle.

I've responded to a few of them because they popped up in my inbox.

So see my messages as an indicator that your thought progression has made participation easier.

Comment on lines +2 to +4
private enum Color {
black, brown, red, orange, yellow, green, blue, violet, grey, white;
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does the Java track exercise a policy or a preference wrt. multiple files handed out?

Comment on lines +7 to +15
int intValue = (10 * Color.valueOf(colors[0]).ordinal() + Color.valueOf(colors[1]).ordinal())
* (int) Math.pow(10, Color.valueOf(colors[2]).ordinal());
String stringValue;
if (intValue >= 1000) {
stringValue = String.valueOf(intValue / 1000) + " kiloohms";
} else {
stringValue = String.valueOf(intValue) + " ohms";
}
return stringValue;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The Haskell solution only goes to giga, but adds optional edge cases that the original exercise does not address, because functional programmers have an obsession with totality. The point is that while you could extend the range beyond giga, you could also do a lot of other things, or not.

@jmrunkle
Copy link
Copy Markdown
Contributor

@HuskyNator - are you still working on this PR?

@HuskyNator
Copy link
Copy Markdown
Contributor Author

Yes sorry, Ive been quite bussy

- upper snake case for enum
- typo & simplification in test
@iHiD iHiD closed this Jan 28, 2021
@iHiD iHiD deleted the branch exercism:master January 28, 2021 19:15
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.

resistor-color-trio: implement exercise

5 participants