From e93c80febf757290fecce6eebce871c2448dad14 Mon Sep 17 00:00:00 2001
From: Steven Daniels <stevendaniels88@gmail.com>
Date: Sat, 31 Dec 2016 13:08:05 -0500
Subject: [PATCH] Refactoring some classes to reduce complexity

---
 lib/roo/excelx.rb               |  2 +-
 lib/roo/excelx/cell/datetime.rb | 66 +++++++++++++++++++--------------
 lib/roo/excelx/cell/number.rb   | 41 +++++++++-----------
 lib/roo/excelx/cell/time.rb     | 26 ++++++-------
 4 files changed, 70 insertions(+), 65 deletions(-)

diff --git a/lib/roo/excelx.rb b/lib/roo/excelx.rb
index 2126af74..82c14314 100644
--- a/lib/roo/excelx.rb
+++ b/lib/roo/excelx.rb
@@ -223,7 +223,7 @@ def empty?(row, col, sheet = nil)
       sheet = sheet_for(sheet)
       key = normalize(row, col)
       cell = sheet.cells[key]
-      !cell || cell.empty? || (cell.type == :string && cell.value.empty?) ||
+      !cell || cell.empty? ||
         (row < sheet.first_row || row > sheet.last_row || col < sheet.first_column || col > sheet.last_column)
     end
 
diff --git a/lib/roo/excelx/cell/datetime.rb b/lib/roo/excelx/cell/datetime.rb
index 0bd62ca0..35d93ac8 100644
--- a/lib/roo/excelx/cell/datetime.rb
+++ b/lib/roo/excelx/cell/datetime.rb
@@ -32,14 +32,9 @@ def initialize(value, formula, excelx_type, style, link, base_date, coordinate)
         #
         # Returns a String representation of a cell's value.
         def formatted_value
-          date_regex = /(?<date>[dmy]+[\-\/][dmy]+([\-\/][dmy]+)?)/
-          time_regex = /(?<time>(\[?[h]\]?+:)?[m]+(:?ss|:?s)?)/
-
           formatter = @format.downcase.split(' ').map do |part|
-            if part[date_regex] == part
-              part.gsub(/#{DATE_FORMATS.keys.join('|')}/, DATE_FORMATS)
-            elsif part[time_regex]
-              part.gsub(/#{TIME_FORMATS.keys.join('|')}/, TIME_FORMATS)
+            if (parsed_format = parse_date_or_time_format(part))
+              parsed_format
             else
               warn 'Unable to parse custom format. Using "YYYY-mm-dd HH:MM:SS" format.'
               return @value.strftime('%F %T')
@@ -51,35 +46,50 @@ def formatted_value
 
         private
 
+        def parse_date_or_time_format(part)
+          date_regex = /(?<date>[dmy]+[\-\/][dmy]+([\-\/][dmy]+)?)/
+          time_regex = /(?<time>(\[?[h]\]?+:)?[m]+(:?ss|:?s)?)/
+
+          if part[date_regex] == part
+            formats = DATE_FORMATS
+          elsif part[time_regex]
+            formats = TIME_FORMATS
+          else
+            return false
+          end
+
+          part.gsub(/#{formats.keys.join('|')}/, formats)
+        end
+
         DATE_FORMATS = {
-          'yyyy'.freeze => '%Y'.freeze,  # Year: 2000
-          'yy'.freeze => '%y'.freeze,    # Year: 00
+          'yyyy' => '%Y',  # Year: 2000
+          'yy' => '%y',    # Year: 00
           # mmmmm => J-D
-          'mmmm'.freeze => '%B'.freeze,  # Month: January
-          'mmm'.freeze => '%^b'.freeze,   # Month: JAN
-          'mm'.freeze => '%m'.freeze,    # Month: 01
-          'm'.freeze => '%-m'.freeze,    # Month: 1
-          'dddd'.freeze => '%A'.freeze,  # Day of the Week: Sunday
-          'ddd'.freeze => '%^a'.freeze,   # Day of the Week: SUN
-          'dd'.freeze => '%d'.freeze,    # Day of the Month: 01
-          'd'.freeze => '%-d'.freeze,    # Day of the Month: 1
+          'mmmm' => '%B',  # Month: January
+          'mmm' => '%^b',   # Month: JAN
+          'mm' => '%m',    # Month: 01
+          'm' => '%-m',    # Month: 1
+          'dddd' => '%A',  # Day of the Week: Sunday
+          'ddd' => '%^a',   # Day of the Week: SUN
+          'dd' => '%d',    # Day of the Month: 01
+          'd' => '%-d'    # Day of the Month: 1
           # '\\\\'.freeze => ''.freeze,  # NOTE: Fixes a custom format's output.
         }
 
         TIME_FORMATS = {
-          'hh'.freeze => '%H'.freeze,    # Hour (24): 01
-          'h'.freeze => '%-k'.freeze,    # Hour (24): 1
+          'hh' => '%H',    # Hour (24): 01
+          'h' => '%-k'.freeze,    # Hour (24): 1
           # 'hh'.freeze => '%I'.freeze,    # Hour (12): 08
           # 'h'.freeze => '%-l'.freeze,    # Hour (12): 8
-          'mm'.freeze => '%M'.freeze,    # Minute: 01
+          'mm' => '%M',    # Minute: 01
           # FIXME: is this used? Seems like 'm' is used for month, not minute.
-          'm'.freeze => '%-M'.freeze,    # Minute: 1
-          'ss'.freeze => '%S'.freeze,    # Seconds: 01
-          's'.freeze => '%-S'.freeze,    # Seconds: 1
-          'am/pm'.freeze => '%p'.freeze, # Meridian: AM
-          '000'.freeze => '%3N'.freeze,  # Fractional Seconds: thousandth.
-          '00'.freeze => '%2N'.freeze,   # Fractional Seconds: hundredth.
-          '0'.freeze => '%1N'.freeze,    # Fractional Seconds: tenths.
+          'm' => '%-M',    # Minute: 1
+          'ss' => '%S',    # Seconds: 01
+          's' => '%-S',    # Seconds: 1
+          'am/pm' => '%p', # Meridian: AM
+          '000' => '%3N',  # Fractional Seconds: thousandth.
+          '00' => '%2N',   # Fractional Seconds: hundredth.
+          '0' => '%1N'    # Fractional Seconds: tenths.
         }
 
         def create_datetime(base_date, value)
@@ -93,7 +103,7 @@ def create_datetime(base_date, value)
         def round_datetime(datetime_string)
           /(?<yyyy>\d+)-(?<mm>\d+)-(?<dd>\d+) (?<hh>\d+):(?<mi>\d+):(?<ss>\d+.\d+)/ =~ datetime_string
 
-          ::Time.new(yyyy.to_i, mm.to_i, dd.to_i, hh.to_i, mi.to_i, ss.to_r).round(0)
+          ::Time.new(yyyy, mm, dd, hh, mi, ss.to_r).round(0)
         end
       end
     end
diff --git a/lib/roo/excelx/cell/number.rb b/lib/roo/excelx/cell/number.rb
index 21e2cf70..20155628 100644
--- a/lib/roo/excelx/cell/number.rb
+++ b/lib/roo/excelx/cell/number.rb
@@ -32,7 +32,7 @@ def formatted_value
           if formatter.is_a? Proc
             formatter.call(@cell_value)
           elsif zero_padded_number?
-            "%0#{@format.size}d"% @cell_value
+            "%0#{@format.size}d" % @cell_value
           else
             Kernel.format(formatter, @cell_value)
           end
@@ -46,12 +46,8 @@ def formats
             '0' => '%.0f',
             '0.00' => '%.2f',
             '0.000000' => '%.6f',
-            '#,##0' => proc do |number|
-              Kernel.format('%.0f', number).reverse.gsub(/(\d{3})(?=\d)/, '\\1,').reverse
-            end,
-            '#,##0.00' => proc do |number|
-              Kernel.format('%.2f', number).reverse.gsub(/(\d{3})(?=\d)/, '\\1,').reverse
-            end,
+            '#,##0' => number_format('%.0f'),
+            '#,##0.00' => number_format('%.2f'),
             '0%' =>  proc do |number|
               Kernel.format('%d%', number.to_f * 100)
             end,
@@ -59,22 +55,10 @@ def formats
               Kernel.format('%.2f%', number.to_f * 100)
             end,
             '0.00E+00' => '%.2E',
-            '#,##0 ;(#,##0)' => proc do |number|
-              formatter = number.to_i > 0 ? '%.0f' : '(%.0f)'
-              Kernel.format(formatter, number.to_f.abs).reverse.gsub(/(\d{3})(?=\d)/, '\\1,').reverse
-            end,
-            '#,##0 ;[Red](#,##0)' => proc do |number|
-              formatter = number.to_i > 0 ? '%.0f' : '[Red](%.0f)'
-              Kernel.format(formatter, number.to_f.abs).reverse.gsub(/(\d{3})(?=\d)/, '\\1,').reverse
-            end,
-            '#,##0.00;(#,##0.00)' => proc do |number|
-              formatter = number.to_i > 0 ? '%.2f' : '(%.2f)'
-              Kernel.format(formatter, number.to_f.abs).reverse.gsub(/(\d{3})(?=\d)/, '\\1,').reverse
-            end,
-            '#,##0.00;[Red](#,##0.00)' => proc do |number|
-              formatter = number.to_i > 0 ? '%.2f' : '[Red](%.2f)'
-              Kernel.format(formatter, number.to_f.abs).reverse.gsub(/(\d{3})(?=\d)/, '\\1,').reverse
-            end,
+            '#,##0 ;(#,##0)' => number_format('%.0f', '(%.0f)'),
+            '#,##0 ;[Red](#,##0)' => number_format('%.0f', '[Red](%.0f)'),
+            '#,##0.00;(#,##0.00)' => number_format('%.2f', '(%.2f)'),
+            '#,##0.00;[Red](#,##0.00)' => number_format('%.2f', '[Red](%.2f)'),
             # FIXME: not quite sure what the format should look like in this case.
             '##0.0E+0' => '%.1E',
             '@' => proc { |number| number }
@@ -83,6 +67,17 @@ def formats
 
         private
 
+        def number_format(formatter, negative_formatter = nil)
+          proc do |number|
+            if negative_formatter
+              formatter = number.to_i > 0 ? formatter : negative_formatter
+              number = number.to_f.abs
+            end
+
+            Kernel.format(formatter, number).reverse.gsub(/(\d{3})(?=\d)/, '\\1,').reverse
+          end
+        end
+
         def zero_padded_number?
           @format[/0+/] == @format
         end
diff --git a/lib/roo/excelx/cell/time.rb b/lib/roo/excelx/cell/time.rb
index 4bea9d06..d661ab8d 100644
--- a/lib/roo/excelx/cell/time.rb
+++ b/lib/roo/excelx/cell/time.rb
@@ -24,19 +24,19 @@ def formatted_value
 
         private
 
-        def create_datetime(base_date, value)
-          date = base_date + value.to_f.round(6)
-          datetime_string = date.strftime('%Y-%m-%d %H:%M:%S.%N')
-          t = round_datetime(datetime_string)
-
-          ::DateTime.civil(t.year, t.month, t.day, t.hour, t.min, t.sec)
-        end
-
-        def round_datetime(datetime_string)
-          /(?<yyyy>\d+)-(?<mm>\d+)-(?<dd>\d+) (?<hh>\d+):(?<mi>\d+):(?<ss>\d+.\d+)/ =~ datetime_string
-
-          ::Time.new(yyyy.to_i, mm.to_i, dd.to_i, hh.to_i, mi.to_i, ss.to_r).round(0)
-        end
+        # def create_datetime(base_date, value)
+        #   date = base_date + value.to_f.round(6)
+        #   datetime_string = date.strftime('%Y-%m-%d %H:%M:%S.%N')
+        #   t = round_datetime(datetime_string)
+        #
+        #   ::DateTime.civil(t.year, t.month, t.day, t.hour, t.min, t.sec)
+        # end
+
+        # def round_datetime(datetime_string)
+        #   /(?<yyyy>\d+)-(?<mm>\d+)-(?<dd>\d+) (?<hh>\d+):(?<mi>\d+):(?<ss>\d+.\d+)/ =~ datetime_string
+        #
+        #   ::Time.new(yyyy.to_i, mm.to_i, dd.to_i, hh.to_i, mi.to_i, ss.to_r).round(0)
+        # end
       end
     end
   end