5
\$\begingroup\$

I am using this merge sort function to sort table rows by Date, Letter, and Number. It works, but it is super messy and hard for others to read.

The dates are being sorted in two formats YYYY-MM-DD and MM/DD/YYYY which is why there are if statements regulating each format.

And the reason I am differentiating between letters and numbers is because when using comparison operators with stringified numbers, it would not process them correctly.

If someone could help me make this code more readable, that would be great, thanks :)

Merge Function:

const Merge = (left_arr, right_arr, is_date) => {
    let arr = [];
    while (left_arr.length && right_arr.length) {
        // Check for undefined variables
        let variable1 = (left_arr[0].getElementsByTagName("td")[sortIndex].innerText == undefined) ? "" : left_arr[0].getElementsByTagName("td")[sortIndex].innerText;
        let variable2 = (right_arr[0].getElementsByTagName("td")[sortIndex].innerText == undefined) ? "" : right_arr[0].getElementsByTagName("td")[sortIndex].innerText;
        if (is_date) {
            // Get the indexer for the dates
            let indexer1 = (variable1.charAt(4) == "-") ? "-" : "/";
            let indexer2 = (variable1.charAt(4) == "-") ? "-" : "/";

            let var1year;
            let var2year;
            if (indexer1 == "-") {
                var1year = variable1.substring(0, 4);
            } else {
                var1year = variable1.substring(variable1.indexOf(indexer1, variable1.indexOf(indexer1) + 1) + 1, variable1.indexOf(indexer1, variable1.indexOf(indexer1) + 1) + 5);
            }

            if (indexer2 == "-") {
                var2year = variable2.substring(0, 4);
            } else {
                var2year = variable2.substring(variable2.indexOf(indexer2, variable2.indexOf(indexer2) + 1) + 1, variable2.indexOf(indexer2, variable2.indexOf(indexer2) + 1) + 5);
            }

            if (var1year == var2year) {
                // Month
                let var1month;
                let var2month;
                if (indexer1 == "-") {
                    var1month = variable1.substring(variable1.indexOf(indexer1) + 1, variable1.indexOf(indexer1) + 3);
                } else {
                    var1month = variable1.substring(0, variable1.indexOf(indexer1));
                }

                if (indexer2 == "-") {
                    var2month = variable2.substring(variable2.indexOf(indexer2) + 1, variable2.indexOf(indexer2) + 3);
                } else {
                    var2month = variable2.substring(0, variable2.indexOf(indexer2));
                }

                if (var1month == var2month) {
                    // Day
                    let var1day;
                    let var2day;
                    if (indexer1 == "-") {
                        var1day = variable1.substring(variable1.indexOf(indexer1, variable1.indexOf(indexer1) + 1) + 1, variable1.indexOf(indexer1, variable1.indexOf(indexer1) + 1) + 3);
                    } else {
                        var1day = variable1.substring(variable1.indexOf(indexer1) + 1, variable1.indexOf(indexer1, variable1.indexOf(indexer1) + 1));
                    }

                    if (indexer2 == "-") {
                        var2day = variable2.substring(variable2.indexOf(indexer2, variable2.indexOf(indexer2) + 1) + 1, variable2.indexOf(indexer2, variable2.indexOf(indexer2) + 1) + 3);
                    } else {
                        var2day = variable2.substring(variable2.indexOf(indexer2) + 1, variable2.indexOf(indexer2, variable2.indexOf(indexer2) + 1));
                    }

                    if (var1day == var2day) {
                        arr.push(right_arr.shift());
                    } else if (Number(var1day) < Number(var2day)) {
                        arr.push(left_arr.shift());
                    } else if (Number(var1day) > Number(var2day)) {
                        arr.push(right_arr.shift());
                    }
                } else if (Number(var1month) < Number(var2month)) {
                    arr.push(left_arr.shift());
                } else if (Number(var1month) > Number(var2month)) {
                    arr.push(right_arr.shift());
                }
            } else if (var1year < var2year) {
                arr.push(left_arr.shift());
            } else if (var1year > var2year) {
                arr.push(right_arr.shift());
            }
        } else {
            if (isNaN(variable1) && isNaN(variable2)) {
                // Letter
                let len = (variable1.length <= variable2.length ? variable1.length : variable2.length);
                for (let i = 0; i < len; i++) {
                    if (variable1[i] < variable2[i]) {
                        arr.push(left_arr.shift());
                        break;
                    } else if (variable1[i] > variable2[i]) {
                        arr.push(right_arr.shift());
                        break;
                    } else if (variable1[i] == variable2[i]) {
                        if (i == len - 1) {
                            arr.push(right_arr.shift());
                            break;
                        } else {
                            continue;
                        }
                    }
                }
            } else {
                // Number
                variable1 = Number(variable1);
                variable2 = Number(variable2);
                if (variable1 < variable2) {
                    arr.push(left_arr.shift());
                } else {
                    arr.push(right_arr.shift());
                }
            }
        }
    }
    return [...arr, ...left_arr, ...right_arr];
}

MergeSort Function:

const MergeSort = (array, is_date) => {
    const half = array.length / 2;
    if (array.length < 2) {
        return array;
    }
    const left = array.splice(0, half);
    return Merge(MergeSort(left, is_date), MergeSort(array, is_date), is_date);
}

SortTable Function:

const SortTable = (tablestr, sort_index, currentElement) => {
    var currentTable = document.getElementsByClassName(`${tablestr}-table-body`)[0];
    let wishdir = "down";
    let isDate = false;
    let rowTitle = currentElement.innerHTML.toLowerCase();

    if (sorted == true) {
        // change direction if clicked again
        wishdir = "up";
        sorted = false;
    } else {
        sorted = true;
    }

    // Global variable used elsewhere
    currentSortIndex = sort_index;
    sortIndex = sort_index;

    // Checks for specific date keywords in row title
    if (rowTitle.includes("date")) {
        isDate = true;
    }

    // Convert the collection into an array
    const init_array = [...currentTable.rows];

    // Replace characters
    for (let i = 0; i < init_array.length; i++) {
        init_array[i].innerText.replace("-", "");
        init_array[i].innerText.replace("(", "");
        init_array[i].innerText.replace(")", "");
        init_array[i].innerText.replace(" ", "");
    }

    // Sort
    const result = MergeSort(init_array, isDate);

    // Remove and then add the rows back in
    currentTable.innerHTML = "";
    if (wishdir == "down") {
        for (let c = 0; c < result.length; c++) {
            currentTable.appendChild(result[c]);
        }
    } else if (wishdir == "up") {
        result.reverse();
        for (let c = 0; c < result.length; c++) {
            currentTable.appendChild(result[c]);
        }
    }
}
```
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Separate the date compairison from the merge sort logic. Merge() should accept a compare function as a parameter. \$\endgroup\$ May 19, 2021 at 18:55

1 Answer 1

5
\$\begingroup\$

Code Style

  • You seem to use var, let, and const randomly. Pick a style and stick with it.
  • The Javascript community generally uses camelCase for both variable names and function names. While it's not extremely important, I would recommend following suit.
  • Always use === when comparing two values. The only exception should be when you want to use == null as a shorthand to check for both null and undefined.
  • Any reason you're doing string.charAt(index) instead of string[index]? You can just use the simpler option here unless you have a good reason not to do so.

You have a lot of long lines that could be broken up over multiple lines (either by just adding new lines in the right places or by using intermediate variables)

Take this for example:

var1year = variable1.substring(variable1.indexOf(indexer1, variable1.indexOf(indexer1) + 1) + 1, variable1.indexOf(indexer1, variable1.indexOf(indexer1) + 1) + 5);

And comopare it with this:

var1year = variable1.substring(
  variable1.indexOf(indexer1, variable1.indexOf(indexer1) + 1) + 1,
  variable1.indexOf(indexer1, variable1.indexOf(indexer1) + 1) + 5
);

Here's another minor improvement - there's a couple places where you have code such as the following:

if (conditionA) {
  ...
} else {
  if (conditionB) {
    ...
  } else {
    ...
  }
}

This could be rewritten as this:

if (conditionA) {
  ...
} else if (conditionB) {
  ...
} else {
  ...
}

Minor Simplifications

Here are a handful of simplifications you can do, using different language features you might not be aware of.

This:

let variable1 = (leftArr[0].getElementsByTagName("td")[sortIndex].innerText == undefined) ? "" : leftArr[0].getElementsByTagName("td")[sortIndex].innerText;

can be written a lot simpler by using the nullish coalescing operator (??)

let variable1 = leftArr[0].getElementsByTagName("td")[sortIndex].innerText ?? ""

this:

let len = (variable1.length <= variable2.length ? variable1.length : variable2.length);

Can be simplified by using Math.min()

let len = Math.min(variable1.length, variable2.length);

You really like using .substring() everywhere to split apart the date string. Try using string.split() instead. With a little destructuring, a whole lot of code can be greatly simplified.

> let [year, month, day] = '12/34/5678'.split('/');
> console.log(year, month, day)
12 34 5678

This chunk of code isn't doing anything:

initArray[i].innerText.replace("-", "");

Strings are immutable - .replace() does not modify the string, instead, it returns a new string with certain characters replaced by other characters.

I presume you're building a custom sorting function here simply for the practice? In a real-world application, we would just use the built-in sort() function.

Overall design

Your sort function is doing too many things, and is not general-purpose at all. (e.g. you've got UI code mixed into the algorithm). Additionally, all parameters to this function should be made explicit - i.e. don't force your caller to correctly set a global sortIndex variable before calling mergeSort - just pass sortIndex into the function instead as an explicit parameter. In general, do what you can to avoid global variables - it makes for difficult-to-follow logic (it certainly caught me off guard when I realized that this function relied on a global variable to function properly). When you have to use a global variable, make it explicit by assigning or reading from globalThis. e.g. globalThis.myGlobal = 2; console.log(globalThis.myGlobal).

To make your sorting function more general purpose, just have it take a function parameter that tells the sort function how you want to sort the given array.

After applying all of these suggestions above, I made the following rewrite. (This by no means is perfect - there's a lot more I could add to simplify it further, nor is it tested, but it gives you a taste of how this advice can be applied).

const strIsNumb = str => !isNaN(Number(str));

function dateStrToTimestamp(dateStr) {
  let divider = dateStr.includes("-") ? "-" : "/";
  if (divider === "-") {
    let [year, month, day] = dateStr.split(divider);
    return new Date(year, month, day).valueOf();
  } else {
    let [month, day, year] = dateStr.split(divider);
    return new Date(year, month, day).valueOf();
  }
}

const merge = (leftArr, rightArr, calcSortKey) => {
  let arr = [];
  while (leftArr.length && rightArr.length) {
    let variable1 = calcSortKey(leftArr[0]);
    let variable2 = calcSortKey(rightArr[0]);

    if (variable1 < variable2) {
      arr.push(leftArr.shift());
    } else {
      arr.push(rightArr.shift());
    }
  }
  return [...arr, ...leftArr, ...rightArr];
};

const mergeSort = (array, calcSortKey) => {
  const half = array.length / 2;
  if (array.length < 2) {
    return array;
  }
  const left = array.splice(0, half);
  return merge(mergeSort(left, calcSortKey), mergeSort(array, calcSortKey), calcSortKey);
};

// ...

// Example usage of the new sort function:
const calcSortKey = cellEl => {
  const cellText = cellEl.getElementsByTagName("td")[sortIndex].innerText ?? "";
  if (isDate) return dateStrToTimestamp(cellText);
  if (strIsNumb(cellText)) return Number(cellText);
  return cellText;
};

let result = mergeSort(initArray, calcSortKey);

In this rewrite, I'm passing in a function to mergeSort() that tells mergeSort how to convert a given array value into something that can be sorted using javascript's <, >, and === operators. This gives the mergeSort() function flexibility to be used in a wide variety of places. For your specific scenario, you're wanting to sort arrays of elements. So, this calcSortKey function will take each element, look-up the corresponding text data, then parse it if needed.

I'm using the following algorithm to parse the text data:

  • If the string contains a number, turn it into a number
  • If it contains a string, just return it. (Strings compare just fine in Javascript when used with <, >, and === operators - no need to compare character-by-character like you were doing)
  • If we're parsing a date column, always parse the string as a date, then turn that date into a timestamp, as that's comparable with the <, >, and === operators.
\$\endgroup\$
1
  • \$\begingroup\$ This is all very useful, thank you :) \$\endgroup\$
    – Gunnarhawk
    May 20, 2021 at 16:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.