PriceLab / STP

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

attributes #23

Open aishahmohamed98 opened 6 years ago

aishahmohamed98 commented 6 years ago

@paul-shannon paul, i've pushed test_utils.R and the file that it sources, utils_COPY.R into the igraph repo. All tests work on my end! Let me know of any comments and suggestions

paul-shannon commented 6 years ago

Hi Aishah,

I cannot tell where the source code is! utils_COPY.R was just a temporary measure, for your eyes only. utils.R and test_utils.R are the only things which should be in the repo.

Let me know when you have rectified this!

On Aug 29, 2018, at 2:28 PM, aishahmohamed98 notifications@github.com wrote:

@paul-shannon paul, i've pushed test_utils.R and the file that it sources, utils_COPY.R into the igraph repo. All tests work on my end! Let me know of any comments and suggestions

— 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, you're right-- sorry about that! i've fixed it!

paul-shannon commented 6 years ago

@aishahmohamed98 good work.

Improve this code:

  if (length(nodeAttributes)>0){
        age <- c("20")
        age <- as.numeric(age)
        V(g)$age <- age
    }
  1. I asked for random numbers to be assigned to each attribute whose type is "numeric"
  2. You have hard-coded the character constant "20"
  3. You do not yet check to see what the type (numeric, character, logical) of node attribute has been requested.

These are GOOD problems! You are moving along!

aishahmohamed98 commented 6 years ago

@paul-shannon paul, i've fixed the code and re-wrote it based on your comments. Take a look and let me know if its satisfactory + any new comments! And thank you!

aishahmohamed98 commented 6 years ago

@paul-shannon paul, i've push my edits to utils.R concerning the nodesAttribute part of the code. let me know of any comments and things i can work more on. I wasn't sure if this is exactly what you had envisioned as far as code, but its what made the most sense to me and the tests run smoothly! let me know what you think.

paul-shannon commented 6 years ago

@aishahmohamed98 all very good! - except that you hard-coded the number of age values created via runif().

aishahmohamed98 commented 6 years ago

@paul-shannon i've push the two code, utils.R and test_utils.R in igraphtoJSON with added unit test for random strings as node attributes as well as a new function in utils.R that creates a random string given a random number. let me know what you think + any tips for improvement.

-Aishah

paul-shannon commented 6 years ago

@aishahmohamed98 - I don't see a test for makeRandomString. Also: spacing and indentation are not fully consistent There is a lingering .#utils.R in the repo

aishahmohamed98 commented 6 years ago

@paul-shannon I see! i've pushed the edits onto the repo including a test that checks functionality of makeRandomString function as well as putting it to use for node attribute type "character".

paul-shannon commented 6 years ago

@aishahmohamed98 I made some changes to your test function for makeRandomStrings:

  1. test function is now the same as the function being tested, prefixed by test_
  2. ! good clear variable and function names are important!

In your makeRandomStrings function, your two variable names sound (to me) like verbs, not nouns.
variable names should be nouns, function names should be verbs. clean this up, okay?