Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

##Room for improvement:

  • ExtensionMethods is a terrible name. What are we extending? We're extending the TimeZoneInfo class, so we should probably call our extension methods something like TimeZoneExtensions. There are lots of optionslots of options for naming schemes. Pick one you like.

  • GetAdjustmentRuleForYear can return null. I'm not a fan of that. What's worse is the XML doc doesn't even mention it. You should at least document it, but I would personally at least consider throwing an exception.

  • Speaking of null, it's possible for this to be null, so you should also consider checking for it.

  • This is a pretty useless comment. It serves no purpose but to clutter the code.

 // Iterate adjustment rules for time zone 
    foreach (TimeZoneInfo.AdjustmentRule adjustment in adjustments)
  • firstDayOfWeek is a misleading name. It looks like it's really the first day of the month. I suggest firstWeedayOfMonth instead.
 // What day of the week does the month start on?
    int firstDayOfWeek = (int)localCalendar.GetDayOfWeek(new DateTime(year, transitionTime.Month, 1));
  • If you need a comment to say what, you generally need to extract a well named method instead.
// Determine how much start date has to be adjusted
    int transitionDay;
    int changeDayOfWeek = (int)transitionTime.DayOfWeek;
    if (firstDayOfWeek <= changeDayOfWeek)
        transitionDay = startOfWeek + (changeDayOfWeek - firstDayOfWeek);
    else
        transitionDay = startOfWeek + (7 - firstDayOfWeek + changeDayOfWeek);
  • That statement could also use some braces in my opinion.

##What I like:

  • Use of XML docs on public methods.
  • You've generally used pretty good names (other than ones I've mentioned).
  • It's fairly readable code.
  • Methods are short and to the point with the exception of GetFixedDateRuleDate.

##Room for improvement:

  • ExtensionMethods is a terrible name. What are we extending? We're extending the TimeZoneInfo class, so we should probably call our extension methods something like TimeZoneExtensions. There are lots of options for naming schemes. Pick one you like.

  • GetAdjustmentRuleForYear can return null. I'm not a fan of that. What's worse is the XML doc doesn't even mention it. You should at least document it, but I would personally at least consider throwing an exception.

  • Speaking of null, it's possible for this to be null, so you should also consider checking for it.

  • This is a pretty useless comment. It serves no purpose but to clutter the code.

 // Iterate adjustment rules for time zone 
    foreach (TimeZoneInfo.AdjustmentRule adjustment in adjustments)
  • firstDayOfWeek is a misleading name. It looks like it's really the first day of the month. I suggest firstWeedayOfMonth instead.
 // What day of the week does the month start on?
    int firstDayOfWeek = (int)localCalendar.GetDayOfWeek(new DateTime(year, transitionTime.Month, 1));
  • If you need a comment to say what, you generally need to extract a well named method instead.
// Determine how much start date has to be adjusted
    int transitionDay;
    int changeDayOfWeek = (int)transitionTime.DayOfWeek;
    if (firstDayOfWeek <= changeDayOfWeek)
        transitionDay = startOfWeek + (changeDayOfWeek - firstDayOfWeek);
    else
        transitionDay = startOfWeek + (7 - firstDayOfWeek + changeDayOfWeek);
  • That statement could also use some braces in my opinion.

##What I like:

  • Use of XML docs on public methods.
  • You've generally used pretty good names (other than ones I've mentioned).
  • It's fairly readable code.
  • Methods are short and to the point with the exception of GetFixedDateRuleDate.

##Room for improvement:

  • ExtensionMethods is a terrible name. What are we extending? We're extending the TimeZoneInfo class, so we should probably call our extension methods something like TimeZoneExtensions. There are lots of options for naming schemes. Pick one you like.

  • GetAdjustmentRuleForYear can return null. I'm not a fan of that. What's worse is the XML doc doesn't even mention it. You should at least document it, but I would personally at least consider throwing an exception.

  • Speaking of null, it's possible for this to be null, so you should also consider checking for it.

  • This is a pretty useless comment. It serves no purpose but to clutter the code.

 // Iterate adjustment rules for time zone 
    foreach (TimeZoneInfo.AdjustmentRule adjustment in adjustments)
  • firstDayOfWeek is a misleading name. It looks like it's really the first day of the month. I suggest firstWeedayOfMonth instead.
 // What day of the week does the month start on?
    int firstDayOfWeek = (int)localCalendar.GetDayOfWeek(new DateTime(year, transitionTime.Month, 1));
  • If you need a comment to say what, you generally need to extract a well named method instead.
// Determine how much start date has to be adjusted
    int transitionDay;
    int changeDayOfWeek = (int)transitionTime.DayOfWeek;
    if (firstDayOfWeek <= changeDayOfWeek)
        transitionDay = startOfWeek + (changeDayOfWeek - firstDayOfWeek);
    else
        transitionDay = startOfWeek + (7 - firstDayOfWeek + changeDayOfWeek);
  • That statement could also use some braces in my opinion.

##What I like:

  • Use of XML docs on public methods.
  • You've generally used pretty good names (other than ones I've mentioned).
  • It's fairly readable code.
  • Methods are short and to the point with the exception of GetFixedDateRuleDate.
Source Link
RubberDuck
  • 30.9k
  • 6
  • 71
  • 174

##Room for improvement:

  • ExtensionMethods is a terrible name. What are we extending? We're extending the TimeZoneInfo class, so we should probably call our extension methods something like TimeZoneExtensions. There are lots of options for naming schemes. Pick one you like.

  • GetAdjustmentRuleForYear can return null. I'm not a fan of that. What's worse is the XML doc doesn't even mention it. You should at least document it, but I would personally at least consider throwing an exception.

  • Speaking of null, it's possible for this to be null, so you should also consider checking for it.

  • This is a pretty useless comment. It serves no purpose but to clutter the code.

 // Iterate adjustment rules for time zone 
    foreach (TimeZoneInfo.AdjustmentRule adjustment in adjustments)
  • firstDayOfWeek is a misleading name. It looks like it's really the first day of the month. I suggest firstWeedayOfMonth instead.
 // What day of the week does the month start on?
    int firstDayOfWeek = (int)localCalendar.GetDayOfWeek(new DateTime(year, transitionTime.Month, 1));
  • If you need a comment to say what, you generally need to extract a well named method instead.
// Determine how much start date has to be adjusted
    int transitionDay;
    int changeDayOfWeek = (int)transitionTime.DayOfWeek;
    if (firstDayOfWeek <= changeDayOfWeek)
        transitionDay = startOfWeek + (changeDayOfWeek - firstDayOfWeek);
    else
        transitionDay = startOfWeek + (7 - firstDayOfWeek + changeDayOfWeek);
  • That statement could also use some braces in my opinion.

##What I like:

  • Use of XML docs on public methods.
  • You've generally used pretty good names (other than ones I've mentioned).
  • It's fairly readable code.
  • Methods are short and to the point with the exception of GetFixedDateRuleDate.