CODEWITHSUNDEEP
c#.netdatedatetime
Michael A
Michael A

Reputation: 9900

Trying to compare today's date to a date a month ago

I have an object that is loading configurations from a database. I store the last time that a job was run with a datetime field (called GroupsLastRun) and I store how often the job should run with a string field called Captureusersandgroups. Captureusersandgroups stores three different types 'DAILY', 'WEEKLY' and 'MONTHLY'.

Basically, I have a loop should only continue if the job is due to run. So far I've got to the following point:

if (configEntity.GroupsLastrun > DateTime.Now.AddDays(-1) && configEntity.Captureusersandgroups == "DAILY") continue;
if (configEntity.GroupsLastrun > DateTime.Now.AddDays(-7) && configEntity.Captureusersandgroups == "WEEKLY") continue;
if (configEntity.GroupsLastrun > DateTime.Now.AddDays(-30) && configEntity.Captureusersandgroups == "MONTHLY") continue;

I'm sure (certain) there's a much better approach to this but primarily being a SQL Server developer I lack the critical thinking / tools for approaching this. What is a better approach or what should I learn so I can think about this better?

Upvotes: 2

Views: 962

Answers (3)

Eren Ersönmez
Eren Ersönmez

Reputation: 39085

As an alternative approach, you could possibly store the run period in an enum like :

enum RunPeriod
{
    Daily = 1,
    Weekly = 7,
    Monthly = 30
}

And then you can store the int values, instead of string, on the database. This allows you to do the filtering on the DB side, e.g.:

var configsToRun = 
    from c in _myContext.Configs
    where EntityFunctions.AddDays(c.LastRun,(int)c.RunPeriod) > DateTime.Now);

Upvotes: 1

Jon Skeet
Jon Skeet

Reputation: 1500275

A few points:

  • Unless you want to be at the mercy of time zones and daylight saving transitions etc, I'd use DateTime.UtcNow rather than DateTime.Now (and make sure you store the UTC value as well)
  • As p.s.w.g. mentioned, it's worth only asking for the current date/time once - but rather than performance, I'd say that the important reason is for consistency. In this case it looks like you're only going to actually use one of the values, but in other cases I've seen people write conditions where two evaluations are used at the same time, leading to issues if the code is run over midnight
  • You've got repeated code due to checking the condition and determining the deadline at the same time. I'd separate the two.

So, I'd have code like this:

// Consider whether you actually want DateTime.UtcNow.Date
DateTime now = DateTime.UtcNow;

DateTime deadline;
switch (configEntity.Captureusersandgroups)
{
    case "DAILY": deadline = now.AddDays(-1);
    case "WEEKYLY": deadline = now.AddDays(-7);
    case "MONTHLY": deadline = now.AddMonths(-1);
    // I'm assuming there's *always* a schedule
    default: throw new InvalidOperationException("Invalid schedule");
}
if (configEntity.GroupsLastrun > deadline)
{
    continue;
}

Note that subtracting a month from "now" isn't the same as adding a month from "then". For example, if the last run was on January 30th, the next run won't be until March 1st using the above code - whereas if you added a month to January 30th, it would next be run on February 28th (unless you use the dates of both values). Consider carefully exactly the behaviour you want.

(As a quick plug, I'd also obviously recommend considering my Noda Time library for date/time work. It makes it clearer whether any particular value is a local time, or in some time zone etc.)

Upvotes: 7

p.s.w.g
p.s.w.g

Reputation: 149010

Two points:

  1. DateTime.Now can (potential) return a different date every time it's called. It's also not very fast. You'll get better consistency and a very slight performance boost by only calling it once.
  2. You should probably use the standard methods for adding weeks and months, for consistency (e.g. there's not always 30 days in a month) and globalization (e.g. not all cultures have a 7-day week). Note that there's no easy way to add weeks with a DateTime alone; you'll have to use a Calendar instead.

Try this:

var now = DateTime.UtcNow; // See Jon Skeet's answer
var cal = CultureInfo.InvariantCulture.Calendar;
if (configEntity.GroupsLastrun > now.AddDays(-1) && configEntity.Captureusersandgroups == "DAILY") continue;
if (configEntity.GroupsLastrun > cal.AddWeeks(now, -1) && configEntity.Captureusersandgroups == "WEEKLY") continue;
if (configEntity.GroupsLastrun > now.AddMonths(-1) && configEntity.Captureusersandgroups == "MONTHLY") continue;

Or just use cal for everything:

var now = DateTime.UtcNow; // See Jon Skeet's answer
var cal = CultureInfo.InvariantCulture.Calendar;
if (configEntity.GroupsLastrun > cal.AddDays(now, -1) && configEntity.Captureusersandgroups == "DAILY") continue;
if (configEntity.GroupsLastrun > cal.AddWeeks(now, -1) && configEntity.Captureusersandgroups == "WEEKLY") continue;
if (configEntity.GroupsLastrun > cal.AddMonths(now, -1) && configEntity.Captureusersandgroups == "MONTHLY") continue;

Upvotes: 3

Related Questions