pharmaverse/admiralvaccine

Update derive_vars_max_flag function

Closed this issue · 0 comments

Feature Idea

  1. Title should have everything in proper case - capitalise "flags"
  2. @param lines: no dash ("-") required at the start.
  3. @param lines: {admiral} convention is not to specify default values here, as you can jsut check the function.
  4. @return line: I woul replace "creates" with "with".
  5. function requires FATPTREF, PARAMCD, AVAL, FATPT but there are no checks for this.
  6. 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.
  7. Instead of "stop("Please mention flag name")" I would be more specfic, eg: "Both flag names cannot be NULL".
  8. 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.
  9. 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