JOSS Review: Code Style and naming arguments of functions

[njtierney]

Using a style helper

To help standardise the code display, I think it would be worthwhile for the authors to run code like:

styler::style_pkg(".")

This would make the package easier to read, add necessary whitespace, and remove large chunks of whitespace throughout the package.

Consistency in naming of functions

Functions in the package are not named consistently. For example:

• SCPRT()
• Size_ord()
• logranktest()
• Size_ord()

I suggest that the authors stick to lower_case_snake_case() which uses all lower case functions, and _ to separate words. The functions above would then be rewritten as:

• scprt()
• size_ord()
• log_rank_test()
• size_ord()

SCPRT

Why is this function called this? The title and description for the function are:

#' @title  Efficacy and Futility boundary values
#' @description This function calculates the upper and lower bound values for MAMS trial.

If I was trying to remember this function, why would SCPRT come to mind? Perhaps the documentation could explain this?

Or does this not matter because it is an internal function?

Consistency in naming of arguments

Arguments to the functions should be lowercase - there is a mix of upper and lowercase letters, which might be hard for the user to remember.

Naming arguments

In addition, the package frequently does not name the arguments of functions in the examples. For example:

op_fwer_surv(20,0.05,0.1,4,c(1/2,1),1,0.75,12,40,20,1,0,12)  

In addition internally the Size_surv() function often does not use named arguments.

JOSS review: https://github.com/openjournals/joss-reviews/issues/6322

[Tpatni719]

Thank you for the comments! So, following are the mains functions:

And the remaining functions are internal functions. Regarding naming the arguments in the examples, I tried to do that but while developing the package for CRAN, the examples were getting trimmed in the package manual. I have standardized the code display based on your recommendation. Thank you!

[njtierney]

Regarding naming the arguments in the examples, I tried to do that but while developing the package for CRAN, the examples were getting trimmed in the package manual.

I'm not sure that I understand this issue?

Function naming

I still think it is actually really important to have consistent naming of your functions in the package, especially given that SCPRT() appears to be a commonly used function. What does SCPRT stand for? If a user wanted to use your code or develop it and understand it I think it is worthwhile to make the function names have a consistent format. I suggest that you use lower_snake_case() as suggested in the previous comment.

Regarding closing issues

I also think that in the spirit of this review it would be best to wait closing the issue and confirming if the issue has been completed before closing it.

[njtierney]

Arguments to the functions should be lowercase

There is a mix of upper and lowercase letters, which might be hard for the user to remember I suggest making all of these lowercase

[Tpatni123]

I'm not sure that I understand this issue?

So, if I include the names of the arguments then the function goes beyond the margins of the PDF manual.

Function naming and Arguments to the functions should be lowercase

I will do that but they all are internal functions and all the detailed information is provided in the description if someone wants to comprehend and use the code.

Regarding closing issues

My apologies for that. I will wait for the confirmation.

[njtierney]

So, if I include the names of the arguments then the function goes beyond the margins of the PDF manual.

So the solution to this is to use linebreaks in your code - in general we recommend not exceeding an 80 character width, as it can run past the margins on a text editor and mean that users will need to horizontally scroll to read code.

So it would change like so:

#' @examples
#' design_ord(0.05, 0.1, K = 4, c(0.075, 0.182, 0.319, 0.243, 0.015, 0.166), or = 3.06, or0 = 1.32, c(1 / 2, 1))

#' @examples
#' design_ord(alpha = 0.05, 
#'            beta = 0.1, 
#'            K = 4, 
#'            prob = c(0.075, 0.182, 0.319, 0.243, 0.015, 0.166), 
#'            or = 3.06, 
#'            or0 = 1.32, 
#'            frac = c(1 / 2, 1))

# or even

#' @examples
#' design_probs <- c(0.075, 0.182, 0.319, 0.243, 0.015, 0.166)
#' design_frac <- c(1 / 2, 1)
#' design_ord(alpha = 0.05,
#'            beta = 0.1,
#'            K = 4,
#'            prob = design_probs,
#'            or = 3.06,
#'            or0 = 1.32,
#'            frac = design_frac)

In general it is considered good practice to name your arguments. There are two reasons for this:

1. Naming arguments makes it explicit which argument is being used
2. R uses argument names first, and if they aren't specified uses position (the order).

If you don't name the arguments then you are expecting the user (and your future self) to remember what the seven numbers mean in order, and this is a very easy way to end up making potentially critical errors regarding something as important as clinical trial design.

If you have a mix of argument names and change the order around then you can end up with unpredictable output:

# what does 0.1  and 0.05 correspond to?
design_ord(K = 4,
           or = 3.06,
           0.1,
           prob = c(0.075, 0.182, 0.319, 0.243, 0.015, 0.166),
           0.05,
           or0 = 1.32,
           frac = c(1 / 2, 1))

# is it the same as this?
design_probs <- c(0.075, 0.182, 0.319, 0.243, 0.015, 0.166)
design_frac <- c(1 / 2, 1)
design_ord(alpha = 0.05,
           beta = 0.1,
           K = 4,
           prob = design_probs,
           or = 3.06,
           or0 = 1.32,
           frac = design_frac)

Re SCPRT

Thanks for explaining SCPRT - I would suggest making it all lower case if you can? The reason is that if you have consistent style in your package it becomes easier for others to predict and use your code without needing to remember if one particular function has mixed case, or all upper case. It's a small thing but it makes a difference for future users and possible contributors to have consistent styling in your code.

[Tpatni719]

All the functions are now in lowercase and the names of the arguments are added in the examples.

[njtierney]

Brilliant, thank you!