CODEWITHSUNDEEP
c#
Sam
Sam

Reputation: 30388

Do while loop is not stopping even though the condition no longer met

I have a do while loop whose condition depends on a property value of an object. The object looks like this:

public class MyObjectType
{
   public int Id { get; set; }
   public bool IsValid { get; set; }
   public DateTime CompletionTimeStamp { get; set; }
   public string Comments { get; set; }
}

The do while loop looks like this:

public Task DoSomething(MyObjectType myObject)
{
   do
   {
      // Call DB get some values
      var someData = _myRepository.Get(myObject.Id);

      // Process data
      someData.SomeValue = SomeFunction(someData.Property1, someData.Property2);

      // Not pretty code but I'm trying to set a few values for myObject here
      if(someData.SomeValue == null)
         MyUtils.PopulateErrorData(ref myObject, "Something went wrong!!!");

      // Save data -- When there's an error, I don't want to this this step
      await _myRepository.Save(someData);
   }
   while(myObject.IsValid);
}

The PopulateErrorData() function is just a place where I set the values of myObject:

public static void PopulateErrorData(ref MyObjectType myObject, string comments)
{
    myObject.IsValid = false;
    myObject.CompletionTimeStamp = DateTime.UtcNow;
    myObject.Comments = comments;
}

I can't say that I'm really happy about this approach but in my DoSomething() function which is a backend maintenance function, I need to go through a lot of steps and if any of them fails, I want to abort the process.

I figured I can just use a do while and when the condition is no longer met, it would kick me out of the loop anyway but it is NOT. It just keeps on going. In the above code, if someData.SomeValue == null, I should never save the data but as I said, the code continues and I end up saving data in the database.

Any idea why it doesn't kick me out of the loop even though myObject.IsValid is set to false by PopulateErrorData()?

Upvotes: 0

Views: 414

Answers (3)

BradleyDotNET
BradleyDotNET

Reputation: 61349

The other upvoted answers show you the changes you need to make, but I want to make sure you know why that is so (you seem to be missing a few C# fundamentals).

The conditional of a while loop is not evaluated continually. It is only evaluated at the beginning (while) or end (do ... while) of the loop. Thus setting the flag won't prevent the next statement from occurring. To exit a loop early you need a break (to exit the loop entirely) or a continue (to skip to the next iteration).

Thus the solution is to put your save in an else, add a break to the existing if block, or do the else and remove the loop entirely unless you actually want to do this more than once.

You also have an unnecessary ref. Classes in C# are already passed as a reference, so changes to their properties will be seen by future users of the same object. The only time you need to use ref is if you actually assign the parameter to something.

Upvotes: 3

John Wu
John Wu

Reputation: 52240

Think you need to change this

  // Not pretty code but I'm trying to set a few values for myObject here
  if(someData.SomeValue == null)
     MyUtils.PopulateErrorData(ref myObject, "Something went wrong!!!");

  // Save data -- When there's an error, I don't want to this this step
  await _myRepository.Save(someData);

To this

  if(someData.SomeValue == null)
  {
     MyUtils.PopulateErrorData(ref myObject, "Something went wrong!!!");
  }
  else
  {
      // Save data -- When there's an error, I don't want to this this step
      await _myRepository.Save(someData);
  }

Or

  if(someData.SomeValue == null)
  {
     MyUtils.PopulateErrorData(ref myObject, "Something went wrong!!!");
     break;
  }
  // Save data -- When there's an error, I don't want to this this step
  await _myRepository.Save(someData);

Although for the life of me I don't understand what purpose is served by a while loop in this particular case.

Upvotes: 3

Faruq
Faruq

Reputation: 1521

In your code, you are saving the data first, then you are checking for IsValid property. Just make the code simpler:

public Task DoSomething(MyObjectType myObject)
{
   // if myObject.IsValid is set initially, then uncomment the following line.
   // myObject.IsValid = true;
   while(myObject.IsValid)
   {
      // Call DB get some values
      var someData = _myRepository.Get(myObject.Id);

      // Process data
      someData.SomeValue = SomeFunction(someData.Property1, someData.Property2);

      // Not pretty code but I'm trying to set a few values for myObject here
      if(someData.SomeValue == null)
         MyUtils.PopulateErrorData(ref myObject, "Something went wrong!!!");

      if(!myObject.IsValid)
          break;

      // Save data -- When there's an error, I don't want to this this step
      await _myRepository.Save(someData);
   }

}

Upvotes: 4

Related Questions