patterncsharpMinor
Long calendar related function
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
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
In general I highly recommend runnign StyleCop and listening to it, except for the line
Btw, I am pretty sure that the C# compiler can handle that
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:
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 CheckYearCode 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 CheckYearContext
StackExchange Code Review Q#8578, answer score: 3
Revisions (0)
No revisions yet.