1
\$\begingroup\$

I'm building a page in LWC this page consist of a table, you can search, sort and paginate with this table.

enter image description here

When the first element is selected: enter image description here

when the last element is selected: enter image description here

When the middle element is selected: enter image description here

Here's pagination HTML code:

<div class="pageButtons">
        <template if:true={buttons} for:each={buttons} for:item="button" for:index="index">
            <template if:true={button.isVisible}>
                <lightning-button key={button.value} variant="Neutral" if:true={button.isVisible} disabled={button.isDisabled} label={button.value} class="pageButton" onclick={paginate}></lightning-button>
            </template>
            <template if:true={button.isPlaceHolder}>
                <lightning-formatted-rich-text key={button.value} value="..."></lightning-formatted-rich-text>
            </template>
        </template>
    </div> 

Here's the JS code:

//For initial load
const getButtonData = (pageSize) => {
    pageSize = pageSize + 1;
    let buttons = [];
    buttons.push(add('First', true, true, false));
    buttons.push(add('Previous', true, true, false));
    for (let i = 1; i <= pageSize; i++) {
        if (pageSize > 10) {
            if (i < 6) {
                buttons.push(add(i, i == 1 ? true : false, true, false));
            }
            else if (pageSize >= 6 && i == 6) {
                buttons.push(add(i, i == 1 ? true : false, pageSize > 6 ? false : true, pageSize > 6 ? true : false));
            }
            else if (i == pageSize) {
                buttons.push(add(i, false, true, false));
            }
        }
        else {
            buttons.push(add(i, i == 1 ? true : false, true, false));
        }
    }
    buttons.push(add('Next', false, true, false));
    buttons.push(add('Last', false, true, false));
    return buttons;
}

//For onclick of buttons
const getButtonDataByPage = (pageSize, currentPage) => {
    pageSize = pageSize + 1;
    let buttons = [];
    let isFirstPrev = currentPage == 1 ? true : false;
    buttons.push(add('First', isFirstPrev, true, false));
    buttons.push(add('Previous', isFirstPrev, true, false));
    if (pageSize > 10) {
        for (const i of getNextElements(currentPage, pageSize)) {
            buttons.push(
                add(
                    i.value,
                    i.value == currentPage,
                    i.isEllipses ? false : true,
                    i.isEllipses
                )
            );
        }
    }
    else {
        for (let i = 1; i <= pageSize; i++) {
            buttons.push(add(i, i == currentPage ? true : false, true, false));
        }
    }
    let isNextLast = currentPage == pageSize ? true : false;
    buttons.push(add('Next', isNextLast, true, false));
    buttons.push(add('Last', isNextLast, true, false));
    return buttons;
}

function add(value, isDisabled, isVisible, isPlaceHolder) {
    return {
        value : value,
        isDisabled: isDisabled,
        isVisible : isVisible,
        isPlaceHolder : isPlaceHolder
    };
}

//To get the index array along with ellipses 
function getNextElements(currentPage, pageSize) {
    let index = [];
    // Is first element
    if (currentPage == 1) {
        for (let j = 1; j <= pageSize; j++) {
            if (j < 6) {
                index.push({value : j, isEllipses: false});
            }
            else if (pageSize >= 6 && j == 6) {
                index.push({value : j, isEllipses: true});
            }
            else if (j == pageSize) {
                index.push({value : j, isEllipses: false});
            }
        }
    }
    // Is last element
    else if (currentPage == pageSize) {
        let lastColumn = (pageSize) - 5;
        for (let j = 1; j <= pageSize ; j++) {
            if (j > lastColumn) {
                index.push({value : j, isEllipses: false});
            }
            else if (j == lastColumn) {
                index.push({value : j, isEllipses: true});
            }
            else if (j == 1) {
                index.push({value : j, isEllipses: false});
            }
        }
    }
    // Is middle element
    else if (currentPage !== 1 || currentPage !== pageSize) {
        let lastColumn = (pageSize) - 5;
        let isCloseToFirstElement = (1 + currentPage <= 6) ? true : false;
        let isCloseToLastElement = ((pageSize - currentPage) < 5) ? true : false;
        if (isCloseToFirstElement) {
            for (let j = 1; j <= pageSize; j++) {
                if (j < 6) {
                    index.push({value : j, isEllipses: false});
                }
                else if (pageSize >= 6 && j == 6) {
                    index.push({value : j, isEllipses: true});
                }
                else if (j == pageSize) {
                    index.push({value : j, isEllipses: false});
                }
            }
        }
        if (isCloseToLastElement) {
            for (let j = 1; j <= pageSize ; j++) {
                if (j > lastColumn) {
                    index.push({value : j, isEllipses: false});
                }
                else if (j == lastColumn) {
                    index.push({value : j, isEllipses: true});
                }
                else if (j == 1) {
                    index.push({value : j, isEllipses: false});
                }
            }
        }
        if (!isCloseToLastElement && !isCloseToFirstElement) {
            for (let i = 1; i <= pageSize; i++) {
                if (i == 1) {
                    index.push({value : 1, isEllipses: false});
                }
                else if ((i !== 1 && i !== pageSize) && i == currentPage) {
                    index.push({value : currentPage - 2, isEllipses: true});
                    index.push({value : currentPage - 1, isEllipses: false});
                    index.push({value : currentPage, isEllipses: false});
                    index.push({value : currentPage + 1, isEllipses: false});
                    index.push({value : currentPage + 2, isEllipses: true});
                }
                else if (i == pageSize) {
                    index.push({value : i, isEllipses: false});
                }
            }
        }
    }
    return index;
}

export {getButtonData, getButtonDataByPage};

I know the JS can be optimized a bit better I'm just not sure how.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to CodeReview@SE. As is, the title of your question is too generic to be helpful. Heed How do I ask a Good Question? \$\endgroup\$
    – greybeard
    Commented Mar 10, 2020 at 8:27

1 Answer 1

2
\$\begingroup\$

Method Names

The most method names do not describe what the method really does and what the method name implies.

  • getButtonData let me imply that it returns information about one button
  • getButtonDataByPage the same like getButtonData
  • add implies that something gets added to a collection

In generell method names that start with get are known as getter and that they return values of an object that has no or less computation. The methods you provide starts with get but relay heavily on computation and do not return just a value.

Names that would better fit are: buildInitialButtonList, buildNavigatedButtonList and createButton.

Hard to Read

if (pageSize > 10) {
   if (i < 6) {
       buttons.push(add(i, i == 1 ? true : false, true, false));
   }
   else if (pageSize >= 6 && i == 6) {
       buttons.push(add(i, i == 1 ? true : false, pageSize > 6 ? false : true, >pageSize > 6 ? true : false));
   }
   else if (i == pageSize) {
       buttons.push(add(i, false, true, false));
   }
}

The code smells boolean flags and magic numbers make your code hard to read an to maintain.

Imagine you want to show only 3 instead 6 buttons: Alone in this small snipped you have to touch the code 5 times and in the whole code you provide 12 times.

Many Conditions

Beside the code smells you have many conditions. The 3 lines below contain 4 conditions:

else if (pageSize >= 6 && i == 6) {
   buttons.push(add(i, i == 1 ? true : false, pageSize > 6 ? false : true, pageSize > 6 ? true : false));
}

Simplify Conditions

Some conditions follow the semantic of condition ? true : false which is the same as condition. For instance:

buttons.push(add(i, i == 1 ? true : false, pageSize > 6 ? false : true, pageSize > 6 ? true : false))

Is the same as

buttons.push(add(i, i == 1, pageSize > 6, pageSize > 6))

Useless Conditions

Let's analyze getButtonData and the same will be apply to getButtonDataByPage.

else if (... && i == 6) {
   buttons.push(add(i, i == 1 ? true : false, ..., ...));
}

We first check if (i == 6) and if this is true we check if i == 1 which will always be false:

else if (... && i == 6) {
    buttons.push(add(i, false, ..., ...));
}

if (pageSize > 10) {
   if (...) {/* ... */}
   else if (pageSize >= 6 && i == 6) {
       buttons.push(add(..., ..., pageSize <= 6, pageSize > 6));
   }
}

First we make sure that pageSize > 10 and than we check if pageSize >= 6, pageSize <= 6 and pageSize > 6. Since we already know that pageSize is greater than 10 we can simplify to:

if (pageSize > 10) {
    if (...) {/* ... */}
    else if (i == 6) {
        buttons.push(add(..., ..., true, true));
    }
}

After the simplification:

for (let i = 1; i <= pageSize; i++) {
    if (pageSize > 10) {
        if (i < 6) {
            buttons.push(add(i, i == 1, true, false));
        }
        else if (i == 6) {
            buttons.push(add(i, false, true, true));
        }
        else if (i == pageSize) {
            buttons.push(add(i, false, true, false));
        }
    }
    else {
        buttons.push(add(i, i == 1, true, false));
    }
}

where i == 1 can have its own branch:

if (i === 1) {
    buttons.push(add(i, true, true, false))
} else if (pageSize > 10) {
    if (i < 6) {
        buttons.push(add(i, false, true, false));
    } else if (i === 6) {
        buttons.push(add(i, false, true, true));
    } else if (i === pageSize) {
        buttons.push(add(i, false, true, false));
    }
} else {
    buttons.push(add(i, false, true, false));
}

Since only the first 6 button gets rendered we do not need to check for pageSize > 10:

if (i === 1) {
    buttons.push(add(i, true, true, false))
} else if (i < 6) {
    buttons.push(add(i, false, true, false));
} else if (i === 6) {
    buttons.push(add(i, false, true, true));
} else if (i === pageSize) {
    buttons.push(add(i, false, true, false));
}

and since the branch body of i < 6 and i === pageSize are the same:

if (i === 1) {
    buttons.push(add(i, true, true, false))
} else if (i < 6 || i === pageSize) {
    buttons.push(add(i, false, true, false));
} else if (i === 6) {
    buttons.push(add(i, false, true, true));
}

Builder Pattern

To go away from all the boolean flags you could create a Builder and refactor the simplified version from above to something like:

const firstButton = new ButtonBuilder().withValue(1).disabled().visible().noPlaceholder();
const placeholder = new ButtonBuilder().enabled().visible().placeholder();
const button = new ButtonBuilder().enabled().visible().noPlaceholder();
for (let i = 1; i <= pageSize; i++) {
    if (i === 1) {
        buttons.push(firstButton);
    } else if (i < 6 || i === pageSize) {
        buttons.push(button.withValue(i).build());
    } else if (i === 6) {
        buttons.push(placeholder.withValue(i).build());
    }
}

The Algorithm

In your example, you can page through 50 buttons, of which only 6 are required to render. For each button click you have to loop 50 times to render 6 buttons again.

But actually you know directly which buttons are required to render without to loop 50 times. There are 3 cases:

  • on the first to 5. button : 1, 2, 3, 4, 5 ... 50
  • on button between 6. to 50.: 1, ... 6, [7], 8, ..., 50
  • on button 47. to 50.: 1, ... 47, 48, [49], 50

some pseudo code:

function buildPagination(current) {
    if (current < 6) {
        const pagination =  [
            button(1),
            button(2),
            button(3),
            button(4),
            button(5),
            placeholder(),
            button(last),
        ];
        pagination[current].disable();
        return pagination;
    }

    if (current > (last - 4)) {
        const pagination = [
            button(1),
            placeholder(),
            button(last - 4),
            button(last - 3),
            button(last - 2),
            button(last - 1),
            button(last),
        ];
        pagination[current].disable();
        return pagination;
    }

    return [
        button(1),
        placeholder(),
        button(current - 1),
        button(current).disable(),
        button(current + 1),
        placeholder(),
        button(last),
    ];
}
\$\endgroup\$

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.