6
\$\begingroup\$

I have a class, Word, that has a String representing a word and an Enum representing the difficulty of the word. Then I have another class, Vocab, that has an ArrayList of Word-objects. I want to sort my ArrayList by either difficulty or by the alphabetical order of the word.

I figured I should use a Comparator, and here is how I did it. All classes are in the same package:

Diff:

public enum Diff {
   EASY, MEDIUM, HARD;
}

Word:

public class Word {
   private String word;
   private Diff diff;

   public Word(String word, Diff diff) {
      this.word = word;
      this.diff = diff;
   }

   public String getWord() {
      return word;
   }

   public void setWord(String word) {
      this.word = word;
   }

   public Diff getDiff() {
      return diff;
   }

   public void setDiff(Diff diff) {
      this.diff = diff;
   }

   @Override
   public String toString() {
      String string = getWord() + ", " + getDiff();
      return string;
   }
}

Vocab:

public class Vocab {
   private ArrayList<Word> words;

   public Vocab() {
      words = new ArrayList<>();
   }

   public void sortByPrio() {
      Collections.sort(words, prioCompare);
   }

   public void sortByName() {
      Collections.sort(words, nameCompare);
   }

   private Comparator<Word> prioCompare = new Comparator<Word>() {
      @Override
      public int compare(Word w1, Word w2) {
         return (w1.getDiff().ordinal() - w2.getDiff().ordinal());
      }
   };

   private Comparator<Word> nameCompare = new Comparator<Word>() {
      @Override
      public int compare(Word w1, Word w2) {
         return (w1.getWord().compareToIgnoreCase(w2.getWord()));
      }
   };

   public void addWord(Word newWord) {
      words.add(newWord);
   }

   @Override
   public String toString() {
      String string = words.toString();
      return string;
   }
}

Main:

public class Main {

   public static void main(String[] args) {
      Vocab myWords = new Vocab();
      myWords.addWord(new Word("Apple", Diff.EASY));
      myWords.addWord(new Word("Besmirch", Diff.MEDIUM));
      myWords.addWord(new Word("Pulchritudinous", Diff.HARD));
      myWords.addWord(new Word("Dog", Diff.EASY));

      System.out.println(myWords);
      myWords.sortByPrio();
      System.out.println(myWords);
   }
}

Is anything inherently wrong with my code above, regarding my implementation of the Comparator? If so, what should I improve? Also, I haven't redefined equals, which I usually do when I implement Comparable. Should I redefine it now as well? I am relatively new to Java and programing in general, so very advanced tips is not necessary.

\$\endgroup\$

3 Answers 3

6
\$\begingroup\$

Don't write a custom comparator implementation when Comparator.comparing exists. Also, you need to reduce the amount of mutation in your code by a significant amount:

package com.stackexchange;

import java.util.Comparator;
import java.util.List;

public class Main {
    public enum Diff { EASY, MEDIUM, HARD }

    public record Word(
        String word, Diff difficulty
    ) {
        @Override
        public String toString() {
            return "%s, %s".formatted(word, difficulty);
        }
    }

    public static class Vocab {
        private final List<Word> words;
        public final static Comparator<Word>
            byDifficulty = Comparator.comparing(Word::difficulty),
            byName = Comparator.comparing(Word::word);

        public Vocab(List<Word> words) { this.words = words; }

        public Vocab sortedByDifficulty() {
            return new Vocab(
                words.stream().sorted(byDifficulty).toList()
            );
        }

        public Vocab sortedByName() {
            return new Vocab(
                words.stream().sorted(byName).toList()
            );
        }

        @Override
        public String toString() {
            return words.toString();
        }
    }

    public static void main(String[] args) {
        Vocab myWords = new Vocab(List.of(
            new Word("Apple", Diff.EASY),
            new Word("Besmirch", Diff.MEDIUM),
            new Word("Pulchritudinous", Diff.HARD),
            new Word("Dog", Diff.EASY)
        ));

        System.out.println(myWords);
        System.out.println(myWords.sortedByDifficulty());
    }
}
[Apple, EASY, Besmirch, MEDIUM, Pulchritudinous, HARD, Dog, EASY]
[Apple, EASY, Dog, EASY, Besmirch, MEDIUM, Pulchritudinous, HARD]
\$\endgroup\$
4
  • \$\begingroup\$ @Reinderien in the sortedByName method, is it ok to use Collections.sort(words, byName) and return void instead? \$\endgroup\$ Apr 3 at 15:23
  • \$\begingroup\$ Not really, no. The whole point of using stream().sorted is to avoid sorting in place, or indeed mutating anything at all. \$\endgroup\$
    – Reinderien
    Apr 3 at 15:25
  • \$\begingroup\$ Is there a way I can use stream().sorted with an ArrayList instead of a List? \$\endgroup\$ Apr 3 at 15:32
  • \$\begingroup\$ Why would you want to? toList produces an immutable list, and that's how it should be. \$\endgroup\$
    – Reinderien
    Apr 3 at 15:35
4
\$\begingroup\$

Is this a correct implementation of Comparator?

private Comparator<Word> prioCompare = new Comparator<Word>() {
    @Override
    public int compare(Word w1, Word w2) {
        return (w1.getDiff().ordinal() - w2.getDiff().ordinal());
    }
};

private Comparator<Word> nameCompare = new Comparator<Word>() {
    @Override
    public int compare(Word w1, Word w2) {
        return (w1.getWord().compareToIgnoreCase(w2.getWord()));
    }
};

Yes, it is.

I have an additional question that I want to address. Is this the proper way to hold reference of anonymous inner classes?

In this case, no, it isn't. Since the comparators are the same regardless of the Vocab instance using them the variables prioCompare and nameCompare should be static (and optionally final) otherwise two new classes implementing Comparator interface are created every time Vocab class is instantiated.

\$\endgroup\$
1
  • \$\begingroup\$ A yes/no answer to "is this correct" is useless here, since in essence correctness is defined by whether the code produces the correct answer and the CodeReview platform rules already state that the code must work before it can be posted. What we discuss here are the nuances in style. \$\endgroup\$ Apr 3 at 4:33
1
\$\begingroup\$

In computing jargon, "diff" means an application that can show the difference between two files (or the output of that application). In general, you are using abbreviations too much and they make our code hard to understand. Use names that describe the meaning of the classes exactly. If the enumeration represents spelling difficulty, name the enum SpellingDifficulty. Likewise Vocab sould just be Vocabulary.

Whoever it was that coined the phrase "There are only two hard things in Computer Science: cache invalidation and naming things" was completely 50% wrong. Naming is really easy. You just think "what does this thing represent" and then write that down. "Are there more than one thing in the universe that matches the name I chose?" Then be more specific in the name. If that was too hard, then you most likely have violated single responsibility principle and need to fix the code.

The comparator you use for sorting by difficulty is named "priority". That is a really weird choice that smells like something needing to be renamed. This is why you should write JavaDocs that describe the reason why each class exists. When there is a naming mistake like this the comments explain the things that got lost.

Comparator and equals are not tightly coupled. The reason why comparator exists is to provide different kinds of ways to compare objects. And a Comparator does not have to adhere to the strict restrictions that the equals method has. The only rule is that you must implement equals if you implement the hashCode and vice versa.

Your vocabulary is just a set of words. Do you actually need a class to represent it? It seems like you could implement the comparators as independent classes and use the standard collection classes with streams, common sorting utilities etc. When doing so you let the user decide what is the best data structure to hold the vocabulary and when to create a new collection for the sorted data vs. when to sort the collection in place. Of course, you may very well have good reasons for needing the Vocabulary class, but that is not clearly evident from the code snippet.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.