An exit condition for a do-while loop using lists [closed]

Posted on

Problem

I have an integer list l and a predefined integer value row (which is never manipulated inside the code). If value of row is 5, then I want the loop to exit if l contains 1,2,3,4. If it is 3, then l should contain 1,2 and so on for any value. I have devised a way of doing this, but since I mean to use this in an app, can someone tell me a better way of doing this?

do
{
}while(check(row,l))

boolean check(int row,list<int> l)
{
 for(int i=1;i<row;i++)
  {
    if((l.contains(i))
      continue();
    else
      return true;
  }
  return false;
}`

Solution

  1. check is not a good name for the function as it does not say what it checks. It returns false if all numbers below row are present and true otherwise. So a better name might be numbersAreMissing.

  2. You can make your loop body a bit shorter by inverting the condition:

    for (int i = 1; i < row; ++i)
    {
        if (!l.contains(i)) { return true; }
    }
    return false;
    
  3. You should really think about whether or not this check is something you want to do on every loop iteration. Especially if row is a bit larger this gets wasteful. There are probably better ways to organize your data so this check can be avoided but that entirely depends on the actual problem you are trying to solve.

(I answered this question on StackOverflow, just copying it here…)

If you want to check if your list contains all numbers from 1 to (row-1), you could use a HashSet like this:

var hashSet = new HashSet(myList.Where(item => item >= 1 && item < row))

This adds all “relevant” items of the list to the Set.

Since a HashSet contains every item at most once, you can check the Count of it to ensure, all numbers are present, like:

var check = hashSet.Count == (row - 1)

Regarding performance, this might be more efficient than your solution, since the list needs only to be iterated once (in your solution, you have row-1 iterations, one for every Contains operation). And adding to a HashSet is considered a O(1) operation.

Note: The major drawback of this solution is that its not immediately apparent what it does. So consider adding a comment to it.

Another, more readable approach would be to explicitely check, if all numbers are contained in the Set, like

var hashSet = new HashSet(myList);
var check = Enumerable.Range(1, row).All(number => hashSet.Contains(number));

If you consider row to be a constant the asymptotical time will be the same: O(n) for constructing the HashSet, and O(1)*O(1)=O(1) for the check itself (first O(1) for the constant number of “rows” to be checked, second O(1) for the Contains function…)

In addition to what has been said already, I’ll add the following points:

  • do { ... } while (condition) isn’t the most readable form of loop construct. If it doesn’t affect the number of iterations, you should prefer putting the condition at the top of the code block, like while (condition) { ... }, so you know what you’re getting yourself into when you’re reading the code from the top to the bottom.
  • doesn’t have a boolean type. There’s System.Boolean and its language alias bool. Did you mean ?
  • Single-letter variable names are evil. Single-letter identifiers that use a lowercase “L” are just plain careless. Use descriptive names.

Leave a Reply

Your email address will not be published. Required fields are marked *