Update derive_vars_max_flag function
Closed this issue · 0 comments
vikrams95 commented
Feature Idea
- Title should have everything in proper case - capitalise "flags"
- @param lines: no dash ("-") required at the start.
- @param lines: {admiral} convention is not to specify default values here, as you can jsut check the function.
- @return line: I woul replace "creates" with "with".
- function requires FATPTREF, PARAMCD, AVAL, FATPT but there are no checks for this.
- for neatness, consider moving the inner "flag" function outside, so that you can write some documentation for it. Then, if you don't want to export it, don't add @export.
- Instead of "stop("Please mention flag name")" I would be more specfic, eg: "Both flag names cannot be NULL".
- I may be misunderstanding, but it looks to me like the four if statements at the end of the function could just be replaced by two if statements: one testing if flag1 is null and one testing if flag2 is null.
- In test file: test3 isn't useful as you are removing the rows with FATESTCD = "OCCUR" before running the test. So you are actually just checking if filtering works!
General comment: this function needs to be styled using styler and lintr.
Relevant Input
No response
Relevant Output
No response
Reproducible Example/Pseudo Code
No response