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):

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

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

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

metoda validate zwracająca jakiś ValidationResult zamiast wyjątka?

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

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.