Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am supposed to take this code (that I made):

public void showUpdatedStatus()
{
    if(((itsUsersNumber - itsSecretNumber) <= 2) && ((itsUsersNumber - itsSecretNumber) >= -2))
        JOptionPane.showMessageDialog(null, "You're hot!");
    if (((itsUsersNumber - itsSecretNumber) <= 6) && ((itsUsersNumber - itsSecretNumber) >= -6))
        JOptionPane.showMessageDialog(null, "You're warm.");
    if ((itsUsersNumber - itsSecretNumber) < -6)
        JOptionPane.showMessageDialog(null, "Too low.");
    if ((itsUsersNumber - itsSecretNumber) > 6)
        JOptionPane.showMessageDialog(null, "Too high.");
}

And change it so it does this:

Have the program lie one-third of the time that the guess is too high; it says it is too low. But it never lies about the guess being too low, nor does it lie twice in a row.

I believe I have managed to do this with this method for checking whether the guess is "too high."

public void TooHigh()
{
    int numLies = 1;
    if ((itsUsersNumber - itsSecretNumber) > 6)
    {
        itsLieNum = randy.nextInt(2);
        if(itsLieNum == 1 && numLies == 1){
            numLies = 2;
            JOptionPane.showMessageDialog(null, "Too low.");}
        else
            numLies = 1;
            JOptionPane.showMessageDialog(null, "Too high.");
    }
}

I just want to know whether or not it seems like it would work fine. I would just test it, but it would probably take me quite a while to make sure that it not only works 1/3 of the time, but also that it doesn't do it twice in a row.

EDIT: I managed to get everything working fine with this code:

public void showUpdatedStatus()
{

    if(((itsUsersNumber - itsSecretNumber) <= 2) && ((itsUsersNumber -itsSecretNumber) >= -2))
        JOptionPane.showMessageDialog(null, "You're hot!");
    else if (((itsUsersNumber - itsSecretNumber) <= 6) && ((itsUsersNumber -itsSecretNumber) >= -6))
        JOptionPane.showMessageDialog(null, "You're warm.");
    if ((itsUsersNumber - itsSecretNumber) < -6)
        JOptionPane.showMessageDialog(null, "Too low.");
    else 
        TooHigh();
}

public void TooHigh()
{
    if ((itsUsersNumber - itsSecretNumber) > 6)
    {
        itsLieNum = randy.nextInt(3);
        if(canLie && itsLieNum == 1){
            canLie = false;
            JOptionPane.showMessageDialog(null, "Too low.");}
        else{
            canLie = true;
            JOptionPane.showMessageDialog(null, "Too high.");}
    }
}

Thanks to rolfl

share|improve this question
1  
this is a really good question.....I don't know how "on-topic" your question is though? –  Malachi Dec 28 '13 at 6:25

2 Answers 2

up vote 3 down vote accepted

The concept you have going is 'OK' (cumbersome, but for a beginner it's OK). There are two bugs, and one recommendation that will help a lot.

First the recommendation....

Change numLies to be a boolean value like: boolean canlie = true;

Then, your code becomes:

if(canlie && itsLieNum == 1){
    canlie = false;
    ....
} else {
    canlie = true;

This makes the purpose of the code more apparent. Also, a bug is that you have to declare this ourside your method. It should be a class-variable, not a method variable, otherwise it will always be the wrong value.

The bug you have is a technical one (but important that you learn) ... Random.nextInt(value) will never return the input value. It will retrun 0(inclusive) to value(exclusive). In your case, with nextInt(2) it will only ever return 0, or 1. Change that to be nextInt(3) and you will get the values 0, 1, or 2 and it will be 1 1 third of the time.

share|improve this answer
    
Oh okay I should have noticed the nextInt(2) thing. For some reason I was thinking that nextInt(2) WAS 0, 1, 2 not sure why haha. I also noticed what was happening from numLies being declared inside of the method after I tested it myself a couple times. :P –  Ian Lundberg Dec 28 '13 at 6:38
    
Also there is another bug that I can't seem to figure out the reason for. It's that for some reason after I enter a number that is "Too High" the dialog that is supposed to tell me whether it is "Too Low" or "Too High" is blank with no buttons or text other than the X button in the top right. Any help on that matter? Thanks! :) –  Ian Lundberg Dec 28 '13 at 6:42
    
I figured out the problem it had to do with the statements checking if it was Hot or Warm I had everything set to an else if off of those I just left hot and warm dependant off of each other and made too low its own if statement and that seemed to fix the issue. –  Ian Lundberg Dec 28 '13 at 6:46

I'll review your Rev 4, though some of these remarks also apply to the original code. Note that all of these criticisms are on style rather than correctness. The correctness of your code is pretty good starting with Rev 4.

  • You use (itsUsersNumber - itsSecretNumber) many times. For brevity, readability, and maybe performance, define int diff = itsUsersNumber - itsSecretNumber.
  • I suggest chaining your conditions as if … else if … else if … else, to emphasize that the code branches are all mutually exclusive, and that one of them must always be true.
  • I strongly recommend writing the braces, and placing them according to the official Java style guide (§ 7.4):

    int diff = itsUsersNumber - itsSecretNumber;
    if (-2 <= diff && diff <= 2) {
        …
    } else if (-6 <= diff && diff <= 6) {
        …
    } else if (diff < -6) {
        …
    } else {
        assert diff > 6;
        …
    }
    

    Your placement of braces in TooHigh() is weird and confusing.

  • I would change public void TooHigh() to private void handleTooHigh(). It's a helper function, not to be called by code in any other class, so access should be private. Method names should start follow the lowerCase capitalization convention.
  • I would expect that TooHigh() would only ever be called when a number is too high. Therefore, TooHigh() shouldn't have to bother checking that the guess is too high.
  • In TooHigh(), itsLieNum should be local, if you need to have a variable at all.
share|improve this answer
    
Thanks for the feedback. You are right about how itsLieNum should be local now that I look back at it. haha –  Ian Lundberg Dec 28 '13 at 20:23

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.