Incrementing time in a discrete-event simulation of a supermarket queue

Question

A method has been written that increments time in minutes that simulates how much time it takes to process item in a supermarket queue and if there is an empty queue, skip it and loops around to find the number of items processed in each interval (each time a minute has passed). I was wondering if there are ways to improve the code I have written??

private Queue[] lines;
private double time; // Time measured in minutes
private static double item_per_min;
private static double time_inc;
private static Random rand = new Random();
static int x = 0;
public static final double lambda = item_per_min * time_inc;
public static double target_Probability = rand.nextDouble();
public static final double target_ProbabilityabilityOfX = Math.pow(lambda, x) * Math.exp(-lambda) / factorial(x); // Poisson
 // distribution
public void add_time(double time_inc)
{
 for (int i = 0; i < lines.length; i++) // Loops
 {
 if (lines[i].empty())
 {
 continue;
 }
 while (true)
 {
 time += time_inc;
 if (target_ProbabilityabilityOfX > target_Probability)
 {
 break;
 }
 x++;
 target_Probability -= target_ProbabilityabilityOfX;
 }
 while (!lines[i].empty() && x > 0)
 {
 lines[i].decrement();
 x--;
 }
 }
 this.display();
}

• greybeard: Do you use an IDE for development? Asking because indentation is inconsistent.
• Xander Powel: I use eclipse, I think i messed with the indentation and dunno how to return it to normal
• greybeard: (Guessing here: you use three spaces per indentation level, and mix tabs and blanks in eclipse. Remedy: convert to blanks before copying to stackexchange.)
• Xander Powel: Stupid question. How would I do that??
• greybeard: (Inside Eclipse, I know no better than temporarily changing the language code style, marking the text, "correct indentation", copy, paste wherever the "blanked" version is wanted, and roll back all changes including Preferences. A perfunctory doc&web search made me no wiser.)

Answer 1

 if (probK <= prob) { // increment k until cumulative probability, prob, reached
 k++;
 prob -= probK;
 } else {
 break;
 }

With this pattern, it's often better to write

 if (probK > prob) {
 break;
 }
 k++;
 prob -= probK;

This saves some indent and makes it clear what is happening. We check for the end condition and then proceed normally if it was not reached. The other way, it's not obvious what the end condition is, since it is hidden in an else.

But I would actually rewrite the whole loop.

 int k = 0;
 for (double probabilityOfK = Math.exp(-lambda);
 probabilityOfK <= targetProbability; 
 probabilityOfK *= lambda / k) {
 targetProbability -= probabilityOfK;
 k++;
 }

The for loop declaration is on three lines to eliminate side scroll on this site. It might fit on one line in actual code.

I prefer probabilityOfK to probK as being more readable. I prefer targetProbablity to prob as being more descriptive.

I stopped using Math.pow as a rather expensive calculation. It is replaced with a simple multiplication. And you were multiplying anyway, so that's effectively free.

Now it doesn't use a loop forever pattern with a break. It just loops normally until the condition fails.

This works because you never use cumulativeProbability outside the loop, but each value relates to the previous one.

The result may change slightly due to double rounding. It's up to you if that matters. And if it does matter, if the original was better.

 for (int i = 0; i < lines.length; i++) {

I would do this with a range-based for loop. Something like

 for (Queue<?> line : lines) {

Then you could replace all the lines[i] with just line. Replace Queue<?> as necessary.

 while (!lines[i].isEmpty() && k > 0) {

Consider something like

 if (k >= line.size()) {
 line.clear();
 continue;
 }
 while (k > 0) {

Now you don't have to check if line is empty, as you'll run out of k before you run out of line.

You could move that check into the previous loop. Then if you reach the maximum possible k, you can stop immediately rather than continuing. That might be more efficient. Something like

 for (Queue<?> line : lines) {
 if (line.isEmpty()) {
 continue;
 }
 updateLine(line);
 }

and

 public void updateLine(Queue<?> line) {
 int k = 0;
 for (double probabilityOfK = Math.exp(-lambda);
 probabilityOfK <= targetProbability; 
 probabilityOfK *= lambda / k) {
 targetProbability -= probabilityOfK;
 k++;
 if (k >= line.size()) {
 line.clear();
 return;
 }
 }
 while (k > 0) {
 line.decrement();
 k--;
 }
 }

• Xander Powel: I have changed the code, based on your suggestions, but don't quite understand the Collections<?> and the if statement you suggested at the bottom. And line is a array of Queue.