1
\$\begingroup\$

I've revised the code in the light of comments (the initial post was here). I've initially just redone the logic to get a random word for the category chosen by the player. I'd appreciate any feedback on that, then I'll revise the rest of it. Main points addressed: separate out GUI code and use streams. I'm sure there's still lots to be done to improve it. You can run it at replit.com.

Game.java

public class Game {

    public static void main (String[] args){
            Word word = new Word();
            String randomWord = word.getWord();
            System.out.println(randomWord);
    }
}

Word.java

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Random;
import java.util.List;
import java.util.stream.Stream;
import static java.util.stream.Collectors.toList;

    public class Word {
    
        private static final String WORD_LIST_DIRECTORY = "wordLists/";
        private static final int MAX_WORD_LENGTH = 20;
        private static final int MIN_WORD_LENGTH = 3;
    
        String getWord(){
            String category = category();
            List<String> words = wordsInCategory(category);
            Random generateRandom = new Random();
            int randomIndex = generateRandom.nextInt(words.size());
            String word = words.get(randomIndex);
            return word.trim();
        }
    
        private String category() {
            List<String> filenames = null;
            Path path = Paths.get(WORD_LIST_DIRECTORY);
            try {
                Stream<Path> stream = Files.list(path);
                filenames = stream.filter(Files::isRegularFile)
                                  .map(Path::getFileName)
                                  .map(Path::toString)
                                  .collect(toList());
            } catch (IOException e) {
                ErrorReporter.missingCategories();
            }
            CategoryChooser chooser = new CategoryChooser();
            String choice = chooser.chooseCategory(filenames);
            return choice;
        }
    
        private List<String> wordsInCategory(String category){
            List<String> words;
            Path path = Paths.get(WORD_LIST_DIRECTORY +category);
            try  {
                Stream<String> stream = Files.lines(path);
                words = stream.map(String::toUpperCase)
                              .filter(str -> (str.length() <= MAX_WORD_LENGTH))
                              .filter(str -> (str.length() >= MIN_WORD_LENGTH))
                              .collect(toList());
            } catch (IOException e) {
                ErrorReporter.missingCategory();
                words=loadDefaultWords();
            }
            if (words.size()<1){
                ErrorReporter.missingWords();
                words=loadDefaultWords();
            }
            return words;
        }
        private List<String> loadDefaultWords(){
            return List.of("FOXGLOVE", "MICROWAVE","ZOMBIE","PUPPY","RHUBARB","DWARF","BICYCLE",
                "BUZZARD","OWL","CHAFFINCH","KIRIBATI","LIECHTENSTEIN","MOZAMBIQUE");
        }
    
    }

CategoryChooser.java

import javax.swing.*;
import java.util.List;
import java.util.Objects;

public class CategoryChooser {

    private String category;

    String chooseCategory(List<String> categoryNames){
        String[] categories = categoryNames.toArray(new String[0]);
        category = categories[0];
        JComboBox<String> jComboBox = new JComboBox<>(categories);
        jComboBox.addActionListener(e -> category = Objects.requireNonNull(jComboBox.getSelectedItem()).toString());
        JOptionPane.showMessageDialog(null, jComboBox, "Word list",
                JOptionPane.QUESTION_MESSAGE);
        return category;
    }
}

ErrorReporter.java

import javax.swing.*;

public class ErrorReporter {

    static void missingCategories(){
        JOptionPane.showMessageDialog(null, "No word lists found, using default words instead");
    }

    static void missingCategory(){
        JOptionPane.showMessageDialog(null, "Word list missing, using default words instead");
    }

    static void missingWords(){
        JOptionPane.showMessageDialog(null, "No words found, using default words instead");
    }

}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

your code does look good as far as i can tell... you are asking about OOP and Streams so let's look at them first:

Streams:

1) Filter

you should create a custom filter (aka Predicate) for your word-length check

Predicate<String> wordLengthPredicate = s -> s.length() <= MAX_WORD_LENGTH && s.length() >= MIN_WORD_LENGTH;
stream.stream().filter(wordLengthPredicate)....; //this is just a stub

2) Mapping

same applies here, instead of adding mapping after mapping, define a proper Function that does the job (by the way, why do you do the mapping twice where you could do as a one-statement? (path.getFileName().toString()) ?

Function<Path, String> toCategory = path -> path.getFileName().toString();
stream.stream().map(toCategory)...; //this is just a stub

3) why?

so why would you separate this during your streams? because you violate the Integration Operation Segregation Principle (IOSP)!

IOSP calls for a clear separation:

  • Either a method contains exclusively logic, meaning transformations, control structures or API invocations. Then it’s called an Operation.

  • Or a method does not contain any logic but exclusively calls other methods within its code basis. Then it’s called Integration.

OOP class word

1) too much responsibilites you have very much responsibility in your class:

  • loading categories
  • loading words of categories
  • select a random word of the list
  • validate file input

that violates the Single Responsibility Principle. take out (at least) the responsibility for loading categories. And best: take out the responsibility for validation the input (see section below)!

2) naming

The class Word has a weird naming, i had expected at first glance that this class would provide a Word Model. Give it a proper name (WordProvider or something similar).

the method category() also lacks the verb, should be loadCategory()

the method getWord() should tell more (Tell - don't ask): getRandomWord()

3) strong coupling

you have a strong coupling between category and word - you should get rid of that.

public static void main (String[] args){
    Category category = ... //i hope this gives you an idea of how to release the coupling
    Word word = new Word(category);
    String randomWord = word.getWord();
    System.out.println(randomWord);
}

OOP class CategoryChooser

this class is a GUI class - i was surprised by that. a proper name had made this class less surprising (Tell, don't ask). Rename it to CategroyChooserDialog (or rename the method chooseCategory() to showChooseCategoryDialog()).

minor issue: you can turnprivate String category into a method variable

OOP: Sanitize your input

make a class that is responsible for that. your file input is valid, when

  • it comes from a defines source,
  • when all words match the size constraint and
  • when all words are upper case.
  • (totally missing in your code: Encoding)

provide either files that match these requirements and don't check the content or create a Input-Validator, that is responsible for that.

OOP Utility classes

you mix up static and non-static context for your utility classes:

  • ErrorReporter.missingCategories() //static
  • new CategoryChooser().chooseCategory(filenames); //non static
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thanks for all your points @MartinFrank. I’ve tried to address them all. The revised code is posted here: codereview.stackexchange.com/questions/273399/…. Additionally: You suggested renaming the class implementing the JOptionPane for choosing the category to make it clear it was a GUI component. I’ve done that, but also made it a subclass of JOptionPane. On sanitizing the input - haven’t succeeded in checking the character encoding for the word lists. Currently, if I feed it a binary file, it crashes inelegantly! Any pointers? \$\endgroup\$
    – scripta
    Commented Jan 26, 2022 at 10:53
  • \$\begingroup\$ i'll have a look, i am interesed in your progress! \$\endgroup\$ Commented Jan 26, 2022 at 12:11

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.