• 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
  • Ron McLeod
  • Paul Clapham
  • Bear Bibeault
  • Junilu Lacar
Sheriffs:
  • Jeanne Boyarsky
  • Tim Cooke
  • Henry Wong
Saloon Keepers:
  • Tim Moores
  • Stephan van Hulst
  • Tim Holloway
  • salvin francis
  • Frits Walraven
Bartenders:
  • Scott Selikoff
  • Piet Souris
  • Carey Brown

2 players take turns consecutively

 
Greenhorn
Posts: 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi, I'm trying to build a game where 2 players take their turns to roll a die and see who gets all 6 numbers first. I've pretty much done everything, however eventhough I have made one player's button disabled on the other player's turn, it won't switch turn until a new number is rolled. So for example if player 1 roll 5 on his first turn, it will switch to player2, and when it switch back to player 1, it will be player 1 turn forever until he rolls a number different from 5. Here are my codes

Die.java - this class is used to generate a random number


DisplaySixNumbersPanel.java - this is the main class to run the whole game


SixNumbersPanel2.java - this is where my ActionListener is located


Player.java  -  I have a method call Complete() in here so that it will be called in SixNumbersPanel2 later to see if a player has completed the game


Please help me to find a way to solve this problems. This is my first post so I apology for any errors I made when posting this. Any helps would be appreciated.
 
Greenhorn
Posts: 15
3
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

I'm trying to take a look through now, but I must say it's incredibly difficult to follow - identifiers like label1, label2, check1, flag, text1, text2 and the like aren't descriptive at all, especially when combined with a distinct lack of comments. It's especially confusing when you player1 is both a SixNumbersPanel2 and a Player, and in one case player1 is player2 and in another player2 is actually player1, and so forth. Making all that more clear would greatly increase readability.

After slowly stepping through it, I've worked out what most of it does - but I'm slightly confused: player1.Complete() is called in the ActionListener, but the Complete() method performs it's check based on results[], which has never changed since results[] is only modified in play() which is never called, so it should never return true.

Nevermind, just noticed that Complete() always returns true no matter what. Correct me if I'm wrong, but it would appear that the only thing ever used from the Player class is the Check method.

I think I've solved your problem:
I'll use your example to explain.
- Player 1 rolled a 5 on his first turn
- Player 2 takes his turn
- Player 1 rolls a 5
Then,
check = player1.Check(list,key);
is called.
The binary search finds that the number 5 is already in the array, so it returns the index.
This index is greater than 0, so none of the code inside the if(check<0) block is executed, hence control is not passed back to the other player.

Can I also recommend that instead of implementing ActionListener and using getSource to add different behaviour for different buttons, to use lambda expressions like the following:
[code=java]
button.addActionListener((ActionEvent event) -> {
//Whatever you want to happen when the button is pressed
});
[/java]
You'd do this at the same time as you construct the button and set it's name and the like.
You can still separate your logic from your UI initialisation by calling a method inside that lambda expression.

Hopefully this helps, I don't mean to be too harsh, but make your code more readable!!! It'll help people help you fix real problems way more easily in the future.













 
Marshal
Posts: 70228
282
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Matt Pitt wrote:. . . After slowly stepping through it, I've worked out what most of it does  . . .

But you shouldn't have to step through code slowly, taking the sort of painstaking effort which will soon earn you a cow As you say, the code itself should be clear, but I think there is even more serious a development problem here: the presence of a GUI at this stage.
OP: You should get the logic of your application working (it is often called, “business logic,” but the same applies to the logic of a game) before trying to write a GUI. The logic should reside in different classes from the GUI; the GUI is there simply to provide the display. Yes, you may have buttons that provide signals for the next move in the game, but whatever they do should be channelled via existing methods in the game's public interface.
Never write == true or == false. Both are poor style and very error‑prone: we see people who write = by mistake and now have two errors for the price of one.
Never
if (b == true) ...
Always
if (b) ...
Never
if (b == false) ...
Always
if (!b) ...

There is lots more to comment about your code. But if you had the game working before the GUI, you wouldn't have such problems.

There are lots of ways to make two players take turns. We had a discussion about that from about three years ago. Here are two techniques which came up:-
  • 1:  Put the players into an array, incrementing the index and using the % operator.
  • 2:  player = player.getOpponent(); (or ....getNextOpponent())
  • I think there were more ways to do it. You can implement that sort of thing very easily in the game classes.
     
    Bartender
    Posts: 4066
    156
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Perhaps you could simplify the game a little, by calculating all the rolls for a player upfront. Not only makes that the code a little easier, but you have all the information before the game starts. For instance
     
    Marshal
    Posts: 15875
    265
    Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator

    Matt Pitt wrote:I don't mean to be too harsh, but make your code more readable!!! It'll help people help you fix real problems way more easily in the future.


    Campbell Ritchie wrote:

    Matt Pitt wrote:. . . After slowly stepping through it, I've worked out what most of it does  . . .

    But you shouldn't have to step through code slowly, taking the sort of painstaking effort which will soon earn you a cow


    I second that with a second cow.
     
    Junilu Lacar
    Marshal
    Posts: 15875
    265
    Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Refactoring is a very important part of writing software and it's a skill that you should try to develop alongside all the other skills you need to write good software. The downside to it is that it's very difficult to learn how to do without putting in a lot of conscious effort and practice.

    The ability to recognize code smells is probably the first thing you need to develop to be able to refactor effectively. The problem is, most developers don't have the nose for them. It's usually only when you have years of experience that you start to use your experience and skills to identify code smells and know enough to be able to address them.

    This is where code reviews and collaborative development can really help. I will add this thread to the Code Reviews forum and we can pick apart the code smells in your code, then show you how you can start addressing them.
     
    Junilu Lacar
    Marshal
    Posts: 15875
    265
    Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    So, as @Matt Pitt was saying, one of the big smells in your code is "lack of clarity" -- if it takes a lot of effort to just read through and understand what your code is doing, that's a big problem. Code that doesn't clearly express its intent and purpose is unmaintainable code or at the very least, difficult to maintain. It's a violation of one of the Four Rules of Simple Design which are:

    1. All tests pass
    2. Clearly expresses intent
    3. Contains no duplication
    4. Small (methods, classes, interfaces, etc.)

    These rules are listed in order of importance. Notice that clearly expressing intent is the second most important consideration after having all your tests pass, which we'll get to later.

    As far as clearly expressing intent, Matt also alluded to this earlier. Names and choosing good ones is probably the most important yet least developed skill I have seen with most of the software developers I have worked with over the years. The good thing is that it's not that hard to learn how to pick good names. You just need to start doing it consciously and intentionally.

    Names are better when they express intent or purpose rather than implementation. Good names contribute to the creation of context for the reader. It helps tell the story of what the program is about, not how it is technically implemented. So names like label and label1 are poor names because they are all about implementation, not about intent or context.

    Now, one thing with the labels is that you don't really do anything with them after you've created them and added them to the panel. So, why do you even need to keep references to them in your program? You can make your program smaller (Rule #4) by eliminating those fields and condensing the code around these.


     
    Junilu Lacar
    Marshal
    Posts: 15875
    265
    Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
    • Likes 1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Let's look at the Die class first and see what can be done with some of the names you chose.

    Terry Chu wrote:


    Think about the context in which dice are used and the language used in that context. I assume the name MAX stands for "maximum". Does that word usually come up in a conversation involving dice? You could say it does but I would argue that the word "sides" is more likely to occur, as in "How many sides does the die have?" and "They are regular six-sided dice." Thus, an alternative name to MAX would be SIDES.

    Next, let's find a better name for die1. This name does not express its intent/purpose well. Reading through the code, it's really the value of the last result of calling the roll() method (method names in Java should start with a lowercase letter). So, this variable would be better named lastRoll to reflect its purpose.

    Next, you can make the roll() method smaller by taking advantage of the fact that an assignment expression returns the value assigned. So the assignment on line 16 can be done in the return statement itself.

    Lastly, comments should not state the obvious. Reserve comments for explaining why you did something. The code itself should explain what is going on in the program. That's what choosing good names that express intent and provide context does for your code.

    With these changes, you'd have this instead:

    One thing that could probably still use some discussion here is line 12. Why do you assign 1 as the initial value of lastRoll when the new Die hasn't even been rolled yet? I would probably choose otherwise and leave it with its default value of 0, thus making it unnecessary to declare an explicit no-args constructor at this point.
     
    Consider Paul's rocket mass heater.
      Bookmark Topic Watch Topic
    • New Topic