From da9985058b390d7725f0bbb916386d0c432c9b6d Mon Sep 17 00:00:00 2001 From: Paul Beckingham Date: Wed, 21 Jul 2010 15:30:37 -0700 Subject: [PATCH] Enhancement - Sorting - Replaced the hand-written combsort with std::sort. - Added exhaustive set of unit tests to cover the single and double column sort orders. --- src/Grid.cpp | 14 +- src/Table.cpp | 391 ++++++++++++++++++++------------------------ src/Table.h | 3 +- src/tests/sorting.t | 149 +++++++++++++++-- 4 files changed, 319 insertions(+), 238 deletions(-) diff --git a/src/Grid.cpp b/src/Grid.cpp index 346338917..42ba0dea8 100644 --- a/src/Grid.cpp +++ b/src/Grid.cpp @@ -369,15 +369,11 @@ Grid::Cell::operator std::string () const switch (mType) { case CELL_BOOL: return mBool ? "true" : "false"; // TODO i18n - case CELL_CHAR: sprintf (s, "%c", mChar); - return std::string (s); - case CELL_INT: sprintf (s, "%d", mInt); - return std::string (s); - case CELL_FLOAT: sprintf (s, "%f", mFloat); - return std::string (s); - case CELL_DOUBLE: sprintf (s, "%f", mDouble); - return std::string (s); - case CELL_STRING: return mString; + case CELL_CHAR: sprintf (s, "%c", mChar); return std::string (s); + case CELL_INT: sprintf (s, "%d", mInt); return std::string (s); + case CELL_FLOAT: sprintf (s, "%f", mFloat); return std::string (s); + case CELL_DOUBLE: sprintf (s, "%f", mDouble); return std::string (s); + case CELL_STRING: return mString; } return std::string (""); diff --git a/src/Table.cpp b/src/Table.cpp index d4ebb8860..a37c86d85 100644 --- a/src/Table.cpp +++ b/src/Table.cpp @@ -55,6 +55,7 @@ #include "Context.h" extern Context context; +Table* table = NULL; //////////////////////////////////////////////////////////////////////////////// Table::Table () @@ -591,239 +592,192 @@ int Table::columnCount () } //////////////////////////////////////////////////////////////////////////////// -// Combsort11, with O(n log n) average, O(n log n) worst case performance. -// -// function combsort11(array input) -// gap := input.size -// -// loop until gap <= 1 and swaps = 0 -// if gap > 1 -// gap := gap / 1.3 -// if gap = 10 or gap = 9 -// gap := 11 -// end if -// end if -// -// i := 0 -// swaps := 0 -// -// loop until i + gap >= input.size -// if input[i] > input[i+gap] -// swap(input[i], input[i+gap]) -// swaps := 1 -// end if -// i := i + 1 -// end loop -// -// end loop -// end function - -#define SWAP \ - { \ - int temp = order[r]; \ - order[r] = order[r + gap]; \ - order[r + gap] = temp; \ - swaps = 1; \ - } -void Table::sort (std::vector & order) +// Essentially a static implementation of a dynamic operator<. +bool sort_compare (int left, int right) { - int gap = order.size (); - int swaps = 1; - - while (gap > 1 || swaps > 0) + for (size_t c = 0; c < table->mSortColumns.size (); ++c) { - if (gap > 1) - { - gap = (int) ((float)gap / 1.3); - if (gap == 10 || gap == 9) - gap = 11; - } + int column = table->mSortColumns[c]; + Table::order sort_type = table->mSortOrder[column]; - int r = 0; - swaps = 0; + Grid::Cell* cell_left = table->mData.byRow (left, column); + Grid::Cell* cell_right = table->mData.byRow (right, column); - while (r + gap < (int) order.size ()) + // nothing < something. + if (cell_left == NULL && cell_right != NULL) + return (sort_type == Table::ascendingNumeric || + sort_type == Table::ascendingCharacter || + sort_type == Table::ascendingPriority || + sort_type == Table::ascendingDate || + sort_type == Table::ascendingDueDate || + sort_type == Table::ascendingPeriod) ? true : false; + + // something > nothing. + if (cell_left != NULL && cell_right == NULL) + return (sort_type == Table::ascendingNumeric || + sort_type == Table::ascendingCharacter || + sort_type == Table::ascendingPriority || + sort_type == Table::ascendingDate || + sort_type == Table::ascendingDueDate || + sort_type == Table::ascendingPeriod) ? false : true; + + // Equally NULL - next column. + if (cell_left == NULL && cell_right == NULL) + continue; + + // Equal - next column + if (cell_left && cell_right && *cell_left == *cell_right) + continue; + + // Differing data - do a proper comparison. + if (cell_left && cell_right) { - bool keepScanning = true; - for (size_t c = 0; keepScanning && c < mSortColumns.size (); ++c) + switch (sort_type) { - keepScanning = false; + case Table::ascendingNumeric: + return (float)*cell_left < (float)*cell_right ? true : false; + break; - Grid::Cell* left = mData.byRow (order[r], mSortColumns[c]); - Grid::Cell* right = mData.byRow (order[r + gap], mSortColumns[c]); + case Table::descendingNumeric: + return (float)*cell_left < (float)*cell_right ? false : true; + break; - // Data takes precedence over missing data. - if (left == NULL && right != NULL) + case Table::ascendingCharacter: + return (std::string)*cell_left < (std::string)*cell_right ? true : false; + break; + + case Table::descendingCharacter: + return (std::string)*cell_left < (std::string)*cell_right ? false : true; + break; + + case Table::ascendingDate: { - SWAP - break; - } + // something > nothing. + if ((std::string)*cell_left != "" && (std::string)*cell_right == "") + return false; - // No data - try comparing the next column. - else if (left == NULL && right == NULL) - { - keepScanning = true; - } + // nothing < something. + else if ((std::string)*cell_left == "" && (std::string)*cell_right != "") + return true; - // Identical data - try comparing the next column. - else if (left && right && *left == *right) - { - keepScanning = true; - } - - // Differing data - do a proper comparison. - else if (left && right && *left != *right) - { - switch (mSortOrder[mSortColumns[c]]) + else { - case ascendingNumeric: - if ((float)*left > (float)*right) - SWAP - break; - - case descendingNumeric: - if ((float)*left < (float)*right) - SWAP - break; - - case ascendingCharacter: - if ((std::string)*left > (std::string)*right) - SWAP - break; - - case descendingCharacter: - if ((std::string)*left < (std::string)*right) - SWAP - break; - - case ascendingDate: - { - if (*left == *right) - break; - - if ((std::string)*left != "" && (std::string)*right == "") - break; - - else if ((std::string)*left == "" && (std::string)*right != "") - SWAP - - else - { - Date dl ((std::string)*left, mDateFormat); - Date dr ((std::string)*right, mDateFormat); - if (dl > dr) - SWAP - } - } - break; - - case descendingDate: - { - if (*left == *right) - break; - - if ((std::string)*left != "" && (std::string)*right == "") - break; - - else if ((std::string)*left == "" && (std::string)*right != "") - SWAP - - else - { - Date dl ((std::string)*left, mDateFormat); - Date dr ((std::string)*right, mDateFormat); - if (dl < dr) - SWAP - } - } - break; - - case ascendingDueDate: - { - if ((std::string)*left != "" && (std::string)*right == "") - break; - - else if ((std::string)*left == "" && (std::string)*right != "") - SWAP - - else - { - std::string format = context.config.get ("report." + mReportName + ".dateformat"); - if (format == "") - format = context.config.get ("dateformat.report"); - if (format == "") - format = context.config.get ("dateformat"); - - Date dl ((std::string)*left, format); - Date dr ((std::string)*right, format); - if (dl > dr) - SWAP - } - } - break; - - case descendingDueDate: - { - if ((std::string)*left != "" && (std::string)*right == "") - break; - - else if ((std::string)*left == "" && (std::string)*right != "") - SWAP - - else - { - std::string format = context.config.get ("report." + mReportName + ".dateformat"); - if (format == "") - format = context.config.get ("dateformat.report"); - if (format == "") - format = context.config.get ("dateformat"); - - Date dl ((std::string)*left, format); - Date dr ((std::string)*right, format); - if (dl < dr) - SWAP - } - } - break; - - case ascendingPriority: - if (((std::string)*left == "" && (std::string)*right != "") || - ((std::string)*left == "M" && (std::string)*right == "L") || - ((std::string)*left == "H" && ((std::string)*right == "L" || (std::string)*right == "M"))) - SWAP - break; - - case descendingPriority: - if (((std::string)*left == "" && (std::string)*right != "") || - ((std::string)*left == "L" && ((std::string)*right == "M" || (std::string)*right == "H")) || - ((std::string)*left == "M" && (std::string)*right == "H")) - SWAP - break; - - case ascendingPeriod: - if ((std::string)*left == "" && (std::string)*right != "") - break; - else if ((std::string)*left != "" && (std::string)*right == "") - SWAP - else if (Duration ((std::string)*left) > Duration ((std::string)*right)) - SWAP - break; - - case descendingPeriod: - if ((std::string)*left != "" && (std::string)*right == "") - break; - else if ((std::string)*left == "" && (std::string)*right != "") - SWAP - else if (Duration ((std::string)*left) < Duration ((std::string)*right)) - SWAP - break; + Date dl ((std::string)*cell_left, table->mDateFormat); + Date dr ((std::string)*cell_right, table->mDateFormat); + return dl < dr ? true : false; } } - } + break; - ++r; + case Table::descendingDate: + { + // something > nothing. + if ((std::string)*cell_left != "" && (std::string)*cell_right == "") + return true; + + // nothing < something. + else if ((std::string)*cell_left == "" && (std::string)*cell_right != "") + return false; + + else + { + Date dl ((std::string)*cell_left, table->mDateFormat); + Date dr ((std::string)*cell_right, table->mDateFormat); + return dl < dr ? false : true; + } + } + break; + + case Table::ascendingDueDate: + { + // something > nothing. + if ((std::string)*cell_left != "" && (std::string)*cell_right == "") + return false; + + // nothing < something. + else if ((std::string)*cell_left == "" && (std::string)*cell_right != "") + return true; + + else + { + std::string format = context.config.get ("report." + table->mReportName + ".dateformat"); + if (format == "") + format = context.config.get ("dateformat.report"); + if (format == "") + format = context.config.get ("dateformat"); + + Date dl ((std::string)*cell_left, format); + Date dr ((std::string)*cell_right, format); + return dl < dr ? true : false; + } + } + break; + + case Table::descendingDueDate: + { + // something > nothing. + if ((std::string)*cell_left != "" && (std::string)*cell_right == "") + return true; + + // nothing < something. + else if ((std::string)*cell_left == "" && (std::string)*cell_right != "") + return false; + + else + { + std::string format = context.config.get ("report." + table->mReportName + ".dateformat"); + if (format == "") + format = context.config.get ("dateformat.report"); + if (format == "") + format = context.config.get ("dateformat"); + + Date dl ((std::string)*cell_left, format); + Date dr ((std::string)*cell_right, format); + return dl < dr ? false : true; + } + } + break; + + case Table::ascendingPriority: + if (((std::string)*cell_left == "" && (std::string)*cell_right != "") || + ((std::string)*cell_left == "L" && ((std::string)*cell_right == "M" || (std::string)*cell_right == "H")) || + ((std::string)*cell_left == "M" && (std::string)*cell_right == "H")) + return true; + else + return false; + break; + + case Table::descendingPriority: + if (((std::string)*cell_left == "" && (std::string)*cell_right != "") || + ((std::string)*cell_left == "M" && (std::string)*cell_right == "L") || + ((std::string)*cell_left == "H" && ((std::string)*cell_right == "L" || (std::string)*cell_right == "M"))) + return true; + else + return false; + break; + + case Table::ascendingPeriod: + if ((std::string)*cell_left == "" && (std::string)*cell_right != "") + return true; + else if ((std::string)*cell_left != "" && (std::string)*cell_right == "") + return false; + else if (Duration ((std::string)*cell_left) < Duration ((std::string)*cell_right)) + return true; + break; + + case Table::descendingPeriod: + if ((std::string)*cell_left != "" && (std::string)*cell_right == "") + return false; + else if ((std::string)*cell_left == "" && (std::string)*cell_right != "") + return true; + else if (Duration ((std::string)*cell_left) < Duration ((std::string)*cell_right)) + return false; + break; + } } } + + return false; } //////////////////////////////////////////////////////////////////////////////// @@ -868,7 +822,10 @@ const std::string Table::render (int maxrows /* = 0 */, int maxlines /* = 0 */) // Only sort if necessary. if (mSortColumns.size ()) - sort (order); + { + table = this; // Substitute for 'this' in the static 'sort_compare'. + std::sort (order.begin (), order.end (), sort_compare); + } // If a non-zero maxrows is specified, then it limits the number of rows of // the table that are rendered. diff --git a/src/Table.h b/src/Table.h index b2aa82353..01d8e3cf8 100644 --- a/src/Table.h +++ b/src/Table.h @@ -35,6 +35,8 @@ class Table { + friend bool sort_compare (int, int); + public: enum just {left, center, right}; enum order {ascendingNumeric, @@ -101,7 +103,6 @@ private: const std::string formatHeader (const int, const int, const int); const std::string formatHeaderDashedUnderline (const int, const int, const int); void formatCell (const int, const int, const int, const int, const int, std::vector &, std::string&); - void sort (std::vector &); private: std::vector mColumns; diff --git a/src/tests/sorting.t b/src/tests/sorting.t index 864417cca..08110e800 100755 --- a/src/tests/sorting.t +++ b/src/tests/sorting.t @@ -28,7 +28,7 @@ use strict; use warnings; -use Test::More tests => 7; +use Test::More tests => 97; # Create the rc file. if (open my $fh, '>', 'sorting.rc') @@ -38,19 +38,146 @@ if (open my $fh, '>', 'sorting.rc') ok (-r 'sorting.rc', 'Created sorting.rc'); } -# Test sort order for most reports (including list) of: -# due+,priority-,active+,project+ -qx{../task rc:sorting.rc add due:eow priority:H project:A due:tomorrow one}; -qx{../task rc:sorting.rc add due:eow priority:H project:B due:tomorrow two}; -qx{../task rc:sorting.rc add due:eow priority:H project:C due:tomorrow three}; -qx{../task rc:sorting.rc add due:eow priority:H project:D due:tomorrow four}; -my $output = qx{../task rc:sorting.rc list}; -like ($output, qr/one.+two.+three.+four/ms, 'no active task sorting'); +# Test assorted sort orders. + +qx{../task rc:sorting.rc add priority:H project:A due:yesterday one}; +qx{../task rc:sorting.rc add priority:M project:B due:today two}; +qx{../task rc:sorting.rc add priority:L project:C due:tomorrow three}; +qx{../task rc:sorting.rc add priority:H project:C due:today four}; qx{../task rc:sorting.rc start 1}; qx{../task rc:sorting.rc start 3}; -$output = qx{../task rc:sorting.rc list}; -like ($output, qr/one.+three.+two.+four/ms, 'active task sorting'); + +# pri:H pro:C due:today four +# pri:H pro:A * due:yesterday one +# pri:M pro:B due:today two +# pri:L pro:C * due:tomorrow three + +my %tests = +( + # Single sort column. + 'priority-' => '(?:one.+four|four.+one).+two.+three', + 'priority+' => 'three.+two.+(?:one.+four|four.+one)', + 'project-' => '(?:three.+four|four.+three).+two.+one', + 'project+' => 'one.+two.+(?:three.+four|four.+three)', + 'active-' => '(?:one.+three|three.+one).+(?:two.+four|four.+two)', + 'active+' => '(?:two.+four|four.+two).+(?:one.+three|three.+one)', + 'due-' => 'three.+(?:two.+four|four.+two).+one', + 'due+' => 'one.+(?:two.+four|four.+two).+three', + 'description-' => 'two.+three.+one.+four', + 'description+' => 'four.+one.+three.+two', + + # Two sort columns. + 'priority-,project-' => 'four.+one.+two.+three', + 'priority-,project+' => 'one.+four.+two.+three', + 'priority+,project-' => 'three.+two.+four.+one', + 'priority+,project+' => 'three.+two.+one.+four', + + 'priority-,active-' => 'one.+four.+two.+three', + 'priority-,active+' => 'four.+one.+two.+three', + 'priority+,active-' => 'three.+two.+one.+four', + 'priority+,active+' => 'three.+two.+four.+one', + + 'priority-,due-' => 'four.+one.+two.+three', + 'priority-,due+' => 'one.+four.+two.+three', + 'priority+,due-' => 'three.+two.+four.+one', + 'priority+,due+' => 'three.+two.+one.+four', + + 'priority-,description-' => 'one.+four.+two.+three', + 'priority-,description+' => 'four.+one.+two.+three', + 'priority+,description-' => 'three.+two.+one.+four', + 'priority+,description+' => 'three.+two.+four.+one', + + 'project-,priority-' => 'four.+three.+two.+one', + 'project-,priority+' => 'three.+four.+two.+one', + 'project+,priority-' => 'one.+two.+four.+three', + 'project+,priority+' => 'one.+two.+three.+four', + + 'project-,active-' => 'three.+four.+two.+one', + 'project-,active+' => 'four.+three.+two.+one', + 'project+,active-' => 'one.+two.+three.+four', + 'project+,active+' => 'one.+two.+four.+three', + + 'project-,due-' => 'three.+four.+two.+one', + 'project-,due+' => 'four.+three.+two.+one', + 'project+,due-' => 'one.+two.+three.+four', + 'project+,due+' => 'one.+two.+four.+three', + + 'project-,description-' => 'three.+four.+two.+one', + 'project-,description+' => 'four.+three.+two.+one', + 'project+,description-' => 'one.+two.+three.+four', + 'project+,description+' => 'one.+two.+four.+three', + + 'active-,priority-' => 'one.+three.+four.+two', + 'active-,priority+' => 'three.+one.+two.+four', + 'active+,priority-' => 'four.+two.+one.+three', + 'active+,priority+' => 'two.+four.+three.+one', + + 'active-,project-' => 'three.+one.+four.+two', + 'active-,project+' => 'one.+three.+two.+four', + 'active+,project-' => 'four.+two.+three.+one', + 'active+,project+' => 'two.+four.+one.+three', + + 'active-,due-' => 'three.+one.+(?:four.+two|two.+four)', + 'active-,due+' => 'one.+three.+(?:four.+two|two.+four)', + 'active+,due-' => '(?:four.+two|two.+four).+three.+one', + 'active+,due+' => '(?:four.+two|two.+four).+one.+three', + + 'active-,description-' => 'three.+one.+two.+four', + 'active-,description+' => 'one.+three.+four.+two', + 'active+,description-' => 'two.+four.+three.+one', + 'active+,description+' => 'four.+two.+one.+three', + + 'due-,priority-' => 'three.+four.+two.+one', + 'due-,priority+' => 'three.+two.+four.+one', + 'due+,priority-' => 'one.+four.+two.+three', + 'due+,priority+' => 'one.+two.+four.+three', + + 'due-,project-' => 'three.+four.+two.+one', + 'due-,project+' => 'three.+two.+four.+one', + 'due+,project-' => 'one.+four.+two.+three', + 'due+,project+' => 'one.+two.+four.+three', + + 'due-,active-' => 'three.+(?:four.+two|two.+four).+one', + 'due-,active+' => 'three.+(?:four.+two|two.+four).+one', + 'due+,active-' => 'one.+(?:four.+two|two.+four).+three', + 'due+,active+' => 'one.+(?:four.+two|two.+four).+three', + + 'due-,description-' => 'three.+two.+four.+one', + 'due-,description+' => 'three.+four.+two.+one', + 'due+,description-' => 'one.+two.+four.+three', + 'due+,description+' => 'one.+four.+two.+three', + + 'description-,priority-' => 'two.+three.+one.+four', + 'description-,priority+' => 'two.+three.+one.+four', + 'description+,priority-' => 'four.+one.+three.+two', + 'description+,priority+' => 'four.+one.+three.+two', + + 'description-,project-' => 'two.+three.+one.+four', + 'description-,project+' => 'two.+three.+one.+four', + 'description+,project-' => 'four.+one.+three.+two', + 'description+,project+' => 'four.+one.+three.+two', + + 'description-,active-' => 'two.+three.+one.+four', + 'description-,active+' => 'two.+three.+one.+four', + 'description+,active-' => 'four.+one.+three.+two', + 'description+,active+' => 'four.+one.+three.+two', + + 'description-,due-' => 'two.+three.+one.+four', + 'description-,due+' => 'two.+three.+one.+four', + 'description+,due-' => 'four.+one.+three.+two', + 'description+,due+' => 'four.+one.+three.+two', + + # Four sort columns. + 'active+,project+,due+,priority+' => 'two.+four.+one.+three', + 'project+,due+,priority+,active+' => 'one.+two.+four.+three', +); + +for my $sort (sort keys %tests) +{ + my $output = qx{../task rc:sorting.rc rc.report.list.sort:${sort} list}; + like ($output, qr/$tests{$sort}/ms, "sort:${sort}"); +} # Cleanup. unlink 'pending.data';