search

PriceLab / STP

Code and commentary from The Self-Taught Programmer, Althoff 2016: python + R
Apache License 2.0
1 stars 0 forks source link

nested functions #24

Open aishahmohamed98 opened 6 years ago

aishahmohamed98 commented 6 years ago

@paul-shannon paul, i've committed and push exploration.R in igraphtoJSON. please take a look and let me know of any comments/suggestions. in the meantime, i will be building more tests for the code!

paul-shannon commented 6 years ago

Very nice work!

I committed a couple of very minor page layout & function close comment changes.

Try “git diff exploration.R” to see what they are.

Thanks - good work.

Question: this is expected, and appropriate:

R> doIT Error: object 'doIT' not found

Do you understand why it is not found?

On Sep 5, 2018, at 3:28 PM, aishahmohamed98 notifications@github.com wrote:

@paul-shannon paul, i've committed and push exploration.R in igraphtoJSON. please take a look and let me know of any comments/suggestions. in the meantime, i will be building more tests for the code!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

aishahmohamed98 commented 6 years ago

@paul-shannon paul, not sure where you get that error code. i've inspected the code and experimented and still do not get that error message.

aishahmohamed98 commented 6 years ago

@paul-shannon i've push exploration.R with a couple more tests to test the nested functions. Currently working on implemented nested functions capability towards makeRandomString() in utils.R. let me know of any comments or advice for improvement!

paul-shannon commented 6 years ago

@aishahmohamed98 This is good.
I do not wish you to make any more changes to this file (exploration.R) but please, in all of your work, look for and use variable and function names which are self explanatory. From these two function names

test_timesTen_lst2()
test_timesTen_lst3()

I cannot guess what they will do. To make this concrete: what is a lst? Your readers - which will include yourself at a later date - deserve better!

aishahmohamed98 commented 6 years ago

@paul-shannon got it! i can see where it can be a little confusing.

i've worked on the utils.R and test_utils.R in order to next the makeRandomString into makeRandomStrings as a nested function with an appropriate test in utils.R to test its functionality. its been working on my end. please look over and let me know of any comments?

paul-shannon commented 6 years ago

in order to next the makeRandomString into makeRandomStrings

I am not sure what this means!

aishahmohamed98 commented 6 years ago

i mean, to make makeRandomString a nested function within makeRandomStrings.. ?

paul-shannon commented 6 years ago

Please proofread!

Also, recall that the standard R approach is that “singular functions” - that is, no explicit mention of plural instances - is the standard exposed function.

For example:

sqrt(4) # 2 sqrt(c(1,4,9,16)) # 1 2 3 4

“sqrt” sounds singular. “sqrts” might sound plural. But R favors the singular name. So your publicly visible function name should read as singular.

What will the name be?

On Sep 7, 2018, at 12:18 PM, aishahmohamed98 notifications@github.com wrote:

i mean, to make makeRandomString a nested function within makeRandomStrings.. ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

aishahmohamed98 commented 6 years ago

i understand.. so makeRandomStrings is not quite the best name.. will work on creating better variable names for the function right now.

paul-shannon commented 6 years ago

I suggest the same (singular) name you started out with: makeRandomString

On Sep 7, 2018, at 12:24 PM, aishahmohamed98 notifications@github.com wrote:

i understand.. so makeRandomStrings is not quite the best name.. will work on creating better variable names for the function right now.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

aishahmohamed98 commented 6 years ago

@paul-shannon paul, i've edited the names of the variables accordingly. After triple checking all of the code, i think it's worth looking over now! Let me know your thoughts

paul-shannon commented 6 years ago

I see two functions at the top of utils.R:

randomString makeRandomString

Please add a blank line in each function after the concluding return statement, before the function’s closing curly bracket (“}”)

On Sep 7, 2018, at 12:24 PM, aishahmohamed98 notifications@github.com wrote:

i understand.. so makeRandomStrings is not quite the best name.. will work on creating better variable names for the function right now.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

aishahmohamed98 commented 6 years ago

got it!

aishahmohamed98 commented 6 years ago

@paul-shannon Paul, i've uploaded utils.R and test_utils.R onto the repo in igraphtoJSON for you to look at! Some improvements I made were cleaning up of the code and removing the function randomString and nesting it, instead, in makeRandomString so it wasn't redundant. Please take a look and let me know your thoughts!