6
\$\begingroup\$

In this game you are a mathematician that has to solve arithmetical problems to be successful. This game includes a save-function. If you want to start from beginning again, you can just delete the save.txt file.

I want ask for suggestions for improving my code quality. What would do you write different and why? Do you maybe have better design choices for the realisation of some aspects of the program idea?

Mathematician.java

import java.util.Scanner;
import java.util.ArrayList;
import java.io.File;
import java.io.FileReader;
import java.io.BufferedReader;
import java.io.FileWriter;
import java.io.BufferedWriter;
import java.io.IOException;

public class Mathematician {
    private int level;
    private int experience;
    private int experienceNeeded;
    final static String[] RANKS = {"Pupil", "Student", "Teacher", "Scientist", "Mathematician"}; 
    private int rank;
    private int rankPoints;
    private int rankPointsNeeded;
    private int gold;
    private ArrayList<Attribute> attributes;

    public Mathematician() {
        File file = new File("save.txt");
        if(!file.exists()) {
            try {
                file.createNewFile();
            } catch(IOException e) { 
                System.out.println("error during creating new file");
            }

            level = 1;
            experience = 0;
            experienceNeeded = 100;
            rank = 0;
            rankPoints = 0;
            rankPointsNeeded = 500;
            gold = 0;
            attributes = new ArrayList();
            attributes.add(new Attribute("intelligence", 1, 100));
            attributes.add(new Attribute("wisdom", 1, 100));
            attributes.add(new Attribute("fortune", 1, 100));
        } else {
            try(BufferedReader br = new BufferedReader(new FileReader("save.txt"))) {
                level = Integer.parseInt(br.readLine());
                experience = Integer.parseInt(br.readLine());
                experienceNeeded = Integer.parseInt(br.readLine());
                rank = Integer.parseInt(br.readLine());
                rankPoints = Integer.parseInt(br.readLine());
                rankPointsNeeded = Integer.parseInt(br.readLine());
                gold = Integer.parseInt(br.readLine());
                attributes = new ArrayList();
                attributes.add(new Attribute("intelligence", Integer.parseInt(br.readLine()), Integer.parseInt(br.readLine())));
                attributes.add(new Attribute("wisdom", Integer.parseInt(br.readLine()), Integer.parseInt(br.readLine())));
                attributes.add(new Attribute("fortune", Integer.parseInt(br.readLine()), Integer.parseInt(br.readLine())));
            } catch(IOException e){
                System.out.println("error during reading save file");
            }
        }
    }

    // getters

    public String getRank() {
        return RANKS[rank];
    }

    public int getExperienceReward(int bonus) {
        return (int)((1 + attributes.get(0).getLevel() * 2 + level * 5) * (1 + bonus * 0.25 + rank * 0.25));
    }

    public int getRankPointsReward(int bonus) {
        return (int)((1 + attributes.get(1).getLevel() * 2 + level * 5) * (1 + bonus * 0.25 + rank * 0.25));
    }

    public int getGoldReward(int bonus) {
        return (int)((1 + attributes.get(2).getLevel() * 2 + level * 5) * (1 + bonus * 0.25 + rank * 0.25));
    }

    // methods

    public void save() {
        try(BufferedWriter bw = new BufferedWriter(new FileWriter("save.txt"))) {
            bw.write(String.valueOf(level)); bw.newLine();
            bw.write(String.valueOf(experience)); bw.newLine();
            bw.write(String.valueOf(experienceNeeded)); bw.newLine();
            bw.write(String.valueOf(rank)); bw.newLine();
            bw.write(String.valueOf(rankPoints)); bw.newLine();
            bw.write(String.valueOf(rankPointsNeeded)); bw.newLine();
            bw.write(String.valueOf(gold)); bw.newLine();
            bw.write(String.valueOf(attributes.get(0).getLevel())); bw.newLine();
            bw.write(String.valueOf(attributes.get(0).getImprovementCosts())); bw.newLine();
            bw.write(String.valueOf(attributes.get(1).getLevel())); bw.newLine();
            bw.write(String.valueOf(attributes.get(1).getImprovementCosts())); bw.newLine();
            bw.write(String.valueOf(attributes.get(2).getLevel())); bw.newLine();
            bw.write(String.valueOf(attributes.get(2).getImprovementCosts())); bw.newLine();
            System.out.println("game saved");
            System.out.println();
        } catch(IOException e) {
            System.out.println("error during saving game");
        }
    }

    public void show() {
        System.out.println("--- Basic information ---");
        System.out.println("Level       " + level);
        System.out.println("Experience  " + experience + " / " + experienceNeeded);
        System.out.println("Rank        " + getRank());
        System.out.println("Rank points " + rankPoints + " / " + rankPointsNeeded);
        System.out.println("Gold        " + gold + "\n");

        System.out.println("--- Minimum profit ---");
        System.out.println("Experience  "+getExperienceReward(1));
        System.out.println("Gold        " + getGoldReward(1));
        System.out.println("Rank Points " + getRankPointsReward(1) + "\n");

        System.out.println("---Attributes---");
        for(Attribute attribute : attributes)
            System.out.printf("%-13s %3d\n", attribute.getName(), attribute.getLevel());
        System.out.println();

    }

    public void increaseExperience(int bonus) {
        experience += getExperienceReward(bonus);
        System.out.println("experience + " + getExperienceReward(bonus));
        increaseLevel();
    }

    public void increaseRankPoints(int bonus) {
        rankPoints += getRankPointsReward(bonus);
        System.out.println("rank points + " + getRankPointsReward(bonus));
        increaseRank();
    }

    public void increaseGold(int bonus) {
        gold += getGoldReward(bonus);
        System.out.println("gold + " + getGoldReward(bonus));
    }

    // doesnt increase level and rank
    public void increaseEverything(int bonus) {
        increaseExperience(bonus);
        increaseRankPoints(bonus);
        increaseGold(bonus);
    }

    public void increaseLevel() {
        while(experience >= experienceNeeded) {
            level++;
            experience -= experienceNeeded;
            experienceNeeded = 50 * (int)Math.pow(level, 2) + 50 * level;
            System.out.println("--------");
            System.out.println("LEVEL UP");
            System.out.println("--------");
        }
    }

    public void increaseRank() {
        while(rankPoints >= rankPointsNeeded && rank < 4) {
            rank++;
            rankPoints -= rankPointsNeeded;
            rankPointsNeeded = 250 * (int)Math.pow(rank+1, 2) + 250 * (rank + 1); 
            System.out.println("--------");
            System.out.println("RANK UP");
            System.out.println("--------");
        }       
    }

    public void calculate(Scanner input) {
        Exercise exercise = new Exercise();
        int bonus = exercise.takeGuess(input); 
        if(bonus > 0)
            increaseEverything(bonus);
        else
            System.out.println("wrong");
    }

    public void improveAttributes(Scanner input) {
        int improve;
        System.out.println("[0] back");
        do {
            System.out.println("gold: " + gold);
            for(int i = 0; i < attributes.size(); i++)
                System.out.printf("[%d] %-13s %3d costs %6d\n", i+1, attributes.get(i).getName(), attributes.get(i).getLevel(), 
                                  attributes.get(i).getImprovementCosts());
            System.out.print("improve: ");
            improve = input.nextInt();
            System.out.println();

            if(improve < 4 && improve > 0 && attributes.get(improve-1).getImprovementCosts() <= gold) {
                gold -= attributes.get(improve-1).getImprovementCosts();
                attributes.get(improve-1).improve();
            } else if(improve >= 4) {
                System.out.println("invalid input");
            } else if(improve < 4 && improve > 0 && attributes.get(improve-1).getImprovementCosts() >= gold) {
                System.out.println("not enough money!");
            }
            System.out.println();
        } while(improve != 0);
    }
}

Exercise.java

import java.util.Random;
import java.util.Scanner;

public class Exercise {
    private int difficulty;
    private int number1;
    private int number2;
    private String task;
    private int solution;
    private int guess;

    public Exercise() {
        Random r = new Random();
        difficulty = r.nextInt(4);
        if(difficulty == 0) {
            number1 = r.nextInt(20);
            number2 = r.nextInt(20);
            solution = number1 + number2;
            task  = number1 + " + " + number2 + " = ";
        } else if(difficulty == 1) {
            number1 = r.nextInt(30);
            number2 = r.nextInt(20);
            solution = number1 - number2;
            task  = number1 + " - " + number2 + " = ";
        } else if(difficulty == 2) {
            number1 = r.nextInt(15);
            number2 = r.nextInt(15);
            solution = number1 * number2;
            task  = number1 + " * " + number2 + " = ";
        } else {
            number1 = r.nextInt(100);
            number2 = r.nextInt(15) + 1;
            solution = number1 / number2;
            task  = number1 + " / " + number2 + " = ";          
        }
    }

    public int takeGuess(Scanner input) {
        System.out.print(task);
        guess = input.nextInt();
        if(guess == solution)
            return difficulty + 1;
        else
            return 0;
    }
}

Attribute.java

public class Attribute {
    private String name;
    private int level;
    private int improvementCosts;

    public Attribute(String name, int level, int improvementCosts) {
        this.name = name;
        this.level = level;
        this.improvementCosts = improvementCosts;
    }

    // getter

    public String getName() {
        return name;
    }

    public int getLevel() {
        return level;
    }

    public int getImprovementCosts() {
        return improvementCosts;
    }

    // methods

    public void improve() {
        level++;
        improvementCosts = 50 * (int)Math.pow(level, 2) + 50 * level;
        System.out.println(name + " improved!");
    }
}

Game.java

import java.util.Scanner;

public class Game {
    public void init() {
        Mathematician player = new Mathematician();
        Scanner input = new Scanner(System.in);

        // i think in this case better than do-while, because you dont have to define a variable, just using break;
        System.out.println("[i] information");
        System.out.println("[s] save");
        System.out.println("[x] kill program (includes save)\n");
        boolean program = true;
        do {
            System.out.println("[1] show character");
            System.out.println("[2] calculate");
            System.out.println("[3] attributes");
            System.out.print("input: ");
            String toDo = input.next();
            System.out.println();
            switch(toDo) {
                case "1":
                    player.show();
                    break;
                case "2":
                    System.out.println("(not more than 100 exercises)");
                    int amount;
                    do {
                        System.out.print("number of exercises: ");
                        amount = input.nextInt();
                    } while(amount > 100 || amount < 0);
                    for(int i = 0; i < amount; i++) {
                        player.calculate(input);
                    }   
                    break;
                case "3":
                    player.improveAttributes(input);
                    break;
                case "i":
                    information();
                    break;
                case "s":
                    player.save();
                    break;
                case "x":
                    player.save();
                    program = false;
            } 
        } while(program);
    }

    public void information() {
        System.out.println("programmed by Henry Weinert");
        System.out.println("division exercises have to be solved without rest");
        System.out.println("when you want to begin a new game, just delete the save file");
        System.out.println();
    }
}

Start.java

public class Start {
    public static void main(String[] args) {
        Game game = new Game();
        game.init();
    }
}
\$\endgroup\$
3
\$\begingroup\$

I think @mdfst13's suggestions are all pretty good. Here are some couple other things I noticed. (Note: I think this code is pretty well organized for its scale. Some of the below decisions make more sense under the assumption that you're going to work on the program more; just like there is danger in premature optimization, there is also danger in premature abstraction).

Serialization

Your Mathematician.save function performs the task of "writing the state of a Java object to a file" (and the corresponding code in the constructor performs the task of "reading the state of a Java object from a file"). Perhaps unsurprisingly, this is a problem developers (in all languages) face a lot. The first task is known as "serialization" and the second task is known as "deserialization".

Writing good serialization/deserialization code is tricky -- for starters, writing it the way you have is pretty repetitive and bug-prone (what if you accidentally swap the order of two of your writes?). Luckily, this is such a common problem that there are lots of ready-made solutions at hand. In fact, Java has a Serialization library built into java.io: see here.

Inflexible Constructors

Some of your constructors are a bit less flexible than they could be and make some strong assumptions on how all objects of that class are going to be created. For example:

  • The Mathematician constructor by default assumes that all Mathematician objects will be loaded from the file "save.txt" if it exists.
  • The Exercise constructor by default assumes that an Exercise must be chosen uniformly at random from each of the four possible exercise types.

Now, of course, both these assumptions are true in your game. But you can imagine that, were you to make changes to your game, these assumptions might not hold true anymore.

Let's look at the Exercise class for starters. Maybe at some point you'll want to implement some feature like, "when the Mathematician ranks up, give him a bunch of easy addition exercises" as a reward. Right now, you would have to actually change the Exercise class to achieve this (probably by adding a new constructor/changing the existing one). Ideally, once you've written the Exercise class, you shouldn't need to change it at all until you need to add new features to actual Exercise objects.

What might be a better solution for the Exercise constructor? Well, one simple change you could make would be to have the Exercise constructor accept as input the difficulty and the two numbers (i.e. public Exercise(int difficulty, int number1, int number2)). Then, whenever you need a random exercise, you can generate the random numbers and pass them into the constructor (you can even have a static function public static Exercise randomExercise()) somewhere in Exercise which does this for you).

An even more abstract solution would be to have a bunch of subclasses that extend Exercise for every type of Exercise; e.g. have an AdditionExercise class, a SubtractionExercise class, etc. Then, whenever you add a new exercise type, you don't have to muck around with adding a new difficulty parameter to the constructor; you can just create a new class which extends Exercise.

Returning to Mathematician, I would likewise probably implement a general constructor, and then a static function which loads a Mathematician from a file (or you could implement this as an alternate constructor). The logic to check whether the file exists and what to do in each case could then be separated and moved out to Game.java.

\$\endgroup\$
4
\$\begingroup\$

enum

    final static String[] RANKS = {"Pupil", "Student", "Teacher", "Scientist", "Mathematician"}; 

When you have a list of possible values, consider creating an enum to hold them.

public enum Rank {

    PUPIL(0) {
        @Override
        public Rank getNext() {
            return STUDENT;
        }
    },
    STUDENT(500) {
        @Override
        public Rank getNext() {
            return TEACHER;
        }
    },
    TEACHER(1500) {
        @Override
        public Rank getNext() {
            return SCIENTIST;
        }
    },
    SCIENTIST(3000) {
        @Override
        public Rank getNext() {
            return MATHEMATICIAN;
        }
    },
    MATHEMATICIAN(5000);

    final private int pointsNeeded;

    Rank(int pointsNeeded) {
        this.pointsNeeded = pointsNeeded;
    }

    public Rank getNext() {
        return null;
    }

    public getPointsNeeded() {
        Rank next = getNext();
        return (next == null) ? Integer.MAX_VALUE : next.pointsNeeded;
    }

}

Java enums are incredibly flexible, being essentially a type of class that extends the base enum type.

    private int rank;

Then this could be

    private Rank rank;

Then

    private int rankPointsNeeded;

could be moved into the enum if it is determined by the current rank.

            rank++;

Consider this Stack Overflow post for implementation of getNext.

            rank = rank.getNext();

Favor interfaces over implementations

    private ArrayList<Attribute> attributes;

Consider changing this to

    final private List<Attribute> attributes = new ArrayList<>();

This way if you want to change the implementation later, you can do so in just one place. And you have to code to the interface rather than the implementation.

Moving the initialization into the declaration gets rid of two lines in the constructor that always do the same thing.

In this context, the final keyword just means that we never replace the list with a different list. It does not make the list itself immutable.

Magic values

Consider adding

    private static final FILE_NAME = "save.txt";

or similar to your Mathematician class. You currently use this value three times. This way, you can use this constant instead. So changes won't get half made.

Reuse

            try(BufferedReader br = new BufferedReader(new FileReader("save.txt"))) {

This could be

            try (BufferedReader br = new BufferedReader(new FileReader(file))) {

Because FileReader can take a File to identify the location.

Always use the block form of control structures

        if(bonus > 0)
            increaseEverything(bonus);
        else
            System.out.println("wrong");

Java, like most of the C-based languages, has both a statement form and a block form of the control structures. So this could be written

        if (bonus > 0) {
            increaseEverything(bonus);
        } else {
            System.out.println("wrong");
        }

If you always do this, it will become easier to avoid a bug where someone adds an extra step without switching to the block form.

Written correctly, this won't matter. But you are committing not only yourself but anyone who might modify your code later to fully understanding when you can and can't use the single statement form. The block form is more reliable, as it will always work. It doesn't matter whether there is one statement or many.

You put

        Random r = new Random();

in the Exercise constructor. This makes a new one each time you make a new Exercise object. Consider instead a class field

    public static final Random random = new Random();

Now you just have one per the entire program. So you get all values taken from the same starting point.

It's often easier to remember what random is than what r is. This becomes especially important when you come back to code after a long time away. This may not matter with this code, which you may not plan to reuse. But if you always assume that your code will be permanent, you won't suddenly be surprised when you find yourself updating code that you had meant to be temporary.

Simplifying

You have

        boolean program = true;
        do {

and

                    program = false;
            } 
        } while(program);

This could be just

        while (true) {

and

                    return;
            }
        }

You don't need the program variable.

\$\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.