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.

Sign up for GitHub

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

resistor-color-trio: add exercise #1551

Merged
merged 14 commits into from
Jul 24, 2019
Merged

resistor-color-trio: add exercise #1551

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3ac8c8e
resistor-color-trio: add exercise
ErikSchierboom Jul 14, 2019
53704f5
Update exercises/resistor-color-trio/description.md
ErikSchierboom Jul 16, 2019
b0d6b45
resistor-color-trio: use normal quotation marks
ErikSchierboom Jul 16, 2019
25a7d23
resistor-color-trio: use kilo ohms
ErikSchierboom Jul 16, 2019
0a96e2b
resistor-color-trio: shorten description
ErikSchierboom Jul 16, 2019
a7104fa
resistor-color-trio: use two different properties
ErikSchierboom Jul 16, 2019
9cc8e61
resistor-color-trio: more unicode character fixes
ErikSchierboom Jul 16, 2019
d2f19d0
resistor-color-trio: move to label property
Jul 18, 2019
3f7032d
Update exercises/resistor-color-trio/description.md
ErikSchierboom Jul 18, 2019
df1109f
Update exercises/resistor-color-trio/description.md
ErikSchierboom Jul 18, 2019
3dd6a99
resistor-color-trio: update expected labels to normalized format
ErikSchierboom Jul 18, 2019
5a50a7b
resistor-color-trio: update blurb
Jul 19, 2019
3d23cd9
resistor-color-trio: change Ohms to ohms
Jul 19, 2019
dca9af3
resistor-color-trio: change expected format
ErikSchierboom Jul 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions exercises/resistor-color-trio/canonical-data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,35 +4,43 @@
"cases": [
{
"description": "Orange and orange and black",
"property": "description",
"property": "value",
"input": {
"colors": ["orange", "orange", "black"]
},
"expected": "This is a 33 Ohms resistor."
"expected": 33
},
{
"description": "Blue and grey and brown",
"property": "description",
"description": "Green and brown and brown",
"property": "value",
"input": {
"colors": ["blue", "grey", "brown"]
"colors": ["green", "brown", "orange"]
Copy link
Contributor

Choose a reason for hiding this comment

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

s/orange/brown to get 510

},
"expected": "This is a 680 Ohms resistor."
"expected": 510
},
{
"description": "Red and black and red",
"description": "Blue and grey and yellow",
"property": "value",
"input": {
"colors": ["blue", "grey", "yellow"]
},
"expected": 680000
},
{
"description": "Brown and black and black",
"property": "description",
"input": {
"colors": ["red", "black", "red"]
"colors": ["brown", "black", "black"]
Copy link
Member

Choose a reason for hiding this comment

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

The inputs for the description property should be integers, not color lists. The student has already written a three-color conversion function, at this point. No need to force them to call it; it is both obvious and natural to hook the output of the color converter into the input of the describer.

Copy link
Member

Choose a reason for hiding this comment

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

Except that the integers don't mean anything by themselves.

"orange" "orange" "orange" would be [3, 3, 3].

(10 * colorValue('orange') + colorValue('orange')) * (10 ** colorValue('orange'))

Would be a readable solution. If we use integers, we are moving away from correct and optimal solutions that look like this:

(colorFirstValue('orange') + colorSecondValue('orange')) * colorMultiplier('orange')

Copy link
Contributor

Choose a reason for hiding this comment

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

@SleeplessByte can you expand on that objection? If I'm reading @coriolinus correctly he's suggesting that value("orange", "orange", "orange") should return 33000 and thus both description(33000) and description(value("orange", "orange", "orange")) should return the same formatted string ... ie description should take ohms as its argument, not band colors. Because value would already be returning ohms I'm not sure how that influences the solution as you've described it.

That said maybe we've got some people thinking:

def value(color, color, color):
    return ohms

def description(color, color, color):
    humanized_ohms = [some operations on value(color, color, color)]
    return "blah blah" + humanized_ohms

Is preferable while others are assuming:

def value(color, color, color):
    return ohms

def description(ohms):
    humanized_ohms = [some operations on ohms]
    return "blah blah" + humanized_ohms

Is the natural form.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't mean to suggest a list of integers; I meant a single integer, the value in ohms.

The intent is to get people to write one calculate_value function, and one format_description function. It should be obvious and trivial to nest the calls: if what you have is a list of colors, and what you want is a string description, there's no reason not to write format_description(calculate_value(color_list)).

However, it would teach bad factorization to include both operations in a single function.

Copy link
Member

@SleeplessByte SleeplessByte Jul 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ah. I understand what you're (both, @coriolinus and @yawpitch) are saying. I think having value return 3300 is fine 👍 😄 .

I think having label return "33 kilo Ohms" is also something we should have.
I think having description is something a track can add based on preference on a sentence.

I don't agree with all the "this will lead to bad code" arguments. I understand what you're saying, but it's up to the student to recognise the two concerns or for the mentor to point them out. Purposefully crafting problem descriptions so a student can't do it wrongly is kinda forcing a certain implementation, which is something we don't want.

I personally think that forcing description to take "ohms" is also a language-specific implementation details. Having a class object with a description property that doesn't take arguments is very reasonable in a lot of languages. Both your arguments are forcing a certain code style (that is functional).

Copy link
Member

Choose a reason for hiding this comment

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

See: #1551 (comment)

value as number (33000) is still an incorrect abstraction.

Copy link
Contributor

@yawpitch yawpitch Jul 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I personally think that forcing description to take "ohms" is also a language-specific implementation details. Having a class object with a description property that doesn't take arguments is very reasonable in a lot of languages. Both your arguments are forcing a certain code style (that is functional).

I'm not sure that it does; it's up to the track implementer to decide on function vs class as the organizing structure, and for instance in Python I might reasonably decide that this should be the excercise in which we introduce subclassing of immutables and implement this as:

class ResistorValue(int):
    def __new__(cls, color, *colors):
         pass

    @property 
    def description(self):
        pass

Now description is perfectly valid and doesn't take ohms as a functional parameter, because the class itself is inherently a value in ohms.

@cmccandless note to us, might be a thing to consider for this exercise, as lord knows we don't have anything that introduces properly subclassing immutable builtins.

},
"expected": "This is a 2 kilo Ohms resistor."
"expected": "This is a 1O Ohms resistor."
},
{
"description": "Green and brown and orange",
"description": "Red and black and red",
"property": "description",
"input": {
"colors": ["green", "brown", "orange"]
"colors": ["red", "black", "red"]
},
"expected": "This is a 51 kilo Ohms resistor."
"expected": "This is a 2 kilo Ohms resistor."
},
{
"description": "Yellow and violet and yellow",
Expand Down