Skip to content

Instantly share code, notes, and snippets.

@philsof
philsof / code-review-pair-scottychou-blog-1-anonymous-blog-challenge.md
Last active January 26, 2016 15:46
Code review pair-scottychou blog-1-anonymous-blog-challenge

Good work! Everything is solid except for what is mentioned below. So if it's not below, you are good. Here are my notes on your code:

##Views##

  • When I go to the edit form at http://localhost:9393/entries/:id/edit the body of the existing entry does not show in the corresponding input field. It looks like this is because you are utilizing a textarea tag in your entries/edit view. Heads up: It doesn't look like the textarea tag supports the value attribute, according to the docs. That would explain why the body doesn't show on the edit form. You will need to fix this, since the user will think the entry doesn't have a body at all (which is not true).
  • Watch out: Your entries/new view cannot handle a request from a user w
@philsof
philsof / code-review-pair-kb-diangelo,kerryimai-javascript-drills.md
Last active January 26, 2016 06:17
Code review pair-kb-diangelo,kerryimai-javascript-drills.md
@philsof
philsof / code-review-pair-edwardgemson,mirascarvalone-active-record-associations-drill-shirts-challenge.md
Last active January 26, 2016 06:17
Code review: pair-edwardgemson,mirascarvalone branch for active-record-associations-drill-shirts-challenge

Good effort on this, you are thisclose! Here are my notes on your code:

  • When I go into the console and enter User.new for the first time, a new User is instantiated, but I also get this warning message: /Users/Phil/.../active-record-associations-drill-shirts-challenge/app/models/user.rb:10: warning: duplicated key at line 10 ignored: :through

    /Users/Phil/.../active-record-associations-drill-shirts-challenge/app/models/user.rb:12: warning: duplicated key at line 12 ignored: :through

This is because you are chaining through: statements on lines 10 and 12 of your User model. T

Nice work on this one. Here are my notes on your code:

  • Your factorial_iterative method does the job concisely, but not as concisely as possible. Specifically, your if condition only handles zero, but not 1. Since you already have if n == 0 you might as well catch 1 with this line also, so a good refactor would be if n <= 1. That way, if n is 1 you don't end up creating an array of [1] and running reduce on it.
  • Your reduce is super sharp. Sweet!
  • Your factorial_recursive method is short and sweet, and it takes care of 0 and 1

Good stuff. Here are my notes on your code:

  • Your anagrams? method is sweet. You got this one 100%.
  • You chose to use map to iterate through each word in list_arr. This works, but it is a misuse of map. Why? Because map has a specific function: to iterate through an existing array, perform the same action on each object in the existing array, and return a new array that contains the resulting objects. Since you do not need to do this in your find_anagrams method, map is not a good choice.
  • BUT, your logic is sound. You are iterating through the list_arr, checking each objec
@philsof
philsof / code-review-pair-plonsker-active-record-intro-belongs-to-association-challenge.md
Last active January 26, 2016 06:17
Code review for branch pair-plonsker on active-record-intro-belongs-to-association-challenge
@philsof
philsof / code-review-pair-jdun10,plonsker-pig-latin-challenge.md
Last active January 26, 2016 06:17
Code review pair-jdun10,plonsker branch for pig-latin-challenge

Good work on this. Here are my notes on your code:

  • Nice use of rotate! This method takes care of two needs simultaneously, which is cool.
  • A quick note: There is no need for the 1 argument to be passed to rotate!, since it is passed by default. (See the docs on rotate! here). You can safely remove that argument.
  • Also heads up: if you were to run convert_word_to_pig_latin on a word that does not contain a vowel (like "why") your code would enter an infinite loop. (Your until loop would keep running forever.) How could you refactor your code to fix this bug?
  • Watch your indentation. Especially here, because it becomes difficult to read your code.

Good

@philsof
philsof / code-review-branch-pair-superboyblue-for-algorithm-drill-factorial-challenge.md
Last active January 26, 2016 06:17
Code review: branch pair-superboyblue for algorithm-drill-factorial-challenge

Your code gets the job done and passes all the tests. Nice! Some notes:

  • Your code is concise and clear. And it works. Good stuff.
  • Heads up: This line of code is being made more complicated by this line of code. Specifically, when you create your array on line 3, you are using ... (three dot operators), which tells Ruby to create an array that does not include the starting and ending values (thus, 1 and n are not included in the array). Because of this, you have to add n to your reduce arguments, since n is not in your array.

You can simplify your code in two ways. First, by changing your array declaration to:

array = (1..n) # notice the two dot operators, not th
@philsof
philsof / code-review-branch-andy4626_and_greensneakers-for-algorithm-drill-shuffle-challenge.md
Last active January 26, 2016 06:17
code-review-branch-andy4626_and_greensneakers-for-algorithm-drill-shuffle-challenge

This is good stuff. Your code does the job and passes all of the tests. Here are some notes:

  • One of your variable names is too vague. Instead of new_array how about dup_of_array or something that indicates what the array is holding/why you need it.
  • You are creating a duplicate array in a very unconventional manner. Check out clone and dup for a better way to do this. (Hint: one of these will be better than the other.)
  • Good use of until and rand and &lt;&lt; and length and especially slice!, which is taking care of two needs simultaneously (removing and
@philsof
philsof / code-review-roman-numerals-challenge-branch-kbjk.md
Last active January 26, 2016 06:17
code review for kbjk branch on roman-numerals-challenge

Your code works, and is easy to follow. Nice! Here are some notes from me:

  • Be sure you use names that clearly convey what your objects are/are doing. Specifically: On line 2 of your code, you declare a variable named new_array. That name is way too vague. From its name, all I know is that it is an array. How about something like roman_num_array? It needs to be something that tells me what the variable represents.
  • Your approach to this challenge is sound. You realized the need to start with the largest Roman numeral amount (1000) and work your way down, and decrease your Arabic number along the way. But there are better ways that eliminates the need to use any while loops (or any loops at all). Hint: check out these methods: div and [divmod](http://ruby-doc.org/cor