Skip to content

Conversation

@yihui
Copy link
Collaborator

@yihui yihui commented Aug 8, 2024

Removed the large amount of code repetition and the unnecessary for-loops. Please see individual commit messages if you find it difficult to understand the full changes.

I did this only because "while I'm looking at the code here..." :)

If my performance is evaluated based on the number of lines of code deleted rather than added, I can probably do better :D I compared the ahr and wlr cases carefully by eyes, and it will be helpful if someone else could give it another pass to make sure I didn't miss other differences in the code for the two cases. First let's see if the PR would pass R CMD check.

Copy link
Collaborator

@LittleBeannie LittleBeannie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much, @yihui! I gained valuable insights from https://github.com/yihui/gsDesign2/blob/to_integer_tweaks/R/to_integer.R#L349 in order to avoid code duplication.

@LittleBeannie LittleBeannie merged commit e0c0b04 into Merck:main Aug 9, 2024
@yihui yihui deleted the to_integer_tweaks branch August 9, 2024 17:29
@yihui
Copy link
Collaborator Author

yihui commented Aug 9, 2024

@LittleBeannie You are welcome! Just FYI, do.call() can be handy sometimes, but it does have one drawback, i.e., when errors occur, the error message and traceback won't be as clear as when you call the function directly (there's no function name in the traceback).

f = function() stop('No')

f()
#> Error in f() : No

do.call(f, list())
#> Error in (function ()  : No

So normally I'd avoid using do.call() unless I need to call functions dynamically (e.g., conditionally) or call a function with dynamic arguments.

@LittleBeannie
Copy link
Collaborator

@LittleBeannie You are welcome! Just FYI, do.call() can be handy sometimes, but it does have one drawback, i.e., when errors occur, the error message and traceback won't be as clear as when you call the function directly (there's no function name in the traceback).

f = function() stop('No')

f()
#> Error in f() : No

do.call(f, list())
#> Error in (function ()  : No

So normally I'd avoid using do.call() unless I need to call functions dynamically (e.g., conditionally) or call a function with dynamic arguments.

Gained another valuable lesson, and thank you for sharing your experience with do.call().

@yihui yihui mentioned this pull request Aug 12, 2024
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

Successfully merging this pull request may close these issues.

3 participants