Refactor LineHighlighter

app/assets/javascripts/line_highlighter.js.coffee

@@ -26,9 +26,13 @@
 #       </pre>
 #     </div>
 #   </div>
+#
 class @LineHighlighter
+  # CSS class applied to highlighted lines
+  highlightClass: 'hll'
+
   # Internal copy of location.hash so we're not dependent on `location` in tests
-  @_hash = ''
+  _hash: ''
 
   # Initialize a LineHighlighter object
   #
@@ -41,7 +45,7 @@ class @LineHighlighter
     unless hash == ''
       range = @hashToRange(hash)
 
-      unless isNaN(range[0])
+      if range[0]
         @highlightRange(range)
 
         # Scroll to the first highlighted line on initial load
@@ -69,7 +73,7 @@ class @LineHighlighter
     lineNumber = $(event.target).data('line-number')
     current = @hashToRange(@_hash)
 
-    if isNaN(current[0]) or !event.shiftKey
+    unless current[0] and event.shiftKey
       # If there's no current selection, or there is but Shift wasn't held,
       # treat this like a single-line selection.
       @setHash(lineNumber)
@@ -85,7 +89,7 @@ class @LineHighlighter
 
   # Unhighlight previously highlighted lines
   clearHighlight: ->
-    $('.hll').removeClass('hll')
+    $(".#{@highlightClass}").removeClass(@highlightClass)
 
   # Convert a URL hash String into line numbers
   #
@@ -93,60 +97,44 @@ class @LineHighlighter
   #
   # Examples:
   #
-  #   hashToRange('#L5')    # => [5, NaN]
+  #   hashToRange('#L5')    # => [5, null]
   #   hashToRange('#L5-15') # => [5, 15]
-  #   hashToRange('#foo')   # => [NaN, NaN]
+  #   hashToRange('#foo')   # => [null, null]
   #
   # Returns an Array
   hashToRange: (hash) ->
-    first = parseInt(hash.replace(/^#L(\d+)/, '$1'))
-    last  = parseInt(hash.replace(/^#L\d+-(\d+)/, '$1'))
+    matches = hash.match(/^#?L(\d+)(?:-(\d+))?$/)
+
+    if matches and matches.length
+      first = parseInt(matches[1])
+      last  = matches[2] and parseInt(matches[2]) or null
 
       [first, last]
+    else
+      [null, null]
 
   # Highlight a single line
   #
-  # lineNumber - Number to highlight. Must be parsable as an Integer.
-  #
-  # Returns undefined if lineNumber is not parsable as an Integer.
-  highlightLine: (lineNumber) ->
-    return if isNaN(parseInt(lineNumber))
-
-    $("#LC#{lineNumber}").addClass('hll')
+  # lineNumber - Line number to highlight
+  highlightLine: (lineNumber) =>
+    $("#LC#{lineNumber}").addClass(@highlightClass)
 
   # Highlight all lines within a range
   #
-  # range - An Array of starting and ending line numbers.
-  #
-  # Examples:
-  #
-  #   # Highlight lines 5 through 15
-  #   highlightRange([5, 15])
-  #
-  #   # The first value is required, and must be a number
-  #   highlightRange(['foo', 15]) # Invalid, returns undefined
-  #   highlightRange([NaN, NaN])  # Invalid, returns undefined
-  #
-  #   # The second value is optional; if omitted, only highlights the first line
-  #   highlightRange([5, NaN]) # Valid
-  #
-  # Returns undefined if the first line is NaN.
+  # range - Array containing the starting and ending line numbers
   highlightRange: (range) ->
-    return if isNaN(range[0])
-
-    if isNaN(range[1])
-      @highlightLine(range[0])
-    else
+    if range[1]
       for lineNumber in [range[0]..range[1]]
         @highlightLine(lineNumber)
+    else
+      @highlightLine(range[0])
 
+  # Set the URL hash string
   setHash: (firstLineNumber, lastLineNumber) =>
-    return if isNaN(parseInt(firstLineNumber))
-
-    if isNaN(parseInt(lastLineNumber))
-      hash = "#L#{firstLineNumber}"
-    else
+    if lastLineNumber
       hash = "#L#{firstLineNumber}-#{lastLineNumber}"
+    else
+      hash = "#L#{firstLineNumber}"
 
     @_hash = hash
     @__setLocationHash__(hash)

spec/javascripts/line_highlighter_spec.js.coffee

@@ -13,6 +13,7 @@ describe 'LineHighlighter', ->
   beforeEach ->
     fixture.load('line_highlighter.html')
     @class = new LineHighlighter()
+    @css   = @class.highlightClass
     @spies = {
       __setLocationHash__: spyOn(@class, '__setLocationHash__').and.callFake ->
     }
@@ -20,12 +21,12 @@ describe 'LineHighlighter', ->
   describe 'behavior', ->
     it 'highlights one line given in the URL hash', ->
       new LineHighlighter('#L13')
-      expect($('#LC13')).toHaveClass('hll')
+      expect($('#LC13')).toHaveClass(@css)
 
     it 'highlights a range of lines given in the URL hash', ->
       new LineHighlighter('#L5-25')
-      expect($('.hll').length).toBe(21)
-      expect($("#LC#{line}")).toHaveClass('hll') for line in [5..25]
+      expect($(".#{@css}").length).toBe(21)
+      expect($("#LC#{line}")).toHaveClass(@css) for line in [5..25]
 
     it 'scrolls to the first highlighted line on initial load', ->
       spy = spyOn($, 'scrollTo')
@@ -50,14 +51,14 @@ describe 'LineHighlighter', ->
     describe 'without shiftKey', ->
       it 'highlights one line when clicked', ->
         clickLine(13)
-        expect($('#LC13')).toHaveClass('hll')
+        expect($('#LC13')).toHaveClass(@css)
 
       it 'unhighlights previously highlighted lines', ->
         clickLine(13)
         clickLine(20)
 
-        expect($('#LC13')).not.toHaveClass('hll')
-        expect($('#LC20')).toHaveClass('hll')
+        expect($('#LC13')).not.toHaveClass(@css)
+        expect($('#LC20')).toHaveClass(@css)
 
       it 'sets the hash', ->
         spy = spyOn(@class, 'setHash').and.callThrough()
@@ -75,8 +76,8 @@ describe 'LineHighlighter', ->
       describe 'without existing highlight', ->
         it 'highlights the clicked line', ->
           clickLine(13, shiftKey: true)
-          expect($('#LC13')).toHaveClass('hll')
-          expect($('.hll').length).toBe(1)
+          expect($('#LC13')).toHaveClass(@css)
+          expect($(".#{@css}").length).toBe(1)
 
         it 'sets the hash', ->
           spy = spyOn(@class, 'setHash')
@@ -87,14 +88,14 @@ describe 'LineHighlighter', ->
         it 'uses existing line as last line when target is lesser', ->
           clickLine(20)
           clickLine(15, shiftKey: true)
-          expect($('.hll').length).toBe(6)
-          expect($("#LC#{line}")).toHaveClass('hll') for line in [15..20]
+          expect($(".#{@css}").length).toBe(6)
+          expect($("#LC#{line}")).toHaveClass(@css) for line in [15..20]
 
         it 'uses existing line as first line when target is greater', ->
           clickLine(5)
           clickLine(10, shiftKey: true)
-          expect($('.hll').length).toBe(6)
-          expect($("#LC#{line}")).toHaveClass('hll') for line in [5..10]
+          expect($(".#{@css}").length).toBe(6)
+          expect($("#LC#{line}")).toHaveClass(@css) for line in [5..10]
 
       describe 'with existing multi-line highlight', ->
         beforeEach ->
@@ -103,26 +104,26 @@ describe 'LineHighlighter', ->
 
         it 'uses target as first line when it is less than existing first line', ->
           clickLine(5, shiftKey: true)
-          expect($('.hll').length).toBe(6)
-          expect($("#LC#{line}")).toHaveClass('hll') for line in [5..10]
+          expect($(".#{@css}").length).toBe(6)
+          expect($("#LC#{line}")).toHaveClass(@css) for line in [5..10]
 
         it 'uses target as last line when it is greater than existing first line', ->
           clickLine(15, shiftKey: true)
-          expect($('.hll').length).toBe(6)
-          expect($("#LC#{line}")).toHaveClass('hll') for line in [10..15]
+          expect($(".#{@css}").length).toBe(6)
+          expect($("#LC#{line}")).toHaveClass(@css) for line in [10..15]
 
   describe '#hashToRange', ->
     beforeEach ->
       @subject = @class.hashToRange
 
     it 'extracts a single line number from the hash', ->
-      expect(@subject('#L5')).toEqual([5, NaN])
+      expect(@subject('#L5')).toEqual([5, null])
 
     it 'extracts a range of line numbers from the hash', ->
       expect(@subject('#L5-15')).toEqual([5, 15])
 
-    it 'returns [NaN, NaN] when the hash is not a line number', ->
-      expect(@subject('#foo')).toEqual([NaN, NaN])
+    it 'returns [null, null] when the hash is not a line number', ->
+      expect(@subject('#foo')).toEqual([null, null])
 
   describe '#highlightLine', ->
     beforeEach ->
@@ -130,36 +131,16 @@ describe 'LineHighlighter', ->
 
     it 'highlights the specified line', ->
       @subject(13)
-      expect($('#LC13')).toHaveClass('hll')
+      expect($('#LC13')).toHaveClass(@css)
 
     it 'accepts a String-based number', ->
       @subject('13')
-      expect($('#LC13')).toHaveClass('hll')
-
-    it 'returns undefined when given NaN', ->
-      expect(@subject(NaN)).toBe(undefined)
-      expect(@subject('foo')).toBe(undefined)
-
-  describe '#highlightRange', ->
-    beforeEach ->
-      @subject = @class.highlightRange
-
-    it 'returns undefined when first line is NaN', ->
-      expect(@subject([NaN, 15])).toBe(undefined)
-      expect(@subject(['foo', 15])).toBe(undefined)
-
-    it 'returns undefined when given an invalid first line', ->
-      expect(@subject(['foo', 15])).toBe(undefined)
-      expect(@subject([NaN, NaN])).toBe(undefined)
-      expect(@subject('foo')).toBe(undefined)
+      expect($('#LC13')).toHaveClass(@css)
 
   describe '#setHash', ->
     beforeEach ->
       @subject = @class.setHash
 
-    it 'returns undefined when given an invalid first line', ->
-      expect(@subject('foo', 15)).toBe(undefined)
-
     it 'sets the location hash for a single line', ->
       @subject(5)
       expect(@spies.__setLocationHash__).toHaveBeenCalledWith('#L5')