Addressed comments from code review of #822

platform/features/table/src/controllers/MCTTableController.js

@@ -286,21 +286,6 @@ define(
             }
         };
 
-        /**
-         * Given a value, if it is a number, or a string representation of a
-         * number, then return a number representation. Otherwise, return
-         * the original value. It's a little more robust than using just
-         * Number() or parseFloat, or isNaN in isolation, all of which are
-         * fairly inconsistent in their results.
-         * @param value The value to return as a number.
-         * @returns {*} The value cast to a Number, or the original value if
-         * a Number representation is not possible.
-         */
-        MCTTableController.prototype.toNumber = function (value){
-            var val = !isNaN(Number(value)) && !isNaN(parseFloat(value)) ? Number(value) : value;
-            return val;
-        };
-
         /**
          * @private
          */
@@ -310,17 +295,14 @@ define(
                 sortKey = this.$scope.sortColumn;
 
             function binarySearch(searchArray, searchElement, min, max){
-                var sampleAt = Math.floor((max - min) / 2) + min,
+                var sampleAt = Math.floor((max - min) / 2) + min;
-                    valA,
+
-                    valB;
                 if (max < min) {
                     return min; // Element is not in array, min gives direction
                 }
 
-                valA = self.toNumber(searchElement[sortKey].text);
+                switch(self.sortComparator(searchElement[sortKey].text,
-                valB = self.toNumber(searchArray[sampleAt][sortKey].text);
+                    searchArray[sampleAt][sortKey].text)) {
-
-                switch(self.sortComparator(valA, valB)) {
                     case -1:
                         return binarySearch(searchArray, searchElement, min,
                             sampleAt - 1);
@@ -358,8 +340,34 @@ define(
          */
         MCTTableController.prototype.sortComparator = function (a, b) {
             var result = 0,
-                sortDirectionMultiplier;
+                sortDirectionMultiplier,
+                numberA,
+                numberB;
+            /**
+             * Given a value, if it is a number, or a string representation of a
+             * number, then return a number representation. Otherwise, return
+             * the original value. It's a little more robust than using just
+             * Number() or parseFloat, or isNaN in isolation, all of which are
+             * fairly inconsistent in their results.
+             * @param value The value to return as a number.
+             * @returns {*} The value cast to a Number, or the original value if
+             * a Number representation is not possible.
+             */
+            function toNumber (value){
+                var val = !isNaN(Number(value)) && !isNaN(parseFloat(value)) ? Number(value) : value;
+                return val;
+            }
 
+            numberA = toNumber(a);
+            numberB = toNumber(b);
+
+            //If they're both numbers, then compare them as numbers
+            if (typeof numberA === "number" && typeof numberB === "number") {
+                a = numberA;
+                b = numberB;
+            }
+
+            //If they're both strings, then ignore case
             if (typeof a === "string" && typeof b === "string") {
                 a = a.toLowerCase();
                 b = b.toLowerCase();
@@ -396,14 +404,7 @@ define(
             }
 
             return rowsToSort.sort(function (a, b) {
-                //If the values to compare can be compared as
+                return self.sortComparator(a[sortKey].text, b[sortKey].text);
-                // numbers, do so. String comparison of number
-                // values can cause inconsistencies
-
-                var valA = self.toNumber(a[sortKey].text),
-                    valB = self.toNumber(b[sortKey].text);
-
-                return self.sortComparator(valA, valB);
             });
         };
 

platform/features/table/test/controllers/MCTTableControllerSpec.js

@@ -180,20 +180,6 @@ define(
                         expect(sortedRows[2].col2.text).toEqual('abc');
                     });
 
-                    it('converts number strings to numbers', function () {
-                        var val1 = "",
-                            val2 = "1",
-                            val3 = "2016-04-05 18:41:30.713Z",
-                            val4 = "1.1",
-                            val5 = "8.945520958175627e-13";
-
-                        expect(controller.toNumber(val1)).toEqual("");
-                        expect(controller.toNumber(val2)).toEqual(1);
-                        expect(controller.toNumber(val3)).toEqual("2016-04-05 18:41:30.713Z");
-                        expect(controller.toNumber(val4)).toEqual(1.1);
-                        expect(controller.toNumber(val5)).toEqual(8.945520958175627e-13);
-                    });
-
                     it('correctly sorts rows of differing types', function () {
                         mockScope.sortColumn = 'col2';
                         mockScope.sortDirection = 'desc';
@@ -224,7 +210,24 @@ define(
                         expect(sortedRows[sortedRows.length-1].col2.text).toEqual('');
                     });
 
-                    describe('Adding new rows', function() {
+                    describe('The sort comparator', function () {
+                        it('Correctly sorts different data types', function () {
+                            var val1 = "",
+                                val2 = "1",
+                                val3 = "2016-04-05 18:41:30.713Z",
+                                val4 = "1.1",
+                                val5 = "8.945520958175627e-13";
+                            mockScope.sortDirection = "asc";
+
+                            expect(controller.sortComparator(val1, val2)).toEqual(-1);
+                            expect(controller.sortComparator(val3, val1)).toEqual(1);
+                            expect(controller.sortComparator(val3, val2)).toEqual(1);
+                            expect(controller.sortComparator(val4, val2)).toEqual(1);
+                            expect(controller.sortComparator(val2, val5)).toEqual(1);
+                        });
+                    });
+
+                    describe('Adding new rows', function () {
                         var row4,
                             row5,
                             row6;