This week's book giveaway is in the Reactive Progamming forum.
We're giving away four copies of Reactive Streams in Java: Concurrency with RxJava, Reactor, and Akka Streams and have Adam Davis on-line!
See this thread for details.
Win a copy of Reactive Streams in Java: Concurrency with RxJava, Reactor, and Akka Streams this week in the Reactive Progamming forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Liutauras Vilda
  • Junilu Lacar
  • Jeanne Boyarsky
  • Bear Bibeault
Sheriffs:
  • Knute Snortum
  • Tim Cooke
  • Devaka Cooray
Saloon Keepers:
  • Ron McLeod
  • Stephan van Hulst
  • Tim Moores
  • Tim Holloway
  • Carey Brown
Bartenders:
  • Piet Souris
  • Frits Walraven
  • Ganesh Patekar

What's wrong with my TicTacToe from an OO standpoint?

 
Greenhorn
Posts: 6
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I am fresh-new to OOP and Java. I had some brief experience in procedural languages like C.

Straight after OO HelloWorld I have written this TicTacToe. What I am trying to say: I have no experience with object oriented paradigm, thus I don't know what kind of mistakes I make.

Would anyone be able to point out my mistakes? I mostly care about the OO mistakes, anti-patterns and such. Not necessarily the game logic itself.

InputManager.java:


Player.java:


Board.java:


Game.java:


Main.java:


Also added following code to gist: https://gist.github.com/Wenox/03cf0dc45aef0bb22820e579659bbf9a

If anyone could point out the mistakes I've made from the object-oriented standpoint, I could appreciate it.

As I have no experience at all I couldn't reason whether X solution would be better than Y solution. I did whatever "felt right".

For example, Game class is never instantiate and uses only static fields. I also feel like that class is bit too long.

I placed `makeMove()` function within `Game` class in order to avoid cyclic class dependency; I am not sure if that function should belong in there or not. Also, because Game class never uses its constructor, I am creating other objects the Game class needs by using initGame() static function. I suppose I could as well do that in a constructor.

I am basically wondering what's wrong the most with my code.

Advices appreciated.
 
author & internet detective
Posts: 39530
776
Eclipse IDE VI Editor Java
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Nani,
Welcome to CodeRanch!

I would change to things to improve this code:

1) Get rid of all the static keywords from methods and variables (except main()). This will get you to write new Game() and be more OO
2) If you've learned about 2D arrays, switch to new int[3][3] instead of new int[9]. It'll be clearer as to which element is which.
 
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
In Board.java, line 16, you have "X" and "O" literal values specified. That's "knowledge" of the values of the player marks recognized by the Board class.

In Game.java, lines 10 and 13, you have the same literal values specified. That's a duplication of the knowledge coded in Board.java. This violates the DRY Principle. This is bad because if for some reason, someone decides to use different characters, then you have to remember to keep the knowledge in both places in synch. If you forget to keep them in synch, the program will not work properly. Also, what's the use of the Player.mark field if the only two values you're using are going to be "X" and "O" anyway?
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Jeanne Boyarsky wrote:
2) If you've learned about 2D arrays, switch to new int[3][3] instead of new int[9]. It'll be clearer as to which element is which.


The structure you use should not have much impact on the clarity of the higher-level code you write. That's an implementation detail. I have written this game with an int[9] without much impact to the clarity of the higher level code. In fact, the lower-level implementation code wasn't unclear either. But it all depends on the programmer really focusing on writing expressive code and pushing implementation details down as low as they be pushed.

You could also use a Map. I find that using a [3][3] array actually tends to make the UI less user-friendly. Compare these prompts:

The second user interface is more cumbersome and whenever I see it in people's code, I can guess 99% of the time that they're using a [3][3] array of some sort underneath. That's a leaky abstraction. On the other hand, the first user interface that asks only for one digit can be implemented a few ways, including a [3][3] if you really feel it's clearer. I personally don't see it being that much clearer based on my experience writing this program (I use it as the problem for a number of programming workshops I have facilitated)
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
An example of what I mean by writing clear high-level code:

This certainly is not very clear. It's all implementation detail. It can easily be refactored for clarity with a few Extract Methods:

Guess where all that noisy code got extracted to...

Now it's easy to make that even more succinct:

 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

I just wrote:
Now it's easy to make that even more succinct:


If you think about it, the Game class has a natural state it keeps: who the current player is. And if you think about it some more, you don't really need to pass the player as a parameter because the game can only ever be won by the current player after they make a valid move. So, if you only call isWon() after a valid move has been made by the current player, then the Game already knows who the player is that you should check to see if they won: the currentPlayer, whoever that may be. Thus, you can simply write this:

But I agree with the point about making these instance methods instead of static methods.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Having movesNum and the static methods related to it smell like a misplaced responsibility. Why would the Player class be responsible for that? That seems like something the Game class should be tracking, not the Player class.

Another smell is that the Player class has no interesting behavior; it only has data fields and getters and setters. This is the smell of an Anemic Domain Model.

This actually relates to the smell of misplaced responsibility for the movesNum concept. If the Game class is always calling the Player class and checking on the state of the movesNum value, and the Player class is just basically a data store, then the movesNum field needs to be moved closer to the logic that uses it and affects it. That means moving it to the Game class and removing all the static methods related to it from the Player class.

In my implementations of this game, I didn't even care about counting number of moves. I simply iterated over the board to see if there was at least one unmarked space in it. The method that does that can be the isDraw() method or the hasAvailableMove() method. It all depends on how you want to tell the story in the code.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
InputManager is a thin facade for a Scanner that is tightly coupled to System.in and is not a particularly useful class because 1. You've limited what you can do to get input down to two methods instead of the many methods in a Scanner. 2. By tightly coupling to System.in, you've deprived yourself of a useful seam, a place you can change behavior without changing code, that can facilitate unit testing.
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Line 53 in the Game class is a poor use of exception handling. Don't catch those kind of exceptions; they are exceptions that you should allow to be thrown so that you can find programming errors when you're testing. Instead, validate input and display a message if the value is out of range. The exception handling mechanism should be used for exceptional cases and invalid input by a user does not rise to that level of severity.

See use exceptions for exceptional cases but keep your critical thinking hat on because you'll find that there are two sides to that story. I'd advise you to read both sides, pro and con, to get a handle of the breadth of opinion. If you're at least aware of different opinions, you can start forming your own and make informed choices.
 
Sheriff
Posts: 6266
167
Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Junilu Lacar wrote:I find that using a [3][3] array actually tends to make the UI less user-friendly. Compare these prompts:


But the implementation wouldn't force you to have any particular UI, would it?  You could implement a 1-9 UI and translate it into a [3][3] array:
 
Junilu Lacar
Marshal
Posts: 14039
234
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Knute Snortum wrote:But the implementation wouldn't force you to have any particular UI, would it?


No, not necessarily. However, my experience is that when I see a UI that asks for row and column to be entered, the implementation is more likely than not to be a [3][3] array. In my mind, there's a definite influence and correlation there. Jeanne's advice seems to support that idea. Your mileage may vary, of course.

You could implement a 1-9 UI and translate it into a [3][3] array:
...


Absolutely. I think that's exactly what I said earlier, or at least tried to say. If it didn't go over that way, then I must not have said it right.

I wrote:On the other hand, the first user interface that asks only for one digit can be implemented a few ways, including a [3][3] if you really feel it's clearer.

 
It's exactly the same and completely different as this tiny ad:
Java file APIs (DOC, XLS, PDF, and many more)
https://products.aspose.com/total/java
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!