isogram : add to track

[Kai-Shiro]

I'll implement the isogram exercise
https://github.com/exercism/x-common/tree/master/exercises/isogram

[jtigger]

Renamed to make clear the status of this PR. @Kai-Shiro, when you're ready for a review, please rename it back to bookstore : add to track or the like. Thanks!

[matthewmorgan]

You can just use the diamond operator for your HashSet declaration like this:

Set<Character> charset = new HashSet<>();

[matthewmorgan]

In Java, the convention is to use camelCase, as in newWord.

[matthewmorgan]

If you like, you could replace this with:

String newWord = "";
for (String word: words){
    newWord += word;
}

No need for the index variable.

Or, for some extra-delicious Java 8:

public static String concat(String[] words){
    return stream(words).collect(joining());
}

To use the Streams version you'll need to import:

import static java.util.Arrays.stream;
import static java.util.stream.Collectors.joining;

[matthewmorgan]

Usually we like to be explicit with our curly braces, even in short blocks like this.

[matthewmorgan]

@Kai-Shiro thanks for doing this! We really appreciate the work.

in the spirit of challenging each other to produce the best possible product, I have made a couple of suggestions that are primarily stylistic. I also threw out a couple of alternative implementations that I thought might be interesting to you. Please feel free to use them, or not, as you prefer.

Finally, there's a merge conflict with config.json that was created when we merged some older PRs. Would you mind fixing that, or letting me know if you would like help doing it?

Thanks again.

[Kai-Shiro]

@matthewmorgan Thanks for the advices! I have applied some changes to the code and i think i have fixed the merge conflict with config.json

[matthewmorgan]

Nice work @Kai-Shiro ! This implementation is looking much cleaner-- concise, yet still easy to read.

If you're up for it, I would like to suggest a couple more changes.

1. We're trying to move away from static methods for problems like this. Please see the discussion about the Pangram exercise here: pangram: add starter implementation and make object-based #344 Two things to really pay attention to in the above discussion are the conversation around the semantics of the class and method names, and the preference for instance methods over class methods.

2. It doesn't appear that your concat method needs to be public. Since it's only used internally by the Isogram class, you can (and should) make it private.

3. Small nit: the charset variable should be camelCased.

Please let me know if you have any questions, or if you would like help with any of the details.

[Kai-Shiro]

@matthewmorgan I have applied some changes.

• There is no longer statics methods
• I change charset to charSet
• I change the code to use instance methods

[matthewmorgan]

@Kai-Shiro this is looking great. I have some minor changes that to request, otherwise bien fait!

[matthewmorgan]

You have a typo here, should be:

package example;

[Kai-Shiro]

Oops, french mistake!

[matthewmorgan]

I am thinking that the semantics are very close on this class, but not quite right. Think of it this way: what is the purpose of this class? What do the methods do? What if, for example:

IsogramChecker isogramChecker = new IsogramChecker();
String word = "isogram";
boolean result = isogramChecker.isIsogram(word);

[matthewmorgan]

You'll want to annotate all tests except the first one with @Ignore, so the users can explore their solution using each test as a goalpost.

[matthewmorgan]

Nicely done!