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

Make code more DRY #173

Closed
echasnovski opened this Issue Aug 6, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@echasnovski
Copy link
Collaborator

echasnovski commented Aug 6, 2018

After reading {infer} code I noticed that some code chunks are repeated multiple times with, possibly, small changes. This seems like a good opportunity to abstract them into functions to facilitate code reusage and increase code stability (see DRY). I created this issue to track those code chunks.

  • Methods of calc_impl for one function, like this one. They can be abstracted into calc_impl_one_f(fun) that returns appropriate functions.
  • Methods of calc_impl for difference, like this one. As in previous one, can be abstracted into calc_impl_diff_f(fun).
  • is.null(attr(x, at)). As this code is repeated many, many times I think it is better to create is_nuat(x, at) function to replace it.
  • Copying attributes from one object to another, for example here. This is currently done with set_attributes(), but I think it should be renamed to copy_attrs(to, from, attr_vec) and have attr_vec as argument.
  • Create separate helper-data.R file in tests/testthat with initializations of data that is used across multiple test files. For example, iris_tbl and mtcars (which probably should be renamed to mtcars_tbl). This file will be sourced by {testthat} before any 'test-*.R' files and results can be used in tests.

If this is considered worthwhile, I plan to make a PR with corresponding changes at the end of this week. Please, add here any thoughts about possibilities of reducing code duplication. Special attention might be devoted to predicates inside if, as there are a lot of them.

@ismayc

This comment has been minimized.

Copy link
Collaborator

ismayc commented Aug 6, 2018

@echasnovski This is all good for me and something we've been wanting to do for awhile. @andrewpbray is also looking into having a function name be an argument to calculate() so that something like calculate(stat_fun = mean, trim = 0.1) would be possible instead of the string input for stat which limits the scope of the package. If you have thoughts on that, please let us know.

@echasnovski

This comment has been minimized.

Copy link
Collaborator Author

echasnovski commented Aug 6, 2018

Great. I would really like to hear about specific places that you think should be DRYed.

I created an issue (#175) to discuss possibility of functional input to calculate().

@ismayc

This comment has been minimized.

Copy link
Collaborator

ismayc commented Aug 6, 2018

I think visualize() and calculate() (with all the impl functions) would benefit most from being DRYed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment