-1
\$\begingroup\$

I have if statement with many checks. There must be a better way to implement this. Does someone have an idea on how to improve this method? I have many similar methods in production. I would like to do the refactoring.

 private void CheckWorkingHoursValidity(WorkingDayRequestDto workingDayDto)
 {
     if (!workingDayDto.MondayEnd.HasValue && workingDayDto.MondayStart.HasValue ||
         workingDayDto.MondayEnd.HasValue && !workingDayDto.MondayStart.HasValue ||
         !workingDayDto.TuesdayEnd.HasValue && workingDayDto.TuesdayStart.HasValue ||
         workingDayDto.TuesdayEnd.HasValue && !workingDayDto.TuesdayStart.HasValue ||
         !workingDayDto.WednesdayEnd.HasValue && workingDayDto.WednesdayStart.HasValue ||
         workingDayDto.WednesdayEnd.HasValue && !workingDayDto.WednesdayStart.HasValue ||
         !workingDayDto.ThursdayEnd.HasValue && workingDayDto.ThursdayStart.HasValue ||
         workingDayDto.ThursdayEnd.HasValue && !workingDayDto.ThursdayStart.HasValue ||
         !workingDayDto.FridayEnd.HasValue && workingDayDto.FridayStart.HasValue ||
         workingDayDto.FridayEnd.HasValue && !workingDayDto.FridayStart.HasValue ||
         !workingDayDto.SaturdayEnd.HasValue && workingDayDto.SaturdayStart.HasValue ||
         workingDayDto.SaturdayEnd.HasValue && !workingDayDto.SaturdayStart.HasValue ||
         !workingDayDto.SundayEnd.HasValue && workingDayDto.SundayStart.HasValue ||
         workingDayDto.SundayEnd.HasValue && !workingDayDto.SundayStart.HasValue)
     {
         throw new ValidationException("Both start and end time must be set or empty");
     }

     if (workingDayDto.MondayEnd.HasValue && workingDayDto.MondayStart.HasValue &&
         TimeSpan.Compare(workingDayDto.MondayEnd.Value, workingDayDto.MondayStart.Value) <= 0 ||
         workingDayDto.TuesdayEnd.HasValue && workingDayDto.TuesdayStart.HasValue &&
         TimeSpan.Compare(workingDayDto.TuesdayEnd.Value, workingDayDto.TuesdayStart.Value) <= 0 ||
         workingDayDto.WednesdayEnd.HasValue && workingDayDto.WednesdayStart.HasValue &&
         TimeSpan.Compare(workingDayDto.WednesdayEnd.Value, workingDayDto.WednesdayStart.Value) <= 0 ||
         workingDayDto.ThursdayEnd.HasValue && workingDayDto.ThursdayStart.HasValue &&
         TimeSpan.Compare(workingDayDto.ThursdayEnd.Value, workingDayDto.ThursdayStart.Value) <= 0 ||
         workingDayDto.FridayEnd.HasValue && workingDayDto.FridayStart.HasValue &&
         TimeSpan.Compare(workingDayDto.FridayEnd.Value, workingDayDto.FridayStart.Value) <= 0 ||
         workingDayDto.SaturdayEnd.HasValue && workingDayDto.SaturdayStart.HasValue &&
         TimeSpan.Compare(workingDayDto.SaturdayEnd.Value, workingDayDto.SaturdayStart.Value) <= 0 ||
         workingDayDto.SundayEnd.HasValue && workingDayDto.SundayStart.HasValue &&
         TimeSpan.Compare(workingDayDto.SundayEnd.Value, workingDayDto.SundayStart.Value) <= 0)
     {
         throw new ValidationException("End time cannot be before or same as start time");
     }
 }
\$\endgroup\$
  • \$\begingroup\$ This question lacks any indication of what the code is intended to achieve. To help reviewers give you better answers, please add sufficient context to your question, including a title that summarises the purpose of the code. We want to know why much more than how. The more you tell us about what your code is for, the easier it will be for reviewers to help you. The title needs an edit to simply state the task. \$\endgroup\$ – Toby Speight Oct 16 '19 at 7:36
4
\$\begingroup\$

WorkingDayRequestDto workingDayDto

The name of this object is IMO misleading because it seems to represent a whole week. I would call it WorkWeek.


If it's possible to change the data model, I would invent a WorkDay object with a Start and End time:

public class WorkDay
{
  public WorkDay(DateTime? start, DateTime? end)
  {
    Start = start;
    End = end;
  }

  public DayOfWeek? DayOfWeek => Start != null ? (DayOfWeek?)Start.Value.DayOfWeek : null;
  public DateTime? Start { get; }
  public DateTime? End { get; }

  public bool IsValid
  {
    get
    {
      return Start != null && End != null  && Start <= End && Start.Value.DayOfWeek == End.Value.DayOfWeek || Start == null && End == null;
    }
  }

  // And/Or

  public void CheckIsValid()
  {
    if (Start == null && End != null || Start != null && End == null)
      throw new WorkDayNullCheckException(this);
    if (Start != null && End != null && (Start > End || Start.Value.Date != End.Value.Date)
      throw new WorkDayInvalidStateException(this);
  }

}

and then WorkWeek could be defined as something like:

public class WorkWeek
{
  public WorkDay Monday { get; set; }
  public WorkDay Tuesday { get; set; }
  public WorkDay Wednesday { get; set; }
  public WorkDay Thursday { get; set; }
  public WorkDay Friday { get; set; }
  public WorkDay Saturday { get; set; }
  public WorkDay Sunday { get; set; }

  public WorkDay[] Days => new WorkDay[] { Monday, Tuesday, Wednesday, Thursday, Friday, Saturday, Sunday };

  public bool IsValid => Days.All(d => d.IsValid);

  // And/Or

  public void CheckValidity()
  {
    foreach (var day in Days.Where(d => d != null))
    {
      day.CheckIsValid();
    }
  }

}
| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

How about you made a list for EndForDays and StartForDays and do loop?

For example, I made a code snippet by Java.

List<Boolean> EndForDays = Arrays.asList(true, false, false);
List<Boolean> StartForDays = Arrays.asList(true, true, true);

for(int i = 0; i < EndForDays.size(); i++) {
    if(!EndForDays.get(i) && StartForDays.get(i)) {
        // throw
    }

    if(EndForDays.get(i) && !StartForDays.get(i)) {
        // throw
    }
}
| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

I'm not that great at C#, however in other languages when I come across something like this, I usually write a private or helper function to iterate through the object so my code doesn't look like large blocks of checks.

Something like

private void checkDays(WorkingDayRequestDto  dayObject){
        string [] days = new string[]  { "Monday", "Tuesday", "Wednesday", "Thursday","Friday","Saturday", "Sunday" };
        foreach(string x in days){
            DayType start = typeof(DayType).GetProperty( days[x] + "Start");
            DayType end = typeof(DayType).GetProperty( days[x] + "End");
            if(!end.HasValue && start.HasValue || end.HasValue && !start.HasValue){
                throw new ValidationException("Both start and end time must be set or empty");
            }
        }
    }
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't speak C#, but you could probably simplify the code somewhat by using the exclusive or operator ^ . if(end.HasValue ^ start.HasValue ){ throw new ValidationException("Both start and end time must be set or empty"); } \$\endgroup\$ – Peter Jennings Oct 15 '19 at 22:08

Not the answer you're looking for? Browse other questions tagged or ask your own question.