Refaktoryzacja if else, switch

Cześć, refaktoryzuję teraz projekt i mam pełno if'ów w kodzie. Zacząłem refaktoryzację i chciałbym zapytać czy w dobrym kierunku idę. Również zapytać czy macie lepsze pomysły, czy są jakieś źródła na ten temat, z których mógłbym się uczyć?

Zacząłem z przykładem 1 i na przykład 2 jest kod po refaktoryzacji.

Przykład 1 (przed):

if (userSession.isInvalid()) {
    throw new Error(`...`);
}
if (userSession.isEmpty()) {
    throw new Error('...');
}
if (userSession.isExpired()) {
    throw new Error('...');
}

...
export default class UserSessionResult {
  userSessions: Array<UserSession>;

  isInvalid(): boolean {
    return this.userSessions.length > 1;
  }

  isEmpty(): boolean {
    return this.userSessions.length === 0;
  }
 ...
}

Przykład 2 (po):

userSession.isInvalid()
      .or()
      .isEmpty()
      .or()
      .isExpired()

...
export default class UserSessionResult {
  userSessions: Array<UserSession>;

...
  isInvalid(): UserSessionResult {
    if (this.userSessions.length > 1) {
      throw new Error('Invalid user session');
    }
    return this;
  }

  isEmpty(): UserSessionResult {
    if (this.userSessions.length === 0) {
      throw new Error('Invalid user session');
    }
    return this;
  }

  or(): UserSessionResult {
    return this;
  }
}

Jakoś bardziej mi się podoba wersja przed refaktorem :) Ewentualnie zamieniłbym ten drugi kod na coś w stylu userSession.assert().valid().notEmpty().

Albo może coś w stylu validate(isValid, isNotEmpty, isNotExpired). Wtedy każda z tych funkcji musiałaby zwracać jakieś obiekt z informacją, czy walidacja przeszła, czy nie, plus ewentualną wiadomość z błędem.

Jeśli ta metoda or() będzie zwracać jedynie this to według mnie jest niepotrzebna.

Xarviel napisał(a):

Jeśli ta metoda or() będzie zwracać jedynie this to według mnie jest niepotrzebna.

Jeden rabin powie tak. Drugi rabin powie nie. Czasami syntactic sugar jest dobrym rozwiązaniem i może polepszyć czytelność kodu

JavalukeScriptwalker napisał(a):

Kopiuj

userSession.isInvalid()
      .or()
      .isEmpty()
      .or()
      .isExpired()

Ja bym proponował odwrócić gramaryke i użyć:

userSession
    .isValid()
        .or()
    .notEmpty()
        .or()
    .notExpired();

Jeśli robisz to na backendzie (bo na frontend nie wrzucałbym kolejnej paczki) i masz dużo takich klas dotyczących samej walidacji to można też skorzystać z jakieś paczki np yup, albo joi. Obie działają w podobny sposób tylko różnią się delikatnie składnią.

https://www.npmjs.com/package/yup
https://www.npmjs.com/package/joi

Musimy na początku określić jedynie wstępną walidacje i Joi.object({ ...}) zwraca nam szablon, który wykorzystujemy do walidacji

const Joi = require('joi');

const schema = Joi.object({
    username: Joi.string()
        .alphanum()
        .min(3)
        .max(30)
        .required(),

    birth_year: Joi.number()
        .integer()
        .min(1900)
        .max(2013),
 });

schema.validate({ username: 'abc', birth_year: 1994 });
// -> { value: { username: 'abc', birth_year: 1994 } }
// obiekt prawidłowy

schema.validate({});
// -> { value: {}, error: '"username" is required' }
// obiekt nieprawidłowy, dostajemy listę błędów

JavalukeScriptwalker napisał(a):

Widzę coś takiego i wiem, że junior pisał. Nie kombinuj na siłę. Zrób metodę validateUserSession(userSession), wrzuć tam te 3 ify i tyle. Ktokolwiek popatrzy w kod, który to wywołuje widzi, że aha, w tym miejscu jest jakaś walidacja, aha w tym miejscu są 3 ify wyglądające spójnie. Koniec tematu.

Takie potworki

Kopiuj

userSession.isInvalid()
      .or()
      .isEmpty()
      .or()
      .isExpired()

To jest próbowanie ładnego pisania na siłę. Te metody nie robią już nawet to co sugerują, że robią - one nie zwracają boolean odpowiadając na swoje pytanie. or() nic nie robi... to jest w ogóle dramat.

W celu zastąpienia if else często używa się command pattern: https://www.industriallogic.com/xp/refactoring/conditionDispatcherWithCommand.html
albo strategy pattern: https://www.industriallogic.com/xp/refactoring/conditionalWithStrategy.html

Generalnie ta druga opcja to coś podobnego do tego co robisz, ale ten or() jest zbędny.