example of feedback

gistfile1.txt

68  week-03/to_roman.rb  Show notes View
@@ -1,20 +1,62 @@
-# Method name: to_roman
-# Inputs:      A number
-# Returns:     A String representing the number in roman numerals
-# Prints:      Nothing
+def to_roman(num)
 
-# to_roman takes a number as input and returns that number using Roman numerals
-# See http://en.wikipedia.org/wiki/Roman_numerals
+#  I == 1   X == 10    L == 50   C == 100   D == 500  M == 1000
 
-# It can be tricky to handle the cases where "IV" means 4.  As a first pass,
-# don't hesitate to implement a simpler version where 4 is denoted by "IIII".
-#
-# Remember: the priority is FEEDBACK, even if your implementation can't handle
-# every single case.
+  thousands = num / 1000
+  num = num - thousands * 1000
John Davison 
jcdavison added a note 5 days ago
I'm finding following the logic of your variable renaming difficult to follow. Consider num starts as an argument and is basically and integer but then you are changing the value of num which is called mutating and generally not a great idea. Also consider, each step of the way, you are reassigning num but if num starts as the number we want to convert to roman numeral, but then has many different values associated with it throughout the lifecycle of this method, how can someone else understand what num actually represents? The only way for me to know what num is on say line 15 is to trace all the way back up to line 6 and follow the logic, that is tough to do and it makes attempting to understand this code especially hard.
Jesse Farmer 
jfarmer added a note 5 days ago
Although @Katzy is overwriting the value of the num variable, nothing is being mutated in this code.
Jesse Farmer 
jfarmer added a note 5 days ago
"Mutation" is when a given object's identity remains the same but its value changes. In Ruby, you can call Object#object_id to see what a given object's internal ID is. This is the only Source of Truth™ for whether we're talking about the same object or different objects. For example,

array = [1,2,3]
# Before:
#   array.object_id == 9876
#   array == [1,2,3]

array.push(10)
# After:
#   array.object_id == 9876 (same object_id)
#   array == [1,2,3,10]
array references the same object throughout the code, but array.push(10) changes its value. Conversely, this does not mutate anything:

array = [1,2,3]
# Before:
#   array.object_id == 9876
#   array == [1,2,3]

array = array + [10]
# After:
#   array.object_id == 4632 (different object_id)
#   array == [1,2,3,10]
Jesse Farmer 
jfarmer added a note 5 days ago
Methods that mutate their arguments are called "destructive" methods. The reason destructive methods are frowned upon is because it makes your code less predictable. If you assign

saved_array = some_array
and another part of your code mutates some_array then saved_array will now point to the mutated object. This makes certain things very difficult, e.g., caching. How can you cache a value if you can't guarantee that other parts of the code can't change the value out from under you? You either have to make the rest of your code "cache aware" or hide your values behind an interface you design.

The latter — information hiding — is ultimately a fool's errand in Ruby thanks to methods like Object#send, Object#instance_variable_get, and Object#instance_variable_set. We just all agree politely not to use such terrible things in most cases.
Add a line note
+  five_hundreds = num / 500
+  num = num - five_hundreds * 500
+  hundreds = num / 100
+  num = num - hundreds * 100
+  fifties = num / 50
+  num = num - fifties * 50
+  tens = num / 10
+  num = num - tens * 10
+  fives = num / 5
+  ones = num - fives * 5
Jesse Farmer 
jfarmer added a note 5 days ago
These lines scream for a loop of some kind. Just look at the repetition!
Add a line note
+
+  thousands = ("M" * thousands).to_s
+
+  if hundreds == 4 && five_hundreds == 0
+    hundreds = "CD".to_s
+  elsif hundreds == 4 && five_hundreds == 1
+    hundreds = "CM".to_s
+    five_hundreds = 0
+  else hundreds = ("C" * hundreds).to_s
+  end
+
+  five_hundreds = ("D" * five_hundreds).to_s
+
+  if tens == 4 && fifties == 0
+    tens = "XL".to_s
+  elsif tens == 4 && fifties == 1
+    tens = "XC".to_s
+    fifties = 0
+  else
+    tens = ("X" * tens).to_s
+  end
+
+  fifties = ("L" * fifties).to_s
+
+  if ones == 4 && fives == 0
+    ones = "IV".to_s
+  elsif ones == 4 && fives == 1
+    ones = "IX".to_s
+    fives = 0
+  else
+    ones = ("I" * ones).to_s
+  end
+
+  fives = ("V" * fives).to_s
+
+  roman = thousands + five_hundreds + hundreds + fifties + tens + fives + ones
 
-def to_roman(num)
 end
 
 if __FILE__ == $0
-  # What are some test cases?
+
+  p to_roman(150) == "CL"
+  p to_roman(3999) == "MMMCMXCIX"
+  p to_roman(994) == "CMXCIV"
+  p to_roman(4989) == "MMMMCMLXXXIX"     #  This does not account for the actual roman numeral 5000
 end
4 comments on commit bc1e39c    Show 5 line notes below
Scott Katz
 
Katzy commented on bc1e39c 5 days ago
right.. i should have renamed it something like num_minus_thousands or something to that degree.
Jesse Farmer
 
jfarmer commented on bc1e39c 5 days ago
@Katzy No, I disagree. I think the way you've written it now is inches away from something more concise. The "repetition" in your code is only really apparent when you use the same variable name. Each line of your code looks like...

place_value = num / place
num = num - place_value * place
where place is successively 1000, 500, 100, 50, ..., 1.

You're only using variable names like hundreds because you feel the need to keep track of each place value in its own variable. Consider using a collection to do this — there are ways to make this work with both Arrays and Hashes.
Jesse Farmer
 
jfarmer commented on bc1e39c 5 days ago
Three things to ponder:

A series of very similar lines of code (repetition) screams for a loop. A loop requires something to "loop over" — what makes sense in this case?
You're comfortable treating 500 as its own place value, even though there's no 500's place in our usual decimal system. What would happen if you also treated, say, 400 and 4 as their own distinct place values?
Try separating the display logic from the rest of the code. Imagine, for example, if you had a method that took as its input a Hash that looked liked

{"M" => 3, "C" => 1, "V" => 1}
and returned the String

"MMMCV"
Scott Katz
 
Katzy commented on bc1e39c 5 days ago
i knew a loop was in order here. I wasn't sure how to go about it. This makes a lot of sense and I'll refactor this code to reflect these ideas.