Здорово будет добавить немного описания в ридми репозитория. Хорошо оформленный репозиторий привлекает внимание, в том числе и потенциальных работодателей
Вы установили шаблонный бэйдж от CodeClimate. Необходимо настроить ссылку на проверку своего проекта.
makefile - хотя так тоже работает, следует придерживаться рекомендации, в которой говорится, что лучше именовать этот файл с большой буквы Makefile.
Приветствую, !
У вас получилось написать собственное приложение и создать пакет для его установки. Это очень хорошо. Сейчас наступает не менее важная часть разработки - рефакторинг кода. Работу выстроим так. Я отправлю вам список замечаний. Ваша задача внимательно изучить их и внести исправления в свой код. Все вопросы, которые возникают, задавайте мне в слаке в личные сообщения. Там мы их сможем оперативно решить.
Эти зависимости не нужны для работы вашего приложения, они нужны только для разработки и их нужно поместить в соответствующую секцию devDependencies.
Хорошей практикой будет контролировать используемую версию nodejs. В коде могут использоваться новые возможности, которые не будут реализованы в версии, которая используется по умолчанию. В таком случае флоу не упадет только потому, что мы не запускаем сам код, а только проверяем его линтером. Для управления версиями используйте соответствующий экшн setup-node. Посмотрите примеры на странице документации, и добавьте использование данного экшна в ваше флоу.
Хорошей практикой будет контролировать используемую версию nodejs. Попробуйте добавить во флоу выполнение команды node -v, посмотрите какая версия используется (12.x). В проекте мы используем модули ("type": "module"), которые поддерживаются начиная с версии 13.x. И флоу не падает только потому, что мы не запускаем сам код, а только проверяем его линтером.
Под одним именем собраны совершенно разные действия: интерактивное взаимодействие с пользователем, вывод на экран и возврат значения. Нет необходимости в таком больше искусственном объединении Кроме того имя функции вводит в заблуждение, потому что по факту функция совершает другие операции. Согласно рекомендации в проекте, не нужно смешивать код модуля cli с остальным кодом. Продублируйте код отвечающий за приветствие и запрос имени пользователя напрямую там где это требуется (в функции движка).
Исполняемые файлы - это пользователи нашего кода, а не сам код. Они не должны содержать никакой логики приложения. В исполняемом файле должен быть один простой вызов функции (одной конкретной игры). Сейчас часть логики движка (вывод в консоль и взаимодействие с пользователем, а именно запрос имени) находится в исполняемом файле. Тут сразу две проблемы. Первая - дублирование. Игры со временем могут добавляться в проект и их может быть в итоге 10, 100 и если понадобится например изменить способ приветствия - это нужно будет делать во всех исполняемых файлах вместо того чтобы сделать это один раз, в движке. Вторая проблема заключается в том, что если движку понадобится обрабатывать ошибки, он не сможет этого сделать так как данные операции выполняются за его пределами.
На уровне модуля не должно быть выполняющегося кода. Поместите запрос имени в функцию, а в исполняемом файле сделайте вызов данной функции.
На уровне модуля не должно быть выполняющегося кода. В данном случае цикл запускается некотролированно сразу же при импорте модуля и константа gameData наполняется данными. Также подумайте, если например будет задача выполнить запуск игры несколько раз подряд, то в итоге игра будет работать с одними и теми же данными, потому что нет возможности управлять запуском генерации данных. Оберните цикл и константу gameData в функцию и экспортируйте из модуля и в этой же функции выполните вызов движка передавая в него нужные данные. А в исполняемом файле сделайте вызов данной функции.
Код данного метода лучше будет продублировать ещё раз в движке. То что мы делали на третьем шаге игрой как таковой не является и не относится к приложению и потому не нужно смешивать код третьего шага с приложением. Данный код нужен только для того чтобы студент быстрее смог освоится с вводом и выводом и создать рабочее приложение. Этот момент описан на четвёртом шаге Файл, который мы добавили ранее (src/main/java/hexlet/code/Cli.java), оставьте как есть и не смешивайте с остальным кодом..
Question: - Вы передаёте в движок отформатированную строку. Это ошибка. Подумайте, если движку необходимо будет не выводить сообщение в консоль, а передавать данные по сети, складывать в массив или записывать в лог, тогда элементы форматирования окажутся лишними и могут даже сломать программу. Кроме того приходится дублировать это строковое значение в каждой из игр. Игр потенциально могут быть десятки, сотни. И если понадобится изменить это строковое значение, нужно будет рефакторить все игры, вместо того чтобы сделать это один раз в движке.
Кто от кого должен зависеть - игры от движка или движок от игр? Здесь вы ставите зависимость движка от игр, заставляя его приводить correctAnswer (это также выглядит странным, если это правильный ответ во всех смыслах, зачем его дальше форматировать?) к строке. Ведь движок, по идее, вообще ничего не может знать об играх, которые будут на нём запускаться. Единственное, что он может сделать - это выставить требования для игр (в т.ч. по типу предоставляемых значений), которые они должны выполнить, если хотят успешно запуститься на движке. Приводите правильный ответ к строковому типу в игре, а в движке используйте строгое сравнение.
Подготовка игровых данных (вопросов и правильных ответов для каждого раунда) - это зона ответственности игры, а не движка. Эта функция специфична для конкретных двух игр. Поэтому её следует перенести в место применения. Надо понимать, что у вас ограниченные возможности организации общего функционала для разных игр, потому что игры независимы друг от друга, могут разрабатываться и добавляться в разное время разными разработчиками.
Не надо под каждое примитивное действие создавать дополнительную функцию-обёртку. Используйте методы console.log и readline напрямую, они достаточно семантичны сами по себе (итак понятно, что делают). Когда (и если) реально встанет необходимость создать новую абстракцию, тогда и отрефакторите код. А плодить кучу абстракций сейчас нет необходимости.
Здесь нет необходимости создавать дополнительные методы-обёртки. Используйте методы System.out.println() и next() напрямую в методе движка, они достаточно семантичны сами по себе (и так понятно, что делают). Когда (и если) реально встанет необходимость создать новую абстракцию, тогда и отрефакторите код.
Игры и движок должны быть независимы друг от друга и движок ничего не может знать об играх которые будут на нём запускатся. Единственное, что он может сделать - это выставить требования для игр, которые они должны выполнить, если хотят успешно запуститься на движке.
Вообще, подобное скопление экспортов должно вызывать настороженность. Экспортированные функции образуют интерфейс модуля. Но что представляет собой модуль index.js в вашем коде, что эта за абстракция? Если это игровой движок, то очевидно, что он должен выставить наружу (в качестве интерфейса) одну единственную функцию - запускатор игр. Это будет правильно выделенной абстракцией. А когда в модуле множество экспортов, то это уже больше похоже на библиотеку функций.
Помните, что движку нужно передавать готовые данные, приведённые к нужному типу. В данном случае движок работает со строками. Используйте, например, функцию String() или метод toString() для приведения вопроса к строковому типу, перед передачей данных движку. В данном случае в движке происходит неявное преобразование вопроса к строковому типу и ошибки нет. Но если, например движку потребуется хранить вопрос в базе данных, а не выводить в консоль - попытка записать данные неверного типа в базу данных может привести к ошибке. Обратите внимание, что вопрос к строке нужно приводить после того как он будет передан в функцию isEven/isPrime (они ожидают на вход число).
Генерация случайного числа не относится к логике движка. Можно разместить эту функцию в отдельный модуль для общеигровых (периспользуемых разными играми) функций, ведь она используется в каждой игре. Такой модуль обычно называют utils. По вызову функции, например getRandomInt(15) сложно понять какое число будет возвращено. Сделайте генератор случайного числа более универсальным, чтобы он возвращал число из диапазона, границы которого определяются двумя входными параметрами. Таким образом по вызову getRandomInt(5, 10) будет сразу понятно, что будет получено случайное число в диапазоне от 5 до 10.
Подобное скопление экспортов должно вызывать настороженность. Экспортированные функции образуют интерфейс модуля. Но что представляет собой модуль index.js в вашем коде, что эта за абстракция? Если это игровой движок, то очевидно, что он должен выставить наружу (в качестве интерфейса) одну единственную функцию - функцию запуска игр и константу с количеством раундов. Это будет правильно выделенной абстракцией. А когда в модуле множество экспортов, то это уже больше похоже на библиотеку функций.
Очевидно, что назрела необходимость выделить для операции генерации случайного числа отдельную функцию и даже можно разместить эту функцию в отдельный модуль для общеигровых (периспользуемых разными играми) функций, ведь она используется в каждой игре. Такой модуль обычно называют utils.
Как по вызову функции, например getRandomInt(15) можно понять какое число будет возвращено. Сделайте генератор случайного числа более универсальным, чтобы он возвращал число из диапазона, границы которого определяются двумя входными параметрами. Таким образом по вызову getRandomInt(5, 10) будет сразу понятно, что будет получено случайное число в диапазоне от 5 до 10.
В идеале здесь должно быть исключение. Но так как студенты пока не знакомы с исключениями, можно намекнуть, что это хороший повод познакомиться с ними. Сейчас, если мы вдруг добавим новый оператор, но забудем добавить действие для него, то нам просто всегда будет возвращаться ноль и мы не узнаем об ошибке. Вот есть хорошая статья на эту тему Дефолты в свичах
Сейчас по факту вопросы и ответы оказываются в разных вложенных массивах. Это довольно хрупкое решение. Если движок решит изменить один из массивов, например отсортировать вопросы, соответствие между вопросом и ответом собьется. Нужно хранить пару вопрос-ответ в одном вложенном массиве, так они всегда будут рядом
Для того что бы построить прогрессию нужно три параметра: значение первого члена прогрессии, шаг прогрессии и её длина. Сделайте данную функцию узконаправленной и чистой (генерация случайных значений не должна происходить внутри, то есть функция должна получать нужные значения через параметры). Чтобы он только генерировал и возвращал прогрессию на основании полученных параметров. При необходимости данный метод можно будет переиспользовать или протестировать отдельно от остального кода. Сейчас же это невозможно сделать, так как он недетерминированный.
Для того что бы построить прогрессию нужно три параметра: значение первого члена прогрессии, шаг прогрессии и её длина. Сделайте данный метод чистым (генерация случайных значений не должна происходить внутри, то есть метод должен получать нужные значения через параметры). Чтобы он генерировал и возвращал прогрессию на основании полученных параметров. При необходимости данный метод можно будет переиспользовать или протестировать отдельно от остального кода. Сейчас же это невозможно сделать, так как он недетерминированный.
Тут ошибки нет, но код станет более выразительным, если выделить отдельную переменную для хранения описания игры description. Так сразу будет понятно, что за сущность передаётся в движок, сейчас же это просто строка непонятного назначения.
${gcd(a, b)} - интерполяция по своему смыслу нужна, чтобы вставить внутрь строки результат какого-либо выражения. У вас же здесь нет такой задачи, потому что в данном случае к строке надо привести всё выражение, а для этого (по смыслу!) больше подходит функция String() или метод toString().
В модулях, вложенные определения (функций/констант) делаются тогда, когда их содержимое зависит от лексического контекста. Во всех остальных случаях предпочтительно выносить определения на уровень модуля (но не обязательно экспортировать). Такой код легче читать, проще переиспользовать и по необходимости можно экспортировать и тестировать. Кроме того, надо понимать, что при каждом вызове вмещающей функции (если вызовы неоднократные) вложенные определения каждый раз заново переопределяются.
Эта функция не является общей переиспользуемой разными играми, она специфична для конкретных двух игр. Поэтому её следует перенести в место применения. Надо понимать, что у вас ограниченные возможности организации общего функционала для разных игр, потому что игры независимы друг от друга, могут разрабатываться и добавляться в разное время разными разработчиками.
Выделите код отвечающий за проверку числа на четность в отдельную функцию-предикат (возвращает логическое значение true/false) и поместите её на уровне модуля (вложенное определение). Так код станет более выразительным, и при необходимости данную функцию можно будет экспортировать из модуля и протестировать отдельно от остального кода.
Тут ошибки нет, но код станет более выразительным, если выделить код, отвечающий за проверку числа на четность, в отдельный метод-предикат (возвращает логическое значение true/false) . И при необходимости метод можно будет протестировать отдельно от остального кода.
Отмечу, что эту часть можно немного упростить. Каждый элемент прогрессии можно вычислить используя формулу для получения значения n-го элемента арифметической прогрессии: current = firstElement + index * step (индексация начинается с нуля как у вас и сделано). В этом случае не нужно будет создавать вначале прогрессию с первым элементом (const progression = [firstElement];). Также кода станет меньше и не нужно будет использовать переменную let temp за пределами цикла. Попробуйте это использовать и упростить реализацию.
Отмечу, что эту часть можно немного упростить. Каждый элемент прогрессии можно вычислить используя формулу для получения значения n-го элемента арифметической прогрессии: current = firstElement + index * step (индексация начинается с нуля как у вас и сделано). В этом случае не нужно будет создавать вначале прогрессию с первым элементом (const progression = [firstElement];). Также кода станет меньше и не нужно будет использовать переменную let temp за пределами цикла. Попробуйте это использовать и упростить реализацию.
Обратите внимание, что в метод может быть передано число 1, и при этом он вернёт true, что неверно, так как число 1 не простое. Добавьте в метод терминальное условие, чтобы он мог работать с любыми числами, в т.ч. 0, 1 и отрицательными. Подсказка - это всё не простые числа. Простые числа
не стоит экономить на символах в именах, интерпретатору этим вы всё равно не поможете, а . Кроме того имя переменной не даёт понимание, какое именно значение она хранит.
не нужно закладывать тип в имя переменной (речь об arr), венгерская нотация очень редко используется в современной разработке. Достаточно использовать имя сущности - progression.
В функциях происходят процессы, поэтому по умолчанию имена функций должны содержать глагол и отражать суть операции которую они описывают. Так бывает не всегда, но в данном случае это именно так. Функция запускает игру и будет понятнее если выразить это действие с помощью глагол run, например. Обращайте внимание на именование функций в вашем коде.
В методах происходят процессы, поэтому по умолчанию имена методов должны содержать глагол и отражать суть операции которую они описывают. Так бывает не всегда, но в данном случае это именно так. Метод запускает игру и будет понятнее если выразить это действие с помощью глагола Обращайте внимание на именование методов в вашем коде.
Магическое число 2 - это зависимость от количества операторов. Если количество операторов изменится и забыть поменять это число - игра будет работать некорректно. Обращайтесь к массиву с операторами operators за этим значением. Используйте свойство length для массивов.
Вывод в консоль, интерактивное взаимодействие с пользователем, сравнение ответа пользователя с правильным - это зона ответственности движка. Игра не может знать то, как используются её данные. Если движку например потребуется не выводить игры в консоль, а например в браузер или отправлять данные по сети - он не сможет это сделать, так как часть его логики разнесена по играм. Кроме того сейчас вам приходится дублировать один и тот же код во всех играх. Как мы обсудили выше, со временем игры могут добавляться в проект, потенциально их может быть 10, 100 и если нужно будет что-то изменить в выводе, или способе использрования игровых данных, придётся рефакторить все игры.
Игра должна только готовить игровые данные: описание игры, вопросы и правильные ответы для каждого игрового раунда. А движок уже должен их использовать так как ему нужно. В данном случае выводить вопросы на экран, получать ответ пользователя и сравнивать его с правильным.
Опишите логику движка в функции. И вызывайте движок в игре передавая в него нужные данные через параметры. Небольшая подсказка: например, вы можете в движок (функцию) передавать также другую функцию, которую движок сможет вызывать и получать нужные данные на каждом из раундов. Постарайтесь выстроить связь: Исполняемый файл -> Игра -> Движок. Так, чтобы исполняемый файл вызывал игру, а игра вызывала движок.