trjhtr

The following code compares two dates and checks if they are within 15 minutes of each other, with a tolerance of a second. All of the test cases below pass with the existing solution. The question is can it be improved by making it more efficient and/or elegant given the following test cases.

TestId, whenCreated, whenToCompareTo, expected Outcome

[(1, "01/01/2017 15:00:00", "01/01/2017 15:15:00", true)]
[(2, "01/01/2017 15:00:00", "01/01/2017 15:00:00", true)]
[(3, "01/01/2017 15:00:00", "01/01/2017 15:16:00", false)]
[(4, "01/01/2017 15:00:00", "01/01/2017 14:59:59", false)]
[(5, "01/01/2017 15:00:00", "01/01/2017 15:14:59", true)]
[(6, "01/01/2017 15:00:00", "01/01/2017 15:16:01", false)]
[(7, "2017-01-01T15:00:00.0000001", "2017-01-01T15:15:00.0000002", true)]
[(8, "2017-01-01T15:15:00.5170000", "2017-01-01T15:15:00.4537587", true)]
[(9, "2017-01-01 15:00:00", "2017-12-31 15:00:00", false)]
[(10, "2018-05-06T13:30:43.2200000", "2018-05-06T13:30:42.9978916", true)]

And here is the code that is doing the comparison:

public bool IsTokenAlive(DateTime timeToCompareTo)
 GetTotalSeconds(timeToCompareTo, Created) < 1) && isExpired;

 return isAlive;


private static bool DateComparison(DateTime timeToCompareTo, DateTime created)

 return DateTime.Compare(timeToCompareTo.Date, created.Date) >= 0;


private static bool DateTimeOfDayComparison(DateTime timeToCompareTo, DateTime created)

 //We are interested to compare up to the seconds, we are not interested in milliseconds
 return TimeSpan.Compare(StripMilliseconds(timeToCompareTo).TimeOfDay, StripMilliseconds(created).TimeOfDay) >= 0;


private static bool AbsoluteSecondsComparison(DateTime timeToCompareTo, DateTime created)

 return GetTotalSeconds(timeToCompareTo, created) < MaxTimeInSecondsTokenIsAlive;


private static DateTime StripMilliseconds(DateTime dateTime)

 return dateTime.AddTicks(-(dateTime.Ticks % TimeSpan.TicksPerSecond));


private static double GetTotalSeconds(DateTime timeToCompareTo, DateTime created)

 return (timeToCompareTo - created).Duration().TotalSeconds;

The Created property is set when the object is created and it will always be the timestamp of the server.

Naming:

Method names should contain a verb and describe exactly what they are doing. For example, look at DateComparison. That sounds more like a type name to me, and I have no idea what the method will do by its name alone.

StripMilliseconds and GetTotalSeconds are much better.

I'd suggest this is as an alternate solution:

public static class TokenValidator

 private const int aliveDurationMinutes = 15;

 /// <summary>
 /// Checks if the token is still alive with a 1-second tolerance.
 /// </summary>
 public static bool CheckTokenAlive(DateTime created, DateTime timeToCompare)
 
 TimeSpan elapsedTime = timeToCompare - created;

 return (elapsedTime.TotalSeconds > -1 && elapsedTime.TotalSeconds < 0) // up to 1 second before
 

I use Math.Floor on the TotalSeconds to add the 1-second tolerance. You can be 0.9999 of a second over and still evaluate as true. You could have done a strictly less than with ((aliveDurationMinutes * 60) + 1) instead.

Benefit from short circuit behavior

In an expression like a() && b() && ..., when a() is false, then the rest of the expression will not need to be evaluated, because we can already know that the value of the entire expression will be false. Btw the same is true in a() || b() || ... when a() is true. This is called short circuit evaluation of conditional expressions and it's good to benefit from it when possible.

If you write x = a(); y = b(); and then use x && y && ... then you don't benefit from short circuit evaluation, because b() has already been computed by the time the conditional expression is reached.

Naming

It's very difficult to understand what the code does, because of the names of methods and variables. For example, the word "comparison" is a noun, so it's not intuitive what will be the return value from that. Then I see the return type is boolean, but then when will it be true or false? Consider as an alternative isBefore. This name naturally implies boolean type, and that the value will be true if the first parameter date is before the second.

Personally, I don't like all the private static methods. In my mind, you should create new methods in these cases:

1. Follow the Single Responsibility Principle


2. Make Code more readable (Long method)


3. Self-Document your code (the name of the method is a self-documentation)


4. Split out logic that needs to be overridden in lower classes (which you can't do with a static method)

Personally, I don't see a benefit to splitting out these particular methods, and I think they actually make the code harder to read. In this case, I would prefer the logic in one single method.

Also, I would find a way to make an adjustable threshold for the time that you allow to be alive. I like @RobH's const int of 15 minutes better than my static Timespans, but I will stick with them for this answer. I made the first one 15 minutes, 1 second. I made the second one -1 seconds. This is to handle the 1 second tolerance you mentioned.

private static TimeSpan MAX_ALLOWED_DIFF = new TimeSpan(0,15,1);
private static TimeSpan MIN_ALLOWED_DIFF = new TimeSpan(0,0,-1);

public bool IsTokenAlive(DateTime timeToCompareTo, DateTime created)

 TimeSpan diff = (created - timeToCompareTo);
 diff = diff - new TimeSpan(0,0,0,0,diff.Milliseconds);

 return diff < MAX_ALLOWED_DIFF && diff > MIN_ALLOWED_DIFF;

Also, for those who are saying this doesn't pass all the tests, here is the linqpad script I used to test. If I messed something up when I copied the tests, please let me know.

void Main()

 IsTokenAlive(Parse("01/01/2017 15:00:00"), Parse("01/01/2017 15:15:00")).Dump(); //true
 IsTokenAlive(Parse("01/01/2017 15:00:00"), Parse("01/01/2017 15:00:00")).Dump(); //true
 IsTokenAlive(Parse("01/01/2017 15:00:00"), Parse("01/01/2017 15:16:00")).Dump(); //false
 IsTokenAlive(Parse("01/01/2017 15:00:00"), Parse("01/01/2017 14:59:59")).Dump(); //false
 IsTokenAlive(Parse("01/01/2017 15:00:0"), Parse("01/01/2017 15:14:59")).Dump(); //true
 IsTokenAlive(Parse("01/01/2017 15:00:00"), Parse("01/01/2017 15:16:01")).Dump(); //false
 IsTokenAlive(Parse("2017-01-01T15:00:00.0000001"), Parse("2017-01-01T15:15:00.0000002")).Dump(); //true
 IsTokenAlive(Parse("2017-01-01T15:15:00.5170000"), Parse("2017-01-01T15:15:00.4537587")).Dump(); //true
 IsTokenAlive(Parse("2017-01-01 15:00:00"), Parse("2017-12-31 15:00:00")).Dump(); //false
 IsTokenAlive(Parse("2018-05-06T13:30:43.2200000"), Parse("2018-05-06T13:30:42.9978916")).Dump(); //true



private DateTime Parse(string s)

 DateTime result = new DateTime();
 DateTime.TryParse(s, out result);
 return result;


// Define other methods and classes here

private static TimeSpan MAX_ALLOWED_DIFF = new TimeSpan(0,15,1);
private static TimeSpan MIN_ALLOWED_DIFF = new TimeSpan(0,0,-1);

public bool IsTokenAlive(DateTime timeToCompareTo, DateTime created)

 TimeSpan diff = (created - timeToCompareTo);
 diff = diff - new TimeSpan(0,0,0,0,diff.Milliseconds);

 return diff < MAX_ALLOWED_DIFF && diff > MIN_ALLOWED_DIFF;