Skaleba assignment5 #26
Conversation
sophie-kaleba
requested review from
GabijaBern,
andrada12,
andreeapetcu and
haticemelikeecevit
andreeapetcu
left a comment
andreeapetcu
left a comment
There was a problem hiding this comment.
The comments are clear and useful in understanding the code 👍 With some minor exceptions where I suggested improvements, your code is nicely structured
|
|
||
| public String[] dictionary; | ||
| public Graph unorderedAlphabet; | ||
| public enum VertexState {WHITE, GREY, BLACK}; |
There was a problem hiding this comment.
I like the idea of using states instead of keeping an agenda of the visited vertices :) It would be great if you could add comments on what each state means (I assume WHITE is used for an unvisited vertex, GREY for the vertex that is currently being visited and BLACK for a visited vertex)
| * it will iterate over both of the words and will add the characters to the alphabet. | ||
| * @return the alphabet built from the dictionary | ||
| */ | ||
| public List<Character> build() { |
There was a problem hiding this comment.
NIT : I believe that buildAlphabet or findAlphabet would be more suggestive method names
| */ | ||
| public void compareWords(String word1, String word2) { | ||
| if (word1.isEmpty() && word2.isEmpty()) { | ||
| throw new IllegalArgumentException("one of the words is empty"); |
There was a problem hiding this comment.
Minor bug: if you want your method to throw an exception only if one of the words is empty you should use word1.isEmpty() || word2.isEmpty()
| else {longestWord = 2;} | ||
|
|
||
| if (longestWord == 1) this.browseLastLettersOfLongest(word1, sizeWord1, i); | ||
| else if (longestWord == 2) this.browseLastLettersOfLongest(word2, sizeWord2, i); |
There was a problem hiding this comment.
One possible style change :
if (sizeWord1 > sizeWord2) {
this.browseLastLettersOfLongest(word1, sizeWord1, i);
} else {
this.browseLastLettersOfLongest(word2, sizeWord2, i);
}
| * @param c1 - the character coming from the first word | ||
| * @param c2 - the character coming from the second word | ||
| * @param firstDifference - true if it is the first time one encounters a difference in characters btwn the 2 words | ||
| */ |
There was a problem hiding this comment.
You should also add @return to make it easier to understand the return values of the method. I am a bit confused about this method's use in the if statement on line 128.
| Map<Character, VertexState> marked = new LinkedHashMap<Character, VertexState>(); | ||
|
|
||
| for (Character c : unsortedAlphabet.getContent().keySet()) { | ||
| marked.put(c, VertexState.WHITE); |
There was a problem hiding this comment.
Nice, you have your own style with the states 😄
| */ | ||
| public void compareWords(String word1, String word2) { | ||
| if (word1.isEmpty() && word2.isEmpty()) { | ||
| throw new IllegalArgumentException("one of the words is empty"); |
| int longestWord; | ||
| int i = 0; | ||
|
|
||
| for (; i < Math.min(sizeWord1, sizeWord2); i++) { |
There was a problem hiding this comment.
Nice use of Math class.
No description provided.