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 issues #24

Open
fabian-s opened this issue May 2, 2022 · 1 comment
Open

Code quality issues #24

fabian-s opened this issue May 2, 2022 · 1 comment

Comments

@fabian-s
Copy link

fabian-s commented May 2, 2022

Running goodpractice::gp on ALUES yields the suggestions I pasted below.
Fixing/improving (some of) them is not required for JOSS publications, but recommended.

It is good practice to

✖ write unit tests for all functions, and all package code in
general. 78% of code lines are covered by test cases.

R/overall_suit.R:101:NA
R/overall_suit.R:104:NA
R/overall_suit.R:105:NA
R/overall_suit.R:110:NA
R/overall_suit.R:113:NA
... and 219 more lines

✖ write short and simple functions. These functions have high
cyclomatic complexity:suitability (171).
✖ not use "Depends" in DESCRIPTION, as it can cause name
clashes, and poor interaction with other packages. Use "Imports"
instead.
✖ omit "Date" in DESCRIPTION. It is not required and it gets
invalid quite often. A build date will be added to the package when
you perform R CMD build on it.
✖ use '<-' for assignment instead of '='. '<-' is the
standard, and R users and developers are used it and it is easier
to read your code for them if you use '<-'.

R/overall_suit.R:137:8
R/overall_suit.R:137:16
R/overall_suit.R:137:27
R/overall_suit.R:137:37
R/overall_suit.R:137:48
... and 153 more lines

✖ avoid long code lines, it is bad for readability. Also,
many people prefer editor windows that are about 80 characters
wide. Try make your lines shorter than 80 characters

R/ALFALFASoil.R:3:1
R/ALFALFASoil.R:24:1
R/ALFALFATemp.R:3:1
R/ALFALFATemp.R:13:1
R/ALFALFATerrain.R:3:1
... and 1395 more lines

✖ omit trailing semicolons from code lines. They are not
needed and most R coding standards forbid them

R/overall_suit.R:137:52
tests/testthat/test_suitability_min.R:63:31
tests/testthat/test_suitability_min.R:117:35
tests/testthat/test_suitability_sowmonth.R:60:137
tests/testthat/test_suitability_sowmonth.R:91:137
... and 30 more lines

✖ avoid sapply(), it is not type safe. It might return a
vector, or a list, depending on the input data. Consider using
vapply() instead.

R/overall_suit.R:163:16

✖ avoid 1:length(...), 1:nrow(...), 1:ncol(...), 1:NROW(...)
and 1:NCOL(...) expressions. They are error prone and result 1:0 if
the expression on the right hand side is zero. Use seq_len() or
seq_along() instead.

R/suitability.R:69:15
R/suitability.R:70:17
R/suitability.R:188:12
tests/testthat/test_suitability.R:75:13

✖ not import packages as a whole, as this can cause name
clashes between the imported packages. Instead, import only the
specific functions you need.
✖ fix this R CMD check NOTE: installed size is 6.6Mb
sub-directories of 1Mb or more: doc 2.4Mb help 2.0Mb libs 1.9Mb


part of this JOSS review

@alstat
Copy link
Owner

alstat commented May 3, 2022

Noted on these @fabian-s, thank you. These are indeed useful suggestions and will consider these on next release.

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

2 participants