Developing by Dmi4er4 · Pull Request #1 · Dmi4er4/DuoLingo

Dmi4er4 · 2022-04-20T20:14:52Z

Thanks for checking and have a nice day :)

…eritance

shandomruffle

Я честно, как мог, старался душнить, но по существу стилевых замечаний нашлось мало. Зато есть претензии к работоспособности. Глобально никакой паттерн применён не был (хотя, возможно, я его просто не заметил), что, мне кажется, в паре мест помогло допустить баги. А ещё сорсовых файликов в корневой папке как-то много, их бы раскидать по вложенным, что ли — выглядит слишком угрожающе.

По фичам ощущается некоторый недостаток: QSettings не использован, сложность в переводах не изменяется, звуки молчат, об успешном прохождении ничто не уведомляет.

Итого: код очень красивый, жаль, что не работает 🙃

shandomruffle · 2022-05-06T19:58:33Z

+  void GenerateNextPart() final;
+  bool CheckAnswer() final;
+
+  std::vector<std::pair<QString, QString>> exercises_;


[nit] вектор из пар — очень по-спшницки, потом путаница с first и second
сюда бы специальную мини-структурку

shandomruffle · 2022-05-06T19:59:10Z

+  static GrammarQuestion GetGrammarFromJson(QJsonObject exercise_obj);
+};
+
+#endif  // TASKS_LOADER_H_


[style] endline

shandomruffle · 2022-05-06T20:05:57Z

+  layout_->addWidget(task_label_, 1);
+
+  layout_->addWidget(sentence_label_, 2);
+
+  layout_->addWidget(answer_, 3);
+
+  layout_->addWidget(submit_button_, 1);
+
+  layout_->addWidget(progress_bar_, 1);


[style] пустые строки для логического разделения блоков кода — это прекрасно, но не после каждой же строчки )

shandomruffle · 2022-05-07T06:56:58Z

+  };
+};
+
+#endif  // EMPTY_EXERCISE_H_


[style] endline

shandomruffle · 2022-05-07T14:31:06Z

+Приложение имеет меню с кнопками выбора типов приложений: ввод перевода или выбор опции.
+Сверху располагается меню с опциями выбора уровня сложности, от которого зависят наборы заданий и
+опциями звука. Меню сложности также может быть вызвано при помощи комбинации `ctrl+c` Также там расположена информация об оставшихся попытках и начисленных очках.
+При привышении заданного максимального количества ошибок выполнение задания прерывается, а при удачном его завершении без ошибок 


[grammar] превышении )

shandomruffle · 2022-05-07T19:44:07Z

+We are going for a walk
+My name is Billy
+Mr Herrington is reading a newspaper
+Bill Gates is mining on my PC


[minor] такие шикарные файлики, и не используются

предположу, что json-ы генерились из этих файликов (было бы интересно увидеть скрипт, который это делает)

shandomruffle · 2022-05-07T20:01:33Z

+  variant_2_->setText(exercises_[cur_num_question_ - 1].variants[1]);
+  variant_3_->setText(exercises_[cur_num_question_ - 1].variants[2]);
+
+  variant_1_->setChecked(true);


[nit] а правильно ли до ответа делать какой-то вариант предвыбранным?

shandomruffle · 2022-05-07T20:10:15Z

+
+  std::vector<std::pair<QString, QString>> exercises;
+
+  for (const auto &exercise_val : json_exs.value(QString("exercises")).toArray()) {


[nit] auto& exercise_val

shandomruffle · 2022-05-07T20:13:38Z

+    exercises.push_back(GetTranslationFromJson(exercise_obj));
+  }
+
+  std::shuffle(exercises.begin(), exercises.end(), std::mt19937(std::random_device()()));


[nit] вот тут шафл здорового человека, а в tasks_loader.cpp:82 шафл курильщика, который устарел и был выведен в C++17

shandomruffle · 2022-05-07T20:16:00Z

+
+class TasksLoader {
+ public:
+  static std::vector<std::pair<QString, QString>> LoadTranslation(int cnt,


[nit] раз уж QT, то может и структуры использовать из QT, например QVector и QPair? в них и фичи интересные есть, и с qDebug() удобно отлаживаться

ну тут мы на лекциях обсуждали, что QVector пользоваться не рекомендуется

elizabethfeden

это кусочек ревью, позже добавлю :)

elizabethfeden · 2022-05-08T20:11:21Z

@@ -0,0 +1,2 @@
+<?xml version="1.0" encoding="UTF-8"?>


лишние файлики (ниже тоже)

elizabethfeden · 2022-05-09T07:36:27Z

+  connect(exercise_widget_,
+          &ExerciseWidget::IncIncorrectSignal,
+          this,
+          &CentralWidget::IncIncorrect);


в целом ChangeToTranslation и ChangeToGrammar почти не отличаются, можно одну функцию сделать с аргументом о том, что именно создавать. Также и два сигнала (ChangeToTranslationSignal и ChangeToGrammarSignal) можно не создавать, а кидать только один с параметром "на что поменять"

elizabethfeden · 2022-05-09T07:37:58Z

+  layout_->addWidget(exercise_widget_, 1);
+
+  connect(exercise_widget_,
+          &ExerciseWidget::IncScoreSignal,


[style] я бы постаралась стилистически сохранить нейминг сигналов qt, т. е. здесь было бы ScoreIncreased, ну и все другие сигналы тоже

elizabethfeden · 2022-05-09T07:40:32Z

+}
+
+void CentralWidget::IncScore() {
+  emit(IncScoreSignal());


[style] обычно после emit не ставят скобочки (как после return)

elizabethfeden · 2022-05-09T07:42:38Z

+  layout_->removeWidget(exercise_widget_);
+  delete exercise_widget_;
+  exercise_widget_ = new GrammarExercise(this, difficulty_level_);


непонятно, зачем создавать новый виджет каждый раз, учитывая, что у виджетов можно менять сложность, не пересоздавая их. можно же хранить по экземпляру GrammarExercise и TranslationExercise в полях класса, или даже в QStackedWidget, просто переключая в нем текущий выбор

elizabethfeden · 2022-05-09T07:49:54Z

+void ChoiceWidget::ChangeToTranslation() {
+  translation_button_->setStyleSheet("background-color: #8b008b");
+  grammar_button_->setStyleSheet("background-color: #50c878");
+  emit(ChangeToTranslationSignal());
+}
+
+void ChoiceWidget::ChangeToGrammar() {
+  translation_button_->setStyleSheet("background-color: #50c878");
+  grammar_button_->setStyleSheet("background-color: #8b008b");
+  emit(ChangeToGrammarSignal());
+}


[nit] вот эти две функции тоже можно было заменить одной, принимающей, на что меняем

elizabethfeden · 2022-05-09T07:50:27Z

+  emit(ChangeToGrammarSignal());
+}
+
+void ChoiceWidget::MyResizeEvent(QResizeEvent* event) {


а чем qt-шный resizeevent тут не подошел?

elizabethfeden · 2022-05-09T07:55:39Z

+  auto* easy_button(new QRadioButton(tr("Easy")));
+  auto* medium_button(new QRadioButton(tr("Medium")));
+  auto* hard_button(new QRadioButton(tr("Hard")));
+
+  difficulty_buttons_.append(easy_button);
+  difficulty_buttons_.append(medium_button);
+  difficulty_buttons_.append(hard_button);


Suggested change

auto* easy_button(new QRadioButton(tr("Easy")));

auto* medium_button(new QRadioButton(tr("Medium")));

auto* hard_button(new QRadioButton(tr("Hard")));

difficulty_buttons_.append(easy_button);

difficulty_buttons_.append(medium_button);

difficulty_buttons_.append(hard_button);

for (const auto& name : {"Easy", "Medium", "Hard"}) {

difficulty_buttons_.append(new QRadioButton(name))

}

точно, кстати, им при создании не нужно родителя указывать?

elizabethfeden · 2022-05-09T07:57:43Z

+
+#include "exercise_widget.h"
+
+class EmptyExercise : public ExerciseWidget {


architecture-wise, для вещей типа "сгенерить новое упражнение" и "проверить ответ" стоило создать класс, не связанный с qt. нужно разделять логку и ее отображение

зачем вообще, кстати, emptyexercise нужен, непонятно. выглядит, будто и без него можно обойтись

elizabethfeden · 2022-05-09T08:04:07Z

+}
+
+void ExerciseWidget::RestartFail() {
+  auto* wrong_dialog(new QDialog(this));


здесь можно было и стандартным диалогом QMessageBox::information обойтись, там названия кнопок можно менять

elizabethfeden

В целом да, ощущается незаконченность/неотполированность, видно, что не успел добавить фичи и поправить баги.
Архитектура местами кривоватая, хотя с большего норм.
Местами сделано не совсем по тз, предполагалось, например, что главная страница и упражнения не показываются одновременно, а так непонятно, ставить ли баллы за пункты типа "должна быть возможность вернуться к главной странице".
Но все равно молодец конечно, видно, что семестр Qt был не зря, и можешь сделать что-то больше, чем пустое приложение с кнопкой "выход" :)

Антон, ревью отличное, всем довольна.

elizabethfeden · 2022-05-10T15:14:08Z

+We are going for a walk
+My name is Billy
+Mr Herrington is reading a newspaper
+Bill Gates is mining on my PC


предположу, что json-ы генерились из этих файликов (было бы интересно увидеть скрипт, который это делает)

elizabethfeden · 2022-05-10T15:15:54Z

@@ -0,0 +1,14 @@
+#ifndef GRAMMAR_QUESTION_H_


[nit] такие структурки имеет смысл объявлять там же, где они используются, в этом случае в grammar_exercise.h

elizabethfeden · 2022-05-10T15:36:23Z

+  void IncScore();
+  void IncIncorrect();
+
+  void MyResizeEvent(QResizeEvent* event);


а зачем "my"?

elizabethfeden · 2022-05-10T15:39:15Z

+  label_->setText(str);
+}
+void Menu::DecTries() {
+  tries_--;


в целом проверка на отрицательность не помешала бы

elizabethfeden · 2022-05-10T15:41:10Z

+  int score_ = 0;
+  int tries_ = 2;
+  QLabel* label_;
+  MusicClass* music_class_;


меня здесь снова архитектурно смущает, что меню почему-то отвечает за музыку. но здесь скорее проблема снова в том, что смешивается несмешиваемое. класс, который отвечает за настройки музыки и класс, который отвечает за проигрывание музыки - разные.

elizabethfeden · 2022-05-10T15:45:46Z

+  switch (level) {
+    case 0: {
+      file_path = ":/exercises/translation_easy.json";
+      break;
+    }
+    case 1: {
+      file_path = ":/exercises/translation_medium.json";
+      break;
+    }
+    case 2: {
+      file_path = ":/exercises/translation_hard.json";
+      break;
+    }
+    default: {
+      break;
+    }
+  }


или на мапку index/enum -> filename

elizabethfeden · 2022-05-10T15:49:13Z

+  exercises.resize(cnt);
+  exercises.shrink_to_fit();


зачем?

elizabethfeden · 2022-05-10T15:50:51Z

+  return exercises;
+}
+
+std::vector<GrammarQuestion> TasksLoader::LoadGrammar(int cnt, int level) {


[style] много дублирования кода с LoadTranslation

elizabethfeden · 2022-05-10T15:54:54Z

+  QString question;
+  QVector<QString> variants;
+  QString answer;
+  QString tip;


подсказка не используется(

elizabethfeden · 2022-05-10T15:57:13Z

+void TranslationExercise::GenerateNextPart() {
+  progress_bar_->setValue(cur_num_question_);
+  sentence_label_->setText(exercises_[cur_num_question_++].first);
+  answer_->setText(tr(""));


зачем тут tr :) вообще, странно эта штука используется, не на всех строках она есть, да и текст сомневаюсь, что планируется переводить

Dmi4er4 added 22 commits April 12, 2022 09:29

Non-working app shell

82e67d8

Added GrammarWidget basic realisation

da3fb50

Add connections from buttons to exercises. Rename of Widgets. Add inh…

7bdc792

…eritance

Update TranslationExercise

c97222d

Update GrammarExercise. Add GrammarQuestion class

b335ff9

Fix minor bugs. Add inheritance

3107873

Set WordWrap. Return aligns. Change stretches

5c26ffe

Add progress bars. Move common fields to ExerciseWidget

134c7ee

Add keyPressEvent

31d59a0

Add MusicClass

e40d54e

Restart exercise when 3 mistakes made

13a71fe

Pop up hint if h pressed

851336d

Add timer

9698f46

Fix bugs

b754bdb

Increase score counter if exercise is made fully and correctly

ba44b56

Add difficulty change shell

957ca26

Add TaskLoader

fb84642

Add TaskLoader in GrammarExercise

a7f7fa7

Add tips. Fix bugs

4160b7c

Add jsons, add exercises folder, fix memory leaks

b545cd0

minor changes

cf8df0a

Update README. Show tries counter

c71623e

shandomruffle reviewed May 7, 2022

View reviewed changes

elizabethfeden reviewed May 9, 2022

View reviewed changes

elizabethfeden reviewed May 10, 2022

View reviewed changes


		std::vector<std::pair<QString, QString>> exercises;

		for (const auto &exercise_val : json_exs.value(QString("exercises")).toArray()) {


		#include "exercise_widget.h"

		class EmptyExercise : public ExerciseWidget {

Conversation

Dmi4er4 commented Apr 20, 2022

Uh oh!

shandomruffle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elizabethfeden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elizabethfeden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees