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

Function #14

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Conversation

drmowinckels
Copy link

No description provided.

@github-actions
Copy link

🆗 Pre-flight checks passed 😃

This pull request has been checked and contains no modified workflow files or spoofing.

It should be safe to Approve and Run the workflows that need maintainer approval.

@joelnitta
Copy link
Collaborator

Thanks @drmowinckels !

I just have one concern with the PR: I'm not sure it's a good idea to use the Extract Function RStudio tool. On one hand, I can see how it demonstrates conversion of non-function code to a function. But on the other hand, it adds cognitive load because the learner has to grasp a new tool. If it worked without further modification I would probably say it's worth it. But it requires additional fiddling, hence additional cognitive load (explanation of NSE). So I think it might be better to leave it out and just focus on manual conversion to a function. Extract function actually strikes me as a tool for advanced users who probably can already write their own functions. It is a shortcut that speeds things up a bit, but you still have to know what you are doing to use it.

@drmowinckels
Copy link
Author

That's an excellent point! Its actually really good tool when dealing with stuff that has standard evaluation, but for the example we have, indeed it ends up being more to explain than necessary.

I'll omit that part.

@drmowinckels
Copy link
Author

I added some info about the tool in a callout. I couldnt get it rendering locally (some os lib issues I need to fix). But I thought it was worth having a look at it like that, and then delete if it still looks confusing.

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.

None yet

3 participants