A refactoring that escapes me

17 Comments

• Why do you need the if statement?
  

• The method returns true or false if it's okay to change the state to active for this class. The if statement checks various properties to see if they meet the condition to return true (or false).
  

• @Stuart: Not sure how the second thing you mention is any different than what I already have. And the first option, well, that's just breaking up the if statements. Maybe that's the best that can be done in this situation then?
  

• Very true, the if statement's evaluation is based on a true/false condition. You can use that evaluation as your return value, instead of using an if statement.
  
  It would look something like this:
  
  bool isValid = ((Job.Key == INVALID_VALUE) ||
  (Category.Key == INVALID_VALUE) ||
  (StartDate == DateTime.MinValue) ||
  (EndDate == DateTime.MinValue) ||
  (Status.Key == INVALID_VALUE) ||
  (Entity.Key == INVALID_VALUE) ||
  (String.IsNullOrEmpty(ProjectManagerId.ToString()) || ProjectManagerId == INVALID_VALUE) ||
  String.IsNullOrEmpty(ClientName) ||
  String.IsNullOrEmpty(Title));
  
  return !isValid;
  
  
  This will only work if you don't have any conditional logic.
  

• Stuarts (http://sab39.netreach.com/) comment (that somehow got deleted):
  
  I'd probably write this as either:
  
  // Job must have a valid key
  if (Job.Key == INVALID_VALUE) return false;
  // Category must have a valid key
  if (Category.Key == INVALID_VALUE) return false;
  ...
  return true;
  
  or alternatively:
  
  return Job.Key != INVALID_VALUE &&
  Category.Key != INVALID_VALUE &&
  ...;
  
  Either of these seem better?
  

• For me, in both cases the key improvement is that you don't have to look down to the end of the list of conditions to see how the condition is being used.
  
  In your version as you read the code you first see that it's checking Job.Key, but not what it's doing with the result, and then you see that it's checking Category.Key, but not what it's doing with that value either, until you get to the bottom of the list and THEN you can see that the action is ultimately just to return true or false.
  
  My first option means that (a) you can comment each check separately easily, and (b) you can see right away that if the Job.Key check fails the result will be returning false.
  
  My second option makes it clear right up front that all the &&'d conditions are just combining to produce the return value of the method, because the "return" comes first rather than afterwards.
  
  And no, I don't think there's a better way to refactor, overall. I think it's just a matter of making the inherently ugly code as readable as possible...
  

• @Stuart: Thanks for the explanation. That makes sense. Now it's just a matter of trying both out and seeing which one looks better.
  

• +1 for Stuart's method. I was going to post the same. It think it is much clearer because you don't have to try to keep the train of though all the way down to the bottom. It is much easier to follow and you aren't left wondering if you missed a || or some special parens that changed the logic.
  

• Could this benefit from application of a Specification pattern?
  

• Or, you can take it one step further, especially if you can use any of these validation methods for other situations:
  
  protected virtual bool ValidateStateChangeToActive()
  {
  bool isValid = IsNotNull(ProjectManagerId.ToString(), ClientName, Title) &&
  IsValidKey(Job.Key, Category.Key, Status.Key, Entity.Key, ProjectManagerId) &&
  IsValidDate(StartDate, EndDate);
  return isValid;
  }
  
  private bool IsValidKey(params Guid[] values)
  {
  foreach ( Guid value in values )
  {
  if ( value == INVALID_VALUE )
  return false;
  }
  
  return true;
  }
  
  private bool IsValidDate(params DateTime[] values)
  {
  foreach ( DateTime value in values )
  {
  if ( value == DateTime.MinValue )
  return false;
  }
  
  return true;
  }
  
  private bool IsNotNull(params String[] values)
  {
  foreach ( String value in values )
  {
  if ( String.IsNullOrEmpty(value) )
  return false;
  }
  
  return true;
  }
  
  Regards,
  Bobby
  

• Or, you can take it one step further, especially if you can use any of these validation methods for other situations:
  
  protected virtual bool ValidateStateChangeToActive()
  {
  bool isValid = IsNotNull(ProjectManagerId.ToString(), ClientName, Title) &&
  IsValidKey(Job.Key, Category.Key, Status.Key, Entity.Key, ProjectManagerId) &&
  IsValidDate(StartDate, EndDate);
  return isValid;
  }
  
  private bool IsValidKey(params Guid[] values)
  {
  foreach ( Guid value in values )
  {
  if ( value == INVALID_VALUE )
  return false;
  }
  
  return true;
  }
  
  private bool IsValidDate(params DateTime[] values)
  {
  foreach ( DateTime value in values )
  {
  if ( value == DateTime.MinValue )
  return false;
  }
  
  return true;
  }
  
  private bool IsNotNull(params String[] values)
  {
  foreach ( String value in values )
  {
  if ( String.IsNullOrEmpty(value) )
  return false;
  }
  
  return true;
  }
  
  Regards,
  Bobby
  

• Other than the idea of individual if statements I don't think you can do much to make it nicer.
  
  The fact that is already it's own individual method I think is as good as it gets.
  

• You list the requirements for a valid project. How about breaking all of these out into separate methods? I.E. Have an HasValidJobNumber method, an HasValidCategory method, and so on and so forth.
  
  Then the method becomes:
  
  protected virtual bool ValidateStateChangeToActive()
  {
  if (
  HasValidJobNumber( ) &&
  HasValidCategory( ) &&
  HasValidStartDate( ) &&
  ...
  )
  {
  return true;
  }
  return false;
  }
  
  
  More typing in favour of more clarity, says I.
  

• Couldnt you use a Strategy to accomplish each validation, and then use a composite Strategy to handle all of them at once?
  

• @The Other Steve (Maine? Eichert?): Yes of course it is being called somewhere. The context would be when the user makes a change in the UI to activate a project. So the sequence would go like this:
  
  1. User makes change in UI to activate a project.
  2. View calls presenter
  3. Presenter updates model from the view (off the top of my head can't remember if this is done with an in-memory object)
  4. Presenter sets models state to Active.
  5. Model calls this method.
  6. Presenter checks return value from change and throws exception or somehow communicates with view that change was a success (or just goes on and only tells the view when the call fails)
  
  Hope that puts some context around the calling of this code.
  

• Ah Acidine Idea.
  If you have a Go int.(ActivateStatus)
  For each property's validation set a bit in ActivateStatus to 1. in ValidateStateChangeToActive() you should only have to check if ActivateStatus == OK_TO_GO
  

• I like an approach similar to Bobby's and Refactoring the common logic into paramaterized methods greatly improves the readability of the code. Also it removes the reliance on comments.