Skip to content

Feedback#1

Open
github-classroom[bot] wants to merge 238 commits intofeedbackfrom
main
Open

Feedback#1
github-classroom[bot] wants to merge 238 commits intofeedbackfrom
main

Conversation

@github-classroom
Copy link
Contributor

@github-classroom github-classroom bot commented Apr 24, 2024

👋! GitHub Classroom created this pull request as a place for your teacher to leave feedback on your work. It will update automatically. Don’t close or merge this pull request, unless you’re instructed to do so by your teacher.
In this pull request, your teacher can leave comments and feedback on your code. Click the Subscribe button to be notified if that happens.
Click the Files changed or Commits tab to see all of the changes pushed to main since the assignment started. Your teacher can see this too.

Notes for teachers

Use this PR to leave feedback. Here are some tips:

  • Click the Files changed tab to see all of the changes pushed to main since the assignment started. To leave comments on specific lines of code, put your cursor over a line of code and click the blue + (plus sign). To learn more about comments, read “Commenting on a pull request”.
  • Click the Commits tab to see the commits pushed to main. Click a commit to see specific changes.
  • If you turned on autograding, then click the Checks tab to see the results.
  • This page is an overview. It shows commits, line comments, and general comments. You can leave a general comment below.
    For more information about this pull request, read “Leaving assignment feedback in GitHub”.

Subscribed: @odiumuniverse @Ycyken @admitrievtsev

odiumuniverse and others added 28 commits April 29, 2024 18:43
the algorithm will only work for undirected graphs; oriented ones will
be converted to undirected
Implement basic gui, remove ios and linux native support
- standart linter
- coverage tests
- gradle test
change: test coverage is start for every push
Now need a description and 2 approvals for the pull request
Ycyken and others added 10 commits May 29, 2024 13:18
Merge branch 'main' into ycyken/neo4j
1. Fix bugs in db's: fix queires (names with spaces, digits etc)
2. Fix styling, sizes of dialogs, sizes of localisation fonts
3. Implemented theme changing depending on saving type you chosen
Copy link

@vacmannnn vacmannnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В целом, конечно, все хорошо и вы молодцы, но

Работа с гитом

Названия коммитов более-менее системные, но не без косяков (зачем писать add: _add_ smthng и подобное ?), а вот с названиями PRов не нашел чего-то однообразоного. В ПРы залезали какие-то лишние вещи. Так же не понял, где искать процент покрытия.
P.s. названия коммитов хоть и более-менее системные, но не без заметных регулярных косяков
image

Ридми

Не совсем понимаю, это задумка в минимализм или нет времени, но ладно. Просто напоминаю типичную структуру ридми приложения:

  • краткое описание проекта с фоткой/гифкой работы
  • как запустить/скачать
  • особенности использования (настройки, в вашем случае еще логи пароли от БД)
  • авторы/лицензия и подобное

Мягко говоря, меня ввело в ступор, когда я тыкнул на neo4j, а от меня просят пароль, логин и путь куда-то. Я ввел admin;admin и предложенный дефолтный путь. Приложение сначала зависло минуты на 2 (в логах, кстати, не было ничего), потом вылетело, и еще минуты через 3 вылетела ошибка (после закрытия приложения) о проблеме с neo4j. После этого приложение категорически не запускалось
image

Код

Общих глобальных замечаний нет (самое большое - не понятно, где сохранение в плоский файл), но, возможно, повлияла ограниченность в возможности проревьювить (время + нет встречи очно и не потыкать вас в лайве)

GUI !

1 (меню). Local file — не переводится на русском + непонятные иконки которые не грузятся
image

2 (сохранение). Графы не сохраняются и сохраняются одновременно: если закрыть и открыть заново приложение, то вершин не будет, если добавить в этот же граф новые вершины, то их номера будут уже не с 0, как будто есть какие-то вершины. + в sql связи не хранятся, как я понял. Также, когда я выходил из графа, не сохраняя вершины, а потом заходил в этот граф (без перезапуска приложения), то вершины сохранялись.

Из интересного, что не успел проверить: что, если сохранить граф в sqlite (условно), в настройке поставить другую бд и открыть приложение заново с этим же графом.
image

3 (общее).

  • Хоткеи в строке поиска не работают (ctl + a точно)

  • Иконка добавить в добавлении тускнеет после введения названия (казалось, что должно быть наоборот)

  • Enter не работает в строках поиска

  • Хочется видеть сообщения об ошибках прямо в GUI (когда вводишь вершину, которой нет, и подобное)

  • Тряска при раскладке (не понял причину тряски, но уровень нормальный)

  • В меню хочется видеть не "алгоритм ...", а конкретное действие (т.к. без контекста не понятно, за что отвечают все алгосы)

  • Кнопки удаления вершин нет

  • Хочется более удобный способ создания рёбер

  • Тип графа не перевелся

image

Общие оценки:
Всем по 30 баллов (ведение ГХ, грубые косякие GUI и мелкие косяки везде)

runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ ubuntu-latest, macos-latest ]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

почему на винде бы не забилдить ?

Comment on lines +22 to +25
// Note, if you develop a library, you should use compose.desktop.common.
// compose.desktop.currentOs should be used in launcher-sourceSet
// (in a separate module for demo project and in testMain).
// With compose.desktop.common you will also lose @Preview functionality

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

это точно тут нужно ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

за что отвечает этот файл ?

Comment on lines +18 to +27
val data = Json.decodeFromString<TranslationList>(File("src/main/resources/localisation/$language.json").readText())
for (wordPair in data.transList) {
if (wordPair.code == text){
return wordPair.localisation
}
}
return text
}
catch (ex: Exception){
File("src/main/kotlin/localisation/logs/localisationError.log").appendText("LOCALISATION ERROR ${LocalDateTime.now()} -- Key $text is not found in $language\nEXCEPTION IS $ex\n")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Можно попробовать, для гибкости, пути вынести в отдельные переменные или конфиги (в лучшем случае)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
session.executeWrite { tx ->
tx.run("CREATE CONSTRAINT IF NOT EXISTS FOR (n: `$graphName`) REQUIRE (n.value) IS UNIQUE;")
tx.run("CREATE INDEX IF NOT EXISTS FOR (n: `$graphName`) ON (n.value);")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

что такое индекс и зачем они нужны ?

Copy link

@vacmannnn vacmannnn May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

вопрос про возможность инъекции так же актуален

Comment on lines +64 to +65
"code": "reset",
"localisation": "Сбросить цвета"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

название кода не однозначно, reset_color

Comment on lines +88 to +89
"code": "find_mst",
"localisation": "Минимальное остовное дерево"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"code": "find_mst",
"localisation": "Минимальное остовное дерево"
"code": "find_mst",
"localisation": "Найти/построить минимальное остовное дерево"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

odiumuniverse and others added 6 commits June 14, 2024 04:29
Co-authored-by: Gleb Nasretdinov <135718038+Ycyken@users.noreply.github.com>
Co-authored-by: Gleb Nasretdinov <135718038+Ycyken@users.noreply.github.com>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

может еще гиф/фото добавить ? 🥺
Пример: https://github.com/spbu-coding-2022/trees-7

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

У меня гифка вышла на 66мб просто,в ближайшем будущем допилим

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants