SumOfMultiples: Add to Track

[ProgrammingFuad]

I have written a solution to the Sum of Multiples problem described here:
https://github.com/exercism/x-common/blob/master/exercises/sum-of-multiples/description.md

First Commit and I am super pumped!
I've tested extensively and used all tests cases listed here:
https://github.com/exercism/x-common/blob/master/exercises/sum-of-multiples/canonical-data.json

A few questions I have are:
1.What is the convention for naming test cases? This is my first time J-unit testing, so I want to make sure Im doing it "right". I couldn't figure out what to name my methods for the the test case besides test1, test2, test3, etc.. so is there a correct naming convention I should be using?

2. Is my file/folder structure correct? I've tested everything to the best of my ability and tried to mimic the file structure from the Hello-World exercise. I copy pasted the gradle doc as well, which I am feeling iffy about.

3. Continuous Integration is spitting on a message saying "All checks have failed". Whats going on?

Im super excited to hear your response!

Thanks.

[matthewmorgan]

@ProgrammingFuad thanks for your contribution!

I'd be happy to help you resolve the issues with this pull request.

For starters, if you look at the output of the CI, line 216: https://travis-ci.org/exercism/xjava/builds/177393056 You will see why the build failed. There's a config file that must be updated for your new exercise.

And yes, you're right to bring up the question of naming the tests-- we would like the tests to have names that indicate what is being tested, because these names provide guidance for the users when they're working on their solutions.

To get this pull request ready for prime time, I would say we need to achieve the following:

1. Update the config.json file as mentioned above.
2. Update the names of the tests. At first glance it appears you've used the canonical-data.json in x-common as a guide to what to test, which is great. Now you just need to update the names. Take a look at the specfile from the ecmascript track for an idea of how to name the tests.
3. There's some minor cleanup that should be done to the example code itself-- normalizing whitespace and removing System.out.println() calls would be a start. We don't adhere to a strict style guide here, but consistent indentation and whitespace around = , > etc is generally preferred.
4. Finally, we like to see all the commits squashed to one. When everything else is ready to go I can help you with that.

Please feel free to hit me up with any questions!

[ProgrammingFuad]

Alright I am ready for some help making a single Pull Request! I tried my own attempt at it and have obviously failed (Check above, you will notice I made a reference to my previous pull and posted here yet again).

I have made all requested changed to the best of my ability. Please let me know if there is anything else I can correct!

thank you for your input I appreciate it.

[matthewmorgan]

OK Fuad,

Nice work so far! I especially appreciate the fact that you've annotated all but the first test with @Ignore. That's something I overlooked but we like to have that. Well done.

I do think the new test names are an improvement, but I think there's one more change that should be made. It would be nice to have the factors mentioned when they are specified since they are not outputted by the test runner in a visible way, EG, from the Ecmascript suite:

xit('[43, 47] to 10000', () => {
    expect(SumOfMultiples([43, 47]).to(10000)).toBe(2203160);
  });

In Java this could be written as:

public void testSumOfMultiplesOf43and47UpToTenThousand() {  		
  SumOfMultiples mySum = new SumOfMultiples();
  int[] set = {43, 47};
  int output = mySum.Sum(10000, set);
  assertEquals(2203160, output);
}

Also note the formatting in the above test method. Try this site for an easy tool that will clean up your whitespace for you: http://codebeautify.org/javaviewer

As far as getting everything into one commit: It looks to me like you've created a new branch with your changes. You can merge that branch back into this one, and then I can show you how to squash them in to one commit.

First do this:

git checkout ProgrammingFuad
git merge programFuad

The above commands will merge the changes from your latest branch programFuad back into this one.

If you could start by updating the test names as above, and merging your new branch back into this one, I can then show you how to combine everything into one commit.

Cheers!

[ProgrammingFuad]

Alright Mr. Mathew Im gonna let you know right now that I've learned a valuable lesson.
Which is to stay away from Githubs Desktop Application and start using the terminal A.S.A.P.
I pressed update too many times on my Github Desktop application which resulted in all those extra commits you will see above this comment.

On the bright side I somehow managed to merge my new branch "programFuad" with the changes you listed above.

Im Sorry! How should we continue from here?

[matthewmorgan]

Hi Fuad,

No worries about the number of commits. They're cheap! We'll get this just right and consolidate the commits after that.

Taking a closer look at your folder structure there are a few things still to be changed:

1. Your folder (and exercise) name should be sum-of-multiples, in keeping with the naming of the other exercises. In Java, your class and classfile will still be camel-cased SumOfMultiples.
2. The folder structure needs to be changed a bit:

sum-of-multiples/
├── build.gradle
└── src
    ├── example
    │   └── java
    │       └── SumOfMultiples.java
    ├── main
    │   └── java
    │       └── SumOfMultiples.java
    └── test
        └── SumOfMultiplesTest.java

3. Finally, your exercise is not currently getting built by gradle. This is because it needs to be added in one more place: settings.gradle.

Please let me know if you have any difficulty or questions, and when this is done I'll take another look.

Thanks!

[ProgrammingFuad]

Alrighty.
I've updated the Gradle file to now include the exercise.
Ive also modified the folder structure and changed the folder names.

Im getting excited just thinking about asking this.

Are we ready to commit??!?!

P.S. All checks have finally passed.

[matthewmorgan]

Nice job Fuad! 🎆

So, you've cleaned up the code, the test names, and your folder structure, and you're passing the CI journey test.

The final step is to consolidate your commits to one commit. This is going to be a bit tricky because of the commit history on this PR, but here's how I suggest we proceed. By the way, if this is something you'd rather not take on, I'm happy to do this final step for you.

First you need to remove a couple of commits that are not related to your PR:

git rebase -i 5f83bc9

This will open an editor that should let you remove two lines:

78af6bb - minesweeper: add to track (2 days ago) <Stuart Kent>
d15f926 - minesweeper: add to track (2 days ago) <Stuart Kent>

Check your git log, and you should see something like this:

* 5f045fe - (HEAD -> ProgrammingFuad-ProgrammingFuad) Update config.json (3 seconds ago) <Fuad Mohamoud>
* 9e03a33 - Updates (3 seconds ago) <Fuad>
* a17176c - Update config.json (3 seconds ago) <Fuad Mohamoud>
* 047e6c1 - Updates (3 seconds ago) <Fuad>
* bed79db - Modified Solution to Pull #179 (4 seconds ago) <Fuad>
* c6b6388 - Trailing Spaces (3 days ago) <Fuad Mohamoud>
* 3a4d035 - Trailing Spaces (3 days ago) <Fuad Mohamoud>
* 5920215 - Sum of Multiples (3 days ago) <Fuad>
* a1cbd0d - err (3 days ago) <Fuad>
* 0fe4b3e - Sum of Multiples Solution (3 days ago) <Fuad>
* 5f83bc9 - Merge pull request #171 from stkent/queen-attack (9 days ago) <John Ryan>

now you can do the final step, which is to consolidate all your comits to one:

git reset --soft 5f83bc9 && git commit -am 'Add Sum of Multiples exercise'

You should now have everything in one commit.

My suggestion-- if you have any trouble with this, or feel uncomfortable doing it, just ask and I can do it for you. You might want to make a copy of your entire xjava directory in case you have any trouble and need a backup copy.

If you are successful, your git log should look like this:

* 883033b - (HEAD -> ProgrammingFuad-ProgrammingFuad) Add Sum of Multiples exercise to track (4 minutes ago) <Matthew Morgan>
* 5f83bc9 - Merge pull request #171 from stkent/queen-attack (9 days ago) <John Ryan>

Of course, the top line will show a commit by YOU, not ME :)

Once things are in the state where you have just one commit, you will need to force push the new commit to your branch:

git push -f

Please don't do the above step if you're not confident that you have correctly consolidated your commits. Just ask for help, I'm happy to do it!

[ProgrammingFuad]

I've just been glazing over my Github Desktop Application trying to figure this out. Im sorry Mathew I dont want to make another mistake and cause more work haha! Thank you for your thorough explanation. Please consolidate the pull request for me.

I'm definitely switching to the command Line version after this crash and burn Github experience.

[matthewmorgan]

OK Fuad,

I've consolidated the commits and merged them to master! The bad news is that it means I will need to close this PR since Github doesn't recognize that the change has been merged (because the commit history has been altered from the history on the PR.) The good news is that your commit shows up in the history for this track!

Nice work, we appreciate your contribution!