Что подразумевается под реафкторингом кода на js и насколько правильно я его сделал для предлагаемого кода?

Question

[Александр Буки]

Есть тестовое задание.
Необходимо сделать рефакторинг кода:

function Journal(date) {
  this.date = date;

  this.formatDate = function(date) {
    return date.getDate() + '.' + (date.getMonth() + 1) + '.' + date.getFullYear();
  };

  this.getTitle = function() {
    return "Выпуск от " + this.formatDate(this.date);
  };

}

Journal.compare = function(journalA, journalB) {
  return journalA.date - journalB.date;
};

// использование:
var journals = [
  new Journal(new Date(2012, 1, 1)),
  new Journal(new Date(2012, 0, 1)),
  new Journal(new Date(2011, 11, 1))
];

function findMin(journals) {
  var min = 0;
  for (var i = 0; i < journals.length; i++) {
    if (Journal.compare(journals[min], journals[i]) > 0) min = i;
  }
  return journals[min];
}

alert( findMin(journals).getTitle() );

Если я правильно понимаю рефакторинг-это оптимизация, поэтому я по максимуму перевел все на es6 и применил метод сортровки для массива, код сократился в два раза:

function Journal(date) {
  this.date = date;
  this.formatDate = date => `${date.getDate()}.${date.getMonth() + 1}.${date.getFullYear()}`;
  this.getTitle = () => `Выпуск от ${this.formatDate(this.date)}`;
}
const journals = [
  new Journal(new Date(2012, 1, 1)),
  new Journal(new Date(2012, 0, 1)),
  new Journal(new Date(2011, 11, 1)),
];
journals.sort((a, b) => a.date - b.date);
alert(journals[0].getTitle());

Answer 1

[Антон Спирин]

1. Ну и где ES6 классы или хотя бы прототипы?
2. В массив следовало добавить строчные значения дат и работать с ними. Так вы избавитесь от дублирования: new Journal(new Date(/* ... */)),

3. У класса Journal есть метод compare, это значит, что this.date подробности реализации, которые могут быть изменены, поэтому вместо сортировки по датам, следует использовать метод compare. Либо реализовать метод Journal.min, который уже будет использовать в реализации сортировку.

4. Рефакторинг это не обязательно сокращение кода. Например для читаемости, стоило вынести минимальную дату в переменную:
const minJournal = findMin(journals);
или:
const minJournal = Journal.min(journals);

5. Для title можно использовать getter. Это позволит, например, использовать деструктуризацию:
const { title } = minJournal;

Comments

[Александр Буки]

спасибо за совет

[Александр Буки]

С учетом замечаний получилось вот так:

class Journal {
  constructor(date) {
    this.date = date;
  }

  formatDate(date) {
    return `${date.getDate()}.${date.getMonth() + 1}.${date.getFullYear()}`;
  }

  get title() {
    return `Выпуск от ${this.formatDate(this.date)}`;
  }
}
Journal.compare = (journalA, journalB) => journalA.date - journalB.date;
Journal.min = (journals) => {
  let min = 0;
  for (let i = 0; i < journals.length; i++) {
    if (Journal.compare(journals[min], journals[i]) > 0) min = i;
  }
  return journals[min];
};

const journals = [
  new Journal(new Date(2012, 1, 1)),
  new Journal(new Date(2012, 0, 1)),
  new Journal(new Date(2011, 11, 1)),
];

const minJournal = Journal.min(journals);
const { title } = minJournal;

Я не совсем понял про строки в датах, поэтому не стал править этот момент

[Антон Спирин]

alexbuki, смотрите статические методы лучше расположить рядом с классом друг за другом.
Насчет дат:

const dates = ['2-1-2012', '1-1-2012', '12-1-2011'];
const journals = dates.map(date => new Journal(new Date(date)));

Journal.min можно реализовать с помощью sort:

Journal.min = journals => journals.sort((a, b) => Journal.compare(a, b))[0];

[Александр Буки]

Антон Спирин, спасибо вам большое!

Answer 2

[Александр Буки]

С учетом замечаний

class Journal {
  constructor(date) {
    this.date = date;
  }

  formatDate(date) {
    return `${date.getDate()}.${date.getMonth() + 1}.${date.getFullYear()}`;
  }

  get title() {
    return `Выпуск от ${this.formatDate(this.date)}`;
  }
}
Journal.compare = (journalA, journalB) => journalA.date - journalB.date;
Journal.min = (journals) => {
  let min = 0;
  for (let i = 0; i < journals.length; i++) {
    if (Journal.compare(journals[min], journals[i]) > 0) min = i;
  }
  return journals[min];
};

const journals = [
  new Journal(new Date(2012, 1, 1)),
  new Journal(new Date(2012, 0, 1)),
  new Journal(new Date(2011, 11, 1)),
];

const minJournal = Journal.min(journals);
const { title } = minJournal;