Generating mathematical questions and answers

Question

I have an Android quiz app that I am building to learn kotlin / android app development. Here is an object I have which acts as one of many question builders.

Is there a better way of writing this?

    package com.maxcell.sumitup

import java.security.SecureRandom
import kotlin.math.roundToInt

object MultiplicationFactory {
    
    //What this code does by step
    // step1 - I set 4 variables using a random number generator to different values. Sometimes the first variable is bigger, sometimes the second variable etc
    // step 2 - based on the first constructor value (ML) the main level is passed to variable templevel
    // This step provides sets a variable called vInc and with the constructor value (SL) is passed to function SLM()
    // The function SLM takes three variables (SL, Num, vInc) and then adds vInc as a flat increase, the new value is then further increased 
    // by the value of SL but this is expressed as a percentage, the value is then returned as an updated integer
    // step 3, the Num variables are then passed to this step to create a sum as a string (the question) and as an expression (the answer)
    // step 4 the object function returns a dataclass containing 5 values for use in the activity:
    // SingleQuestion(ML,vType,vQuestion,vAnswer,vTime) - Currently the values used from the return is the question and the answer, i then create
    // three other answers, shuffle all of them and pass them to buttons: See screenshot for example of a question being used within the activity
    // this code is used in different places in the app where a question is required under the specific minigame
    
    
    var Num1 = 0 //Number for use in sums
    var Num2 = 0 //Number for use in sums
    var Num3 = 0 //Number for use in sums
    var Num4 = 0 //Number for use in sums
    var GrabQuestion = 0 //base level numbers
    var Operations = 0 // how many calculations someone has to do to reach the answer

        var vType = "" //e.g. multiplication, this is redundant soon to be redundant once all question types migrated
        var vQuestion  = "" //The question as the user will see it
        var vAnswer = 0 //answer to the sum
        var vTime = 0 //Time to offer for the question
        var vInc = 0 //sub level multiplier variable
        var tempLevel = 0 //main level
        var BSL = 0 // Bonus round selector


    fun newQ(ML:Int, SL:Int,BR:Boolean,gameType:String):SingleQuestion {
        vType = "Multiplication"

        //map old game levels to new broader levelling logic - ignore this snippet in codereview
        if (gameType=="FreeStyle"){
        when (ML){
            1->{tempLevel = rand(1,3)}
            2->{tempLevel = rand(4,5)}
            3->{tempLevel = rand(6,8)}
            4->{tempLevel = rand(9,10)}
        }} else {tempLevel = ML}
        GrabQuestion = rand(1,23)

        //Step 1
        //to create a greater feel of randomness in the questions multiple possibilities created. These are base level figures
        when(GrabQuestion){
            1->{Num1 = rand(1,5);Num2 = rand(1,5);Num3 = rand(2,6);Num4 = rand(4,8)}
            2->{Num1 = rand(2,6);Num2 = rand(1,5);Num3 = rand(2,6);Num4 = rand(4,8)}
            3->{Num1 = rand(3,7);Num2 = rand(1,5);Num3 = rand(2,6);Num4 = rand(4,8)}
            4->{Num1 = rand(4,8);Num2 = rand(1,5);Num3 = rand(2,6);Num4 = rand(4,8)}
            5->{Num1 = rand(5,9);Num2 = rand(1,5);Num3 = rand(2,6);Num4 = rand(4,8)}
            6->{Num1 = rand(6,10);Num2 = rand(7,11);Num3 = rand(2,6);Num4 = rand(4,8)}
            7->{Num1 = rand(1,5);Num2 = rand(6,10);Num3 = rand(2,6);Num4 = rand(4,8)}
            8->{Num1 = rand(2,6);Num2 = rand(5,9);Num3 = rand(2,6);Num4 = rand(4,8)}
            9->{Num1 = rand(3,7);Num2 = rand(4,8);Num3 = rand(2,6);Num4 = rand(4,8)}
            10->{Num1 = rand(4,8);Num2 = rand(3,7);Num3 = rand(2,6);Num4 = rand(4,8)}
            11->{Num1 = rand(5,9);Num2 = rand(2,6);Num3 = rand(2,6);Num4 = rand(4,8)}
            12->{Num1 = rand(6,10);Num2 = rand(1,5);Num3 = rand(2,6);Num4 = rand(4,8)}
            13->{Num1 = rand(1,5);Num2 = rand(1,5);Num3 = rand(2,6);Num4 = rand(4,8)}
            14->{Num1 = rand(2,6);Num2 = rand(2,6);Num3 = rand(2,6);Num4 = rand(4,8)}
            15->{Num1 = rand(3,7);Num2 = rand(3,7);Num3 = rand(2,6);Num4 = rand(4,8)}
            16->{Num1 = rand(4,8);Num2 = rand(4,8);Num3 = rand(2,6);Num4 = rand(4,8)}
            17->{Num1 = rand(5,9);Num2 = rand(5,9);Num3 = rand(2,6);Num4 = rand(4,8)}
            18->{Num1 = rand(6,10);Num2 = rand(6,10);Num3 = rand(2,6);Num4 = rand(4,8)}
            19->{Num1 = rand(1,5);Num2 = rand(1,5);Num3 = rand(2,6);Num4 = rand(4,8)}
            20->{Num1 = rand(1,5);Num2 = rand(2,6);Num3 = rand(2,6);Num4 = rand(4,8)}
            21->{Num1 = rand(1,5);Num2 = rand(3,7);Num3 = rand(2,6);Num4 = rand(4,8)}
            22->{Num1 = rand(1,5);Num2 = rand(4,8);Num3 = rand(2,6);Num4 = rand(4,8)}
            23->{Num1 = rand(1,5);Num2 = rand(5,9);Num3 = rand(2,6);Num4 = rand(4,8)}
        }
        //when not a bonus round
        //step2
        if (BR==false){
                    //based on level increase base numbers (flat increase), based on sublevel increase numbers (%)
                when (tempLevel){
                    1->{vInc = 0;Operations = 1;vTime = 10;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc)}
                    2->{vInc = 1;Operations = 1;vTime = 10;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc)}
                    3->{vInc = 2;Operations = 1;vTime = 10;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc)}
                    4->{vInc = 3;Operations = 1;vTime = 10;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc)}
                    5->{vInc = 4;Operations = 1;vTime = 10;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc)}
                    6->{vInc = 0;Operations = 2;vTime = 15;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc);Num3 = SLM(SL,Num3,vInc)}
                    7->{vInc = 1;Operations = 2;vTime = 15;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc);Num3 = SLM(SL,Num3,vInc)}
                    8->{vInc = 2;Operations = 2;vTime = 15;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc);Num3 = SLM(SL,Num3,vInc)}
                    9->{vInc = 3;Operations = 2;vTime = 15;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc);Num3 = SLM(SL,Num3,vInc)}
                    10->{vInc = 4;Operations = 2;vTime = 15;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc);Num3 = SLM(SL,Num3,vInc)}
                }
                //grab appropriate question format for number of calculations required
                // step 3
                when (Operations){
                    1->{vQuestion ="$Num1 \u00D7 $Num2";vAnswer = Num1 * Num2}
                    2->{vQuestion ="($Num1 \u00D7 $Num2) \u00D7 $Num3";vAnswer = (Num1 * Num2) *Num3}
                    else->{vQuestion ="$Num1 \u00D7 $Num2";vAnswer = Num1 * Num2}
                }
            } else {
                //bonus games include different operators
                    //step 2
                when (tempLevel){
                    in 1..3->{vInc = 0;BSL = rand(1,4);vTime = 20;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc);Num3 = SLM(SL,Num3,vInc);Num4 = SLM(SL,Num4,vInc)}
                    in 4..5->{vInc = 2;BSL = rand(1,4);vTime = 20;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc);Num3 = SLM(SL,Num3,vInc);Num4 = SLM(SL,Num4,vInc)}
                    in 6..8->{vInc = 4;BSL = rand(1,4);vTime = 20;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc);Num3 = SLM(SL,Num3,vInc);Num4 = SLM(SL,Num4,vInc)}
                    in 9..10->{vInc = 6;BSL = rand(1,4);vTime = 20;Num1 = SLM(SL,Num1,vInc);Num2 = SLM(SL,Num2,vInc);Num3 = SLM(SL,Num3,vInc);Num4 = SLM(SL,Num4,vInc)}
                }
            //grab appropriate question format for number of calculations required
            //step 3
                when (BSL){
                    1->{vQuestion ="($Num1 × $Num2) + ($Num3 x $Num4)";vAnswer = (Num1 * Num2) + (Num3 * Num4)}
                    2->{vQuestion ="$Num1 + ($Num2 × $Num3) + $Num4";vAnswer = Num1 + (Num2 * Num3) + Num4}
                    3->{vQuestion ="(($Num1 × $Num2) + $Num3) - $Num4";vAnswer = ((Num1 * Num2) + Num3) - Num4}
                    4->{vQuestion ="$Num1 + ($Num2 × $Num3) - $Num4";vAnswer = Num1 + (Num2 * Num3) -Num4}
                }
            }
        //return question to the minigame
        // step 4
        return SingleQuestion(ML,vType,vQuestion,vAnswer,vTime)
    }
}
// Function to provide a random number. Set the lowest number and highest number.
private fun rand(start: Int, end: Int): Int {
    require(start <= end) { "Illegal Argument" }
    val random = SecureRandom()
    random.setSeed(random.generateSeed(20))

    return random.nextInt(end - start + 1) + start
}
//This function is aimed at providing an optimisation capability.
// sb levels are 1-25 e.g sub level 1 increases each number by 1.01% up to 1.25% (1% increase to 25% increase)
// you can lower the division factor (e.g. /100 to /50) to create bigger gaps between sub levels
private fun SLM(subLevel:Int, num:Int, increment:Int):Int{
    var multiplier = ((subLevel / 100)+1).toDouble()
    var newNum = ((num + increment) * multiplier).roundToInt()
    return newNum
}

The screenshot shows a division question and my code is from a multiplication question builder but it's just to show how the data is used once returned

See Generating multiplication questions and answers for a quiz style app for follow up

Answer 1

Readability

The most important part about any code is readability.

BSL, ML, SL, BR, vInc, etc. might mean something to you, but it's not clear what it means by just looking at the names.

Example:

var BSL = 0 // Bonus round selector

This would be much clearer as

var bonusRoundSelector = 0

You have done the same thing for a lot of your variables. In some cases it's even contradictory.

var tempLevel = 0 //main level

Is it temporary or main? Those are two very different things.

Variables should be named so that they don't require a comment next to them about what their purpose is.

---

Declaring variables

All your variables are declared outside of the function scope, that is unnecessary, they can be moved to inside the fun newQ instead.

Prefer using val instead of var whenever possible in Kotlin. You might get an error message from it but the solution to errors is to understand them and not workaround them just because you know how to workaround them.

There's multiple steps to be involved but here's some things that I notice:

• Num1 should be called num1 but you might want to try and come up with a different name for it as the four Num-variables does not seem very similar to me.
• If we would make Num1 a val we would be told that it cannot be reassigned. That is because you give it a default value of 0. This already is a problem, what if GrabQuestion is not in the range of 1..23 ? You might think that it always is because of you calling rand(1, 23), but what if there would one day be a bug in the rand function? Solutions: Use an else statement in when to throw an exception and don't initialize Num1 to anything, declare it as val num1: Int.
• Num1 is mutated again in step2, all these steps could be extracted as different methods and their data stored in one or several data class structures. For example, val numbers = initializeNumbers(grabQuestion) to initialize the num1, num2, num3, num4 values.

---

Copied code

This is just an example, but BSL is always set to rand(1,4) so take it out of the duplication in the when and only do it once.

The same goes for vTime, Num1, and many other values.

Additionally, you have several magic numbers copy-pasted in your code, things like vTime = 20 could maybe be vTime = TIME_LONG where TIME_LONG is declared as const val TIME_LONG = 20

---

SecureRandom

First of all, it's not necessary to set the seed on SecureRandom, it can do that by itself.

Secondly, I don't think it's necessary to use SecureRandom here at all. It's a Math Quiz, not a password generation service. Use Random.Default instead like this:

val random = Random.Default
return random.nextInt(start, end + 1)

---

Overall

The current code is difficult to read and difficult to maintain, using better variable naming and less code duplication and structuring the code into multiple separated methods for each step, this will improve drastically.

I started refactoring the code but realized that it was a bit of work and that you wouldn't learn much by copy-pasting my code, but I promise you that your main method can be reduced to something like this:

val vType = "Multiplication"
val level = determineLevel(gameType, ML)
val grabQuestion: Int = rand(1, 23) //base level numbers
var numbers = initializeNumbers(grabQuestion)

val operations = determineOperations(level)
numbers = processNumbersStep2(bonusRound, level)

val questionAndAnswer = createQuestion(bonusRound, level, operations)
return createQuestion(questionAndAnswer)