Skip to main content
2 of 4
deleted 21 characters in body
Roman
  • 2.8k
  • 11
  • 23

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 (pageSize >= 6 && 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 the last button : 1, ... 46, 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) {
        return [
            button(1),
            placeholder(),
            button(last - 4),
            button(last - 3),
            button(last - 2),
            button(last - 1),
            button(last).disable(),
        ];
    }

    return [
        button(1),
        placeholder(),
        button(current - 1),
        button(current).disable(),
        button(current + 1),
        placeholder(),
        button(last),
    ];
}
Roman
  • 2.8k
  • 11
  • 23