Use magic string literal comment over String#freeze

Use Time#to_datetime over Datetime.civil
roo-rb · Sep 13, 2018 · 699ff68 · 699ff68
1 parent b7a58e2
commit 699ff68
Show file tree

Hide file tree

Showing 12 changed files with 94 additions and 78 deletions.
diff --git a/lib/roo.rb b/lib/roo.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'roo/version'
 require 'roo/constants'
 require 'roo/errors'
@@ -10,7 +12,7 @@ module Roo
   autoload :Excelx,       'roo/excelx'
   autoload :CSV,          'roo/csv'
 
-  TEMP_PREFIX = 'roo_'.freeze
+  TEMP_PREFIX = 'roo_'
 
   CLASS_FOR_EXTENSION = {
     ods: Roo::OpenOffice,

diff --git a/lib/roo/base.rb b/lib/roo/base.rb
@@ -19,8 +19,8 @@ class Roo::Base
   include Roo::Formatters::XML
   include Roo::Formatters::YAML
 
-  MAX_ROW_COL = 999_999.freeze
-  MIN_ROW_COL = 0.freeze
+  MAX_ROW_COL = 999_999
+  MIN_ROW_COL = 0
 
   attr_reader :headers
 

diff --git a/lib/roo/constants.rb b/lib/roo/constants.rb
@@ -1,5 +1,7 @@
+# frozen_string_literal: true
+
 module Roo
-  ROO_EXCEL_NOTICE   = "Excel support has been extracted to roo-xls due to its dependency on the GPL'd spreadsheet gem. Install roo-xls to use Roo::Excel.".freeze
-  ROO_EXCELML_NOTICE = "Excel SpreadsheetML support has been extracted to roo-xls. Install roo-xls to use Roo::Excel2003XML.".freeze
-  ROO_GOOGLE_NOTICE = "Google support has been extracted to roo-google. Install roo-google to use Roo::Google.".freeze
+  ROO_EXCEL_NOTICE   = "Excel support has been extracted to roo-xls due to its dependency on the GPL'd spreadsheet gem. Install roo-xls to use Roo::Excel."
+  ROO_EXCELML_NOTICE = "Excel SpreadsheetML support has been extracted to roo-xls. Install roo-xls to use Roo::Excel2003XML."
+  ROO_GOOGLE_NOTICE = "Google support has been extracted to roo-google. Install roo-google to use Roo::Google."
 end
diff --git a/lib/roo/excelx/cell/boolean.rb b/lib/roo/excelx/cell/boolean.rb
@@ -1,3 +1,4 @@
+# frozen_string_literal: true
 module Roo
   class Excelx
     class Cell
@@ -11,7 +12,7 @@ def initialize(value, formula, style, link, coordinate)
         end
 
         def formatted_value
-          value ? 'TRUE'.freeze : 'FALSE'.freeze
+          value ? 'TRUE' : 'FALSE'
         end
 
         private

diff --git a/lib/roo/excelx/cell/datetime.rb b/lib/roo/excelx/cell/datetime.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'date'
 
 module Roo
@@ -80,7 +82,7 @@ def parse_date_or_time_format(part)
 
         TIME_FORMATS = {
           'hh' => '%H',    # Hour (24): 01
-          'h' => '%-k'.freeze,    # Hour (24): 1
+          'h' => '%-k',    # Hour (24): 1
           # 'hh'.freeze => '%I'.freeze,    # Hour (12): 08
           # 'h'.freeze => '%-l'.freeze,    # Hour (12): 8
           'mm' => '%M',    # Minute: 01
@@ -96,9 +98,7 @@ def parse_date_or_time_format(part)
 
         def create_datetime(base_timestamp, value)
           timestamp = (base_timestamp + (value.to_f.round(6) * SECONDS_IN_DAY)).round(0)
-          t = ::Time.at(timestamp).utc
-
-          ::DateTime.civil(t.year, t.month, t.day, t.hour, t.min, t.sec)
+          ::Time.at(timestamp).utc.to_datetime
         end
       end
     end

diff --git a/lib/roo/excelx/cell/number.rb b/lib/roo/excelx/cell/number.rb
@@ -1,4 +1,5 @@
 # frozen_string_literal: true
+
 module Roo
   class Excelx
     class Cell

diff --git a/lib/roo/excelx/extractor.rb b/lib/roo/excelx/extractor.rb
@@ -1,6 +1,18 @@
+# frozen_string_literal: true
+
 module Roo
   class Excelx
     class Extractor
+
+      COMMON_STRINGS = {
+        t: "t",
+        r: "r",
+        s: "s",
+        ref: "ref",
+        html_tag_open: "<html>",
+        html_tag_closed: "</html>"
+      }
+
       def initialize(path, options = {})
         @path = path
         @options = options

diff --git a/lib/roo/excelx/format.rb b/lib/roo/excelx/format.rb
@@ -1,4 +1,5 @@
 # frozen_string_literal: true
+
 module Roo
   class Excelx
     module Format
@@ -9,34 +10,34 @@ module Format
       }
 
       STANDARD_FORMATS = {
-        0 => 'General'.freeze,
-        1 => '0'.freeze,
-        2 => '0.00'.freeze,
-        3 => '#,##0'.freeze,
-        4 => '#,##0.00'.freeze,
-        9 => '0%'.freeze,
-        10 => '0.00%'.freeze,
-        11 => '0.00E+00'.freeze,
-        12 => '# ?/?'.freeze,
-        13 => '# ??/??'.freeze,
-        14 => 'mm-dd-yy'.freeze,
-        15 => 'd-mmm-yy'.freeze,
-        16 => 'd-mmm'.freeze,
-        17 => 'mmm-yy'.freeze,
-        18 => 'h:mm AM/PM'.freeze,
-        19 => 'h:mm:ss AM/PM'.freeze,
-        20 => 'h:mm'.freeze,
-        21 => 'h:mm:ss'.freeze,
-        22 => 'm/d/yy h:mm'.freeze,
-        37 => '#,##0 ;(#,##0)'.freeze,
-        38 => '#,##0 ;[Red](#,##0)'.freeze,
-        39 => '#,##0.00;(#,##0.00)'.freeze,
-        40 => '#,##0.00;[Red](#,##0.00)'.freeze,
-        45 => 'mm:ss'.freeze,
-        46 => '[h]:mm:ss'.freeze,
-        47 => 'mmss.0'.freeze,
-        48 => '##0.0E+0'.freeze,
-        49 => '@'.freeze
+        0 => 'General',
+        1 => '0',
+        2 => '0.00',
+        3 => '#,##0',
+        4 => '#,##0.00',
+        9 => '0%',
+        10 => '0.00%',
+        11 => '0.00E+00',
+        12 => '# ?/?',
+        13 => '# ??/??',
+        14 => 'mm-dd-yy',
+        15 => 'd-mmm-yy',
+        16 => 'd-mmm',
+        17 => 'mmm-yy',
+        18 => 'h:mm AM/PM',
+        19 => 'h:mm:ss AM/PM',
+        20 => 'h:mm',
+        21 => 'h:mm:ss',
+        22 => 'm/d/yy h:mm',
+        37 => '#,##0 ;(#,##0)',
+        38 => '#,##0 ;[Red](#,##0)',
+        39 => '#,##0.00;(#,##0.00)',
+        40 => '#,##0.00;[Red](#,##0.00)',
+        45 => 'mm:ss',
+        46 => '[h]:mm:ss',
+        47 => 'mmss.0',
+        48 => '##0.0E+0',
+        49 => '@'
       }
 
       def to_type(format)

diff --git a/lib/roo/excelx/shared_strings.rb b/lib/roo/excelx/shared_strings.rb
@@ -1,16 +1,10 @@
+# frozen_string_literal: true
+
 require 'roo/excelx/extractor'
 
 module Roo
   class Excelx
     class SharedStrings < Excelx::Extractor
-
-      COMMON_STRINGS = {
-        t: "t",
-        r: "r",
-        html_tag_open: "<html>",
-        html_tag_closed: "</html>"
-      }
-
       def [](index)
         to_a[index]
       end
@@ -46,14 +40,14 @@ def extract_shared_strings
         document = fix_invalid_shared_strings(doc)
         # read the shared strings xml document
         document.xpath('/sst/si').map do |si|
-          shared_string = ''
+          shared_string = String.new
           si.children.each do |elem|
             case elem.name
-            when 'r'.freeze
+            when 'r'
               elem.children.each do |r_elem|
                 shared_string << r_elem.content if r_elem.name == 't'
               end
-            when 't'.freeze
+            when 't'
               shared_string = elem.content
             end
           end
@@ -66,16 +60,16 @@ def extract_html
         fix_invalid_shared_strings(doc)
         # read the shared strings xml document
         doc.xpath('/sst/si').map do |si|
-          html_string = '<html>'
+          html_string = '<html>'.dup
           si.children.each do |elem|
             case elem.name
-            when 'r'.freeze
+            when 'r'
               html_string << extract_html_r(elem)
-            when 't'.freeze
+            when 't'
               html_string << elem.content
             end # case elem.name
           end # si.children.each do |elem|
-          html_string << '</html>'.freeze
+          html_string << '</html>'
         end # doc.xpath('/sst/si').map do |si|
       end # def extract_html
 
@@ -96,7 +90,7 @@ def extract_html
       #
       # Expected Output ::: "<html><sub|sup><b><i><u>TEXT</u></i></b></sub|/sup></html>"
       def extract_html_r(r_elem)
-        str = ''
+        str = String.new
         xml_elems = {
           sub: false,
           sup: false,
@@ -141,7 +135,7 @@ def extract_html_r(r_elem)
 
       # This will return an html string
       def create_html(text, formatting)
-        tmp_str = ''
+        tmp_str = String.new
         formatting.each do |elem, val|
           tmp_str << "<#{elem}>" if val
         end

diff --git a/lib/roo/excelx/sheet_doc.rb b/lib/roo/excelx/sheet_doc.rb
@@ -5,7 +5,7 @@ module Roo
   class Excelx
     class SheetDoc < Excelx::Extractor
       extend Forwardable
-      delegate [:workbook, :shared_strings] => :@shared
+      delegate [:workbook] => :@shared
 
       def initialize(path, relationships, shared, options = {})
         super(path)
@@ -41,7 +41,7 @@ def each_cell(row_xml)
         row_xml.children.each do |cell_element|
           # If you're sure you're not going to need this hyperlinks you can discard it
           hyperlinks = unless @options[:no_hyperlinks]
-                         key = ::Roo::Utils.ref_to_key(cell_element['r'.freeze])
+                         key = ::Roo::Utils.ref_to_key(cell_element[COMMON_STRINGS[:r]])
                          hyperlinks(@relationships)[key]
                        end
 
@@ -53,13 +53,13 @@ def each_cell(row_xml)
 
       def cell_value_type(type, format)
         case type
-        when 's'.freeze
+        when 's'
           :shared
-        when 'b'.freeze
+        when 'b'
           :boolean
-        when 'str'.freeze
+        when 'str'
           :string
-        when 'inlineStr'.freeze
+        when 'inlineStr'
           :inlinestr
         else
           Excelx::Format.to_type(format)
@@ -82,32 +82,32 @@ def cell_value_type(type, format)
       #
       # Returns a type of <Excelx::Cell>.
       def cell_from_xml(cell_xml, hyperlink)
-        coordinate = ::Roo::Utils.extract_coordinate(cell_xml['r'.freeze])
+        coordinate = ::Roo::Utils.extract_coordinate(cell_xml[COMMON_STRINGS[:r]])
         cell_xml_children = cell_xml.children
         return Excelx::Cell::Empty.new(coordinate) if cell_xml_children.empty?
 
         # NOTE: This is error prone, to_i will silently turn a nil into a 0.
         #       This works by coincidence because Format[0] is General.
-        style = cell_xml['s'.freeze].to_i
+        style = cell_xml[COMMON_STRINGS[:s]].to_i
         formula = nil
 
         cell_xml_children.each do |cell|
           case cell.name
-          when 'is'.freeze
-            content = ""
+          when 'is'
+            content = String.new
             cell.children.each do |inline_str|
-              if inline_str.name == 't'.freeze
+              if inline_str.name == 't'
                 content << inline_str.content
               end
             end
             unless content.empty?
               return Excelx::Cell.cell_class(:string).new(content, formula, style, hyperlink, coordinate)
             end
-          when 'f'.freeze
+          when 'f'
             formula = cell.content
-          when 'v'.freeze
+          when 'v'
             format = style_format(style)
-            value_type = cell_value_type(cell_xml['t'.freeze], format)
+            value_type = cell_value_type(cell_xml[COMMON_STRINGS[:t]], format)
 
             return create_cell_from_value(value_type, cell, formula, format, style, hyperlink, coordinate)
           end
@@ -169,8 +169,8 @@ def extract_hyperlinks(relationships)
         return {} unless (hyperlinks = doc.xpath('/worksheet/hyperlinks/hyperlink'))
 
         Hash[hyperlinks.map do |hyperlink|
-          if hyperlink.attribute('id'.freeze) && (relationship = relationships[hyperlink.attribute('id'.freeze).text])
-            [::Roo::Utils.ref_to_key(hyperlink.attributes['ref'.freeze].to_s), relationship.attribute('Target'.freeze).text]
+          if hyperlink.attribute('id') && (relationship = relationships[hyperlink.attribute('id').text])
+            [::Roo::Utils.ref_to_key(hyperlink.attributes[COMMON_STRINGS[:ref]].to_s), relationship.attribute('Target').text]
           end
         end.compact]
       end
@@ -179,7 +179,7 @@ def expand_merged_ranges(cells)
         # Extract merged ranges from xml
         merges = {}
         doc.xpath('/worksheet/mergeCells/mergeCell').each do |mergecell_xml|
-          tl, br = mergecell_xml['ref'.freeze].split(/:/).map { |ref| ::Roo::Utils.ref_to_key(ref) }
+          tl, br = mergecell_xml[COMMON_STRINGS[:ref]].split(/:/).map { |ref| ::Roo::Utils.ref_to_key(ref) }
           for row in tl[0]..br[0] do
             for col in tl[1]..br[1] do
               next if row == tl[0] && col == tl[1]
@@ -196,7 +196,7 @@ def expand_merged_ranges(cells)
       def extract_cells(relationships)
         extracted_cells = {}
         doc.xpath('/worksheet/sheetData/row/c').each do |cell_xml|
-          key = ::Roo::Utils.ref_to_key(cell_xml['r'.freeze])
+          key = ::Roo::Utils.ref_to_key(cell_xml[COMMON_STRINGS[:r]])
           extracted_cells[key] = cell_from_xml(cell_xml, hyperlinks(relationships)[key])
         end
 
@@ -207,7 +207,7 @@ def extract_cells(relationships)
 
       def extract_dimensions
         Roo::Utils.each_element(@path, 'dimension') do |dimension|
-          return dimension.attributes['ref'.freeze].value
+          return dimension.attributes[COMMON_STRINGS[:ref]].value
         end
       end
 

diff --git a/lib/roo/open_office.rb b/lib/roo/open_office.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'date'
 require 'nokogiri'
 require 'cgi'
@@ -11,9 +13,9 @@ module Roo
   class OpenOffice < Roo::Base
     extend Roo::Tempdir
 
-    ERROR_MISSING_CONTENT_XML = 'file missing required content.xml'.freeze
-    XPATH_FIND_TABLE_STYLES   = "//*[local-name()='automatic-styles']".freeze
-    XPATH_LOCAL_NAME_TABLE    = "//*[local-name()='table']".freeze
+    ERROR_MISSING_CONTENT_XML = 'file missing required content.xml'
+    XPATH_FIND_TABLE_STYLES   = "//*[local-name()='automatic-styles']"
+    XPATH_LOCAL_NAME_TABLE    = "//*[local-name()='table']"
 
     # initialization and opening of a spreadsheet file
     # values for packed: :zip

diff --git a/lib/roo/utils.rb b/lib/roo/utils.rb
@@ -91,6 +91,7 @@ def each_element(path, elements)
     end
 
     private
+
     def char_index(byte)
       if byte >= 65 && byte <= 90
         byte - 64