HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Long calendar related function

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
functionrelatedlongcalendar

Problem

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;
}

Solution

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

Code Snippets

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);
}
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

Context

StackExchange Code Review Q#8578, answer score: 3

Revisions (0)

No revisions yet.