Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code Quality Issue #2

Open
bartmika opened this issue May 17, 2022 · 0 comments
Open

Code Quality Issue #2

bartmika opened this issue May 17, 2022 · 0 comments
Assignees
Labels
enhancement New feature or request

Comments

@bartmika
Copy link
Owner

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 Monday

Others in this thread have pointed out obvious misspellings:

func GetForstDateFromMonthAndYear
You are SHOUTING at us:
// iterate ONCE AGAIN back to Monday
@bartmika bartmika added the enhancement New feature or request label May 17, 2022
@bartmika bartmika self-assigned this May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant