Java OOP Mental Math Program

Question

I am currently working on an object-oriented mental math program using Java Swing. Since I am relatively new to the object oriented principals of design, it'd be nice to know how well the code conforms to OOP principals, and what kinds of improvements to make, including:

• If it would be possible to introduce subclasses, especially for the MathQuestionGenerator (e.g. AdditionQuestionGenerator, SubtractionQuestionGenerator etc.)
• If there is a better way to handle the score rather than returning -1 when the session has not completed yet

Overview of Project: The following code is essentially a model that a GUI can use to handle mental math sessions. The code will generate 20 random questions, and keeps track of time and number of wrong questions. It can work with 5 types of sessions (addition, subtraction, multiplication, division, or mixed) and 3 difficulty settings (easy, medium, difficult)

MathSession.java - Represents a mental math session where the user answers 20 questions. The GUI will call startSession() and proceed to submitAnswer() until all 20 questions are completed. Then the GUI can get the user's calculated score with getFinalScore()

public class MathSession {
    public SessionType type;
    public SessionDifficulty difficulty;
    public MathQuestionGenerator generator;
    public MathQuestion currentQuestion;
    public int progress;
    public int numberWrongQuestions;
    public final int numberOfQuestions = 20;
    private long startTime;
    private long endTime;

    // Returns whether or not the session has completed
    public boolean isComplete() {
        return progress == numberOfQuestions;
    }

    // Calculates and returns a final score
    // Returns -1 when the session has not been completed yet
    public int getFinalScore() {
        if (!isComplete()) {
            return -1;
        } else {
            double secondsElapsed = (endTime - startTime)/ 1000;
            double penalty = numberWrongQuestions * 5;
            int score = (int) Math.round(secondsElapsed + penalty);
            return score;
        }
    }

    public MathSession(SessionType operator, SessionDifficulty difficulty) {
        this.type = operator;
        this.difficulty = difficulty;
        this.generator = new MathQuestionGenerator(operator, difficulty);
    }

    // Starts the session
    public void startSession() {
        startTime = System.currentTimeMillis();
        progress = 0;
        numberWrongQuestions = 0;
        currentQuestion = generator.generateQuestion();
    }

    // The user submits an answer, it is either right (returns true) or wrong (returns false)
    public boolean submitAnswer(int answer) {
        if (answer == currentQuestion.correct) {
            progress++;
            if (isComplete()) {
                endTime = System.currentTimeMillis();
            } else {
                currentQuestion = generator.generateQuestion();
            }
            return true;
        } else {
            numberWrongQuestions++;
            return false;
        }
    }
}

MathQuestionGenerator.java - Generates mental math questions given a difficulty and session type.

public class MathQuestionGenerator {
    public SessionType type;
    public SessionDifficulty difficulty;
    private Random random;

    public MathQuestionGenerator(SessionType type, SessionDifficulty difficulty) {
        this.type = type;
        this.difficulty = difficulty;
        random = new Random();
    }

    // Generates a random math question given the session type and difficulty
    public MathQuestion generateQuestion() {
        int num1Max, num2Max, num1, num2, min = 1;

        MathOperator operation = null;
        if (type == SessionType.Addition) {
            operation = MathOperator.Addition;
        } else if (type == SessionType.Subtraction) {
            operation = MathOperator.Subtraction;
        } else if (type == SessionType.Multiplication) {
            operation = MathOperator.Multiplication;
        } else if (type == SessionType.Division) {
            operation = MathOperator.Division;
        } else if (type == SessionType.Mixed) {
            // Choose an operation randomly
            operation = random.nextInt(1) == 0 ? 
                    (random.nextInt(1) == 0 ? MathOperator.Addition : MathOperator.Subtraction) : 
                    (random.nextInt(1) == 0 ? MathOperator.Multiplication : MathOperator.Division);
        }

        if (operation == MathOperator.Addition || operation == MathOperator.Subtraction) {
            if (difficulty == SessionDifficulty.Easy) {
                num1Max = 12;
                num2Max = 12;
            } else if (difficulty == SessionDifficulty.Medium) {
                num1Max = 100;
                num2Max = 100;
            } else {
                num1Max = 10000;
                num2Max = 10000;
            }
        } else {
            if (difficulty == SessionDifficulty.Easy) {
                num1Max = 12;
                num2Max = 12;
            } else if (difficulty == SessionDifficulty.Medium) {
                num1Max = 100;
                num2Max = 10;
            } else {
                num1Max = 100;
                num2Max = 100;
            }
        }

        num1 = min + random.nextInt(num1Max - min);
        num2 = min + random.nextInt(num2Max - min);

        MathQuestion question;
        if (type == SessionType.Addition) {
            question = new MathQuestion(MathOperator.Addition, num1, num2, num1 + num2);
        } else if (type == SessionType.Subtraction) {
            question = new MathQuestion(MathOperator.Subtraction, num1 + num2, num1, num2);
        } else if (type == SessionType.Multiplication) {
            question = new MathQuestion(MathOperator.Multiplication, num1, num2, num1 * num2);
        } else {
            question = new MathQuestion(MathOperator.Division, num1 * num2, num2, num1);
        }

        return question;
    }
}

MathQuestion.java - Represents a math question with two numbers, an operation and a correct answer

public class MathQuestion {
    public MathOperator operator;
    public int number1;
    public int number2;
    public int correct;

    public MathQuestion(MathOperator operator, int number1, int number2, int correct) {
        this.operator = operator;
        this.number1 = number1;
        this.number2 = number2;
        this.correct = correct;
    }
}

MathOperator.java

public enum MathOperator {
    Addition,
    Subtraction,
    Multiplication,
    Division
}

SessionType.java

public enum SessionType {
    Addition,
    Subtraction,
    Multiplication,
    Division,
    Mixed
}

SessionDifficulty.java

public enum SessionDifficulty {
    Easy,
    Medium,
    Hard
}

Answer 1

Welcome to Code Review and thanks for sharing your code!

This is what I think about it:

Formal aspects

order of elements in files

There is a convention of the order of elements inside a java file. The most important you violated is, that constructors come before any (non static) method.

comments

Comments should explain why the code is like it is. Your comments merely repeat what the code expresses itself and are therefore useless.

avoid work in constructors

In the constructor MathSession() you create an instance of MathQuestionGenerator using the both parameters operator and difficulty. forthemore these two parameters (respectively the properties, they are set to) are not used within the class.

The better way would be to create the MathQuestionGenerator instance outside MathSession() and pass this instance of MathQuestionGenerator as constructor parameter into MathSession() instead of operator and difficulty.

Limit visibility and force immutability

Your class MathQuestion is merly a Data Transfer Object. Most of its properties will not change during the LifeTime of the Object. Therefore they should be final.

Declaring all the properties in your classes public makes them accessible from the outside. This is a violation of the most important principle of OOP: Information hiding / encapsulation. So declare your properties private by default and change that only if you have a very good reason to do so.

OOP

OOP doesn't mean to "split up" code into random classes with random inheritance relationships.

The ultimate goal of OOP is to reduce code duplication, improve readability and support reuse as well as extending the code.

Doing OOP means that you follow certain principles which are (among others):

• information hiding / encapsulation
• single responsibility
• separation of concerns
• KISS (Keep it simple (and) stupid.)
• DRY (Don't repeat yourself.)
• "Tell! Don't ask."
• Law of Demeter ("Don't talk to strangers!")

replace branching with polymorphism

You created some enum types which is a good thing in the first place. But You only use the enum values to select execution branches in your MathQuestionGenerator class. e.g.:

    MathOperator operation = null;
    if (type == SessionType.Addition) {
        operation = MathOperator.Addition;
    } else if (type == SessionType.Subtraction) {
        operation = MathOperator.Subtraction;
    } else if (type == SessionType.Multiplication) {
        operation = MathOperator.Multiplication;
    } else if (type == SessionType.Division) {
        operation = MathOperator.Division;
    } else if (type == SessionType.Mixed) {
        // Choose an operation randomly
        operation = random.nextInt(1) == 0 ? 
                (random.nextInt(1) == 0 ? MathOperator.Addition : MathOperator.Subtraction) : 
                (random.nextInt(1) == 0 ? MathOperator.Multiplication : MathOperator.Division);
    }

The better way would be to transfer the "special behavior" in the individual branches into the various constants of the enum:

public enum SessionType {
    Addition{
       return MathOperator.Addition;
    },
    Subtraction{
       return MathOperator.Subtraction;
    },
    Multiplication{
       return MathOperator.Multiplication;
    },
    Division{
       return MathOperator.Division;
    },
    Mixed{
       return random.nextInt(1) == 0 ? 
                (random.nextInt(1) == 0 ? MathOperator.Addition : MathOperator.Subtraction) : 
                (random.nextInt(1) == 0 ? MathOperator.Multiplication : MathOperator.Division);
    };
    public abstract MathOperator selectOperator();
}

Your original code would change to:

MathOperator operation = type.selectOperator();

Avoid special return values

Your method getFinalScore() returns a special value to thell the caller, that something went wrong (the user did not yet answer enough questins). The better way here would be to throw an exception.

The use of Exceptions is a mine field. ;o)
The rule of thumb is tat you do use Exceptions to to signal unusual behavior and not to implement control flow. In My view your example is a corner case, since having not yet answered all questions is clearly not the "happy day path".

An alternative to the Exception could be returning an Java8-Optional instead of the primitive int.