Skip to content

armstrong-numbers: add to track #1073

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 7 commits into from
Dec 20, 2017
Merged

Conversation

sjwarner-bp
Copy link
Contributor

As per #1072


Reviewer Resources:

Track Policies

@sjwarner-bp sjwarner-bp changed the title [WIP] armstrong-numbers: add to track armstrong-numbers: add to track Dec 5, 2017
Copy link
Contributor

@FridaTveit FridaTveit left a comment

Choose a reason for hiding this comment

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

This looks great @sjwarner-bp! I've made a couple of small comments but it's pretty much ready to merge :)

Sorry it took so long to review! I completely missed that it was ready for review! Will try to be quicker next time :)

int input = 5;
boolean expected = true;

assertEquals(expected, armstrongNumbers.isArmstrongNumber(input));
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use assertTrue and assertFalse instead of assertEquals for these tests :)

"integers",
"mathematics"
],
"unlocked_by": null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be unlocked by some other exercise? Or do you think it should be always accessible? I think either is fine :)

@sjwarner-bp
Copy link
Contributor Author

Don't worry about it @FridaTveit, I have been a little busy too! 🙂

I'll get on those changes in a moment - however because the exercise is quite an easy one, and one of the first arithmetic-based exercises that it should remain always accessible 🙂

@@ -0,0 +1,9 @@
class ArmstrongNumbers {

boolean isArmstrongNumber(int i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, just one thing I missed last time; i is normally only used as a variable name for an index. Maybe numberToCheck, possibleArmstrongNumber or inputNumber or somthing might be more descriptive? :)

@sjwarner-bp
Copy link
Contributor Author

Sorry this took so long @FridaTveit - I have been studying for my OCA!

Copy link
Contributor

@FridaTveit FridaTveit left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @sjwarner-bp! :)

@FridaTveit FridaTveit merged commit 233fd87 into exercism:master Dec 20, 2017
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