-1

I refactored this code already, but it still feels like this could be written better (without external libs). Any suggestions? 

  // Old
  const filters = [];
  for (let i = 1; i < 54; i += 1) {
    const currentDateDisplay = getStartDate(i, new Date().getFullYear());
    filters.push({ text: currentDateDisplay, value: currentDateDisplay });
  }

  // New
  const filters2 = [...Array(53).keys()].map((item) => ({
    text: getStartDate(item, new Date().getFullYear()),
    value: getStartDate(item, new Date().getFullYear()),
  }));

Edit: Looks like sometimes it's better to use the old for loop, especially when it comes to performance. Thanks for the suggestions.

phifa
  • 856
  • 7
  • 11
  • 1
    the first one is more elementary and `better` – brk Apr 21 '20 at 09:08
  • Save the `getStartDate(item, new Date().getFullYear())` in a variable instead of repeating it twice – CertainPerformance Apr 21 '20 at 09:08
  • 2
    Where is your external lib? Your code example is incomplete. – wederer Apr 21 '20 at 09:10
  • Why does the first example use `getDateDisplay` and the second does not? What exactly is returned by `getStartDate`? If it is an object, should it be a shared reference to the same object for both the `text` and `value` properties? – str Apr 21 '20 at 09:18
  • good catch, forgot that function, i removed it in the examples, it is not needed. @str – phifa Apr 21 '20 at 09:30
  • 1
    You shouldn't be worrying about performance if you're only creating dealing with 53 objects inside of an array. Pick whatever is easier for you to understand and **then** worry about performance if it becomes a bottleneck in your application. – goto Apr 21 '20 at 11:21

4 Answers4

0

I have to agree that the 1st implementation you wrote seems more intuitive, however, the 2nd method could be written differently.

By first calling new Array(53), you create an array of 53 elements. By then calling .map((_, index) => {}), you have access to the index variable (using _ for the item, as it's undefined anyway). You don't have to clone the new array by spreading it, so simply using this code would work for you:

const filters = new Array(53).map((_, index) => ({
  text: getStartDate(index, new Date().getFullYear()),
  value: getStartDate(index, new Date().getFullYear())
}));
Ruben Rutten
  • 1,659
  • 2
  • 15
  • 30
0

You want to initiate an array with an initial values? thats the use-case for Array.fill()

const filters2 = new Array(53).fill({
  text: getStartDate(0, new Date().getFullYear()),
  value: getStartDate(0, new Date().getFullYear())
});
Mechanic
  • 5,015
  • 4
  • 15
  • 38
  • This would not work, as the `Array.prototype.fill` method is used to fill every element of the array with the same value. Judging by the use of `item` (which is actually the index in his case) in the `getStartDate` function, each element should have different values. – Ruben Rutten Apr 21 '20 at 09:27
0

I prefer your old variant as it is more readable for me. In my view, code should be more readablem not shorter.

In addition, according to the test the old variant is faster:

const filters = [];
for (let i = 1; i < 54; i += 1) {
    const currentDateDisplay = getStartDate(i, new Date().getFullYear());
    filters.push({ text: currentDateDisplay, value: currentDateDisplay });
}
StepUp
  • 36,391
  • 15
  • 88
  • 148
0

Which style you prefer is ultimately a matter of taste. If you prefer the second—more functional-like—style you can simplify it as below:

const year = new Date().getFullYear();
const filters2 = Array.from(new Array(53), (_, i) => {
  const startDate = getStartDate(i, year);
  return {text: startDate, value: startDate};
});

It replaces [...Array(53).keys()].map() with the more suitable Array.from and does not evaluate the same values multiple times. The performance differences in such a simple case are negligible and you should use the code that is more readable to you and your team.

str
  • 42,689
  • 17
  • 109
  • 127