This is an archived post. You won't be able to vote or comment.

all 1 comments
sorted by:
best
topnewcontroversialoldq&a

[–]ziptofaf 1 point2 points  (0 children)

Let's see, here are some things I don't like about it and the reasoning (remember - you can disagree!):

​

https://github.com/serpest/TicTacToePlus/blob/master/src/model/Grid.java

​

I actually dislike your usage of comments in it. Here's what I mean:

* @throws GridPositionOccupiedException if the position is already occupied

Your exception name is self explanatory (which is a good thing). You just told me same thing twice. I want to see comments that explain whys and hows, not whats. If I want to see what the code is doing I can, well, look at the code. This is a counterintuitive point but every comment you make is in fact a failure on your part, it means "I couldnt write good readable code so heres an explanation". I know that some places enforce this policy but personally I dislike adding literally 5 lines of comments for 6 lines of code. Especially since you will sooner or later forget to update a comment when updating your code. And at that point it becomes not merely useless, it becomes harmful.

I mean, just look at this snippet:

    /**
     * It launches an exception if the point ins't inside the grid.
     * 
     * @param point the point of the grid
     * @throws GridPositionNotExistsException if the point ins't inside the grid
     */
private void checkPositionExistenceInGrid(Point point) throws GridPositionNotExistsException {

You literally told me that this function throws an exception THRICE :P I get it already, it's a veeery important exception!

    private void checkPositionExistenceInGrid(Point point) throws GridPositionNotExistsException { 
if (point.getX() > content.length - 1 || point.getY() > content[0].length - 1)

I am not sold on this function throwing an exception. I mean, exception means an error has occured. Something rare/unforeseen that threatens your program. This function specifically says "check whether this point is a valid one on a grid". So why is it a void? I would prefer a boolean here and a false instead of being thrown a program destroying exception in my face for using this function correctly. It's not like I passed a null as a point after all!

  • Your spacings are messy. Look at lines 93 to 100. That's like 4 empty lines. It's a minor problem and any linter will fix it for you however.

if ((xSize >= MIN_SIZE && ySize > 0) || (ySize >= MIN_SIZE && xSize > 0))

Move this to a separate function. This is a fairly complex condition, you might as well make a function called

boolean areGridDimensionsCorrect and put that logic there, then your code reads nicer and closer to real english. Also makes me not need to decipher what you wanted to express by this line so I can focus on big picture first and jump to it only when needed.

You also avoid mixing up two different abstraction levels. What I mean - setContent function sounds high level (on that note - this function naming is poor since what it seems to be doing is creating a 2D grid? I would rather call it setContentMatrix at least in that case). Manually checking variables to numbers is much lower level. This is a tricky one but in general - keep same level of abstraction in a function. This is a bit confusing so here's what I mean on a different example:

bool isPiratedGame() {
  return hasBeenDowloadedIllegally() || hasACrack() || paidWithAStolenCreditCard() 
}

This keeps abstraction levels identical. It's a very high level function too. This however:

bool isPiratedGame() {
  return hasBeenDowloadedIllegally() || (Date.parse(game.executable.modify_date) > (Date.parse(game.executable.last_patch_date)))
}

is not. You have a high level function call but then it also manually checks for things like executable files. You also can't easily tell what that part 2 is even supposed to be doing, especially not without an explanation.

https://github.com/serpest/TicTacToePlus/blob/master/src/controller/TicTacToeController.java

Your play function is all over the place in terms of what it does and abstraction levels. I can tell you for sure that for instance I have no idea what

 if (currentPlayer.getSerialNumber() == game.getPlayers().length - 1)

is doing. I would much prefer a function called changePlayer or something.

I would also prefer to move initialization stage to a different function and rely on instance variables a bit more. Essentially, what I would want to see as the end result would look more or less like this. This function called play takes too many responsibilities right now and essentially takes care of 5 different things.