Validation default value

Question

I want to check if all items of a list meet a set of criteria. Currently I'm doing it as below:

bool AreValid(list<string> vals)
{
   bool allValid=true;

   foreach(string val in vals)
   {
       if(!condition1)
          ...
          allValid=false;
       else if(!condition2)
          ...
          allValid=false;
   }

   return allValid;

} 

The reason I can't use LINQ All() is that for each failed condition I'm doing some job.

Now I wonder it is safe to set allValid as true by default?

Answer 1

Yes, it is fine - I mean it definitelly works and there is nothing unsafe in assigning default value of True to allValid.

My concern is the design. The name of your method AreValid suggest that it is just checking if objects meet specific criteria. You've mentioned that for all those objects that do not meet the criteria there is something being done on them. That doesn't seem to be the very greatest approach. I would suggest you consider separating checking if objects meets condition and changing state of the objects. It will make your code simpler and easier to read and understand to others. Is changing object state really what you expect from the method named AreValid? It would be quite a surprise for me if I found similar method in the code base.

The other thing is that you could use LiNQ and check for all the objects that do not meet the criteria and then perform some actions on those objects.

Answer 2

It's safe. A few things to consider:

1. When the list is empty you might want to return false.

2. Consider using an abstract data type (instead of string) which stores your data (the string) and move the validation logic to there.

3. As Jeff Vanzella already suggested, you can return false immediately when you now that a value is not valid. It's faster and easier to read:

   bool AreValid(list<string> vals)
   {
      foreach(string val in vals)
      {
          if(!condition1)
             ...
             return false;
          if(!condition2)
             ...
             return false;
      }
   
      return true;
   
   } 
   

   Note that you don't need the else keyword anymore.