Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Styling and readability

###Styling and readability YouYou are omitting the semicolon after most return-statements. I recommend putting a semicolon after every statement, even though a newline character will close the return statement.

Semantics

###Semantics YouYou define this.constants as a public mutable property. You can consider at least using Object.freeze (mdn).

Over-engineering

###Over-engineering ForFor code that is supposed to output a simple list that could be generated with a range-generator (more or less your sequencer), 4 additions and 4 comparisons, or a little more if you want to keep the length constant, this class seems highly over-engineered. All these 'strategies' and 'actions' do not seem to add anything other than making the code execution jump all over the place, while most of it can just be generalized.

###Styling and readability You are omitting the semicolon after most return-statements. I recommend putting a semicolon after every statement, even though a newline character will close the return statement.

###Semantics You define this.constants as a public mutable property. You can consider at least using Object.freeze (mdn).

###Over-engineering For code that is supposed to output a simple list that could be generated with a range-generator (more or less your sequencer), 4 additions and 4 comparisons, or a little more if you want to keep the length constant, this class seems highly over-engineered. All these 'strategies' and 'actions' do not seem to add anything other than making the code execution jump all over the place, while most of it can just be generalized.

Styling and readability

You are omitting the semicolon after most return-statements. I recommend putting a semicolon after every statement, even though a newline character will close the return statement.

Semantics

You define this.constants as a public mutable property. You can consider at least using Object.freeze (mdn).

Over-engineering

For code that is supposed to output a simple list that could be generated with a range-generator (more or less your sequencer), 4 additions and 4 comparisons, or a little more if you want to keep the length constant, this class seems highly over-engineered. All these 'strategies' and 'actions' do not seem to add anything other than making the code execution jump all over the place, while most of it can just be generalized.

Source Link
Sumurai8
  • 2.7k
  • 11
  • 19

Disclaimer: I am not very familiar with the Ecmascript 6 class syntax.

###Styling and readability You are omitting the semicolon after most return-statements. I recommend putting a semicolon after every statement, even though a newline character will close the return statement.

Consider following these naming conventions.

"execute" is not a very descriptive name for a function. It outputs a formatting array, so a possible alternative could be to name it "format".

###Semantics You define this.constants as a public mutable property. You can consider at least using Object.freeze (mdn).

I do not like the following syntax. It is for the logic in the rest of the class irrelevant if, for example, totalPages was defined by the user or a pre-defined constant. If you decide to let the user define, for example, the starting page too in the future, you suddenly need to go through the entire code and change from where the variable is loaded. Instead, I would suggest just using properties such as this.firstPage which is either the user-defined value, or a default value.

constructor(params) {
 this.params = {
 totalPages: params.totalPages,
 currentPage: params.currentPage,
 };
 //...
}

Note: I am assuming you have renamed your class to Paginator since your usage example shows this name. Your class name PaginationStrategy does not really define what the class is supposed to do.

###Over-engineering For code that is supposed to output a simple list that could be generated with a range-generator (more or less your sequencer), 4 additions and 4 comparisons, or a little more if you want to keep the length constant, this class seems highly over-engineered. All these 'strategies' and 'actions' do not seem to add anything other than making the code execution jump all over the place, while most of it can just be generalized.

export class Pagination {
 constructor(params) {
 this.defaultValues = {
 FIRST_PAGE: 1,
 SEQUENCE_LENGTH: 9, //Must be at least 5
 STEP_SIZE: 1,
 };
 if(Object.freeze) {
 Object.freeze(this.defaultValues);
 }
 this.firstPage = this.defaultValues.FIRST_PAGE;
 this.lastPage = params.totalPages;
 this.currentPage = params.currentPage;
 this.sequenceLen = this.defaultValues.SEQUENCLE_LENGTH;
 this.stepSize = this.defaultValues.STEP_SIZE;
 }
 format() {
 //Generate these sequences:
 //1. [this.firstPage]
 //2. [null] or []
 //3. Sequence starting at
 // max( this.firstPage + this.stepSize, this.currentPage - offsetLeft )
 // where offsetLeft is min( based on sequenceLength, based on lastPage )
 // ending similarly
 //4. [null] or []
 //5. [this.lastPage]
 }
 range(start, end, stepSize) {
 const length = Math.floor( (end - start) / stepSize );
 return Array.apply(0, Array(length)).map( (curr, idx) => (start + (idx * stepSize)) );
 }
}
lang-js

AltStyle によって変換されたページ (->オリジナル) /