4
\$\begingroup\$

This program simulates flipping a coin repeatedly and continues until however many consecutive heads are tossed.

I'm wondering if there are any issues when initializing a variable in a for loop the way I did. Also I assume assigning -1 to i was an appropriate move as well because after a loop cycle it will iterate (i++) causing i to become 1 if I had not, resulting in one less loop.

/*
 * File: ConsecutiveHeads.java
 * ---------------------------
 * This program simulates flipping a coin 
 * repeatedly and continues until however many
 * consecutive heads are tossed.
 */

/* Library packages */
import acm.program.*;
import acm.util.RandomGenerator;

public class ConsecutiveHeads extends ConsoleProgram {

    public void run() {
        int flips = 0;
        for (int i = 0; i < CONSECUTIVE_HEADS; i++) {
            String coinSide = flipCoin();
            println(coinSide);
            flips++;
            if (coinSide.equals("Tails")) {
                i = -1;
            }
        }
        println("It took " + flips + " flips to "
                + "get " + CONSECUTIVE_HEADS + " consecutive heads.");
    }

    /* Returns true 50% of the time */
    private String flipCoin() {
        return rgen.nextBoolean() ? "Tails" : "Heads";
    }

    /* Create an instance variable for the random number generator */
    private RandomGenerator rgen = RandomGenerator.getInstance();

    /* Private constant */
    private static final int CONSECUTIVE_HEADS = 3;
}
\$\endgroup\$
  • 4
    \$\begingroup\$ I think it would be more readable to just use a while loop and reset the headsCount after a tails. \$\endgroup\$ – RobAu Oct 13 '17 at 5:31
  • 2
    \$\begingroup\$ I’m assuming this is a theoretical example, and you’re not interested in a pragmatic solution or optimisations? There’s a closed formula solution to this problem, which would decimate both your running time and code size. For these reasons, in practice you wouldn’t want to use a loop here. \$\endgroup\$ – Konrad Rudolph Oct 13 '17 at 17:02
  • \$\begingroup\$ I personally wouldn't do String comparison inside your loop; you're really just assigning a String value to a boolean value then evaluating it to boolean anyway. I would invert your True:False = Tails:Heads to True:False = Heads:Tails, then you just do flips++; if (rgen.nextBoolean()) { i++; println('Heads'); continue; } i = 0; println('Tails');but i'll be honest, that's probably splitting hairs. \$\endgroup\$ – Daevin Oct 13 '17 at 20:46
  • \$\begingroup\$ What was the idea behind converting a boolean to a string? \$\endgroup\$ – user1993392 Oct 14 '17 at 6:56
  • \$\begingroup\$ @user1993392 I'm a novice. I'm was thinking that it makes it more "readable" but obviously that does not fit. I am aware my thoughts on my code are not what they should be. From the responses I get here I am learning what is bad and not necessary and what to include. \$\endgroup\$ – Alexander John Oct 14 '17 at 19:58
10
\$\begingroup\$

for loop

I find both, your solution and the approach of @BenC difficult to understand.

I'd suggest to use a while loop instead:

int consecutiveHeads = 0
while (consecutiveHeads < consecutiveHeadsThreshold) {
  CoinSide toss = flipCoin();
  flips++;
  println(toss.getName());

  if (CoinSide.HEADS.equals(toss)) {
    consecutiveHeads++;
  } else {
    consecutiveHeads = 0;
  }
}

Or you are consequent with your abuse of the for loop:

public void run(final int consecutiveHeadsThreshold) {
    int flips = 0;
    for (int consecutiveHeads = 0;
             consecutiveHeads < consecutiveHeadsThreshold;
             consecutiveHeads = doFlip(consecutiveHeads )){
        flip++;  
    }
   // ...
}

private int doFlip(int consecutiveHeads) {
  CoinSide toss = flipCoin();
  println(toss.getName());
  if (CoinSide.HEADS.equals(toss)) {
    consecutiveHeads++;
  } else {
    consecutiveHeads = 0;
  }
  return consecutiveHeads;
}
\$\endgroup\$
  • \$\begingroup\$ While I agree that it's practically a while loop already, I don't see how the for loop is any harder to understand -- it's just where you place the consecutiveHeads var, inside or outside the loop scope. \$\endgroup\$ – BenC Oct 13 '17 at 9:40
  • 7
    \$\begingroup\$ @BenC for loops implies that the number of iterations is somehow determined before entering the loop whereas while implies that we iterate an unknown number of times until a condition is meet. Maybe its just me, but I have a strange feeling by using a forin this case... \$\endgroup\$ – Timothy Truckle Oct 13 '17 at 9:58
  • \$\begingroup\$ I don't share that mental model / semantic distinction (I see a for loop as just a loop with three statements, all optional, and which provides the ability to constrain the scope of loop vars), but I understand where you're coming from, cheers. \$\endgroup\$ – BenC Oct 13 '17 at 10:32
  • 2
    \$\begingroup\$ I agree it's "strange" to see the counter of the for loop changed. And setting consecutiveHeads = 0; seems more logical than i = -1. \$\endgroup\$ – Steve Smith Oct 13 '17 at 15:16
  • 1
    \$\begingroup\$ I don’t think I’ve seen a for loop used like that before, so for me it took extra brain-cycles to understand. A while loop wouldn’t have that issue \$\endgroup\$ – matt freake Oct 13 '17 at 19:16
10
\$\begingroup\$

Loop

Also i assume assigning -1 to i was an appropriate move as well because after a loop cycle it will iterate (i++) causing i to become 1 if I had not, resulting in one less loop.

Well, that works, but I think it would be more clear to explicitly increment i on heads, and reset it otherwise:

for (int consecutiveHeads = 0; consecutiveHeads < CONSECUTIVE_HEADS_THRESHOLD;) {
  ...
  if (coinSide.equals("Heads")) {
    consecutiveHeads++;
  } else {
    consecutiveHeads = 0;
  }
}

That could be a while loop as well, depending on your tastes.

Comments

/* Create an instance variable for the random number generator */

This is not really necessary. It's clearly an instance variable, and the names and types show that it is a RNG.

/* Private constant */

Again, this isn't really necessary. private final already shows that is is a private constant. Even if you these comments are more intended for organization, a common approach would be to group these variables in blocks at the top of the class, with implicit meaning.

They don't take away from anything, they're just not really necessary.

/* Returns true 50% of the time */
private String flipCoin() {

This is not accurate. It will never return true. It returns strings.

Matching strings

Matching against strings is somewhat fragile. If this was a larger codebase, you would probably implement an enum for the coin sides, although it's overkill for this example. Altogether, a slightly more genericized version might look something like this:

public class ConsecutiveHeads extends ConsoleProgram {

  // These traditionally go up top, not at the bottom of the class.
  private final RandomGenerator rgen = RandomGenerator.getInstance();
  private static final int DEFAULT_CONSECUTIVE_HEADS_THRESHOLD = 3;

  public void run() {
    run(DEFAULT_CONSECUTIVE_HEADS_THRESHOLD);
  }

  // Theshold is parameterized
  public void run(final int consecutiveHeadsThreshold) {
    int flips = 0;
    for (int consecutiveHeads = 0; consecutiveHeads < consecutiveHeadsThreshold;) {
      CoinSide toss = flipCoin();
      flips++;
      println(toss.getName());

      if (CoinSide.HEADS.equals(toss)) {
        consecutiveHeads++;
      } else {
        consecutiveHeads = 0;
      }
    }
    System.out.printf("It took %d flips to get %d consecutive heads.", flips, consecutiveHeadsThreshold);
  }

  private CoinSide flipCoin() {
    return CoinSide.from(rgen.nextBoolean());
  }

  // Alternatively, in the limited scope of this example, you could
  // just define `true` to be heads and then not bother defining tails
  // at all.
  enum CoinSide {
    HEADS("Heads"), TAILS("Tails");

    private final String name;
    CoinSide(String name) { this.name = name; }
    public String getName() { return name; }

    public static CoinSide from(bool value) { return value ? HEADS : TAILS; }
  }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ For for-loops with omitted terms, I'd be sorely tempted to do something like for (int i = 0; i < X; /* explain why this is missing */) {... \$\endgroup\$ – Roger Lipscombe Oct 13 '17 at 9:01
  • \$\begingroup\$ If you use enum you can use ==. Or switch and case. Both are more readable than the if (Enum.ENUMVALUE.equals(testValue)) \$\endgroup\$ – RobAu Oct 13 '17 at 12:06
  • \$\begingroup\$ Don't forget a %n in the printf to also print a new line. \$\endgroup\$ – Octavia Togami Oct 13 '17 at 13:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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