I've recently inherited some code that needs a bit of polish.
Here's an example of one method I didn't like:
/********************************************************
*Sunday == 1 and Saturday == 7
* In C#, Sunday == 0 and Saturday == 6
* *****************************************************/
public DateTime GetWeekEndDayDate(DateTime date, int WeekEndDay) {
int temp = 0;
DateTime retval = DateTime.Now;
int Day = (int)date.DayOfWeek;
WeekEndDay--;//decrease by 1 so it lines up with C# enumeration of days
//get the difference in days
if (WeekEndDay > Day)
temp = WeekEndDay - Day;
else if (Day > WeekEndDay)
temp = WeekEndDay - Day + 7;
else
temp = 0;//currently on WeekEndDay
retval = date.AddDays((double)temp);
return retval;
}
So, how to fix?
I need to verify that after my refactoring I still get the correct result. Since this class lacks a constructor that I can use without going through some other non-related initialization I've added my own constructor for the time being:
public Tables(bool skipInitialization) {}
I will use this in my test case that will check the existing behavior.
[RowTest]
[Row("10/15/2007",1)]
[Row("10/15/2007",2)]
[Row("10/15/2007",3)]
[Row("10/16/2007",4)]
[Row("10/15/2007",5)]
[Row("10/15/2007",6)]
[Row("10/15/2007",7)]
public void TestInputs_WithExpectedValuesFromOldAlgorithm(string startDate, int dayOfWeekIntValue) {
bool skipInitialization = true;
Tables t = new Tables(skipInitialization);
DateTime start = DateTime.Parse(startDate);
DateTime converted = t.GetWeekEndDayDate(start, dayOfWeekIntValue);
System.Diagnostics.Debug.WriteLine(converted);
}
All I want to do here is pass in some input and record the output. Once I have the output I change the test method and give it some values to check against.
[RowTest]
[Row("10/15/2007",1,"10/21/2007")]
[Row("10/15/2007",2,"10/15/2007")]
[Row("10/15/2007",3,"10/16/2007")]
[Row("10/16/2007",4,"10/17/2007")]
[Row("10/15/2007",5,"10/18/2007")]
[Row("10/15/2007",6,"10/19/2007")]
[Row("10/15/2007",7,"10/20/2007")]
public void TestInputs_WithExpectedValuesFromOldAlgorithm(string startDate, int dayOfWeekIntValue,string expected) {
bool skipInitialization = true;
Tables t = new Tables(skipInitialization);
DateTime start = DateTime.Parse(startDate);
DateTime converted = t.GetWeekEndDayDate(start, dayOfWeekIntValue);
Assert.AreEqual(DateTime.Parse(expected),converted);
}
This test should of course pass, but now I can do some real work. Since the bulk of this is just a generic date manipulation and not the primary responsibility of this class I want to extract that into a helper method. For now I'm must putting it into a Util class that has a bunch of static methods.
public class Util {
public static DateTime GetWeekEndDayDate(DateTime date, int WeekEndDay) {
int temp = 0;
DateTime retval = DateTime.Now;
int Day = (int)date.DayOfWeek;
WeekEndDay--;//decrease by 1 so it lines up with C# enumeration of days
//get the difference in days
if (WeekEndDay > Day)
temp = WeekEndDay - Day;
else if (Day > WeekEndDay)
temp = WeekEndDay - Day + 7;
else
temp = 0;//currently on WeekEndDay
retval = date.AddDays((double)temp);
return retval;
}
}
The goal of this method should just be to advance the input date to the next DayOfWeek passed as the second argument. So I'm going to rename the method to MoveToNext and change the second argument to take a DayOfWeek enumeration value. This means it will be the respibility of the calling code to do that WeekEndDay-- before calling my method.
I've now got this Util method
public static DateTime MoveToNext(DateTime date, DayOfWeek day) {
int temp = 0;
DateTime retval = DateTime.Now;
int Day = (int)date.DayOfWeek;
int dayToMoveTo = (int) day;
//get the difference in days
if (dayToMoveTo > Day)
temp = dayToMoveTo - Day;
else if (Day > dayToMoveTo)
temp = dayToMoveTo - Day + 7;
else
temp = 0;//currently on WeekEndDay
retval = date.AddDays((double)temp);
return retval;
}
And this in my Tables class
public DateTime GetWeekEndDayDate(DateTime date, int WeekEndDay) {
DayOfWeek dayValue = (DayOfWeek) WeekEndDay - 1;
return Util.MoveToNext(date, dayValue);
}
At this point my tests all still pass so now it's time to simplify that move to next method. I find it helps to think of this in terms of looking at a calendar and needing to get to the next Saturday or Monday or whatever. So that's what the code should do. I should be able to remove the conditionals and just walk the calendar until I get to the correct day. Rewritten with this in mind my method becomes
public static DateTime MoveToNext(DateTime date, DayOfWeek day) {
DateTime retval = date;
while(retval.DayOfWeek != day) {
retval = retval.AddDays(1);
}
return retval;
}
IMO this is much simpler to read, and since my tests still pass I haven't changed any functionality. I really wish I could use extension methods in this project because this is just screaming at me for a good case for them. No matter though, I can at least be prepared.I think we can improve this just a bit by renaming the method and changing the order of the parameters.
public static class DateTimeExtensions {
public static DateTime MoveToNext(DateTime date, DayOfWeek day) {
DateTime retval = date;
while (retval.DayOfWeek != day) {
retval = retval.AddDays(1);
}
return retval;
}
}
And, since my tests still pass I am all done.