armstrong-numbers: new exercise #1018

pheanex · 2017-11-20T23:59:53Z

How is this for a first draft on #1017 ?

Closes #1017.

coriolinus

Generally speaking, this looks good; thanks for your work contributing this! However, there are a few areas which I feel can be improved, noted below:

coriolinus · 2017-11-21T00:05:55Z

exercises/armstrong-numbers/canonical-data.json

+      "expected": true
+    },
+    {
+      "description": "10 is not an Armstrong number in base 8",


Isn't this a duplicate of L135?

Removed, Thank you

coriolinus · 2017-11-21T00:08:07Z

exercises/armstrong-numbers/description.md

+An [Armstrong number](https://en.wikipedia.org/wiki/Narcissistic_number) is a number that is the sum of its own digits each raised to the power of the number of digits.
+It also depends on the base of the number being used (for example base = 10 for the decimal system or base = 2 for the binary system).
+
+For example:<sup></sup>


While HTML is valid Markdown, we're also aiming to keep these files readable without rendering. As such, let's get rid of empty tags.

I removed the empty tag. Do you want no html at all in the description-Markdown then?

coriolinus · 2017-11-21T00:08:27Z

exercises/armstrong-numbers/description.md

+It also depends on the base of the number being used (for example base = 10 for the decimal system or base = 2 for the binary system).
+
+For example:<sup></sup>
+153<sub>10</sub> is &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;an armstrong number in base 10, because: 153<sub>10</sub> = 1<sup>3</sup> + 5<sup>3</sup> + 3<sup>3</sup> = 1 + 125 + 27 = 154<sub>10</sub>


Why six non-breaking spaces in a row?

It also doesn't render well.

The idea was to keep the indentation for the two example the same, so that differences are more obvious. => Removed

coriolinus · 2017-11-21T00:09:45Z

exercises/armstrong-numbers/metadata.yml

@@ -0,0 +1,4 @@
+---
+blurb: "Given two decimal parameters (number and base) return True if the number 'number' is an armstrong number with respect to the numeral system with base 'base'."


Minor nitpick: instead of "return True if", possibly phrase this as "determine whether".

"Determine if a number is an Armstrong number"

would be enough.

The blurb doesn't need to completely specify what you have to do. It should be a teaser to encourage the student to look at the detail of the problem.

Ok, I wasn't really sure what the "blurb" was good for ... now i know 😄
=> Fixed as suggested

ErikSchierboom · 2017-11-21T08:01:25Z

exercises/armstrong-numbers/description.md

+It also depends on the base of the number being used (for example base = 10 for the decimal system or base = 2 for the binary system).
+
+For example:<sup></sup>
+153<sub>10</sub> is &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;an armstrong number in base 10, because: 153<sub>10</sub> = 1<sup>3</sup> + 5<sup>3</sup> + 3<sup>3</sup> = 1 + 125 + 27 = 154<sub>10</sub>


Although I agree that using <sup></sup> makes for the best display of exponentiation, I don't really like using HTML in markdown. I've looked at the all-your-base exercise, and it's description uses the caret sign (^), which I think is still quite readable.

To be consistent here, I'd also remove the "sub" additional to the "sup" then.
This would "cost" quite a bit of readability for the rendered version because we lose the hint in what base a number is represented. If we decide to drop all html, then I'd also vote for dropping the "base" completely and go with @Insti argument at the bottom. Do we have a consensus here?

ErikSchierboom · 2017-11-21T08:02:04Z

exercises/armstrong-numbers/description.md

+It also depends on the base of the number being used (for example base = 10 for the decimal system or base = 2 for the binary system).
+
+For example:<sup></sup>
+153<sub>10</sub> is &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;an armstrong number in base 10, because: 153<sub>10</sub> = 1<sup>3</sup> + 5<sup>3</sup> + 3<sup>3</sup> = 1 + 125 + 27 = 154<sub>10</sub>


It also doesn't render well.

Insti · 2017-11-21T09:50:12Z

exercises/armstrong-numbers/canonical-data.json

+      "expected": false
+    },
+    {
+      "description": "111 is not an Armstrong number in base 10",


(Feedback applies to all tests.)

The description here doesn't tell me anything already covered by the input and output values.
What is this test testing?
Why is 111 useful number to test?
If we had a solution that passed the previous n tests, what code would need to change that would make this test pass?

I just wanted to have another false 3-digit 10-base test additionally to 100.
I guess you are right and this does not catch anything special additionally.
=> Removed

Why is 100 useful to test?

Insti · 2017-11-21T09:54:43Z

Is this problem about Armstrong numbers or base calculations?

Given one could express the Armstrong number portion of this exercise using only "standard" base 10. What does the additional base parameter add to the problem?

Insti · 2017-11-21T10:00:01Z

How is this for a first draft on #1017 ?

This is a great first draft. 👍

Thanks. ❤️

coriolinus · 2017-11-21T10:01:52Z

@Insti I requested that @pheanex write the tests across several bases in #1017, when the idea was proposed. The reason is that whether a given integer is an Armstrong number varies according to the base in use. That is, 153 is an Armstrong number in base 10, but 0x99 and 0o231, which represent the same number in different bases, may not be. Therefore, the base used is a relevant property to vary across in the tests.

I'd expect most students not to bother with base conversions on their own for most cases, instead using the relevant string formatting options of their chosen language to generate a string representation, and then iterating over the digits. If you like, we can restrict the options to octal, decimal, and hexadecimal in order to minimize the chance that students need to manually implement any kind of base conversion.

coriolinus · 2017-11-21T10:13:06Z

@Insti Another relevant property to vary over is the number of digits in the number; that affects the way the property is computed. Therefore, looking at the tests, I'd expect to see for each of some number of bases, for each of some number of digits in that base, a few numbers to check which are Armstrong numbers, and a few which are not. This is what I observe in these tests, so I'm satisfied.

The precise details of which bases are tested, which digit lengths are tested, and how many and which numbers are tested could be debated, but I don't have any objection to the examples already chosen.

Insti · 2017-11-21T10:13:36Z

153 is an Armstrong number in base 10, but not necessarily in base 16. Therefore, the base used is a relevant property to vary across in the tests.

5 + 5 = 10 in base 10 but not in base 16, but we don't ask for base as an argument to every exercise that uses addition.

I think my point is that the base stuff adds unnecessary complexity to the problem.

coriolinus · 2017-11-21T10:33:13Z

5+5 == 10 is always true, regardless of the number system; you can write it as 0o5 + 0o5 == 0o12, or 0x5+0x5 == 0xA, but the math doesn't change. 18, 0o22, and 0x12 are all even, regardless of base. Regardless of what base a number is represented in, it will always be even; we can detect evenness by use of the modulo operator.

Armstrong numbers are different: whether or not a number is an Armstrong number is intrinsically linked to the base in which it is represented. That's precisely why Armstrong numbers are interesting; most numerical properties hold true regardless of representation, and this one doesn't. To test only a single base would ignore the property which makes Armstrong numbers interesting.

Or, to put it otherwise: as the README states, Armstrong numbers come from recreational math. We could create an exercise which tested them in a single base, but it would be pure symbolic manipulation, uninteresting in itself. What makes it interesting is what makes it recreational: the fact that the representation changes the property.

We're not even asking students to parse numbers of varying base; all the inputs are given as standard integers. We just ask them to compute a property which varies according to base. Testing only base 10 would both be less interesting for the student, and incomplete within the scope of the problem.

pheanex · 2017-11-21T18:52:50Z

@Insti @coriolinus I'm fine with both your versions.
How do we proceed? I don't want to bikeshed this 😃

ErikSchierboom · 2017-11-21T19:55:43Z

exercises/armstrong-numbers/canonical-data.json

+  "exercise": "armstrong-number",
+  "version": "1.0.0",
+  "comments": [
+    "Both input-parameters are always given in base-10(decimal)."


I think "base-10(decimal)" should be "base-10 (decimal)"

Missing space added

Insti · 2017-11-21T22:15:14Z

exercises/armstrong-numbers/canonical-data.json

+      "expected": true
+    },
+    {
+      "description": "370 is an Armstrong number in base 10",


I had no specific objection to 111, but rather a general objection to ALL the tests.

The description here doesn't tell me anything already covered by the input and output values.
What is this test testing?
Why is this number useful to test?
If we had a solution that passed the previous n tests, what code would need to change that would make this test pass?

What is it about each of these numbers that makes them interesting/important to test?

I guess its easier if we turn this around:
What numbers do you suggest we test/are interesting to test?

Insti · 2017-11-21T22:16:14Z

exercises/armstrong-numbers/description.md

+It also depends on the base of the number being used (for example base = 10 for the decimal system or base = 2 for the binary system).
+
+For example:
+153<sub>10</sub> is an armstrong number in base 10, because: 153<sub>10</sub> = 1<sup>3</sup> + 5<sup>3</sup> + 3<sup>3</sup> = 1 + 125 + 27 = 154<sub>10</sub>


Use a capital A for all usage of Armstrong in English.

Insti · 2017-11-21T22:16:48Z

exercises/armstrong-numbers/description.md

+153<sub>10</sub> is an armstrong number in base 10, because: 153<sub>10</sub> = 1<sup>3</sup> + 5<sup>3</sup> + 3<sup>3</sup> = 1 + 125 + 27 = 154<sub>10</sub>
+154<sub>10</sub> is *not* an armstrong number in base 10, because: 154<sub>10</sub> &ne; 1<sup>3</sup> + 5<sup>3</sup> + 4<sup>3</sup> = 1 + 125 + 64 = 190<sub>10</sub>
+17<sub>10</sub> is an armstrong number in base 3, because: 17<sub>10</sub> = 122<sub>3</sub> = 1<sup>3</sup> + 2<sup>3</sup> + 2<sup>3</sup> = 1 + 8 + 8 = 17<sub>10</sub>
+18<sub>10</sub> is *not* an armstrong number in base 3, because: 18<sub>10</sub> = 200<sub>3</sub> &ne; 2<sup>3</sup> + 0<sup>3</sup> + 0<sup>3</sup> = 8 + 0 + 0 = 8<sub>10</sub>


Make sure all these lines are separated in the markdown rendered view.
They currently confusingly run into each other.

Insti · 2017-11-21T22:28:28Z

as the README states, Armstrong numbers come from recreational math.

The README doesn't actually state this.

Including more of the information from #1018 (comment) in the README would help.

We just ask them to compute a property which varies according to base. Testing only base 10 would both be less interesting for the student, and incomplete within the scope of the problem.

It is still necessary to do base conversion to get the digits to test though?

What do you think the core of this exercise is?

Insti · 2017-11-21T22:40:36Z

What makes it interesting is what makes it recreational: the fact that the representation changes the property.

I guess I just don't find it interesting. You're still just taking n digits and raising them to the power of n and summing them. How you get those n digits and what you want them to equal is arbitrary.

You could just as easily say the digits are "122" and you want them to equal 17.
No mention of bases required.

I suspect there is an interesting problem in here somewhere, but I don't think we've found it yet.

coriolinus · 2017-11-21T23:26:43Z

The README doesn't actually state this.

I apologize, you're right; that info is in the wikipedia page which describes Armstrong numbers.

You could just as easily say the digits are "122" and you want them to equal 17.
No mention of bases required.

I think this is the key issue. You're absolutely right that we could just say "sum the digits 122 and compare them to 17", and we'd have an exercise which might introduce students to iterating through characters of a string and string to int conversion. However, without context, that exercise is pointless and boring. In contrast, if the exercise talks about Armstrong numbers and links me to a wiki page, and I know that 17₃ is 122, then I'm perfectly happy solving it; that context is enough to interest me. Perhaps I'm unusually susceptible to mathy nerd-sniping? I don't think so, but I suppose few do.

I do see the point that we don't want to send students down a base-conversion rabbit hole, particularly if we expect this exercise to appear fairly early in the tracks. To that end, I propose that we limit the canonical data to using bases 8, 10, and 16. Most languages should have some equivalent to sprintf, str.format, or equivalent, which can handle the base conversion to these targets automatically; we might add a hints.md pointing students in that direction. Would that satisfy your need to minimize the base conversion portion?

coriolinus · 2017-11-21T23:38:51Z

To phrase it more concisely, the point of this exercise could be to teach intermediate use of the chosen language's string formatting system, using Armstrong numbers so that it doesn't come across as too contrived.

Insti · 2017-11-23T10:13:03Z

the point of this exercise could be to teach intermediate use of the chosen language's string formatting system, using Armstrong numbers so that it doesn't come across as too contrived.

In this case the description would need to be significantly rewritten to emphasise this.

BUT string formatting seems to be a rather implementation specific thing.
There are ways to solve this problem that don't involve using string formatting, and languages that do not include such string formatting tricks.
Exercism problems try to be language agnostic and not force a particular implementation.

But I think we've found the source of our differences.
As I understand it you would like the exercise to be about base conversion via string formatting and I would prefer it to be more focused on simply verifying Armstrong numbers.

coriolinus · 2017-11-23T10:36:01Z

@pheanex As the person submitting this exercise, it's only fair that you get to pick the direction to take it. If in your mind this is more interesting with the bases, you'll need to remove the base-3 tests and update the description to suggest using string formatting to the student. If you think it's more interesting as a pure Armstrong number verification exercise, you'll need to remove the non-base-10 tests and refactor to remove mention of the bases. I'm ok with either variant.

coriolinus · 2017-11-23T10:42:11Z

If it's decided to use the version with the bases, it might be desirable to put a phrase in the description similar to the following:

Challenge: implement this exercise without using sprintf, str.format, or your language's equivalent.

This suggests to the student that string formatting may be a simple way to implement the cases, without doing so explicitly. Simultaneously, it reminds them that there are other methods available for doing base conversion. This should serve the goals of maximizing student flexibility while hinting at a possible path for those who need help.

Insti · 2017-11-27T10:57:58Z

Aren't we driving this a little far with the reviews here?

We are nearly there. 😄

I am glad for all the reviews, but some seem a little too pedantic for my taste for an exercise of this magnitude ... at least compared to all the other already existing exercises.

It is important that new exercises meet the current standards for formatting and clarity.
Many of the existing were added before these standards were determined so perhaps do not achieve the levels expected of new exercises.

There are already a lot of exercises, why should we add a new ones?
If something is worth doing it's worth doing well.

Insti · 2017-11-27T11:48:22Z

exercises/armstrong-numbers/description.md

+9 is an Armstrong number, because 9 = 9^1 = 9
+10 is *not* an Armstrong number, because 10 != 1^2 + 0^2 = 2
+153 is an Armstrong number, because: 153 = 1^3 + 5^3 + 3^3 = 1 + 125 + 27 = 153
+154 is *not* an Armstrong number, because: 154 != 1^3 + 5^3 + 4^3 = 1 + 125 + 64 = 190


These line all still need some way to separate them in the rendered output, they currently run into each other in one paragraph.

see: https://github.com/pheanex/problem-specifications/blob/ff8b492d3b1863a7456c66f60fd7f646335745a9/exercises/armstrong-numbers/description.md

pheanex · 2017-11-28T20:56:37Z

Is there anything still preventing this PR getting merged?

Insti · 2017-11-29T12:57:17Z

This exercise is mostly fine. Thanks for all your work on it. ❤️

I'm not merging it because I still don't think the test case descriptions are good, and would like to see more logical and instructive test cases chosen.

But I'm not the only one here. There are many others with commit access who have different opinions. Maybe they will review and merge this.

It may also be that no-one has had a chance to review it yet, we're all volunteers here and do this in our spare time.

rpottsoh · 2017-11-29T13:25:48Z

exercises/armstrong-numbers/canonical-data.json

+      "expected": false
+    },
+    {
+      "description": "153 is an Armstrong number",


"description": "Three digit number that is an Armstrong number",

rpottsoh · 2017-11-29T13:26:47Z

exercises/armstrong-numbers/canonical-data.json

+      "expected": false
+    },
+    {
+      "description": "100 is not an Armstrong number",


"description"; "Three digit number that is not an Armstrong number",

rpottsoh · 2017-11-29T13:28:24Z

exercises/armstrong-numbers/canonical-data.json

+      "expected": true
+    },
+    {
+      "description": "370 is an Armstrong number",


This test is redundant. Previous test already tests for three digit numbers, unless there is some other relevance regarding 370.

rpottsoh · 2017-11-29T13:29:10Z

exercises/armstrong-numbers/canonical-data.json

+      "expected": true
+    },
+    {
+      "description": "9474 is an Armstrong number",


"description": "Four digit Armstrong number",

Perhaps a test of a non-Armstrong four digit number would be prudent here as well.

rpottsoh · 2017-11-29T13:31:12Z

exercises/armstrong-numbers/canonical-data.json

+      "expected": true
+    },
+    {
+      "description": "9926314 is not an Armstrong number",


"description": "Seven digit number that is not an Armstrong number",

rpottsoh · 2017-11-29T13:48:20Z

exercises/armstrong-numbers/canonical-data.json

+      "expected": false
+    },
+    {
+      "description": "9926315 is an Armstrong number",


"description": "Seven digit number that is an Armstrong number",

rpottsoh · 2017-11-29T13:54:44Z

@Insti said:

I'm not merging it because I still don't think the test case descriptions are good

I agree. I have taken a quick crack at the case descriptions...

rpottsoh · 2017-11-29T21:16:24Z

exercises/armstrong-numbers/canonical-data.json

+{
+  "exercise": "armstrong-numbers",
+  "version": "1.0.0",
+  "description": "There are no 2 digit Armstrong numbers",


I don't think this line is needed. It is also causing Travis CI to fail as it violates the canonical-schema.json

~~This should be the description for a test of a 2 digit number.~~
It is, the version I was looking at was out of date.

It is, the version I was looking at was out of date.

Wouldn't it still be 1.0.0?

The canonical-data version key is correct.

It was an out of date "commit" I was looking at.
Confusing word choice by me there sorry.

rpottsoh · 2017-11-29T21:38:23Z

exercises/armstrong-numbers/canonical-data.json

+      "expected": true
+    },
+    {
+      "description": "9 is an Armstrong number",


Appears as though the first three tests could be consolidated into a single test since all single digit numbers are Armstrong numbers.

"description": "Single digit numbers are Armstrong numbers",

My wording may need some work. I don't want to imply that all single digit number need to be checked.

rpottsoh

Thanks for making these changes. Now lets allow some time for others to weigh in.

pheanex · 2017-11-30T16:39:15Z

@Insti is there anything concrete (maybe on the test-cases?) i can do to get your approval as well?

ErikSchierboom

I have just one small request. For consistency, could you order the test cases such that they always use the same "is"/"is not" order? For example, we now have "Three digit number that is not an Armstrong number" before "Three digit number that is an Armstrong number" and "Four digit number that is an Armstrong number" before "Four digit number that is not an Armstrong number". I think it would make sense to consistently use the same ordering. My preference would be to start with the "is a" case, followed by the "is not" case.

ErikSchierboom

Thanks for the hard work!

rpottsoh · 2017-12-04T17:41:25Z

I am going to go ahead and Squash & Merge since everyone is holding their peace... If something is discovered later, a new issue/PR can be opened.

Thanks @pheanex for all your time and energy and helping to improve Exercism!

Insti · 2017-12-04T20:20:01Z

Sorry @pheanex, I've been busy the last few days.

This looks fine. Thanks for all your work on it ❤️

add new exercise "armstrong-numbers"

0914768

coriolinus suggested changes Nov 21, 2017

View reviewed changes

ErikSchierboom reviewed Nov 21, 2017

View reviewed changes

Insti reviewed Nov 21, 2017

View reviewed changes

pheanex added 5 commits November 21, 2017 19:34

remove duplicate test

f6e8996

remove empty tag in description

3419018

remove html spaces from description

b3edac1

Simplify the "blurb" in metadata

bebc995

remove 111 test-case

874014e

ErikSchierboom reviewed Nov 21, 2017

View reviewed changes

Add missing space in canonical data comment

1210c00

Insti reviewed Nov 21, 2017

View reviewed changes

Capitalize all armstrong-refernces

28eba60

remove "base"-ness

6f1c4f4

rephrase task description

ff8b492

Insti reviewed Nov 27, 2017

View reviewed changes

update description and fix broken rendering

2dd4d68

Insti changed the title ~~add new exercise "armstrong-numbers"~~ armstrong-numbers: new exercise Nov 27, 2017

rpottsoh reviewed Nov 29, 2017

View reviewed changes

pheanex added 2 commits November 29, 2017 21:52

add description for non existant armstrong numbers

a5ce348

update test descriptions

9c77324

rpottsoh reviewed Nov 29, 2017

View reviewed changes

remove unnecessary/wrong comment

9fadff0

rpottsoh reviewed Nov 29, 2017

View reviewed changes

Merge single digit number-tests into one test

9959df6

rpottsoh approved these changes Nov 29, 2017

View reviewed changes

ErikSchierboom requested changes Dec 1, 2017

View reviewed changes

change order of tests (consistent first "is an" followed by "is not an")

041eba5

ErikSchierboom approved these changes Dec 1, 2017

View reviewed changes

rpottsoh merged commit 5c3baae into exercism:master Dec 4, 2017

rpottsoh mentioned this pull request Dec 5, 2017

armstrong-numbers: New exercise exercism/delphi#190

Merged

sjwarner-bp mentioned this pull request Dec 5, 2017

armstrong-numbers: add to track exercism/java#1072

Closed

		@@ -0,0 +1,4 @@
		---
		blurb: "Given two decimal parameters (number and base) return True if the number 'number' is an armstrong number with respect to the numeral system with base 'base'."

armstrong-numbers: new exercise #1018

armstrong-numbers: new exercise #1018

Conversation

pheanex commented Nov 20, 2017 • edited by coriolinus Loading

coriolinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pheanex Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Insti commented Nov 21, 2017

Insti commented Nov 21, 2017

coriolinus commented Nov 21, 2017 • edited Loading

coriolinus commented Nov 21, 2017

Insti commented Nov 21, 2017

coriolinus commented Nov 21, 2017 • edited Loading

pheanex commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Insti commented Nov 21, 2017

Insti commented Nov 21, 2017

coriolinus commented Nov 21, 2017

coriolinus commented Nov 21, 2017

Insti commented Nov 23, 2017

coriolinus commented Nov 23, 2017

coriolinus commented Nov 23, 2017

Insti commented Nov 27, 2017

Choose a reason for hiding this comment

pheanex commented Nov 28, 2017

Insti commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpottsoh commented Nov 29, 2017

Choose a reason for hiding this comment

Insti Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

rpottsoh Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpottsoh left a comment

Choose a reason for hiding this comment

pheanex commented Nov 30, 2017

ErikSchierboom left a comment

Choose a reason for hiding this comment

ErikSchierboom left a comment

Choose a reason for hiding this comment

rpottsoh commented Dec 4, 2017

Insti commented Dec 4, 2017

pheanex commented Nov 20, 2017 •

edited by coriolinus

Loading

pheanex Nov 21, 2017 •

edited

Loading

coriolinus commented Nov 21, 2017 •

edited

Loading

coriolinus commented Nov 21, 2017 •

edited

Loading

Insti Nov 30, 2017 •

edited

Loading

rpottsoh Nov 30, 2017 •

edited

Loading