https://kotlinlang.org logo
#codereview
Title
# codereview
a

Ayden

01/23/2018, 3:25 PM
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

Andreas Sinz

01/23/2018, 3:35 PM
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

leosan

01/23/2018, 3:39 PM
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

edwardwongtl

01/23/2018, 3:57 PM
@leosan You're correct
a

Ayden

01/23/2018, 3:57 PM
Would you mind to tell me what is this mean?
List<Pair<List<List<int>>, int>>
e

edwardwongtl

01/23/2018, 3:58 PM
Should better be
Map<List<Int>, Int>
a

Ayden

01/23/2018, 4:00 PM
@edwardwongtl is that something like create Set of set?
g

gaetan

01/23/2018, 4:04 PM
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

edwardwongtl

01/23/2018, 4:05 PM
You can lift the assignment of
cell
outside, also replace all those
if else if
with
when
g

gaetan

01/23/2018, 4:06 PM
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

edwardwongtl

01/23/2018, 4:08 PM
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

gaetan

01/23/2018, 4:09 PM
It’s not the same code: you should have a default branch.
e

edwardwongtl

01/23/2018, 4:09 PM
ya you're right, just added
g

gaetan

01/23/2018, 4:11 PM
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

Ayden

01/23/2018, 4:11 PM
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

edwardwongtl

01/23/2018, 4:17 PM
A default value of
0
seems to be a nice choice
Since you're indicating the valid cells by
1-9
a

Ayden

01/23/2018, 4:18 PM
Ya. Whenever the cell = 0;
Then it will keep looping.
e

edwardwongtl

01/23/2018, 4:24 PM
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

Andreas Sinz

01/23/2018, 5:00 PM
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

edwardwongtl

01/23/2018, 6:06 PM
We didn't set the default value to the same space as the valid cells...
a

Andreas Sinz

01/23/2018, 6:30 PM
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

edwardwongtl

01/24/2018, 3:30 AM
@Andreas Sinz He said the function will loop when
cell == 0
before though
a

Ayden

01/24/2018, 1:58 PM
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

edwardwongtl

01/24/2018, 4:20 PM
@Ayden Its more light weight then a set of set IMO
a

andyb

01/25/2018, 8:42 AM
@edwardwongtl It's not more lightweight than just a standard
Copy code
class Condition(...)
though.
e

edwardwongtl

01/25/2018, 8:46 AM
@andyb You're right, but using a
data class
allows you to use the destructing syntax
3 Views