-
Notifications
You must be signed in to change notification settings - Fork 3
Description
The following comment was made by ppetreus via this reddit post.
Before using an external package, I perform a code quality audit.
I reviewed this function for code quality:
// FirstDayOfLastISOWeek returns the previous week's monday date.
func FirstDayOfLastISOWeek(now func() time.Time) time.Time {
dt := now()
// iterate back to Monday
for dt.Weekday() != time.Monday {
dt = dt.AddDate(0, 0, -1)
}
dt = dt.AddDate(0, 0, -1) // Skip the current monday we are on!
// iterate ONCE AGAIN back to Monday
for dt.Weekday() != time.Monday {
dt = dt.AddDate(0, 0, -1)
}
return dt
}Algorithms are important.
You use expensive iteration twice. Also, you begin the second iteration on the last day (Sunday) of the previous week. Why iterate a constant number of times (6) for the first day (Monday) of the previous week?
A simple integer arithmetic computation and date normalization will do.
package time
The month, day, hour, min, sec, and nsec values may be outside their usual ranges and will be normalized during the conversion. For example, October 32 converts to November 1.
// FirstDayOfLastISOWeek returns the previous week's Monday date.
func FirstDayOfLastISOWeek(now func() time.Time) time.Time {
dt := now()
// the number of days from this week's Monday
thisMonday := (int(dt.Weekday()) + 6) % 7
return dt.AddDate(0, 0, -thisMonday-7)
}Your spelling of Monday is inconsistent: monday and Monday.
// FirstDayOfLastISOWeek returns the previous week's monday date.
// iterate back to Monday
// Skip the current monday we are on!
// iterate ONCE AGAIN back to MondayOthers in this thread have pointed out obvious misspellings:
func GetForstDateFromMonthAndYear
You are SHOUTING at us:
// iterate ONCE AGAIN back to Monday