jbrains · August 29, 2015 13:57 · Mar 9, 2014 · Mar 9, 2014 · Mar 9, 2014 · Mar 9, 2014
diff --git a/gistfile1.rb b/gistfile1.rb
@@ -44,6 +44,7 @@ def count_lines_for_gem(gem)
   return line_count_for_this_gem
 end
 
-gems = extract_gems_from_gemfile_lock_file(File.read("Gemfile.lock"))
-total = gems.map { |each| count_lines_for_gem(each) }.reduce(:+)
+total = extract_gems_from_gemfile_lock_file(File.read("Gemfile.lock"))
+        .map { |each| count_lines_for_gem(each) }.reduce(:+)
+
 puts "Total Lines: #{total}"
diff --git a/gistfile1.rb b/gistfile1.rb
@@ -30,7 +30,7 @@ def count_lines_in_file(file)
   # We could turn this comment into code by matching with a regex, which some
   # would find clearer and others less clear. I could do either.
   line_count_text = output.strip.split(' ').first
-  line_count_text.to_i
+  return line_count_text.to_i
 end
 
 # SMELL Depends on globals like 'puts' and `` (execute process).

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -36,10 +36,10 @@ def count_lines_in_file(file)
 # SMELL Depends on globals like 'puts' and `` (execute process).
 def count_lines_for_gem(gem)
   puts "Processing #{gem}"
-  contents = `gem contents #{gem}`.split
+  gem_filenames = `gem contents #{gem}`.split
   # If you prefer to 'inject', then feel free to 'inject'.
   # Either way, the name duplication disappears.
-  line_count_for_this_gem = contents.map { |each| count_lines_in_file(each) }.reduce(:+)
+  line_count_for_this_gem = gem_filenames.map { |each| count_lines_in_file(each) }.reduce(:+)
   puts "  LOC: #{line_count_for_this_gem}"
   return line_count_for_this_gem
 end

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -2,6 +2,7 @@
 # what else to call it. I want to distinguish it from Gemfile.
 def extract_gems_from_gemfile_lock_file(contents)
   gems = []
+  # Extract all the non-empty lines between and excluding 'specs:' and 'PLATFORMS'
   contents.each_line do |line|
     line = line.strip
     # It takes a few seconds to understand this algorithm,

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -1,8 +1,8 @@
 # SMELL I don't like the name gemfile_lock_file, but I don't know
 # what else to call it. I want to distinguish it from Gemfile.
-def extract_gems_from_gemfile_lock_file(gemfile_lock_file)
+def extract_gems_from_gemfile_lock_file(contents)
   gems = []
-  gemfile_lock_file.each_line do |line|
+  contents.each_line do |line|
     line = line.strip
     # It takes a few seconds to understand this algorithm,
     # but I can't yet justify replacing it with a functional

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -1,6 +1,6 @@
 # SMELL I don't like the name gemfile_lock_file, but I don't know
 # what else to call it. I want to distinguish it from Gemfile.
-def extract_gems_from_gemfile(gemfile_lock_file)
+def extract_gems_from_gemfile_lock_file(gemfile_lock_file)
   gems = []
   gemfile_lock_file.each_line do |line|
     line = line.strip
@@ -43,6 +43,6 @@ def count_lines_for_gem(gem)
   return line_count_for_this_gem
 end
 
-gems = extract_gems_from_gemfile(File.read("Gemfile.lock"))
+gems = extract_gems_from_gemfile_lock_file(File.read("Gemfile.lock"))
 total = gems.map { |each| count_lines_for_gem(each) }.reduce(:+)
 puts "Total Lines: #{total}"
diff --git a/gistfile1.rb b/gistfile1.rb
@@ -43,7 +43,6 @@ def count_lines_for_gem(gem)
   return line_count_for_this_gem
 end
 
-gemfile_lock_file = File.read("Gemfile.lock")
-gems = extract_gems_from_gemfile(gemfile_lock_file)
+gems = extract_gems_from_gemfile(File.read("Gemfile.lock"))
 total = gems.map { |each| count_lines_for_gem(each) }.reduce(:+)
 puts "Total Lines: #{total}"
diff --git a/gistfile1.rb b/gistfile1.rb
@@ -1,5 +1,3 @@
-gemfile_lock_file = File.read("Gemfile.lock")
-
 # SMELL I don't like the name gemfile_lock_file, but I don't know
 # what else to call it. I want to distinguish it from Gemfile.
 def extract_gems_from_gemfile(gemfile_lock_file)
@@ -23,8 +21,6 @@ def extract_gems_from_gemfile(gemfile_lock_file)
   return gems
 end
 
-gems = extract_gems_from_gemfile(gemfile_lock_file)
-
 # Guess what? Now independent of "gem" concept. More reusable. MAGIC!
 # Guess what? Almost dead-simple wrapper around command line tool. Cohesive. SRP. Nice.
 def count_lines_in_file(file)
@@ -47,5 +43,7 @@ def count_lines_for_gem(gem)
   return line_count_for_this_gem
 end
 
+gemfile_lock_file = File.read("Gemfile.lock")
+gems = extract_gems_from_gemfile(gemfile_lock_file)
 total = gems.map { |each| count_lines_for_gem(each) }.reduce(:+)
 puts "Total Lines: #{total}"
diff --git a/gistfile1.rb b/gistfile1.rb
@@ -1,10 +1,10 @@
-list = File.read("Gemfile.lock")
+gemfile_lock_file = File.read("Gemfile.lock")
 
-# SMELL Now the name 'list' seems really strange.
-# REFACTOR Rename 'list', since it's not at all a list.
-def extract_gems_from_gemfile(list)
+# SMELL I don't like the name gemfile_lock_file, but I don't know
+# what else to call it. I want to distinguish it from Gemfile.
+def extract_gems_from_gemfile(gemfile_lock_file)
   gems = []
-  list.each_line do |line|
+  gemfile_lock_file.each_line do |line|
     line = line.strip
     # It takes a few seconds to understand this algorithm,
     # but I can't yet justify replacing it with a functional
@@ -23,7 +23,7 @@ def extract_gems_from_gemfile(list)
   return gems
 end
 
-gems = extract_gems_from_gemfile(list)
+gems = extract_gems_from_gemfile(gemfile_lock_file)
 
 # Guess what? Now independent of "gem" concept. More reusable. MAGIC!
 # Guess what? Almost dead-simple wrapper around command line tool. Cohesive. SRP. Nice.

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -1,9 +1,18 @@
 list = File.read("Gemfile.lock")
 
+# SMELL Now the name 'list' seems really strange.
+# REFACTOR Rename 'list', since it's not at all a list.
 def extract_gems_from_gemfile(list)
   gems = []
   list.each_line do |line|
     line = line.strip
+    # It takes a few seconds to understand this algorithm,
+    # but I can't yet justify replacing it with a functional
+    # approach. Again, it depends what the reader understands
+    # better.
+    # If we're going to let iterating and filtering stay
+    # intertwined like this, I'm glad we've separated it from
+    # the rest of the code.
     break if line.include?("PLATFORMS")
     next if line.include?("GEM")
     next if line.include?("remote:")

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -1,17 +1,21 @@
 list = File.read("Gemfile.lock")
 
-gems = []
-
-list.each_line do |line|
-  line = line.strip
-  break if line.include?("PLATFORMS")
-  next if line.include?("GEM")
-  next if line.include?("remote:")
-  next if line.include?("specs:")
-  next if line.empty?
-  gems.push(line.split(' ').first)
+def extract_gems_from_gemfile(list)
+  gems = []
+  list.each_line do |line|
+    line = line.strip
+    break if line.include?("PLATFORMS")
+    next if line.include?("GEM")
+    next if line.include?("remote:")
+    next if line.include?("specs:")
+    next if line.empty?
+    gems.push(line.split(' ').first)
+  end
+  return gems
 end
 
+gems = extract_gems_from_gemfile(list)
+
 # Guess what? Now independent of "gem" concept. More reusable. MAGIC!
 # Guess what? Almost dead-simple wrapper around command line tool. Cohesive. SRP. Nice.
 def count_lines_in_file(file)

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -1,8 +1,4 @@
-# REFACTOR Inline temporary variable that's not pulling its weight
-# and whose name I don't particularly like.
-GFILE = "Gemfile.lock"
-
-list = File.read(GFILE)
+list = File.read("Gemfile.lock")
 
 gems = []
 

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -1,3 +1,5 @@
+# REFACTOR Inline temporary variable that's not pulling its weight
+# and whose name I don't particularly like.
 GFILE = "Gemfile.lock"
 
 list = File.read(GFILE)

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -36,9 +36,5 @@ def count_lines_for_gem(gem)
   return line_count_for_this_gem
 end
 
-total = 0
-gems.each do |gem|
-  total += count_lines_for_gem(gem)
-end
-
+total = gems.map { |each| count_lines_for_gem(each) }.reduce(:+)
 puts "Total Lines: #{total}"
diff --git a/gistfile1.rb b/gistfile1.rb
@@ -17,12 +17,12 @@
 # Guess what? Now independent of "gem" concept. More reusable. MAGIC!
 # Guess what? Almost dead-simple wrapper around command line tool. Cohesive. SRP. Nice.
 def count_lines_in_file(file)
-  # SMELL 'output' and 'amount' are both purely structural names (don't reveal intent)
-  # SMELL 'output' and 'amount' names don't reflect their relationship to each other
-  # (both related to the same gem file)
   output = `wc -l #{file}`
-  amount = output.strip.split(' ').first.to_i
-  amount
+  # ASSERT: output is of the form <whitespace><line count><whitespace><filename>
+  # We could turn this comment into code by matching with a regex, which some
+  # would find clearer and others less clear. I could do either.
+  line_count_text = output.strip.split(' ').first
+  line_count_text.to_i
 end
 
 # SMELL Depends on globals like 'puts' and `` (execute process).

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -29,12 +29,9 @@ def count_lines_in_file(file)
 def count_lines_for_gem(gem)
   puts "Processing #{gem}"
   contents = `gem contents #{gem}`.split
-  # REFACTOR Remove duplication between this name and the function's name.
-  # REFACTOR Let's reduce, shall we?
-  line_count_for_this_gem = 0
-  contents.each do |file|
-    line_count_for_this_gem += count_lines_in_file(file)
-  end
+  # If you prefer to 'inject', then feel free to 'inject'.
+  # Either way, the name duplication disappears.
+  line_count_for_this_gem = contents.map { |each| count_lines_in_file(each) }.reduce(:+)
   puts "  LOC: #{line_count_for_this_gem}"
   return line_count_for_this_gem
 end

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -30,6 +30,7 @@ def count_lines_for_gem(gem)
   puts "Processing #{gem}"
   contents = `gem contents #{gem}`.split
   # REFACTOR Remove duplication between this name and the function's name.
+  # REFACTOR Let's reduce, shall we?
   line_count_for_this_gem = 0
   contents.each do |file|
     line_count_for_this_gem += count_lines_in_file(file)

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -14,19 +14,25 @@
   gems.push(line.split(' ').first)
 end
 
+# Guess what? Now independent of "gem" concept. More reusable. MAGIC!
+# Guess what? Almost dead-simple wrapper around command line tool. Cohesive. SRP. Nice.
+def count_lines_in_file(file)
+  # SMELL 'output' and 'amount' are both purely structural names (don't reveal intent)
+  # SMELL 'output' and 'amount' names don't reflect their relationship to each other
+  # (both related to the same gem file)
+  output = `wc -l #{file}`
+  amount = output.strip.split(' ').first.to_i
+  amount
+end
+
 # SMELL Depends on globals like 'puts' and `` (execute process).
 def count_lines_for_gem(gem)
   puts "Processing #{gem}"
   contents = `gem contents #{gem}`.split
   # REFACTOR Remove duplication between this name and the function's name.
   line_count_for_this_gem = 0
   contents.each do |file|
-    # SMELL 'output' and 'amount' are both purely structural names (don't reveal intent)
-    # SMELL 'output' and 'amount' names don't reflect their relationship to each other
-    # (both related to the same gem file)
-    output = `wc -l #{file}`
-    amount = output.strip.split(' ').first.to_i
-    line_count_for_this_gem += amount
+    line_count_for_this_gem += count_lines_in_file(file)
   end
   puts "  LOC: #{line_count_for_this_gem}"
   return line_count_for_this_gem

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -21,6 +21,9 @@ def count_lines_for_gem(gem)
   # REFACTOR Remove duplication between this name and the function's name.
   line_count_for_this_gem = 0
   contents.each do |file|
+    # SMELL 'output' and 'amount' are both purely structural names (don't reveal intent)
+    # SMELL 'output' and 'amount' names don't reflect their relationship to each other
+    # (both related to the same gem file)
     output = `wc -l #{file}`
     amount = output.strip.split(' ').first.to_i
     line_count_for_this_gem += amount

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -18,6 +18,7 @@
 def count_lines_for_gem(gem)
   puts "Processing #{gem}"
   contents = `gem contents #{gem}`.split
+  # REFACTOR Remove duplication between this name and the function's name.
   line_count_for_this_gem = 0
   contents.each do |file|
     output = `wc -l #{file}`

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -30,17 +30,7 @@ def count_lines_for_gem(gem)
 
 total = 0
 gems.each do |gem|
-  # REFACTOR Extract this body as a function.
-  puts "Processing #{gem}"
-  contents = `gem contents #{gem}`.split
-  line_count_for_this_gem = 0
-  contents.each do |file|
-    output = `wc -l #{file}`
-    amount = output.strip.split(' ').first.to_i
-    line_count_for_this_gem += amount
-  end
-  puts "  LOC: #{line_count_for_this_gem}"
-  total += line_count_for_this_gem
+  total += count_lines_for_gem(gem)
 end
 
 puts "Total Lines: #{total}"
diff --git a/gistfile1.rb b/gistfile1.rb
@@ -14,6 +14,20 @@
   gems.push(line.split(' ').first)
 end
 
+# SMELL Depends on globals like 'puts' and `` (execute process).
+def count_lines_for_gem(gem)
+  puts "Processing #{gem}"
+  contents = `gem contents #{gem}`.split
+  line_count_for_this_gem = 0
+  contents.each do |file|
+    output = `wc -l #{file}`
+    amount = output.strip.split(' ').first.to_i
+    line_count_for_this_gem += amount
+  end
+  puts "  LOC: #{line_count_for_this_gem}"
+  return line_count_for_this_gem
+end
+
 total = 0
 gems.each do |gem|
   # REFACTOR Extract this body as a function.

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -16,6 +16,7 @@
 
 total = 0
 gems.each do |gem|
+  # REFACTOR Extract this body as a function.
   puts "Processing #{gem}"
   contents = `gem contents #{gem}`.split
   line_count_for_this_gem = 0

diff --git a/gistfile1.rb b/gistfile1.rb
@@ -18,15 +18,14 @@
 gems.each do |gem|
   puts "Processing #{gem}"
   contents = `gem contents #{gem}`.split
-  # SMELL 'local' is a purely structural name; doesn't reveal any intent.
-  local = 0
+  line_count_for_this_gem = 0
   contents.each do |file|
     output = `wc -l #{file}`
     amount = output.strip.split(' ').first.to_i
-    local += amount
+    line_count_for_this_gem += amount
   end
-  puts "  LOC: #{local}"
-  total += local
+  puts "  LOC: #{line_count_for_this_gem}"
+  total += line_count_for_this_gem
 end
 
 puts "Total Lines: #{total}"
diff --git a/gistfile1.rb b/gistfile1.rb
@@ -18,6 +18,7 @@
 gems.each do |gem|
   puts "Processing #{gem}"
   contents = `gem contents #{gem}`.split
+  # SMELL 'local' is a purely structural name; doesn't reveal any intent.
   local = 0
   contents.each do |file|
     output = `wc -l #{file}`
@@ -28,4 +29,4 @@
   total += local
 end
 
-puts "Total Lines: #{total}"
+puts "Total Lines: #{total}"
diff --git a/gistfile1.rb b/gistfile1.rb
@@ -0,0 +1,31 @@
+GFILE = "Gemfile.lock"
+
+list = File.read(GFILE)
+
+gems = []
+
+list.each_line do |line|
+  line = line.strip
+  break if line.include?("PLATFORMS")
+  next if line.include?("GEM")
+  next if line.include?("remote:")
+  next if line.include?("specs:")
+  next if line.empty?
+  gems.push(line.split(' ').first)
+end
+
+total = 0
+gems.each do |gem|
+  puts "Processing #{gem}"
+  contents = `gem contents #{gem}`.split
+  local = 0
+  contents.each do |file|
+    output = `wc -l #{file}`
+    amount = output.strip.split(' ').first.to_i
+    local += amount
+  end
+  puts "  LOC: #{local}"
+  total += local
+end
+
+puts "Total Lines: #{total}"