https://kotlinlang.org logo
Join Slack
Powered by
``` if ( player1List.containsA...
# codereview
a
Copy code
if (
                    player1List.containsAll(listOf(1, 3, 7)) ||
                    player1List.containsAll(listOf(1, 3, 9)) ||
                    player1List.containsAll(listOf(1, 7, 9)) ||
                    player1List.containsAll(listOf(1, 3, 8))
                ) {
                    cell = 5
                } else if (
                    player1List.containsAll(listOf(1, 3, 4))
                ) {
                    cell = 2
                } else if (
                    player1List.containsAll(listOf(1, 5, 7))
                ) {
                    cell = 3
                } else if (
                    player1List.containsAll(listOf(1, 3, 5)) ||
                    player1List.containsAll(listOf(1, 2, 4))
                ) {
                    cell = 7
                } else if (player1List.containsAll(listOf(3, 5, 9))) {
                    cell = 1
                }
a
you could create a 
List<Pair<List<List<int>>, int>>
. 
first
is your list of condition-lists and 
second
is the cell value. then you can do something like 
cell = map.firstOrNull { pair -> pair.first.all { player1List.containsAll(it) } }?.second
l
also I’m not 100% but I think when you do 
listOf(1,2,3)
you are creating a new object every time you check the condition
e
@leosan You're correct
a
Would you mind to tell me what is this mean?
List<Pair<List<List<int>>, int>>
e
Should better be 
Map<List<Int>, Int>
a
@edwardwongtl is that something like create Set of set?
g
If I had to keep the same structure of code, I would encapsulate it to make it more readable. I would also break the code convention to simplify the reading using horizontal alignment:
Copy code
if (p1.hasPlayed(1, 3, 7) ||
        p1.hasPlayed(1, 3, 9) ||
        p1.hasPlayed(1, 7, 9) ||
        p1.hasPlayed(1, 3, 8))      { cell = 5 }
    else if (p1.hasPlayed(1, 3, 5) ||
        p1.hasPlayed(1, 2, 4))      { cell = 7 }
    else if (p1.hasPlayed(1, 3, 4)) { cell = 2 } 
    else if (p1.hasPlayed(1, 5, 7)) { cell = 3 } 
    else if (p1.hasPlayed(3, 5, 9)) { cell = 1 }
e
You can lift the assignment of 
cell
outside, also replace all those 
if else if
with 
when
g
Yes, it’s more readable:
Copy code
when {
        p1.hasPlayed(1, 3, 7) || 
        p1.hasPlayed(1, 3, 9) ||
        p1.hasPlayed(1, 7, 9) ||
        p1.hasPlayed(1, 3, 8) -> cell = 5
        p1.hasPlayed(1, 3, 5) ||
        p1.hasPlayed(1, 2, 4) -> cell = 7
        p1.hasPlayed(1, 3, 4) -> cell = 2
        p1.hasPlayed(1, 5, 7) -> cell = 3
        p1.hasPlayed(3, 5, 9) -> cell = 1
    }
👍 2
e
Even better
Copy code
cell = when {
        p1.hasPlayed(1, 3, 7) || 
        p1.hasPlayed(1, 3, 9) ||
        p1.hasPlayed(1, 7, 9) ||
        p1.hasPlayed(1, 3, 8) -> 5
        p1.hasPlayed(1, 3, 5) ||
        p1.hasPlayed(1, 2, 4) -> 7
        p1.hasPlayed(1, 3, 4) -> 2
        p1.hasPlayed(1, 5, 7) -> 3
        p1.hasPlayed(3, 5, 9) -> 1
        else -> // some default value
    }
g
It’s not the same code: you should have a default branch.
e
ya you're right, just added
g
Your code is now ok, but it’s not doing the same logic as the beginning.
can we set a default value ? I don’t know.
a
This is the code of doing Tic Tac Toe.
I think last time they suggest me to do a set of set.
I am looking for a way to optimize the code. 😞
e
A default value of 
0
seems to be a nice choice
Since you're indicating the valid cells by 
1-9
a
Ya. Whenever the cell = 0;
Then it will keep looping.
e
I think I found a better way then set of set
Copy code
data class Condition(val pos1: Int, val pos2: Int, val pos3: Int, val value: Int)

val conditions = listOf(
            Condition(1, 3, 7, 5),
            Condition(1, 3, 9, 5),
            Condition(1, 7, 9, 5),
            Condition(1, 3, 8, 5),
            Condition(1, 3, 4, 2),
            Condition(1, 5, 7, 3),
            Condition(1, 3, 5, 7),
            Condition(1, 2, 4, 7),
            Condition(3, 5, 9, 1)
    )

    val player1List = listOf(1,2,4)

    val cell = conditions.firstOrNull { (pos1, pos2, pos3, _) -> 
        player1List.contains(pos1) and player1List.contains(pos2) and player1List.contains(pos3) 
    }?.value ?: 0
➕ 2
K 2
a
IMO a default value which is in the same space as the valid cells is wrong. If you use 
null
, the compiler enforces that you check whether its a valid cell
e
We didn't set the default value to the same space as the valid cells...
a
0
is an ordinary integer just like 
1
or 
9
and can be mistaken for a valid cell if you forget to check 
cell != 0
➕ 1
e
@Andreas Sinz He said the function will loop when 
cell == 0
before though
a
Thanks everyone for giving me suggestion! Much appreciated.
Thanks, again! ❤️
@edwardwongtl would you mind to tell me why you need to create a data class?
e
@Ayden Its more light weight then a set of set IMO
a
@edwardwongtl It's not more lightweight than just a standard
Copy code
class Condition(...)
though.
e
@andyb You're right, but using a 
data class
allows you to use the destructing syntax
3 Views
Open in Slack
PreviousNext
Powered by