Reorder columns in an HTML form

Question

I have the following HTML structure for a form (simplified):

<form class="form">
 <div class="formRow">
 <div class="formCol" data-id="First">First</div>
 <div class="formCol" data-id="Last">Last</div>
 <div class="formCol" data-id="Whatever">Whatever</div>
 <div class="clear"></div>
 </div>
 <div class="formRow">
 <div class="formCol" data-id="City">City</div>
 <div class="clear"></div>
 </div>
 <div class="formRow">
 <div class="formCol" data-id="Country">Country</div>
 <div class="formCol" data-id="Phone">Phone</div>
 <div class="clear"></div>
 </div>
</form>

As I don't have any control over the output, I need to reorder the columns by using JavaScript:

const form = (() => {
 const colClassName = 'formCol';
 const rowClassName = 'formRow';
 const clearClassName = 'clear';
 function createRow(elForm) {
 const elRow = document.createElement('div');
 const elClear = document.createElement('div');
 elRow.className = rowClassName;
 elClear.className = clearClassName;
 elRow.appendChild(elClear);
 elForm.insertAdjacentElement('beforeEnd', elRow);
 return elRow;
 }
 function moveColumn(elForm, elRow, colName) {
 const elCol = elForm.querySelector(`.${colClassName}[data-id='${colName}']`);
 elRow.insertAdjacentElement('afterbegin', elCol);
 }
 function removeEl(el) {
 el.parentNode.removeChild(el);
 }
 function reorder(elForm, order) {
 const elRows = elForm.querySelectorAll(`.${rowClassName}`);
 const maxRows = Math.max(elRows.length, order.length);
 let elCol;
 for (var i = 0; i < maxRows; i++) {
 if (order[i]) {
 if (!elRows[i]) {
 elRows[i] = createRow(elForm);
 }
 if (!Array.isArray(order[i])) {
 moveColumn(elForm, elRows[i], order[i]);
 } else {
 for (j = order[i].length - 1; j >= 0; j--) {
 moveColumn(elForm, elRows[i], order[i][j]);
 };
 }
 } else {
 elCol = elRows[i].querySelector(`.${colClassName}`);
 if (!elCol) {
 removeEl(elRows[i]);
 }
 }
 }
 }
 return {
 reorder,
 // other functions for form
 };
})();
form.reorder(document.querySelector('.form'), [
 ['Last', 'City'],
 'First',
 'Country',
 'Phone',
]);

It does the following:

• Reorders the elements as specified in the array (multiple columns).
• Does not delete columns if they are not in the array (see column "Whatever").
• Creates new rows if necessary and deletes empty rows

While that works as is (see https://jsfiddle.net/6cy4mjvd/), I don't know if there are ways to improve readability / code in general, e.g. it feels wrong how I currently create new rows and modify elRows and also that I pass elForm to all helper functions... and probably a few other spots.

Btw. I plan on adding other functions other than reorder to modify the form. That's why I return an object (line // other functions for form) in the function.

• woxxom: I don't understand why 'Last' and 'City' are inside an array so I would simply inject CSS style that sets form's display to flex and column's order to a value derived from the element's index in the array.
• mrksbnch: @wOxxOm In this case, 'Last' and 'City' are in an array because they should be displayed in two columns. Using display: flex on the form would allow me to reorder the rows, but I also (sometimes) want to move the columns to a completely different row. That's why I only used display: flex on the rows (see jsfiddle). Using JavaScript is also sightly easier to use (IMO) than having to hard code the order in CSS, e.g. I can display the form in 3 columns with reorder(el, ['Phone', 'Country', 'City'], ['First', 'Last', 'Whatever']).
• Gerrit0: Based on how the reorder function is called, I expected to see Phone in the fourth row. In the fiddle, it is left in the third row. Am I misunderstanding something or is this a bug?
• mrksbnch: @Gerrit0 "Phone" should be in the fourth column and actually is for me in Chrome/Safari at least. I should probably check FF... May I ask what browser you are using?

Answer 1

@demrks please take a look at the code below. It's a result of multiple refactoring iterations.

While the original code does the job, I find it very hard to read. Hopefully, you agree that the improved version reads better, even though there's very little new in it. Many developers including myself believe that readability is a super important aspect of coding because it directly affects complexity, and thus correctness of the code.

Here's the list of techniques I applied (in order of importance as I understand things):

---

Naming

Name things properly and do not abbreviate words. createNewRowElementAndInsertIntoForm(...) is definitely not a short name, but it's describing what it does with 100% precision.

Similarly, original elCol transformed into columnElement. It may be funny, but first seeing elCol I thought I'm reading code in Spanish. :)

Please never never never use the i, j, c, and similar. They are absolutely meaningless. Yes, the other dev "may understand" it, but you don't want anyone to guess what the code does. Spell things out, in most of the modern editors it's a matter of hitting <F2>.

Temporary (削除) variables (削除ここまで) constants

Do not hesitate to introduce reusable temporary constants with descriptive names. They make cryptic conditions if (order[i]) or if (!elRows[i]) relay the intent of the writer.

Avoid branching when possible, or minimize its "scope"

An unnecessary if-then-else for processing array/non-array orders may be generalized, if we normalize the array first.

Was:

if (!Array.isArray(order[i])) {
 moveColumn(elForm, elRows[i], order[i]);
} else {
 for (j = order[i].length - 1; j >= 0; j--) {
 moveColumn(elForm, elRows[i], order[i][j]);
 };
}

Became (longer because of the name, but the branching is almost eliminated by a ternary operator that cleans the input):

const normalizerRowPrescribedOrder =
 Array.isArray(rowPrescribedOrder) ? rowPrescribedOrder : [rowPrescribedOrder];
for (let columnIndex = normalizerRowPrescribedOrder.length - 1; columnIndex >= 0; columnIndex--) {
 findColumnByNameAndMoveToBeginningOfRow(formElement, currentRowElement, normalizerRowPrescribedOrder[columnIndex]);
};

"Newspaper"

Make code read top to bottom aka newspaper style (reorder() is the entry point which invokes createNewRowElementAndInsertIntoForm(), therefore reorder() should go first). Forcing the reader to jump around the code is distracting from the act of active reading, therefore leading to avoidable complications.

---

Result

const form = (() => {
 const columnClassName = 'formCol';
 const rowClassName = 'formRow';
 const clearClassName = 'clear';
 function reorderForm(formElement, prescribedOrder) {
 const rowElementList = formElement.querySelectorAll(`.${rowClassName}`);
 const maxRowLength = Math.max(rowElementList.length, prescribedOrder.length);
 for (let rowIndex = 0; rowIndex < maxRowLength; rowIndex++) {
 const currentRowElement = rowElementList[rowIndex] || createNewRowElementAndInsertIntoForm(formElement);
 const rowPrescribedOrder = prescribedOrder[rowIndex];
 if (!rowPrescribedOrder) {
 const columnElement = currentRowElement.querySelector(`.${columnClassName}`);
 if (!columnElement) {
 removeRowElement(currentRowElement);
 }
 } else {
 const normalizedRowPrescribedOrder = Array.isArray(rowPrescribedOrder) ? rowPrescribedOrder : [rowPrescribedOrder];
 for (let columnIndex = normalizedRowPrescribedOrder.length - 1; columnIndex >= 0; columnIndex--) {
 findColumnByNameAndMoveToBeginningOfRow(formElement, currentRowElement, normalizedRowPrescribedOrder[columnIndex]);
 };
 }
 }
 }
 function createNewRowElementAndInsertIntoForm(formElement) {
 const rowElement = document.createElement('div');
 rowElement.className = rowClassName;
 const clearElement = document.createElement('div');
 clearElement.className = clearClassName;
 rowElement.appendChild(clearElement);
 formElement.insertAdjacentElement('beforeEnd', rowElement);
 return rowElement;
 }
 function findColumnByNameAndMoveToBeginningOfRow(formElement, rowElement, columnName) {
 const columnElement = formElement.querySelector(`.${columnClassName}[data-id='${columnName}']`);
 rowElement.insertAdjacentElement('afterbegin', columnElement);
 }
 function removeRowElement(rowElement) {
 rowElement.parentNode.removeChild(rowElement);
 }
 return {
 reorderForm,
 // other functions for form
 };
})();
form.reorderForm(document.querySelector('.form'), [
 ['Last', 'City'],
 'First',
 'Country',
 'Phone',
]);

P.S.

It's really nice to see how one uses native API instead of jQuery!

• mrksbnch: Thanks, the names make it indeed easier to read (although they look a little log at first). I only used jQuery a few years back because it made it easier to work with old IE versions :)