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 Review #2

Open
DarioS opened this issue Mar 17, 2022 · 0 comments
Open

Code Review #2

DarioS opened this issue Mar 17, 2022 · 0 comments

Comments

@DarioS
Copy link

DarioS commented Mar 17, 2022

  • Use spaces for better readability. For example, combineDist <- function(dist.dat) should be combineDist <- function(dist.dat).
  • Always use <- for variable assignment. Sometimes you use <- and sometimes =. Of course, you should continue to use = for default values of function parameters, such as function(cv.fit, kk = NULL, cvr = 50).
  • Use consistent capitalisation. Instead of functions combineDist, getstats, plotstats have combineDist, getStats, plotStats.
  • Use informative variable names. For example, dd, kk are too short and not informative names.
  • Use lapply or mapply instead of for loops wherever possible. For instance, the code
centroids<-list()
for (i in 1:cv.rounds)
{
  centroids[[i]]<-get.centroid(cmd.mat, fit[[i]]$cv.labels,i)
}

Could possibly be more concisely replaced by

centroids <- lapply(fits, function(fit) get.centroid(cmd.mat, fit[["cv.labels"]]))
  • All exported functions should be documented. In the previous point, I didn't know what the purpose of f is for get.centroid. get.centroid is exported because all of the functions of your package which don't begin with a dot are because of exportPattern("^[[:alpha:]]+") in NAMESPACE. But, I see no Rd file for get.centroid and using a variable name like f isn't intuitive for guessing its purpose, either. So, I didn't know how to adapt the above example to include f, so it was ignored.
  • Name the variables to indicate whether it contains one value or more. I would use fits or modelFits instead of fit above.
  • VIgnette not available. It should show the reader what functions they should use and in which order. Right now, the user has no guidance about which function they should use first, which function they should use second and how to interpret the output of each function. There should be a data directory in your package which stores one example data set which your vignette uses to demonstrate the package's features. See Data in Packages section of Writing R Extensions for more details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant