[lmi-commits] [lmi] master dd5ca93 2/3: Avoid magic values

[Greg Chicares]

branch: master
commit dd5ca9324155c2ed988b8c540e9a9cbe89860606
Author: Gregory W. Chicares <address@hidden>
Commit: Gregory W. Chicares <address@hidden>

    Avoid magic values
    
    PDF-generator columns may be hidden or not. Expressing this boolean
    property as a boolean variable is clean and direct; expressing it by
    changing the header to an empty string was obscure.
---
 group_quote_pdf_gen_wx.cpp  | 15 ++++--------
 ledger_pdf_generator_wx.cpp | 12 ++++------
 wx_table_generator.cpp      | 57 ++++++++++++++++-----------------------------
 wx_table_generator.hpp      |  1 +
 4 files changed, 30 insertions(+), 55 deletions(-)

diff --git a/group_quote_pdf_gen_wx.cpp b/group_quote_pdf_gen_wx.cpp
index 562dc22..cf71b00 100644
--- a/group_quote_pdf_gen_wx.cpp
+++ b/group_quote_pdf_gen_wx.cpp
@@ -674,6 +674,7 @@ void group_quote_pdf_generator_wx::save(std::string const& 
output_filename)
         {
         column_definition const& cd = column_definitions[col];
         std::string header;
+        bool hidden = false;
 
         // The cast is only used to ensure that if any new elements are added
         // to the enum, the compiler would warn about their values not being
@@ -682,11 +683,7 @@ void group_quote_pdf_generator_wx::save(std::string const& 
output_filename)
             {
             case e_col_supplemental_face_amount:
             case e_col_total_face_amount:
-                if(!has_suppl_amount)
-                    {
-                    // Leave the header empty to hide this column.
-                    break;
-                    }
+                if(!has_suppl_amount) {hidden = true;}
                 // Fall through
             case e_col_number:
             case e_col_name:
@@ -698,11 +695,7 @@ void group_quote_pdf_generator_wx::save(std::string const& 
output_filename)
                 break;
             case e_col_additional_premium:
             case e_col_total_premium:
-                if(!has_addl_premium)
-                    {
-                    // Leave the header empty to hide this column.
-                    break;
-                    }
+                if(!has_addl_premium) {hidden = true;}
                 // Fall through
             case e_col_basic_premium:
                 {
@@ -721,7 +714,7 @@ void group_quote_pdf_generator_wx::save(std::string const& 
output_filename)
                 break;
             }
 
-        vc.push_back({header, cd.widest_text_});
+        vc.push_back({header, cd.widest_text_, hidden});
         }
 
     wx_table_generator table_gen
diff --git a/ledger_pdf_generator_wx.cpp b/ledger_pdf_generator_wx.cpp
index cb5b14e..617d8e5 100644
--- a/ledger_pdf_generator_wx.cpp
+++ b/ledger_pdf_generator_wx.cpp
@@ -362,13 +362,11 @@ class using_illustration_table
         int column = 0;
         for(auto const& i : get_table_columns())
             {
-            std::string header;
-            if(should_show_column(ledger, column++))
-                {
-                header = i.header;
-                }
-            //else: Leave the header empty to avoid showing the column.
-            vc.push_back({header, i.widest_text});
+            vc.push_back
+                ({i.header
+                 ,i.widest_text
+                 ,!should_show_column(ledger, column++)
+                });
             }
 
         // Set the smaller font used for all tables before creating the table
diff --git a/wx_table_generator.cpp b/wx_table_generator.cpp
index 8d3c93d..6f580cc 100644
--- a/wx_table_generator.cpp
+++ b/wx_table_generator.cpp
@@ -27,11 +27,6 @@
 #include "assert_lmi.hpp"
 #include "miscellany.hpp"               // count_newlines(), split_into_lines()
 
-//   is_centered_ is a member variable, initialized in the ctor
-//   is_hidden() is a member function, whose return value is dynamic
-// Should these really be implemented in those two different ways?
-// Wouldn't it be better to treat is_hidden() the same as is_centered_?
-//
 // Is this a struct only because we want its members to be publicly
 // accessible? But their values can also be changed by clients, and
 // isn't that undesirable?
@@ -56,27 +51,8 @@
 //
 // - is_hidden()
 //
-// Apparently used only for group premium quotes, e.g.:
-//
-//             case e_col_total_face_amount:
-//                 if(!has_suppl_amount)
-//                     // Leave the header empty to hide this column.
-//                     break;
-//                 // Fall through
-//             ...
-//                 header = cd.header_;
-//
-// Some columns are conditionally hidden by should_show_column():
-//
-//     // May be overridden to return false if the given column shouldn't be 
shown
-//     // for the specific ledger values (currently used to exclude individual
-//     // columns from composite illustrations).
-//     virtual bool should_show_column(Ledger const& ledger, int column) const
-//
-// but that technique seems to be orthogonal to is_hidden() and used
-// only for illustration PDFs.
-// --No, it's not orthogonal, should_show_column() is used to decide whether
-// the column label should be left empty, making the column hidden.
+// All potential data are passed for every row; is_hidden() suppresses
+// any column that needs to be filtered out.
 //
 //  - is_centered()
 //
@@ -164,8 +140,9 @@
 // used, while the various accessors discussed above are just its
 // implementation details.
 
-//  - is_hidden(): A column with empty header is considered to be
-//    suppressed and doesn't appear in the output at all.
+//  - is_hidden(): A hidden column is present in the data passed into
+//    this class, but is to be suppressed so that it doesn't appear in
+//    the output at all.
 //
 //  - is_centered(): Indicate whether column should be centered,
 //    rather than left-aligned. Ignored for globally right-aligned
@@ -185,14 +162,15 @@
 class wx_table_generator::column_info
 {
   public:
-    column_info(std::string const& header, int width)
+    column_info(std::string const& header, int width, bool hidden)
         :col_header_       (header)
         ,col_width_        (width)
+        ,is_hidden_        (hidden)
         ,is_variable_width_(0 == width)
         {
         }
 
-    bool is_hidden()         const {return col_header().empty();}
+    bool is_hidden()         const {return  is_hidden_;}
     bool is_centered()       const {return !is_variable_width_;}
     bool is_variable_width() const {return  is_variable_width_;}
     bool needs_clipping()    const {return  is_variable_width_;}
@@ -211,6 +189,7 @@ class wx_table_generator::column_info
     int col_width_;
 
   private:
+    bool const is_hidden_;
     bool const is_variable_width_;
 };
 
@@ -311,7 +290,7 @@ std::vector<wx_table_generator::column_info> const& 
wx_table_generator::all_colu
 /// The total number of columns thus enrolled determines the cardinality
 /// of the 'values' argument in output_row() calls.
 ///
-/// Providing an empty header suppresses the column display, while still
+/// Making a column hidden suppresses the column display, while still
 /// taking it into account in output_row(), providing a convenient way to
 /// hide a single column without changing the data representation.
 ///
@@ -324,12 +303,11 @@ std::vector<wx_table_generator::column_info> const& 
wx_table_generator::all_colu
 
 void wx_table_generator::enroll_column(column_parameters const& z)
 {
-    // If a column's header is empty, then it is to be hidden--and its
-    // width must be initialized to zero, because other member functions
-    // calculate total width by accumulating the widths of all columns,
-    // whether hidden or not.
+    // A hidden column's width must be initialized to zero, because
+    // other member functions calculate total width by accumulating
+    // the widths of all columns, whether hidden or not.
     int width = 0;
-    if(!z.header.empty())
+    if(!z.hidden)
         {
         wxDCFontChanger header_font_setter(dc_);
         if(use_bold_headers_)
@@ -365,7 +343,7 @@ LMI_ASSERT(w == dc_.GetMultiLineTextExtent(z.header).x);
             }
         }
 
-    all_columns_.push_back(column_info(z.header, width));
+    all_columns_.push_back(column_info(z.header, width, z.hidden));
 }
 
 /// Return the font used for the headers.
@@ -839,6 +817,11 @@ void wx_table_generator::output_header
     for(std::size_t col = 0; col < num_columns; ++col)
         {
         column_info const& ci = all_columns().at(col);
+        if(ci.is_hidden())
+            {
+            continue;
+            }
+
         std::vector<std::string> const 
lines(split_into_lines(ci.col_header()));
 
         // Fill the elements from the bottom line to the top one, so that a
diff --git a/wx_table_generator.hpp b/wx_table_generator.hpp
index 2003d1a..b0cb546 100644
--- a/wx_table_generator.hpp
+++ b/wx_table_generator.hpp
@@ -39,6 +39,7 @@ struct column_parameters
 {
     std::string header;
     std::string widest_text;
+    bool        hidden;
 };
 
 /// Specialized styles for first wx_table_generator ctor argument.