Правильно ли так делать?

Question

[Chelentano1993]

Подскажите, правильно ли будет такой код:

class Popup {
  constructor() {};
  open(event) {
    if(event.target.classList.contains('user-info__button')) {
      togglePopup(popupCard); // здесь добавляется класс, который открывает попап
    }
    if(event.target.classList.contains('user-info__edit')) {
      togglePopup(popupProfile); // здесь добавляется класс, который открывает попап
    }
  };
  close(event) {
    if(event.target.classList.contains('popup__close-card')) {
      togglePopup(popupCard);
    }
    if(event.target.classList.contains('popup__close-profile')) {
      togglePopup(popupProfile);
    }
  };
}

Comments

[Дмитрий Беляев]

Зачем здесь класс?
Ну и бросается в глаза сразу 4 куска однотипного кода, который вполне можно вынести в функцию

[Chelentano1993]

Дмитрий Беляев, в задании так требуется. Чтоб был класс, а в нем были два метода, которые будут открывать и закрывать попапы. Я тоже хочу вынести все if в отдельную функцию, но тогда как выполнить задание...

Answer 1

[Дима Паутов]

Есть мысли, что правильной код тот, который решает поставленную задачу. Что именно вы хотите узнать?

Чисто из наблюдений:

1. Что такое togglePopup?
   
2. Откуда вообще берётся эта функция? И что она делает внутри?
   
3. Зачем постоянно event.target, почему не просто селектор?? Что это за элемент? Как разработчику понять, у какого элемента проверяется класс?
   
4. Откуда берутся переменные popupCard, popupProfile и почему они не в конструкторе?
   
5. Какую задачу вы решаете? Ваш код зависит от html, укажите его тоже, что бы более менее обрисовать картину.
   
6. Покажите, где вызываются методы open и close.
   

Зачем вообще для этого используется класс? Он ничего не делает, точно так же можно было сделать просто объект, а лучше просто 2 функции

Comments

[Chelentano1993]

Дима Турков сам код работает, но мне нужно узнать, правильно так писать его...Класс используется по заданию, в котором два метода должны открывать или закрывать попап. Тут дело не в том где и как вызывается, а правильно ли делать if и там писать условия

[Дима Паутов]

Chelentano1993, Какое-то глупое задание, либо вы его не правильно поняли!
Метод close должен закрыть лишь 1 окно, а у вас закрывает множество исходя из такого себе условия.

open(popup) {
// а тут уже код с открытием, добавить класс
}
close(popup) {
// а тут уже код с закрытием, удалить класс
}
toggle(popup){
// тут проверка состояния указанного окна, открыто оно или закрыто и вызов соответствующих методов
}

user-info__button, user-info__edit и т.п. должны быть заменены на 1 селектор, по которому будет происходить действие, иначе вы будете плодить эти условия до бесконечности.

<button data-popup="popup1">Открыть 1</button>
<button data-popup="popup2">Открыть 2</button>

<div id="popup1">
  <button data-popup="popup1">Закрыть</button>
</div>

<div id="popup2">
  <button data-popup="popup2">Закрыть</button>
</div>

const popupButtons = document.querySelectorAll('[data-popup]');
const popups = new Popup();

popupButtons.forEach(button => {
  button.addEventListener('click', function(){
    const popup = document.getElementById(this.dataset.popup);

    popups.toggle(popup);
  })
})

А так, вы ищите любой элемент, который попадётся курсору удовлетворяющий вашим условиям, какая-то дичь

Answer 2

[Евгений]

Лучше использовать elseif вместо нескольких if.