aspose file tools*
The moose likes Java in General and the fly likes Why am I getting negative numbers? Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


Win a copy of Spring in Action this week in the Spring forum!
JavaRanch » Java Forums » Java » Java in General
Bookmark "Why am I getting negative numbers?" Watch "Why am I getting negative numbers?" New topic
Author

Why am I getting negative numbers?

Vivienne Ryan
Greenhorn

Joined: Dec 12, 2013
Posts: 12

Hi there I am working on a piece of code that takes a number(150), then depending on the roll of the dice takes away 10, 30 or 50. If the roll is less than 2, it takes away nothing and passes over to the next person where the same thing happens again. This is in a do while loop that has an exit condition where if either of the numbers reaches 0, it exits and displays the winner.

The problem I'm having is I'm outputting large negative numbers eg: -2266740.
I don't know what I am doing wrong and if I can get some guidance that would be cool, thanks.

Stephan van Hulst
Bartender

Joined: Sep 20, 2010
Posts: 3647
    
  17

Hi Vivienne.

The reason you're seeing negative values is because your loop will probably never terminate. Let's say one player has 10 hp left over, and you score a hit that deals 30 damage. This player will then have -20 hp, and the condition in the while loop will not reach 0.

There are also some other matters you should address. Right now the loop is going on while EITHER player has hitpoints other than 0. So if the loop terminates (IF it terminates indeed), both players are guaranteed to have 0 hitpoints. Combined with the following if statement, this means P1 will never win.

The inside of your loop does *lots* of things you probably didn't intend it to do. You're re-rolling the die for each condition (you probably meant to save the first result) and some if statements probably should only be executed if the previous ones failed. Did you mean to use if-else?

Also, you should NOT create a class named Character. Character is already defined in the Standard API. Name it something like Person, Fighter, Player or something similar instead.

Your DiceRoller generates values between 0 and 5, included. This may be confusing, when you think of what most dice look like. Your variable names are similarly confusing. Don't call player 1 "c", and player 2 "c1". Call them "player1" and "player2".

Finally, I'd suggest you'd write your loop in such a way that in its body, one player gets to attack, and at the end of it, the turn goes to the other player.
Vivienne Ryan
Greenhorn

Joined: Dec 12, 2013
Posts: 12

Stephan van Hulst wrote:Hi Vivienne.

The reason you're seeing negative values is because your loop will probably never terminate. Let's say one player has 10 hp left over, and you score a hit that deals 30 damage. This player will then have -20 hp, and the condition in the while loop will not reach 0.

There are also some other matters you should address. Right now the loop is going on while EITHER player has hitpoints other than 0. So if the loop terminates (IF it terminates indeed), both players are guaranteed to have 0 hitpoints. Combined with the following if statement, this means P1 will never win.

The inside of your loop does *lots* of things you probably didn't intend it to do. You're re-rolling the die for each condition (you probably meant to save the first result) and some if statements probably should only be executed if the previous ones failed. Did you mean to use if-else?

Also, you should NOT create a class named Character. Character is already defined in the Standard API. Name it something like Person, Fighter, Player or something similar instead.

Your DiceRoller generates values between 0 and 5, included. This may be confusing, when you think of what most dice look like. Your variable names are similarly confusing. Don't call player 1 "c", and player 2 "c1". Call them "player1" and "player2".

Finally, I'd suggest you'd write your loop in such a way that in its body, one player gets to attack, and at the end of it, the turn goes to the other player.


Ok, so I have edited my code so the loop does end but still have a problem with minus numbers. Also my variable names are temporary they will be changed to better ones(I just want to test the thing out and get it working). Have also changed Character to Player. Here is my new code:

As you see I have assigned 2 variables p1Roll and p2Roll to d.roll(). Will this generate a new number everytime a turn ends?
Also my apologies for starting the new thread, know now for future reference.
EDIT: Forgot to change character to player
Jan Hoppmann
Ranch Hand

Joined: Jul 19, 2010
Posts: 147

Vivienne Ryan wrote:Ok, so I have edited my code so the loop does end but still have a problem with minus numbers.


You could check whether the hitpoints for the character drop below zero and assign zero to them to circumvent this.

Also, look at your if-statements. A 1, for example, is smaller than 2, 4 and 6, so each case will be true. (Ah, Stephen already spotted this)


Life is full of choices. Sometimes you make the good ones, and sometimes you have to kill all the witnesses.
Winston Gutkowski
Bartender

Joined: Mar 17, 2011
Posts: 8008
    
  22

Vivienne Ryan wrote:Ok, so I have edited my code so the loop does end but...

It's still wrong. It will now produce a result between 0 and 6 (inclusive).

Also my variable names are temporary they will be changed to better ones(I just want to test the thing out and get it working)

Unfortunately, if your names are bad, it may well hamper your testing as well.

My advice: fix them NOW (you might want to look at my signature quote for inspiration ).

Also: Your if statements are not exclusive, so there's a lot of redundant (and probably incorrect) code being executed. You might want to have a look at the switch statement.

Winston

Isn't it funny how there's always time and money enough to do it WRONG?
Articles by Winston can be found here
Vivienne Ryan
Greenhorn

Joined: Dec 12, 2013
Posts: 12

Winston Gutkowski wrote:
Vivienne Ryan wrote:Ok, so I have edited my code so the loop does end but...

It's still wrong. It will now produce a result between 0 and 6 (inclusive).

Also my variable names are temporary they will be changed to better ones(I just want to test the thing out and get it working)

Unfortunately, if your names are bad, it may well hamper your testing as well.

My advice: fix them NOW (you might want to look at my signature quote for inspiration ).

Also: Your if statements are not exclusive, so there's a lot of redundant (and probably incorrect) code being executed. You might want to have a look at the switch statement.

Winston


Ok so I fixed the DiceRoller(I think)so it should return a number between 1 - 6. I also changed the if statements to switch and while I still get the negative numbers my code is much more readable and look much more better. Anyways So I added 2 if statements where they check if the characters hp is <= 0 and if so to assign them to 0. However I'm still getting the negative numbers, is this down to variable scope?

Campbell Ritchie
Sheriff

Joined: Oct 13, 2005
Posts: 39409
    
  28
I am not sure about the design of your classes.
You have a method in Player which doesn't use any info apart from its parameters and doesn't return or record any info other than its return type. That makes it a 1368 in the most dubious classification of methods known to modern science. So why isn't it static?

There are good reasons for static members, but I get suspicious that you are straying from proper object‑oriented design when you get things static. I suggest your Player class should be changed:-
  • 1: All fields with private access.
  • 2: All fields initialised in constructor (some people will disagree with me). You only have 1 field and it appears always to have the same starting value, so your constructor can be quite simple.
  • 3: Provide toString method, getXXX methods and maybe even setXXX methods, as you see the need.
  • 4: A class should take responsibility for itself, so you should not interrogate the fields outside the class. You should provide methods like isAlive()
  • Your random number generator has an out‑by‑one error. I don't like doing arithmetic with Math#random; I prefer to use a Random object. Look at this discussion, for example.
    Oddly enough, if you have one Die and only one Die in the game, you could make the method static. Or you could create a single‑member enum whose instance has a die‑rolling method, which will implement a one‑die game. You can even give it methods to throwOneDie, throwTwoDice, etc.
    Winston Gutkowski
    Bartender

    Joined: Mar 17, 2011
    Posts: 8008
        
      22

    Vivienne Ryan wrote:Ok so I fixed the DiceRoller(I think)so it should return a number between 1 - 6. I also changed the if statements to switch and while I still get the negative numbers my code is much more readable and look much more better. Anyways So I added 2 if statements where they check if the characters hp is <= 0 and if so to assign them to 0.

    Aaargh! Do NOT do this.

    Do things step by step, and test thoroughly after every single change you make; otherwise how can you ever know what caused a problem (or indeed fixed one) if it happens?

    However I'm still getting the negative numbers, is this down to variable scope?

    Possibly, but TBH it's difficult to tell, so I certainly wouldn't assume it.

    The main problem, as I see it, is that you're doing all your logic in main(), which is a bad way to go. I suspect that most of this code should actually be in your Player class.

    Try this: Turn off your computer and explain to me (or us) IN DETAIL and in English, exactly what an "attack" is supposed to do?

    Winston
    Stephan van Hulst
    Bartender

    Joined: Sep 20, 2010
    Posts: 3647
        
      17

    I like your player names ;)

    You're getting a negative value at the end, because you're printing the negative value in your case clause BEFORE you're correcting it.

    Note that the switch statement allows for cases to fall into each other. If you remove everything behind case 1: (including the break;) the program will automatically perform the weak attack for both case 1 and case 2. This avoids code duplication.

    Two more things. Your code will become much more reliable if a Player can't have negative hp in the first place. Make Player responsible for this. Secondly, you duplicate a lot of code, because you're doing the same stuff for both ash and gary. What if you have an attacker and a defender variable, and you just flip the two at the end of the loop?
    Vivienne Ryan
    Greenhorn

    Joined: Dec 12, 2013
    Posts: 12

    Winston Gutkowski wrote:
    Vivienne Ryan wrote:Ok so I fixed the DiceRoller(I think)so it should return a number between 1 - 6. I also changed the if statements to switch and while I still get the negative numbers my code is much more readable and look much more better. Anyways So I added 2 if statements where they check if the characters hp is <= 0 and if so to assign them to 0.

    Aaargh! Do NOT do this.

    Do things step by step, and test thoroughly after every single change you make; otherwise how can you ever know what caused a problem if you get one?

    However I'm still getting the negative numbers, is this down to variable scope?

    Possibly, but TBH it's difficult to tell, so I certainly wouldn't assume it.

    The main problem, as I see it, is that you're doing all your logic in main(), which is a bad way to go. I suspect that most of this code should actually be in your Player class.

    Try this: Turn off your computer and explain to me (or us) IN DETAIL and in English, exactly what an "attack" is supposed to do?

    Winston


    Essentially what I want to happen is : Two characters have a base Health points = 150. First player does an attack, which depending on the dice roll does 10/30/50 damage. This amount is taken from the other players Health points(so an attack that deals 30 damage -> removes 30 from opponents Health, leaving 120 health), and vice versa. This continues until one characters health has reached 0, then the computer displays who has won.

    As to the player names... twitch plays pokemon is stuck on my mind :P
    Stephan van Hulst
    Bartender

    Joined: Sep 20, 2010
    Posts: 3647
        
      17

    Yes. So make Players that can't even have less than 0 hp in the first place. The outside world should not be allowed to access their hp field.
    Winston Gutkowski
    Bartender

    Joined: Mar 17, 2011
    Posts: 8008
        
      22

    Vivienne Ryan wrote:Essentially what I want to happen is : Two characters have a base Health points = 150. First player does an attack, which depending on the dice roll does 10/30/50 damage. This amount is taken from the other players Health points(so an attack that deals 30 damage -> removes 30 from opponents Health, leaving 120 health), and vice versa. This continues until one characters health has reached 0, then the computer displays who has won.

    Right, now we're getting somewhere (and good explanation, BTW).

    So:
    1. All the effects of an attack happen to the Player that is attacked, not the one doing the attacking. It therefore seems more sensible to call the method attacked(), not attack(), since gary.attack() will have absolutely no effect whatsoever on gary. But gary.attacked() will.
    2. The business of dice rolling, and determination of damage, all seem to be part of the "attacking" process, so why not put it in there?

    So, what do you think your Player class might look like if you did that?

    Winston
    Winston Gutkowski
    Bartender

    Joined: Mar 17, 2011
    Posts: 8008
        
      22

    Winston Gutkowski wrote:But gary.attacked() will.

    And if this seems like an odd way to do things, you could always add an attack() method to your AttackSimulation class that simply forwards. Maybe something like:and then in your main() method you'd write:
    attack(gary);
    which might be a bit more readable.

    Right now, there is no link between the attacker and the attackee, so it really doesn't matter who is doing the attacking. Perhaps later on, you might need it (for example, if there is some penalty to the attacker for a failed attack), but not right now.

    Winston
    Winston Gutkowski
    Bartender

    Joined: Mar 17, 2011
    Posts: 8008
        
      22

    Winston Gutkowski wrote:Right now, there is no link between the attacker and the attackee...

    And just to continue on this theme, you could allow for this eventuality by keeping your attack() method, and have it take the Player being attacked, and also add a injure() method that takes the amount of damage inflicted on a Player. It might actually be a better way to go, even though you don't really need it right now.

    Have a think how your Player class might look with that set-up.

    Programming is about thinking, not coding; and sometimes you have to kiss a few frogs to find your prince.

    Winston
     
    I agree. Here's the link: http://aspose.com/file-tools
     
    subject: Why am I getting negative numbers?