12
\$\begingroup\$

I wrote a program to simulate this week's 538 Riddler, reproduced below:

You’ve just finished unwrapping your holiday presents. You and your sister got brand-new smartphones, opening them at the same moment. You immediately both start doing important tasks on the Internet, and each task you do takes one to five minutes. (All tasks take exactly one, two, three, four or five minutes, with an equal probability of each).

After each task, you have a brief moment of clarity. During these, you remember that you and your sister are supposed to join the rest of the family for dinner and that you promised each other you’d arrive together. You ask if your sister is ready to eat, but if she is still in the middle of a task, she asks for time to finish it. In that case, you now have time to kill, so you start a new task (again, it will take one, two, three, four or five minutes, exactly, with an equal probability of each). If she asks you if it’s time for dinner while you’re still busy, you ask for time to finish up and she starts a new task and so on.

From the moment you first open your gifts, how long on average does it take for both of you to be between tasks at the same time so you can finally eat? (You can assume the “moments of clarity” are so brief as to take no measurable time at all.)

And the program itself:

private static int numberOfSimulations = 100000;
static void Main(string[] args)
{
    var rand = new Random();
    var simulationTimes = new List<int>();
    for (int i = 0; i < numberOfSimulations; i++)
    {
        var totalTime = 0;
        var myTaskTime = rand.Next(1, 6);
        var herTaskTime = rand.Next(1, 6);

        do
        {
            if (herTaskTime > myTaskTime)
            {
                totalTime += myTaskTime;
                herTaskTime -= myTaskTime;
                myTaskTime = rand.Next(1, 6);
            }
            else if (herTaskTime < myTaskTime)
            {
                totalTime += herTaskTime;
                myTaskTime -= herTaskTime;
                herTaskTime = rand.Next(1, 6);
            }
            else
            {
                totalTime += myTaskTime;
            }
        } while (myTaskTime - herTaskTime != 0);

        simulationTimes.Add(totalTime);
    }
    Console.WriteLine(simulationTimes.Average());
    Console.ReadLine();
}

Anecdotally, I'm consistently getting ~7.5 minutes.

I'm just looking for a general code review - does this look correct? Am I doing anything dumb? Should I have done it differently?

\$\endgroup\$
  • \$\begingroup\$ "I'm consistently getting ~7.5 minutes." And that's wrong or what? What are you asking to be reviewed/improved actually? \$\endgroup\$ – πάντα ῥεῖ Dec 22 '15 at 15:53
  • \$\begingroup\$ That's more anecdotal than anything, maybe I should have specified that. I'm just looking for a general review - does this look correct? Am I doing anything dumb? Should I have done it differently? \$\endgroup\$ – MikeTheLiar Dec 22 '15 at 15:54
  • \$\begingroup\$ I might be wrong, but I think your code is flawed. You assign new tasks even if the current task is not confirmed completed. You need to check if myTaskTime and/or herTaskTime is zero before assigning a new task. Here's a quick and very dirty example (avg. ~9): dotnetfiddle.net/goaWaI \$\endgroup\$ – Bjørn-Roger Kringsjå Dec 22 '15 at 18:13
  • \$\begingroup\$ @Bjørn-RogerKringsjå you could very well be right. The logic here is treating the if/else if as allowing the shorter task to elapse (remove the remaining time from the other's task, assign a new task). I'm taking a look at your approach now, trying to figure out where the difference is \$\endgroup\$ – MikeTheLiar Dec 22 '15 at 18:26
  • 1
    \$\begingroup\$ The difference might be related to the fact that the do statement terminates if rand.Next generates a value that meets the myTaskTime - herTaskTime == 0 condition, thus the last task time is not added to the total. \$\endgroup\$ – Bjørn-Roger Kringsjå Dec 22 '15 at 19:02
5
\$\begingroup\$

There's definitely an issue that will occur when a new task comes up with the same amount of time as what is left on the other person's task. Take for example the times.

MyTime | HerTime
----------------
3      | 5  

Now the first loop occurs, HerTime gets reduced to 2, TotalTime gets set to 3, then MyTime gets rerolled to 2. Now, your while condition hits as MyTime = HerTime and it breaks out, but those 2 minutes were never accounted for.

An easy fix would be to add a guard condition to your while that gets set in the Else part of your If.

(Warning, the below changes are untested air code)

private static int numberOfSimulations = 100000;
static void Main(string[] args)
{
    var rand = new Random();
    var simulationTimes = new List<int>();
    for (int i = 0; i < numberOfSimulations; i++)
    {
        var totalTime = 0;
        var myTaskTime = rand.Next(1, 6);
        var herTaskTime = rand.Next(1, 6);
        var equal = false;

        do
        {
            if (herTaskTime > myTaskTime)
            {
                totalTime += myTaskTime;
                herTaskTime -= myTaskTime;
                myTaskTime = rand.Next(1, 6);
            }
            else if (herTaskTime < myTaskTime)
            {
                totalTime += herTaskTime;
                myTaskTime -= herTaskTime;
                herTaskTime = rand.Next(1, 6);
            }
            else
            {
                totalTime += myTaskTime;
                equal = true;
            }
        } while (!equal);

        simulationTimes.Add(totalTime);
    }
    Console.WriteLine(simulationTimes.Average());
    Console.ReadLine();
}
\$\endgroup\$
  • \$\begingroup\$ Ah, yep. When I incorporate these changes I'm getting 9 now as well. \$\endgroup\$ – MikeTheLiar Dec 22 '15 at 21:20
1
\$\begingroup\$

It's pretty readable. I would make the following suggestions: move the code in the for loop into a separate function, called something like RunSimulation. Define constants for the 1 and 6 you have sprinkled around and/or make task creation a separate function.

while (myTaskTime - herTaskTime != 0)

This is strange, is there a reason it's not

while (myTaskTime != herTaskTime)
\$\endgroup\$
  • 1
    \$\begingroup\$ I think I had a reason for that when I wrote it this morning but I can't remember what it was now. ¯\_(ツ)_/¯ \$\endgroup\$ – MikeTheLiar Dec 22 '15 at 20:14

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.