Lesson 12-13 - Githubissues

Замечания

С форматированием случилось что-то поистине ужасное. Как тайфун прошёл. Разумеется, это неприемлемо.

Метод createElement - неплохо) Высокоуровневая абстракция. Но она несколько "протекает", как это и бывает с абстракциями.

1) Есть отдельный параметр inputType - который в случае создания чего угодно, кроме кнопки или инпута, бесполезен. Это называется "раздутый интерфейс" - частные случаи торчат наружу.

Раз уж хочется универсальную фабрику, надо подняться чуть выше - пусть будет параметр attributes, и циклом значения из него переносятся на DOM-элемент. Метод для установки атрибута на элементе найдёшь в документации. К className и content это не относится - с ними всё нормально.

2) Параметр parentElement - плохо. Создание элемента и помещение его в DOM - две совершенно разные операции. Они не должны выполняться в одной функции. Это называется side-effect. Тем более, когда функция называется createElement - она должна его только создать и нам вернуть, а дальше мы уже будем решать, что мы хотим с ним делать и куда его запихнуть.

3) Несогласованность. Для добавления класса параметр есть. А отдельные элементы в style почему-то ручками добавляются уже после создания. Соотвественно, опять же, можно сделать параметр style и в цикле переносить значения.

А ещё можно все эти стили вынести в отдельный css файл и в коде оставить только классы. Это было бы гораздо лучше. Представь, что понадобится стилизацию поправить для страницы - поди ещё вычитай, где там в этом коде у конкретного элемента паддинг выставляется. Очень неудобно.

Именование переменных.

У тебя здесь присутствует очень серьёзная проблема, суть которой в следующем: привязывать имена переменных к именам тегов - крайне плохая идея с точки зрения поддержки кода. Это как раз то, что называется "смешивать логику с представлением". Завтра придёт заказчик и скажет: "а я хочу чтоб ответы не столбиком были, а плиточкой, по два в ряд". Ты заменяешь списки на, допустим, таблицы - и опа, все твои переменные разом перестают соответствовать действительности. Теперь надо по всему коду их переименовывать. Получается куча работы, огромный апдейт с массой изменений - хотя, казалось бы, сути-то там, всего-то - пару тегов заменить.

То же самое касается и вёрстки - ты называешь класс white-left-sidebar, а завтра редизайн, и этот блок уже синяя в полосочку панелька справа внизу.

В общем, имена переменных всегда должны соответствовать тому, что эти переменные делают, а не как они в итоге выглядят. Разделяй (логику и представление) и властвуй =)

Кроме того, и читается всё это дело так себе - кругом ul, ol, li - что все они делают? Приходится вычитывать, что именно им там контентом передаётся - это медленно и мучительно. Разве что с label всё нормально - label он и в Африке label, как говорится.

А также можно сюда применить нотацию, доставшуюся нам от jQuery: начинать имена переменных, содержащих DOM-элементы, с $. Это широко распространённая практика, понятная всем.

Будет что-то вроде:

var $questions = app.createElement({
    tagName: 'ol',
       ...
});

https://github.com/iricea/iricea.github.io/blob/edbd4664f875ba94471a27426b7c2bce52d4b81f/GoIT/js/lesson12-13/js/script.js#L28
generateQuestions: function (questionsAmount, answersAmount) {

А это уже читерство =)) Хотя строго формально задача выполнена, тут вообще не спорю) Но в реальной-то жизни там всё-таки будут какие-то осмысленные вопросы и ответы. Для одного блока вопроса входные данные - текст вопроса и массив с текстами ответов. Код должен работать, исходя из этого условия.

https://github.com/iricea/iricea.github.io/blob/edbd4664f875ba94471a27426b7c2bce52d4b81f/GoIT/js/lesson12-13/js/script.js#L44

А вот это совсем плохо. Про parentElement уже сказано выше - но здесь этот parentElement даже не определён внутри этого метода. side-effect в квадрате. Глядя на сигнатуру метода (название и параметры) - невозможно понять, что оказывается, если его вызвать, то где-то там в документ вдруг контент добавится. Очень плохо. А учитывая, что определён этот ol по тексту ниже, чем данный метод, всё становится совсем печально. Всё это хозяйство сейчас работает исключительно благодаря особенностям устройства JavaScript - читай "на магии". В чуточку более строгом языке это уже не взлетит.

Избавься от parentElement и соединяй элементы друг с другом явно - это решит проблему.

Замечания

https://github.com/iricea/iricea.github.io/blob/dead20a62b77e5eeb17276c39cfd710fe8ad497f/GoIT%2Fjs%2Flesson12-13%2Fjs%2Fscript.js#L40

Форматирование. Отступ съехал.

https://github.com/iricea/iricea.github.io/blob/edbd4664f875ba94471a27426b7c2bce52d4b81f/GoIT/js/lesson12-13/js/script.js#L28 generateQuestions: function (questionsAmount, answersAmount) {

Имелось же ввиду не избавиться от функции, а сделать возможность создания вопросов-ответов с произвольным контентом. Может, мы друг друга где-то недопоняли. Я говорил:

Для одного блока вопроса входные данные - текст вопроса и массив с текстами ответов.

То есть:

createQuestion: function(question, answers) {
    var $question = ...; // создать блок, контентом которого будет текст из `question`
    // перебрать все `answers`, создать из них блоки, прикрепить к `$question`
    // вернуть `$question`. 
}

createQuestionsBlock: function(questions) {
    var $questionBlock = ...; 
    // воспользоваться `createQuestion` для наполнения `$questionBlock`
    // вернуть `$questionBlock`. 
}

// где-то внизу:
// document.body.appendChild(createQuestionsBlock(...массив вопросов...))
// остальные части формы аналогично

В целом, задание говорит "создать объект с методами, генерирующими разметку для страницы". У тебя это условие не выполнено. Функция createElement может быть самостоятельной и лежать отдельно - она вспомогательная. А объект app должен содержать все эти createQuestion, createHeader etc.

iricea / iricea.github.io

Lesson 12-13 #6

Замечания

Рекомендации

Замечания

Рекомендации