1
\$\begingroup\$

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
}

screen shot

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

\$\endgroup\$
11
  • 1
    \$\begingroup\$ Please provide some more details, not in code but in pure English, about what your code is doing. Can you provide some examples? Screenshots? Anything to make the question clearer. See codereview.meta.stackexchange.com/a/6429/31562 \$\endgroup\$ Jan 11 at 13:59
  • \$\begingroup\$ I'll add some more info shortly to show the steps the code goes through. Screenshots not possible as this is an object class that returns a question which is then dealt with in a separate activity \$\endgroup\$ Jan 11 at 14:27
  • 1
    \$\begingroup\$ @UnknownError Sorry for being unclear, my recommendation was about adding the "What this code does by step" part outside of the code block (unless you actually want to have it as comments inside the code, in which case parts of a review could potentially address these comments as well). \$\endgroup\$ Jan 11 at 18:27
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Vogel612
    Jan 19 at 15:46
  • 1
    \$\begingroup\$ @UnknownError Yes, absolutely \$\endgroup\$ Jan 19 at 19:29
2
\$\begingroup\$

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)
\$\endgroup\$
11
  • \$\begingroup\$ Thank you for your feedback. This is really useful. Some of it doesn't yet make total sense but like you point out the purpose is to learn. I'm going to digest some of this, have a crack at making some changes and I'll report back. Thank you for taking the time to review and provide me some good food for thought \$\endgroup\$ Jan 11 at 21:09
  • \$\begingroup\$ I have a lot to go through still but a quick initial question. With the naming of num1,2,3,4. Each represents a number in a sum e.g num1 x num2. I allow up to 4 as some questions require 4 variables. They can all be a different number but they are all numbers used in a sum. Based on that how would you better describe them? Or does that help clarify how they not that different from eachother? \$\endgroup\$ Jan 13 at 20:02
  • \$\begingroup\$ @UnknownError My main concern about them is that if they are indeed 4 similar variables, they would best be represented by an array, but they seem to be initialized and set somewhat differently. \$\endgroup\$ Jan 13 at 21:02
  • \$\begingroup\$ I still have some areas to look at but my first run through has been added as an addition to my OP, am I on the right track or have I misunderstood your feedback? \$\endgroup\$ Jan 19 at 0:55
  • 1
    \$\begingroup\$ @UnknownError Having multiple code samples, some more improved, some less, causes a lot of confusion when multiple answers appear, as it is unclear which answer addresses which code. (We have some experience of this happening on this site) So the recommended way is to post multiple questions and add links between them. \$\endgroup\$ Jan 19 at 19:31

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.