lpetito / groupedCS

Files needed to build R package groupedCS. Code written to facilitate simulations in paper "Misclassified Group Tested Current Status Data"
github.com/lpetito/groupedCS
1 stars 1 forks source link

function names use .'s but aren't S3/S4 methods? #2

Open stevenpollack opened 8 years ago

stevenpollack commented 8 years ago

Like with https://github.com/lpetito/groupedCS/blob/master/R/grouped_csdata_weibull.R#L4... Why are functions and columns named with .s?

Best practice suggest either using camel-case, or _'s. E.g.

genDataWeibullUnif # my preference: lower camel-case
gen_data_weibull_unif # more pythonic

A convention I typically employ, when using data.frame's (or data.tables) is to name the columns with _ separators so that I cannot confuse them with other variables in the scope. E.g.,

someVariable <- 5
demoDF <- data.frame(some_variable = rnorm(5))

Thus, when you're searching up the lexical scope for "some variable", you're forced to differentiate between someVariable and some_variable...

I can help you refactor this, if necessary.

stevenpollack commented 8 years ago

Also, when it comes to production code, I avoid $ at all costs. That is, demoDF$some_variable demoDF[["some_variable"]]...

I understand that this creates 6 more characters 😱 but you're writing code for other people:

  1. $'s lookup times are never faster than [[,
  2. breaks code completion in things like Rstudio which forces you to pay more attention to what you're doing
lpetito commented 8 years ago

Interesting. I honestly wasn't even thinking about it - I wrote it for myself, the only reason it's online is because I was anticipating the journal asking for it. I'll go in and fix it. Any more glaring novice coding errors?

Sent from my iPhone, please excuse any typos!

On Jan 4, 2016, at 3:59 AM, stevenpollack notifications@github.com wrote:

Also, when it comes to production code, I avoid $ at all costs. That is, demoDF$some_variable demoDF[["some_variable"]]...

I understand that this creates 6 more characters 😱 but you're writing code for other people:

$'s lookup times are never faster than [[, breaks code completion in things like Rstudio which forces you to pay more attention to what you're doing — Reply to this email directly or view it on GitHub.

stevenpollack commented 8 years ago

I wouldn't be able to tell you without really giving your code a thorough review. There are classic things, I see, though, like lines 22-25:

  Delta.groups <- sapply(1:ceiling(n/k), FUN = function(X){
    temp <- delta.ind[groups==X]
    ifelse(sum(temp) != 0, 1, 0)
  })

sequences like c(1,2,3,...,n) can be generated with seq.int. So, the first line would be replaced with

Delta.groups <-
  sapply(seq.int(ceiling(n/k)),
         function(X) {

I don't know if there are any suggestions for the use of ifelse over traditional blocks. My general advice is to write your code as if it was something a human would want to try and read. So, if you think ifelse improves readability, keep it. The use of variable names like temp and X are pretty big red-flags that you're not writing this for anyone else, though. They'd need to be renamed.

Trust, naming your variables properly -- though the second hardest problem in computer science -- is an investment your future self will constantly thank her past self for.

lpetito commented 8 years ago

Thanks, don't bother doing a thorough review until I fix up those things, which should be in the next few days. We just got off the plane in SF like 3 hours ago so my body clock is out of whack.

On Thu, Jan 7, 2016 at 2:10 AM, stevenpollack notifications@github.com wrote:

I wouldn't be able to tell you without really giving your code a thorough review. There are classic things, I see, though, like lines 22-25 https://github.com/lpetito/groupedCS/blob/master/R/grouped_csdata_weibull.R#L22-L25 :

Delta.groups <- sapply(1:ceiling(n/k), FUN = function(X){ temp <- delta.ind[groups==X] ifelse(sum(temp) != 0, 1, 0) })

sequences like c(1,2,3,...,n) can be generated with seq.int. So, the first line would be replaced with

Delta.groups <- sapply(seq.int(ceiling(n/k)), function(X) {

I don't know if there are any suggestions for the use of ifelse over traditional blocks. My general advice is to write your code as if it was something a human would want to try and read. So, if you think ifelse improves readability, keep it. The use of variable names like temp and X are pretty big red-flags that you're not writing this for anyone else, though. They'd need to be renamed.

Trust, naming your variables properly -- though the second hardest problem in computer science -- is an investment your future self will constantly thank her past self for.

— Reply to this email directly or view it on GitHub https://github.com/lpetito/groupedCS/issues/2#issuecomment-169617037.

Cheers, Lucia