2
\$\begingroup\$

I've written two classes that I use to create and manage a high score list. I plan to use this code for my own, larger project. I would like to ask if there is something to complain about my code. Were the language tools and the libraries of the language used meaningfully? Does my implementation meet the requirements of good, object-oriented design? Is the code easy to read?

ScoreList

import java.util.List;
import java.util.ArrayList;
import java.util.Comparator;
import java.io.Serializable;
import java.io.ObjectOutputStream;
import java.io.FileOutputStream;
import java.io.IOException;

public class ScoreList implements Serializable {
    private String description;
    private List<Score> scores;
    private int numberOfEntries;
    private String saveName;
    private boolean autoSave;

    // should be used, if the user wants to store his scores on disk
    public ScoreList(String description, int numberOfEntries, String saveName, boolean autoSave) {
        this.description = description;
        scores = new ArrayList<>();
        this.numberOfEntries = numberOfEntries;
        this.saveName = saveName;
        this.autoSave = autoSave;
    }

    // should be used, if the scores dont have to be saved to a file
    public ScoreList(String description, int numberOfEntries) {
        this.description = description;
        this.numberOfEntries = numberOfEntries;
        saveName = null;
        autoSave = false;
    }

    private void sort() {
        scores.sort(new Comparator<Score>() {
            @Override
            public int compare(Score score1, Score score2) {
                Integer int1 = score1.getPoints();
                Integer int2 = score2.getPoints();
                return int1.compareTo(int2);
            }
        });
    }

    public boolean addScore(Score score) {
        if (scores.size() < numberOfEntries) {
            scores.add(score);
            sort();
            if (autoSave) {
                save();
            }
            return true;
        }

        Score lowestScore = scores.get(0);
        if (score.getPoints() > lowestScore.getPoints()) {
            scores.remove(lowestScore);
            scores.add(score);
            sort();
            if (autoSave) {
                save();
            }
            return true;
        }

        return false;
    }

    public void save() {
        try (ObjectOutputStream oos = new ObjectOutputStream(new FileOutputStream(saveName))) {
            oos.writeObject(this);
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    @Override
    public String toString() {
        StringBuilder returnValue = new StringBuilder();
        returnValue.append(description);
        returnValue.append("\n\n");
        for (Score score : scores) {
            returnValue.append(score.getName());
            returnValue.append(" - ");
            returnValue.append(score.getPoints());
            returnValue.append("\n");
        }
        return returnValue.toString();
    }
}

Score

import java.io.Serializable;

public class Score implements Serializable {
    private String name;
    private int points;

    public Score(String name, int points) {
        this.name = name;
        this.points = points;
    }

    public String getName() {
        return name;
    }

    public int getPoints() {
        return points;
    }
}

Main

import java.util.Random;

// This class is the test class of the list
public class Main {
    public static void main(String[] args) {
        // Create a ScoreList object and add scores to it
        ScoreList scoreList = new ScoreList("Who has the most flowerpots?", 10, "flowerPotsFile", true);
        String[] names = {"Klaus", "Dieter", "Hans", "Peter", "Maria", "Jürgen", };
        Random random = new Random();
        for (int i = 0; i < 20; i++) {
            String name = names[random.nextInt(names.length)];
            int points = random.nextInt(10) + 1;
            if (scoreList.addScore(new Score(name, points))) {
                System.out.println(name + " has " + points + " flowerpots and can be added to the score list!");
            } else {
                System.out.println(name + " has only " + points + " flowerpots and that is not enough.");
            }
        }
        System.out.println("\n");

        // print out score list
        System.out.println(scoreList);
    }
}
\$\endgroup\$
1
\$\begingroup\$

just some minor issues...

adding a proper serializable interface:

while you provide a filename within the constructor and a save() methode i would apprechiate to let the ScoreListclass to implement a proper (at least a minimal) serialization interface (yeah, what happened to load()? are those highscore files write-only?)

public interface MiniSerializable {
    public boolean saveToFile(String filename);
    public boolean loadFromFile(String filename);

}

DRY (don't repeat yourself)

it's a minor one, so don't worry - helps to prevent Bugs (see inline code comment)

public ScoreList(String description, int numberOfEntries, boolean autoSave) {
    this(description, numberOfEntries);   
    //filename has already been removed, see above     
    this.autoSave = autoSave;
}

should be used, if the scores dont have to be saved to a file
public ScoreList(String description, int numberOfEntries) {
    this.description = description;
    this.numberOfEntries = numberOfEntries;
    scores = new ArrayList<>(); //i guess here's a bug, you forgot to init the list in this constructor, but don't mind
    autoSave = false;
}

another issue of DRY is when adding scores:

public boolean addScore(Score score) {
    if (scores.size() < numberOfEntries) {
        addAndSave(score)
        return true;
    }

    Score lowestScore = scores.get(0);
    if (score.getPoints() > lowestScore.getPoints()) {
        scores.remove(lowestScore);
        addAndSave(score);
        return true;
    }

    return false;
}

private void addAndSave(Score score){
    scores.add(score);
    scores.sort(); //see notes below
    if (autoSave) {
        save();
    }
}

Segregation of concerns

i think it's not the best way to compare scores in the least instead move the comparator to the class where it belongs to: Score you can now agrue that if you apply a special sort order that it's the wrong place, but for default behaviour (as used in here) you should move it to Score.

public class Score implements Serializable, Comparator<Score> {

    //... 

    @Override
    public int compare(Score score1, Score score2) {
        return Integer.compare(score1.getPoints(),score2.getPoints() );
    }

}

that makes the whole sorting kind of unneccessary...

private void sort() {
    scores.sort(); //do you really need a method for this?
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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