3
\$\begingroup\$

This works but feels like alot of code for what it accomplishes. I feel like I'm maybe missing something obvious.

// When current month is same as target month, then return the current day of the month.
// Else return the number of days in the target month.
static int DefaultDaysToSynch()
{
    var now = DateTime.Now;
    int result = now.Day;
    int month = Properties.Settings.Default.StatsMonth;
    int year = Properties.Settings.Default.StatsYear;
    if (!((year == now.Year) && (month == now.Month)))
    {
        result = DateTime.DaysInMonth(year, month);
    }
    return result;
}
\$\endgroup\$
3
\$\begingroup\$

It is hard to improve on Jesse's answer (there are only so many ways to write this method), but I will add my alternative, which is a matter of style. I personally tend to not use var in place of any of the built-in types listed here: http://msdn.microsoft.com/en-us/library/ms228360(v=vs.90).aspx it does not save me any more space than writing "don't" instead of "do not".

I also do not like it when the lines get too long, particularly ones that involve the ternary operator, so here is my version (assuming public modifier):

public static int DefaultDaysToSynch()
{
    int month = Properties.Settings.Default.StatsMonth;
    int year = Properties.Settings.Default.StatsYear;
    var now = DateTime.Now;
    bool isTargetMonth = (year == now.Year) && (month == now.Month);
    return isTargetMonth ? now.Day : DateTime.DaysInMonth(year, month);
}

In general I highly recommend runnign StyleCop and listening to it, except for the line bool isTargetMonth = (year == now.Year) && (month == now.Month); - keep those parenthesis. Btw, I am pretty sure that the C# compiler can handle that bool variable efficiently. The descriptive variable name also reduces (though maybe not completely) the need for a comment.

The comment would have to pass the StyleCop's standard if I wrote it out, but I am too lazy, sorry. There is not much to write really, because it does not take any arguments.

I am not a huge fan of the input values being read directly from settings - it makes this function harder to test, it makes it less "functional" as in functional style, it also allows for bad input (such as year -2598, month # 455) to go unnoticed. I would rather pass both values into the function like so:

public static int DefaultDaysToSynch(int targetYear, int targetMonth)
{
    CheckYear(targetYear);
    CheckMonth(targetMonth);
    var now = DateTime.Now;
    bool isTargetMonth = (targetYear == now.Year) && (targetMonth == now.Month);
    return isTargetMonth ? now.Day : DateTime.DaysInMonth(year, month);
}

// You will need to catch this at the right place.
// This is lengthy, I know, but makes a decent exception message.
private static CheckMonth(int monthNumber)
{
    if (monthNumber >= 1 && monthNumber <= 12)
    {
        return;
    }

    string exceptionMessage = String.Format(
        "Month number must be between 1 and 12 but it was {0}.",
        monthNumber);

    throw new ArgumentOutOfRangeException(
        paramName: "monthNumber",
        actualValue: monthNumber,
        message: exceptionMessage);
}

// Similar implementation for CheckYear
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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