1
\$\begingroup\$

I have several components/pages in my personal project that contain complex logic and even nested ternary operators. Since I'm the only person working on this project, I don't have a hard time figuring out where to make changes if I need to.

But I'm trying to improve my code in a way that others could work on it too. I am looking for some input to understand what kind of logic is acceptable.

Below is an example.

This component is a reusable pagination bar. By default, it renders links for each page number, which, when clicked, modify the ?page query param of the current URL. This behavior can be overridden by passing an optional onPageSelected callback, which renders each page number as a button with an onClick handler instead.

This logic makes the pagination bar reusable within my app but as you can see, the code also becomes quite complex.

  • (How) would you change this?
  • Do you think I'm making the code too complex by reusing the same component for the link- and button variations?
  • Would you use separate components for rendering links & buttons?
  • Would you move the ternary operators into variables? Anything else?
import Link from "next/link";
import { useRouter } from "next/router";
import { Pagination } from "react-bootstrap";

interface PaginationBarProps {
    pageCount: number,
    currentPage: number,
    onPageSelected?: (page: number) => void,
}

const PaginationBar = ({ pageCount, currentPage, onPageSelected }: PaginationBarProps) => {
    const router = useRouter();

    const paginationMinPage = Math.max(1, currentPage - 5);
    const paginationMaxPage = Math.min(pageCount, Math.max(currentPage + 4, 10));

    function getHrefForPage(page: number) {
        const href = new URL("http://localhost:3000" + router.asPath);
        href.searchParams.set('page', page.toString());
        return href.toString();
    }

    const pageItems: JSX.Element[] = [];

    for (let i = paginationMinPage; i <= paginationMaxPage; i++) {
        let paginationItem: JSX.Element;

        if (i === currentPage) {
            paginationItem =
                <>
                    <Pagination.Item active className='d-none d-md-block' key={i}>{i}</Pagination.Item>
                    <Pagination.Item active className='d-sm-block d-md-none' key={i + "-mobile"}>Page: {i}</Pagination.Item>
                </>
        } else {
            if (onPageSelected) {
                paginationItem =
                    <Pagination.Item
                        key={i}
                        className='d-none d-md-block'
                        onClick={() => onPageSelected(i)}>
                        {i}
                    </Pagination.Item>
            } else {
                paginationItem =
                    <Link href={getHrefForPage(i)} passHref legacyBehavior key={i}>
                        <Pagination.Item className='d-none d-md-block'>
                            {i}
                        </Pagination.Item>
                    </Link>
            }
        }
        pageItems.push(paginationItem);
    }

    return (
        <Pagination className="mt-4">
            {currentPage > 1 &&
                (
                    onPageSelected
                        ?
                        <>
                            <Pagination.First onClick={() => onPageSelected(1)} key="first" />
                            <Pagination.Prev onClick={() => onPageSelected(Math.max(currentPage - 1, 1))} key="prev" />
                        </>
                        :
                        <>
                            <Link href={getHrefForPage(1)} passHref legacyBehavior key="first">
                                <Pagination.First />
                            </Link>
                            <Link href={getHrefForPage(Math.max(currentPage - 1, 1))} passHref legacyBehavior key="prev">
                                <Pagination.Prev />
                            </Link>
                        </>
                )
            }
            {pageItems}
            {currentPage < pageCount &&
                (
                    onPageSelected
                        ?
                        <>
                            <Pagination.Next onClick={() => onPageSelected(Math.min(currentPage + 1, pageCount))} key="next" />
                            <Pagination.Last onClick={() => onPageSelected(pageCount)} key="last" />
                        </>
                        :
                        <>
                            <Link href={getHrefForPage(Math.min(currentPage + 1, pageCount))} passHref legacyBehavior key="next">
                                <Pagination.Next />
                            </Link>
                            <Link href={getHrefForPage(pageCount)} passHref legacyBehavior key="last">
                                <Pagination.Last />
                            </Link>
                        </>
                )
            }
        </Pagination>
    );
}

export default PaginationBar;
```
\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$
  • I don't have a hard time figuring out where to make changes if I need to.

Give me a call in two years!

Seriously though this rationale is very common. If we do not take our coding opportunities to "do it right" years later one sees that person's work, a knowledgeable language lawyer who cannot code a large problem into an understandable and manageable program.


  • improve my code in a way that others could work on it too

Encapsulate esoteric code in meaningful name/actions, especially the conditionals. Then encapsulate the conditional code blocks in meaningful name/actions.


  • an optional onPageSelected callback, which renders each page number as a button with an onClick handler instead.

This is thoroughly obscure. There is no "optional" code apparent. onPageSelected is a very bad name for "hey! Make it a button, not a link". Also put the button rendering code in that renamed object.

"Optional" is the wrong concept. It should be "alternative" because code will render either buttons or links, not both. This perspective change is best expressed as injecting either button object or link object - not in-line branching selection. Future others do not need comments to explain this now obvious expression.

As a requirement/intent issue it doesn't make sense to dynamically decide "button or link" when that decision can be - should be - made up front with that function parameter. Then the code will be telling me it is optional alternate.


  • ... too complex and should be broken up?

McCabe complexity metric is a measure of the total possible paths through the code. However at first glance I think "busy", "noisey" because all the detail is exposed. There is no encapsulation.

  • Factor out button and link into separate objects, to be injected into the construction code. This will reduce complexity - i.e. reduce the number of branches.
  • Name things for what they are or what they do - not for the code implementation details.
  • Now the conceptual purpose/function difference - not button vs link - should stand out better.
  • Make the "is a link" code a separate object just like the "is a button" object.
  • PaginationBarProps interface will declare method names for rendering the different link-types ("current" & "not current"?)

Wild and Crazy Idea

There seems to be "current page" and "not current page" renderings. ... Generate them both, up front, encapsulate each pair with the page number in an object. Then dynamically pick the desired one.

This will also reduce complexity I expect.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you too for your review! That was very helpful. I don't understand what you mean by "injecting either button object or link object". Should I inject the already constructed Pagination.Item? \$\endgroup\$ Feb 6, 2023 at 7:02
  • \$\begingroup\$ What do you think about the idea of separating the components into a LinkPaginationBar and ButtonPaginationBar (names subject to improvement) and extracting the duplicate logic into a more generic PaginationBar component? The downside is that I have 3 types in my namespace now. \$\endgroup\$ Feb 6, 2023 at 7:31
  • \$\begingroup\$ injecting either button object or link object => PaginationBarProps is the object for passing in button data, yes? If you want to create links instead, then pass in an object with the links data. \$\endgroup\$
    – radarbob
    Feb 6, 2023 at 10:21
  • \$\begingroup\$ LinkPaginationBar & ButtonPaginationBar => I'm seeing the difference is a "look and feel" UI issue, not a functional issue so I'm seeing a single, configurable component. But if that doesn't fit in the overall code design then do what you have to do. \$\endgroup\$
    – radarbob
    Feb 6, 2023 at 10:34
  • \$\begingroup\$ Ok gotcha. Thank you very much! \$\endgroup\$ Feb 6, 2023 at 10:41

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.