From code review: Simplifying / Reducing code duplication

[steffilazerte]

As I mentioned in the parent issue, none of this actually needs to be addressed from a user perspective, but it might simplify things for you or other developers.

Caveat: I'm familiar with S3 but I don't use it in development a whole lot, so I sound wonky, that may be the reason 😁

Methods for base functions

You 'Suggest' the vctrs package to export the S3Methods for the vec_ptype_abbr etc. I think this is because you don't want to depend on the vctrs package but would like grates to work nicely with it for people who use it.

However, if you were willing to import vctrs, I think much of the code could be simplified (the functionality won't change, it just makes it quicker to read). I appreciate that would result in your package being only lightweight, rather than super lightweight, however 😉.

For example to following

out <- NextMethod()
class(out) <- class(x)
out

could be simplified to

vctrs::vec_restore(NextMethod(), x)

Which also should keep attributes, meaning that

out <- NextMethod()
class(out) <- class(x)
attr(out, "n") <- attr(x, "n")
attr(out, "offset") <- attr(x, "offset")

can also be replaced with:

vctrs::vec_restore(NextMethod(), x)

From https://adv-r.hadley.nz/s3.html#s3-subclassing (and from my tests)

Split functions

For more complex methods for base functions, some could use a generic internal function to avoid duplicate code (e.g., c.xxx, seq.xxxx, [<-.xxx, Ops). For those which use attributes, you could programatically loop through attributes which exist.

Could also do this for other re-occurring checks (like the ones you already have in utils.R). For example where you check and convert intergerish numbers you could use:

# Generic check
.as_integerish <- function(x, name) {
  if (is.vector(x, "double")) {
        x <- as.integer(floor(x))
    } else if (!is.integer(x)) {
        stop("`", name, "` must be integer.")
    }
}
# Usage
year <- .as_integerish(year, "year")

week classes

There seems to be a lot of duplication among the week classes and I wonder if it could be reduced to make it easier to maintain (i.e. any changes to a function would only need to be made once, rather than three times).

• Could yearweek(), epiweek(), isoweek() be built on a set of generic 'week' functions? It wouldn't necessarily reduce a lot of code, but it would definitely reduce duplication and the main working code.
• Instead of having yearweek_monday, yearweek_tuesday etc. why not have an attribute which defines the starting week day? Or have I missed some complexity?
• .isoweek(), .epiweek() could probably be removed and instead use direct calls to .yearweek()?
• Since epiweek() is really just yearweek() with firstday 7, is it necessary to have a whole set of epiweek functions? If it is necessary (for clarity and usability), could they be simplified to refer to directly to all yearweek functions to reduce duplication?

[TimTaylor]

Added a .make_floored_integer() function in e138ea3