-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\$

closed as off-topic by 200_success, pacmaninbw, AlexV, Linny, Toby Speight Oct 16 at 7:36

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – 200_success, pacmaninbw, AlexV, Linny, Toby Speight
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\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 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();
    }
  }

}
\$\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
    }
}
\$\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");
            }
        }
    }
\$\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 at 22:08

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