Skip to main content
added 312 characters in body
Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 160
  • 312

It's method extraction time!

But before that, I have to comment on your Player enum:

  • str is a bad name, name would be better.
  • Speaking of name, all enums have a name() method by default. You don't need your strstr variable, return name(); instead.
  • Speaking of return name();, that's exactly what the default implementation of toStringtoString() already does for enums. Absolutely no need to override it.

And therefore, we've reduced your PlayerPlayer enum to:

public enum Player {
    X, O;
}

Ain't it lovely with enums? :)

Now, back to your getWinner() method:

You have a whole bunch of duplicated code there indeed. It would be handy if you could get a Collection of some kind (or an array), add some elements to it and check: Is there a winner given by these Player values?

This is just one version of doing it, it's not the optimal one but it should get you started. The idea here is that weThis code will add a bunch of PlayerPlayer objects to a list and then we check if those Player objects match to find if there's a winner or not.

List<Player> players = new ArrayList<>();
for (int x = 0; x < fields.length; x++) {
    for (int y = 0; y < fields[x].length; y++) {
        players.add(fields[x][y]);
    }
    Player winner = findWinner(players);
    if (winner != null) 
        return winner;
}

Player findWinner(List<Player> players) {
    Player win = players.get(0);
    for (Player pl : players) {
        // it's fine that we loop through index 0 again,
        // even though we've already processed it. 
        // It's a fast operation and it won't change the result
        if (pl != win)
            return null;
    }
    return pl;
}

Please note that there are even more improvements possible for this getWinnerthe getWinner method, I don't want to reveal all my secrets for now though ;) ThisAnd also, this is just one way of cleaningdoing it up, which will reduce your code duplication a bit at least. There are other possible approaches here as well.

It's method extraction time!

But before that, I have to comment on your Player enum:

  • str is a bad name, name would be better.
  • Speaking of name, all enums have a name() method by default. You don't need your str variable, return name() instead
  • Speaking of return name(), that's exactly what the default implementation of toString already does for enums.

And therefore, we've reduced your Player enum to:

public enum Player {
    X, O;
}

Ain't it lovely with enums? :)

Now, back to your getWinner() method:

This is just one version of doing it, it's not the optimal one but it should get you started. The idea here is that we add a bunch of Player objects to a list and then we check if those Player objects match to find a winner or not.

List<Player> players = new ArrayList<>();
for (int x = 0; x < fields.length; x++) {
    for (int y = 0; y < fields[x].length; y++) {
        players.add(fields[x][y]);
    }
    Player winner = findWinner(players);
    if (winner != null) 
        return winner;
}

Player findWinner(List<Player> players) {
    Player win = players.get(0);
    for (Player pl : players) {
        // it's fine that we loop through index 0 again,
        // even though we've already processed it. 
        // It's a fast operation and it won't change the result
        if (pl != win)
            return null;
    }
    return pl;
}

Please note that there are even more improvements possible for this getWinner method, I don't want to reveal all my secrets for now though ;) This is just one way of cleaning it up, which will reduce your code duplication a bit at least.

It's method extraction time!

But before that, I have to comment on your Player enum:

  • str is a bad name, name would be better.
  • Speaking of name, all enums have a name() method by default. You don't need your str variable, return name(); instead.
  • Speaking of return name();, that's exactly what the default implementation of toString() already does for enums. Absolutely no need to override it.

And therefore, we've reduced your Player enum to:

public enum Player {
    X, O;
}

Ain't it lovely with enums? :)

Now, back to your getWinner() method:

You have a whole bunch of duplicated code there indeed. It would be handy if you could get a Collection of some kind (or an array), add some elements to it and check: Is there a winner given by these Player values?

This is just one version of doing it, it's not the optimal one but it should get you started. This code will add a bunch of Player objects to a list and then we check if those objects match to find if there's a winner.

List<Player> players = new ArrayList<>();
for (int x = 0; x < fields.length; x++) {
    for (int y = 0; y < fields[x].length; y++) {
        players.add(fields[x][y]);
    }
    Player winner = findWinner(players);
    if (winner != null) 
        return winner;
}

Player findWinner(List<Player> players) {
    Player win = players.get(0);
    for (Player pl : players) {
        // it's fine that we loop through index 0 again,
        // even though we've already processed it. 
        // It's a fast operation and it won't change the result
        if (pl != win)
            return null;
    }
    return pl;
}

Please note that there are even more improvements possible for the getWinner method, I don't want to reveal all my secrets for now though ;) And also, this is just one way of doing it, which will reduce your code duplication a bit at least. There are other possible approaches here as well.

Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 160
  • 312

It's method extraction time!

But before that, I have to comment on your Player enum:

  • str is a bad name, name would be better.
  • Speaking of name, all enums have a name() method by default. You don't need your str variable, return name() instead
  • Speaking of return name(), that's exactly what the default implementation of toString already does for enums.

And therefore, we've reduced your Player enum to:

public enum Player {
    X, O;
}

Ain't it lovely with enums? :)

Now, back to your getWinner() method:

This is just one version of doing it, it's not the optimal one but it should get you started. The idea here is that we add a bunch of Player objects to a list and then we check if those Player objects match to find a winner or not.

List<Player> players = new ArrayList<>();
for (int x = 0; x < fields.length; x++) {
    for (int y = 0; y < fields[x].length; y++) {
        players.add(fields[x][y]);
    }
    Player winner = findWinner(players);
    if (winner != null) 
        return winner;
}

Player findWinner(List<Player> players) {
    Player win = players.get(0);
    for (Player pl : players) {
        // it's fine that we loop through index 0 again,
        // even though we've already processed it. 
        // It's a fast operation and it won't change the result
        if (pl != win)
            return null;
    }
    return pl;
}

Please note that there are even more improvements possible for this getWinner method, I don't want to reveal all my secrets for now though ;) This is just one way of cleaning it up, which will reduce your code duplication a bit at least.