• 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 Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Ron McLeod
  • Paul Clapham
  • Devaka Cooray
  • Tim Cooke
Sheriffs:
  • Rob Spoor
  • Liutauras Vilda
  • paul wheaton
Saloon Keepers:
  • Tim Holloway
  • Tim Moores
  • Mikalai Zaikin
  • Carey Brown
  • Piet Souris
Bartenders:
  • Stephan van Hulst

ticTacToe trying to learn object oriented programming

 
Ranch Hand
Posts: 94
Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
so basically i have a two player pvp game of ticTacToe with a 3x3 board created buy an Array Board [2][2];.
My problem is trying to figure out how to tell whos turn it is (seems simple) im pretty sure i have a way to do it but would like yalls input to see if there is a better way.

So far i have is 4 classes MAIN, GAME, PLAYER and BOARD

the game class is what starts the game

Game Class pisuado code



One reason i would like to keep track of players turns is to not have two separate codes for each players turn.... I could call a player.playerOneTurn(); or something but then, my player class would get messy and not just be a blueprint.

Also to tell who won right now im just going to add a additional System.out "(players name) wins!".    

But is there a way to shorten my while loop and be able to tell whos turn it is so i can just write my checkWin() then if it returns win = true it will just run a gameOver() call to the board class that would display if it is a tie or which of the two players one.



My idea basically what i am thinking so far is using the playerTurn boolean and set it to false if its player ones turn and true if it is player twos turn.
so basically the checkWin() Call would say if(playerTurn = false) then player one wins or otherwise.

PS. also if you have a solution if you could show how you are passing stuff to and from classes including passing objects to a third class, I think this is what has been throwing me off.
 
Master Rancher
Posts: 5025
38
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

My idea basically what i am thinking so far is using the playerTurn boolean and set it to false if its player ones turn and true if it is player twos turn.  


That sounds like a reasonable solution.  The testing of the boolean variable would not be done with == or !=
The test for true would be the boolean's value itself or with the NOT (!) operator:  
if(player1Turn)  <<< test if player 1's turn
if(!player1Turn)  <<< test if player 2's turn (Not player 1's turn)

To swap turns use the ! operator:
player1Turn = !player1Turn; // swap turns
 
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
You could also just use reference equality. Since you only have two players, then this code should work:

IMO, this is more straightforward and reads better.
 
Marshal
Posts: 79800
385
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Find out about enumerated types for the symbols. I think you may need a Square class too. Your array would be Square[3][3] not [2][2]. 2 is the index, but the numbers you want are the sizes: 3.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
One problem I have with a Boolean variable named playerTurn is that it doesn't make any grammatical sense. Code that doesn't read well grammatically is difficult to understand or nonintuitive at best and misleading or outrightly nonsensical at worst.

There's a huge difference in the expressiveness of this code

versus the lack of expressiveness in this code:

Also, as a matter of style, don't compare a boolean variable with true or false. That is redundant and error prone because it's very easy to type = false instead of == false.
 
Sean Paulson
Ranch Hand
Posts: 94
Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:You could also just use reference equality. Since you only have two players, then this code should work:

IMO, this is more straightforward and reads better.




Ok i see what you are saying i will probably go with this.
 
Sean Paulson
Ranch Hand
Posts: 94
Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:One problem I have with a Boolean variable named playerTurn is that it doesn't make any grammatical sense. Code that doesn't read well grammatically is difficult to understand or nonintuitive at best and misleading or outrightly nonsensical at worst.

There's a huge difference in the expressiveness of this code

versus the lack of expressiveness in this code:

Also, as a matter of style, don't compare a boolean variable with true or false. That is redundant and error prone because it's very easy to type = false instead of == false.




makes sense i will for sure change it around a bit.
Yeah im a little rusty i forgot about comparing booleans like that thanks for pointing that out.
 
Sean Paulson
Ranch Hand
Posts: 94
Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:Find out about enumerated types for the symbols. I think you may need a Square class too. Your array would be Square[3][3] not [2][2]. 2 is the index, but the numbers you want are the sizes: 3.



ok i am reading about enum classes right now in java complete edition book and some videos right now. I am not sure how you are imagining implementing it to keep track of the symbols (could be i just have not read far enough)?
Also what would the square class be for? for the board?

Thanks for the comment.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Just to give you an idea for a simpler design, I just used a char passed to the Player constructor.

The reason I pass in a Game object is so that a Player object can call a Game object's mark(row, column, symbol) method. The constructor basically says ”you are playing in this game using this symbol for your moves."

My actual code looks like this:


This should give you an idea where this code resides. I got it all working using the base design I suggested earlier. I only had the two classes: Game and Player.

Well, three classes if you count the Console class I used to encapsulate a Scanner object for user input. Both the Game and Player objects used the Console utility class to get user input.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I can see how one might think that an enum could be useful but when coding it out, I never had a compelling reason to go that far. The only place where I use the literal symbol values is in the init method that I showed. I did, however, define a symbolic constant for a char SPACE to represent an empty spot on the board because there are a few places in the Game class where it is needed and char 0 isn't printable.

I suppose I should rename that constant to EMPTY instead to better reflect it's purpose rather than its implementation. Or NONE maybe.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Here's something I found somewhat interesting.

With the above code, you might reasonably expect that currentPlayer.makesMove() could return true or false. However, the implementation always returns true. That may seem pointless and therefore a little smelly but it helps avoid having to write this if makesMove() returned nothing instead:

I think the first version reads better and the code looks tighter. There is a caveat for the first version though: The call to currentPlayer.makesMove() must be on the left side of the && operator so that it is executed before the check for gameNotOver() is made.

Another small refactoring I might try would result in this:

Another option is to rename the method to something like switchTurn() but I think I like the more explicit assignment better because it also keeps the otherPlayer() method more functional by not having a side-effect.
 
Sean Paulson
Ranch Hand
Posts: 94
Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Ok guys this is what i ended up doing.....let me know what you think.

I have 5 classes actually seems like a lot but youll see why.

CLASSES:tickTackToe(main), game, player(player class is just a sub class to activePlayer class that does not have any methods), activePlayer, board.

i like the system i came up with to switch players. Also i was able to shorten my while loop in the game class by adding the active player class.

The hardest part was checking for a win....i did not change it i just used my old code...if you have a better idea please let me know.

Also i tried to make sure everything was as descriptive as possible

Game Class


currentPlayer CLASS




and the board CLASS

 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Thanks for sharing. I haven't tried it out for myself but you obviously put a lot of effort into writing that so congratulations on getting it running.

Your stated intent in doing this exercise was to learn object-oriented programming. We'll get to that later, if you want, because there are a lot of non-object-oriented things going on with what you did.  I'll point out a few style issues first:

1. Class names should be capitalized. So, instead of board, currentPlayer, and game, those should be Board, CurrentPlayer, and Game, respectively.

2. Your code is very poorly indented. Proper indentation helps you see which blocks of code belong together and helps give a sense of the structure of the code. Without proper indentation, your code is very hard to read. Your code is very kind of hard to read.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Now to object-orientation.

Methods should be short and they should generally be focused on doing one and only one thing. That "one thing" should be reflected in / suggested by the name of the method. That is, the method name should not suggest one thing being done while the method body does either something totally different, something more than what the name suggests, or something less than what the name suggests.  

For example, why would a player object clear the board when you call its setMove() method? "Set move" does not in any way suggest "clearing the board" as well. That's a case of a method doing something more than what its name suggests. This is what I call a "misassigned responsibility" because it's not really a player's responsibility to clear the board. That's something you'd expect a Board object to do.

Look at the startGame() method. That's a very long method and it does way more than just start the game.  

Here's what I came up with:

Do you see how very little code there is in each of these methods? These high-level methods give the outline of the "story" that tells what happens in the rest of the code without drowning the reader in all the nitty-gritty details of how it happens. This is the object-oriented principle of abstraction.  (Edit: I struck out "object-oriented" there because that would actually be misleading. Showing intent/purpose while hiding implementation details is what abstraction is about in general -- I'll get into what abstraction means in an object-oriented sense later, if you're interested)
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Sean Paulson wrote:

Junilu Lacar wrote:You could also just use reference equality. Since you only have two players, then this code should work:
...
IMO, this is more straightforward and reads better.


Ok i see what you are saying i will probably go with this.


I was hoping you would but you seem to have gone with what you had before instead. Here's another thing you're going to have to learn if you want to get better as a programmer: Don't get too attached to the code that you write. Learn to throw things out without hesitation.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Sean Paulson wrote:
I have 5 classes actually seems like a lot but youll see why.

CLASSES:tickTackToe(main), game, player(player class is just a sub class to activePlayer class that does not have any methods), activePlayer, board.


There are only 3 classes in the code you shared: Game, CurrentPlayer, and Board.  (using the properly capitalized class names)

activePlayer is a local variable of the Game.startGame() method. It is an instance of the CurrentPlayer class; it's not a separate class.

Your code references a player (should be Player) class but I don't see any code for that class. You must have it somewhere, otherwise your code will not compile. From what you said, it probably looks something like this:

Don't get confused between instances, variables, classes, and objects. These are related but they each have specific meanings that are not usually interchangeable with each other.

Edit: Actually, "instance" and "object" are usually interchangeable and in certain contexts you can get away with interchanging "class" and "object". The way you refer to the activePlayer variable as a class, however, is incorrect.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Speaking of misplaced responsibility, I actually cleaned up that play() method so that drawBoard() is called every time a player marks the board with a move. So the code becomes:

Again, you have very short and focused methods doing only what you'd expect they would do based on what their names imply.

The mark method is not private because it is called by a Player object. Since the Player class is in the same package, I just keep its visibility to package private (default). The other methods are private because only the TicTacToe class will call them.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
You wrote:

I suppose you did this to help the player specify which space to mark. I just added column and row numbers. Here's what a part of a run of my game looks likes:
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
You wrote:

This is actually very similar to what I wrote:

Which is clearer though? Do you see how keeping things abstract and hiding a lot of the implementation details can help make your code a lot better? Just reading my code, you can probably guess what the rowMarks(), colMarks(), and winsWithDiagonal() methods do. And I didn't have to write comments either. The code kind of explains itself. As much as possible, you want your code to be self-documenting. Good names and abstraction helps you achieve that.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
You can probably tell by now that I'm kind of obsessed with writing small, abstract methods. Here's what winsWithDiagonal() looks like:
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Here's one more step to more self-documenting code:

This is a result of extracting line 14, which used to be on line 7, to its own method and leaving an intention-revealing name in its place. This is known as Extract Method refactoring. This makes the play() method a Composed Method that has a Single Level of Abstraction
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
In the Board class, you wrote:

Reading just this method, these things come to mind:

1. Why do we need to save the parameter values with lines 75 & 76? The motivation to do that seems questionable and makes this code smelly.

2. Lines 78 and 79 are not idiomatic. Should be < 3 instead of <= 2. I already commented on this previously.

3. Line 82 is kind of smelly, a return statement from deep inside a nested for-loop is not something you want to get into a habit of doing.

4. Lines 86 and 87 mix in the task of displaying a message with the other things that the method is doing that's not related to display. Try to keep display tasks separate from calculations.

5. The name setMove is not logically or grammatically compatible with a Boolean return value. "Set move" is an imperative sentence, it's a command to do something. A boolean return value implies a query that is answerable by "Yes" or "No", "True" or "False".  Again, you are mixing concerns here and crossing signals. This makes the semantics of the setMove() method confusing.

6. Method names that start with "set" are usually taken to be following the Java bean convention for mutator method names, also known as setters.  So, a method named setName() is expected to set the value of an object's name field to something. Your setMove() method does not behave like a conventional mutator/setter. That makes your code misleading.

7. Your use of the names playerSymbol and userSymbol seems arbitrary. Why the change from "user" to "player"? How does the "user" prefix add/contribute to the semantics of the code? There might be some significance in naming the parameter as just symbol and then assigning its value to a variable named playerSymbol but I don't see any value in essentially saying that "the userSymbol is the playerSymbol".  This is assuming that you can even justify the need to save these values in the first place, as I noted in #1 above.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
In the Board class, you wrote:

1. If you just want to print an empty line, you don't have to pass anything to println(), just write System.out.println() without any parameter value.

2. This method's name is a lie. It does not clear the board at all. In fact, it doesn't even touch the board in any way. It prints out 50 empty lines so that it effectively scrolls the console display until it's "clear" but that only works if your console display has up to 50 visible lines. If your window is taller than 50 lines then this method won't even really completely clear the display.

3. And because it's not idiomatic, your for-loop is misleading. It's actually iterating 51 times, not 50 times. If you get used to writing this kind of non-idiomatic code, you're going to end up potentially writing a lot of bugs or causing others who have to read/maintain your code misunderstand what you're doing or worse, introduce more bugs.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
This is my Player class. I just put it in the same file as the TicTacToe class so it's a non-public class.

Notes:

1. The Keyboard class just encapsulates a Scanner object. Its inputInt() method is a convenience method that displays the specified prompt text then returns the next int the user enters on the console.

2. The moves() method is probably the messiest method in this entire program. I find it messy because it looks like it violates the guideline to keep display-related tasks from non-display-related tasks. But it's not so messy that I have a compelling reason to separate things further. It does a few different things but I think it's still cohesive and coherent enough as a whole. Sometimes you just can't get everything perfect.  

3. Notice how a game object is passed in to the Player constructor. This makes the Player object "aware" of the TicTacToe game object that it's going to have to interact with.  On line 20, the game object is asked whether the row and column that the player just entered constitutes a valid move.  That method will return false if that space on the board has already been marked or if the coordinates (row, column) are not valid.  You might be wondering what while (error = !game.canMark(row, col)); does. There's some cheating and trickery involved here but it's all legal Java syntax. What that does is calls game.canMark() first. The ! operator flips the value returned by the game.canMark() method. The value is then assigned to the boolean error variable. That is, if the game says that the space can be marked (canMark returns true), then error will be set to false. If the game says the space can't be marked for some reason (returns false), then error will be set to true.  The while condition simply ends up being whatever value is assigned to error. So, if there is an error, the loop repeats, otherwise, it terminates.

4. Once a valid move has been entered by the player and the do-while loop terminates, the game.mark() method is called. Because we told the Player object which symbol it was going to use in the game, it simply passes that symbol to the game.mark() method along with the row and column numbers that have already been validated. Note that the Player object has no knowledge whatsoever of the actual value of symbol. That is, it doesn't know or care if the symbol it's using is an 'X' or an 'O' or whatever. There is no code in Player where a literal value of symbol is hard-coded. It's the Game's responsibility to "know" what symbols the Player objects are using and that "knowledge" is concentrated in the game object's init() method and nowhere else. This is in keeping with the Don't Repeat Yourself (DRY) Principle of programming.

5. I already discussed why the moves() method always returns true in a previous reply.

This illustrates what object-oriented programming is about at its core: it's about abstracting and assigning responsibilities and behavior and lumping them together with relevant information in a self-contained program entity.

A well-factored object-oriented program then becomes like a choreographed dance between any number of these self-contained program entities (objects) that interact and work together to accomplish a goal.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
And in keeping with my obsession for small and abstract code, here's what the TicTacToe.canMark() method looks like:
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I just got done facilitating a Global Day of Code Retreat over the weekend so I'm still in kind of a "stretch yourself" mood. I said that I didn't have a compelling reason to refactor the Player.moves() method further. I guess I was wrong. You can do some reification and turn (row, col) into a real object:

Here, I have reified (row, column), two separate values that when taken together signify a "position" on the game board and made it into a real object of type Position. That causes a lot of ripple refactoring but it can be done slowly and methodically.

You can make that even smaller like this:

Note that I added a comment to explain WHY the return value is hard-coded to true. To channel The Most Interesting Man in the World, I don't often write comments but when I do, it's to explain WHY I'm doing something.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
After reifying (row, col) into a Position object, this code:

becomes this:

That reads even better, IMO.

All the ideas behind the refactorings I've shown so far are from Corey Haines' excellent book, Understanding the Four Rules of Simple Design available on LeanPub.com for less than US$20.  It's a small 70-page e-book but it's chock full of insights and advice about object-oriented programming and design. He uses Ruby in his examples but the ideas are easily applied in Java, as you can probably see here.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
in your Game class, you wrote:

1. This code is not object-oriented. It's very procedural in nature. It keeps information that's relevant to a Player in the Game class so you have to take extra efforts to track which is which and who is who.

2. The variable name switchTurn still does not have very good semantics. When true, it signals that it's the first player's turn to move. When false, it signifies that it's the second player's turn to move. That name just doesn't make any grammatical sense to me in that context that it's used. Renaming the variable to something like playerOnesTurn would at least come closer to making sense because if was true, then yes, it would be playerOne's turn to move and if false, then no, it's not playerOne's turn to move therefore it must be playerTwo's turn to move.

3. Don't compare boolean variables to true or false. It's redundant. You probably feel compelled to do it because the variable name does not lend itself to making the code read more naturally as a true/false statement. If you changed the name to playerOnesTurn, it would read better:

4. I don't really see why you even need to count how many moves have been made. You can just go to the board and see if somebody won and/or if all the spaces have been marked. Having that moveCount variable just adds one more thing to manually track that you could easily calculate or infer from the current state of the board.

5. Regardless of any of the above improvements you could make, that whole section is just kind of clunky and there's way too much code than there needs to be, IMO.

Consider the alternative way of doing it that I suggested before:

Where otherPlayer() is simply:

It's straightforward, expressive, and simple.
 
Sean Paulson
Ranch Hand
Posts: 94
Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:Now to object-orientation.
... This is the principle of abstraction ... Showing intent/purpose while hiding implementation details is what abstraction is about in general


ok that makes a lot of sense, I actually have not learned abstraction yet it was coming up in a couple chapters

[Moderator edit: elided long quote]
 
Sean Paulson
Ranch Hand
Posts: 94
Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:You could also just use reference equality.


im not sure if i know what you mean by using reference equality.

[Moderator edit: shortened long quote]
 
Sean Paulson
Ranch Hand
Posts: 94
Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:
Your code references a player (should be Player) class but I don't see any code for that class. You must have it somewhere, otherwise your code will not compile. From what you said, it probably looks something like this:



yeah i have more classes i can upload them if you would like a look at it.
Also yes the player class just extends to the "currentPlayer" class (did not mean to say activePlayer before) i did think having a empty Class (Player) was a little weird and maybe not correct.

[Moderator edit: shortened long quote]
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Sean Paulson wrote:im not sure if i know what you mean by using reference equality.


Please don't quote the entire response. It makes the thread much longer than it needs to be and more difficult to read. Delete the parts of the quote that are not directly relevant to your response. Thanks.

Reference equality simply means you're using == to compare reference variables. In my code, playerX, playerO, and currentPlayer are all reference variables that point to a Player object. When you use == to compare two reference variables, that's checking for reference equality. You're basically checking if the two variables point to the same object in memory.

In contrast, the equals() method is supposed to check for logical equality, assuming that the equals() method that you're calling has been overridden. When it hasn't been overridden to provide custom behavior, equals() will be inherited from the base Object class which really just boils down to being the same as reference equality (checking with ==).  I use == in my code to make it clear that I'm using reference equality checks rather than a logical equality check.

The caveat here is that when it comes to comparing objects with each other, you're usually going to use an overridden equals() method to check for logical equality. In this case, however, reference equality is fine and, IMO, appropriate.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Sean Paulson wrote:yeah i have more classes i can upload them if you would like a look at it.


No need to do that, there's enough to discuss in the code that you have already posted.

I hope the feedback I've given you so far is useful and will help you write better code going forward.
 
Sean Paulson
Ranch Hand
Posts: 94
Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:Now to object-orientation...



Ok for sure like this a lot. I also like how simply you are switching players by creating another currentPlayer obj? otherObject (basically a temporary holder).

Moderator edit: shortened long quote. @OP - Please don't quote the entire response.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Sean Paulson wrote:I also like how simply you are switching players by creating another currentPlayer obj? otherObject (basically a temporary holder).


My currentPlayer variable is not really that much different in purpose as your activePlayer variable.  The difference is that you're using more brute force to track the program state. Good object-oriented programs have objects track their own states. Calling method will cause objects to change their own states depending on the semantics of the methods being called. In contrast, procedural-style code takes values out of the object via getter methods, changes them, then puts them back into the object via setter methods.  This is NOT good object-oriented practice.
 
Sean Paulson
Ranch Hand
Posts: 94
Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:Here's one more step to more self-documenting code:

This is a result of extracting line 14, which used to be on line 7, to its own method and leaving an intention-revealing name in its place. This is known as Extract Method refactoring. This makes the play() method a Composed Method that has a Single Level of Abstraction



so more methods to keep the code cleaner and simpler. also i have not even heard of extract method refactoring or the such (i dont even think the books im using mention those:( ) guess i have a bit more studding to do. (not that i mind lol)
 
Sean Paulson
Ranch Hand
Posts: 94
Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:In the Board class, you wrote:

Reading just this method, these things come to mind:

1. Why do we need to save the parameter values with lines 75 & 76? The motivation to do that seems questionable and makes this code smelly.

2. Lines 78 and 79 are not idiomatic. Should be < 3 instead of <= 2. I already commented on this previously.

3. Line 82 is kind of smelly, a return statement from deep inside a nested for-loop is not something you want to get into a habit of doing.

4. Lines 86 and 87 mix in the task of displaying a message with the other things that the method is doing that's not related to display. Try to keep display tasks separate from calculations.

5. The name setMove is not logically or grammatically compatible with a Boolean return value. "Set move" is an imperative sentence, it's a command to do something. A boolean return value implies a query that is answerable by "Yes" or "No", "True" or "False".  Again, you are mixing concerns here and crossing signals. This makes the semantics of the setMove() method confusing.

6. Method names that start with "set" are usually taken to be following the Java bean convention for mutator method names, also known as setters.  So, a method named setName() is expected to set the value of an object's name field to something. Your setMove() method does not behave like a conventional mutator/setter. That makes your code misleading.

7. Your use of the names playerSymbol and userSymbol seems arbitrary. Why the change from "user" to "player"? How does the "user" prefix add/contribute to the semantics of the code? There might be some significance in naming the parameter as just symbol and then assigning its value to a variable named playerSymbol but I don't see any value in essentially saying that "the userSymbol is the playerSymbol".  This is assuming that you can even justify the need to save these values in the first place, as I noted in #1 above.



to answer these
1.I am not really sure, I think it is because i have not read i did not have to...but in setter methods they usually do so i guess i assumed it was proper coding. ps and from your last statement the use of this is just if you need to save the variable.

2, 5 and 6.yes that is because it is supposed to set the players board position...i guess that is not technically a setter method. Also i was using this to set the players move on the board and check for an error...i suppose that should be two different methods.

3.Should i use a break statement and just change the value of win?

7. Kind of the same reason as answer one. I was shying away from using the this. operator.

 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Sean Paulson wrote:i have not even heard of extract method refactoring or the such (i dont even think the books im using mention those


The granddaddy of refactoring books is Martin Fowler's Refactoring: Improving the Design of Existing Code which was published way back in 1999. It's still a great book to read and it should be required reading for all students of programming.

It's unfortunate that very few schools teach refactoring in the introductory programming courses. I have always asserted that teaching at least Refactor Rename, Refactor Extract, and Compose Method refactoring as fundamental techniques for keeping code clean is totally appropriate and absolutely necessary, especially for beginners. There's nothing complicated about these techniques and most modern IDEs like Eclipse and IntelliJ IDEA have great built-in support for rename and extract refactoring. If you want to get better, learn how and why you should try to do those three refactoring techniques for almost every line of code that you write. When I'm coding, these three techniques make up 80% to 90% of the refactoring that I do, and I do it a lot. Like I said, almost every line of code I write undergoes intensive refactoring before I'm done with a program.

In this thread alone, there are a number of instances where those three techniques have been applied to great effect and benefit to the quality of the code.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I purposely didn't dump my code all at once so that you'd have to go through each response and understand the points I was making about each snippet of code that I shared. If you go through this thread and pull together all the snippets I posted, you can see what I wrote in pretty much its entirety, with the exception of a number of low-level methods that I intentionally left out.  (I'll leave it to you to try to implement those as an exercise).

Here are just some things about the code I wrote for this problem:

1. The longest methods are TicTacToe.play() and TicTacToe.drawBoard() with 7 lines of code each and Player.pickAnOpenSpot() with 10 lines of code.

2. Most of my other methods have only one line of code in them and it's usually a single return statement that performs some kind of calculation. There are a few methods with 2 to 5 lines of code. I have a total of 207 lines of code in my program and that includes generous white spaces and some high-level comment sections. If I just count executable lines of code, it's probably somewhere around 150 to 160.

3. The trick to getting code that's as concise as that is to try to stay as abstract as you can until you get to a point where can't decompose the problem into smaller problems any more and you have to write some implementation details.
 
Junilu Lacar
Sheriff
Posts: 17665
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Sean Paulson wrote:

Junilu Lacar wrote:3. Line 82 is kind of smelly, a return statement from deep inside a nested for-loop is not something you want to get into a habit of doing.

3.Should i use a break statement and just change the value of win?


Part of the objection I have to having a return statement like this is that it's in the middle of a larger section of code. I actually had a similar construct at first, except the method was only 9 lines of code. I actually refactored it down to a single line of code. Imagine that, from 9 lines of code that looked like the code above, down to this one-liner:

I guess the point to my question really was to hint that you don't really need to do any of that. IMO, the design you have of putting a string in the board and having the player enter a string to indicate which space he wants to mark is just not a very good design decision. It's kind of goofy, to be brutally honest. The design you chose practically forces you to write that very awkward code.  Like I said before, you need to learn to throw things away when they don't work well. This is one of those things I think you should flush down the drain because it stinks.
 
You didn't tell me he was so big. Unlike this tiny ad:
Smokeless wood heat with a rocket mass heater
https://woodheat.net
reply
    Bookmark Topic Watch Topic
  • New Topic