Feedback #1
Feedback #1
Conversation
1) Added Compose for Desktop base structure. 2) Initialized README.md. 3) Added LICENSE.txt.
Create graph class graph.kt Create 2 graph generators: star.kt and flower_snark.kt Create GraphView class to process visual component of graph Create GraphViewComponent to use GraphView in terms of compose
More options to graphView. Change alpha, shape
Create class to parse all algorithms
Adding AlgorithmView Component
Added a toggle to hide/show left side menu with animation.
Refactored code for Algorithms menu. Renamed App name.
refactored the structure of the code to make it more readable and understandable.
GraphView now have simple layout algo Now NodeViewUpdate is working and you can write your own algorithm's
added a reverse method that will be needed for community detection
Added initial version of community detection using Leiden algorithm.
Fixed a problem when GrapView didn't refresh itself after applying algorithm action.
Fixed an isse when graph's nodes become immovable after user changes the algorithm
1) Reformatted code to make consistent tabs and spaces. 2) Renamed files and class names and composable function to make them more descriptive.
VertViewUpdate class created Update support not only NodeViewUpdate, but also VertViewUpdate
Adding Crtl+Z function using stack with "negative updates"
removed unnecessary function and unnecessary prints
Refactored the project to meet the requerements of the MVVM architecture.
Now algoritms situased in model.algoritms
1) Added a menu on the top left bar 2) Added a possibility to open multiple windows
Changed the way we collect the algorithms to the list and display them.
There was a problem hiding this comment.
Почему сделано через функции верхнего уровня, а не используя синглтон
There was a problem hiding this comment.
Для чего этот класс? Он нигде явно не используется
This comment was marked as resolved.
This comment was marked as resolved.
При удалении сохранённого графа, он удаляется из базы данных. Однако, к сожалению, мы упустили из виду необходимость обновления локального списка сохранённых графов. В результате, невозможно сразу же добавить граф с таким же названием, и для обновления локального списка приходится переоткрывать окно работы с графом. |
This comment was marked as resolved.
This comment was marked as resolved.
| import androidx.compose.runtime.Stable | ||
|
|
||
| @Stable | ||
| open class Graph { |
There was a problem hiding this comment.
Почему решили реализовать ориентированный и неориентированный граф, используя открытый класс, а не, например, с помощью абстрактных классов?
There was a problem hiding this comment.
Ориентированный граф открыт, т.к. от него наследуется неориентированный граф. Абстрактным его не сделали, чтобы была возможность его использовать. А неориентированный не является открытым.
|
|
||
| @Stable | ||
| open class Graph { | ||
| var vertices: MutableMap<String, MutableSet<Pair<String, Float>>> = mutableMapOf() |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Показалось это излишним. Решили, что достаточно хранить граф просто как список смежности.
|
Уточню, что задавая вопросы, я не подразумеваю, что ваше решение неверное (за некоторым исключением может быть). Мне интересно обоснование выбора конкретного решения |
| val data = mutableMapOf<String, String>() | ||
| it.split(" ").forEach { | ||
| if ("=" in it) { | ||
| data[it.split("=")[0]] = | ||
| it.split("=")[1] | ||
| .trim('>') | ||
| .trim('/') | ||
| .trim('"') | ||
| } | ||
| } |
| val data = mutableMapOf<String, String>() | ||
| it.split(" ").forEach { | ||
| if ("=" in it) { | ||
| data[it.split("=")[0]] = | ||
| it.split("=")[1] | ||
| .trim('>') | ||
| .trim('/') | ||
| .trim('"') | ||
| } | ||
| } |
|
|
||
| tag.startsWith("<node") -> { | ||
| if (!curData.isEmpty()) { | ||
| throw NullPointerException("Invalid File Format: No node exit \nProblem in line \"$it\"") |
There was a problem hiding this comment.
Можно вынести в константу
|
|
||
| tag.startsWith("<edge") -> { | ||
| if (!curData.isEmpty()) { | ||
| throw NullPointerException("Invalid File Format: No edge exit \nProblem in line \"$it\"") |
There was a problem hiding this comment.
Можно вынести в константу
| idToKey[curData["id"]!!] = curData["id"]!! | ||
| graph.addNode(curData["id"]!!) | ||
| } catch (e: NullPointerException) { | ||
| throw NullPointerException("Invalid File Format: No node id\nProblem in line \"$it\"") |
There was a problem hiding this comment.
Можно вынести в константу
| idToKey[curData["id"]!!] = curData["key"]!! | ||
| graph.addNode(curData["key"]!!) | ||
| } catch (e: NullPointerException) { | ||
| throw NullPointerException("Invalid File Format: No node id or key\nProblem in line \"$it\"") |
There was a problem hiding this comment.
Можно вынести в константу
| graph.addNode(curData["id"]!!) | ||
| curData = mutableMapOf() | ||
| } catch (e: NullPointerException) { | ||
| throw NullPointerException("Invalid File Format: No node id\nProblem in line \"$it\"") |
There was a problem hiding this comment.
Можно вынести в константу
| curData["source"] = data["source"]!! | ||
| curData["target"] = data["target"]!! | ||
| } catch (e: NullPointerException) { | ||
| throw NullPointerException("Invalid File Format: No edge source or target\nProblem in line \"$it\"") |
There was a problem hiding this comment.
Можно вынести в константу
| idToKey[curData["target"]!!]!! | ||
| ) | ||
| } catch (e: NullPointerException) { | ||
| throw NullPointerException("Invalid File Format: No edge source or target\nProblem in line \"$it\"") |
There was a problem hiding this comment.
Можно вынести в константу
| ) | ||
| } catch (e: NullPointerException) { | ||
| throw NullPointerException( | ||
| "Invalid File Format: No edge source, target or weight\nProblem in line \"$it\"" |
There was a problem hiding this comment.
Можно вынести в константу
| ) | ||
| } catch (e: NullPointerException) { | ||
| throw NullPointerException( | ||
| "Invalid File Format: No edge source or target\nProblem in line \"$it\"" |
There was a problem hiding this comment.
Можно вынести в константу
| } | ||
| } | ||
| } catch (e: IndexOutOfBoundsException) { | ||
| throw NullPointerException("Invalid File Format: No needed data\nProblem in line \"$it\"") |
There was a problem hiding this comment.
Можно вынести в константу
There was a problem hiding this comment.
В принципе здесь используется много строковых значений, которые стоило бы вынести в константу
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
А можно пожалуйста конкретный файл, который отрабатывает не верно. Поправим.
There was a problem hiding this comment.
Повторно не получается воспроизвести. Возможно я тогда не так посмотрел. В любом случае значит, что ошибки нет)
This comment was marked as resolved.
This comment was marked as resolved.
У меня тоже macOS, и у меня всё работает. Возможно, вы что-то делаете не так. Нужно выбрать две вершины, удерживая кнопку “Shift” (у выбранных вершин края станут жирными). Затем, если вы пользуетесь мышкой, нужно нажать правой кнопкой мыши на пустую область графа, а не на саму вершину. Обычно мы нажимаем между двумя выбранными вершинами, но это не обязательно — любая пустая зона графа подойдёт. |
Действительно работает, я делал иначе, без шифта. Можете тогда на всякий случай в ридми добавить более подробную инструкцию |
|
Еще классная вещь, которую не сразу заметил и за что точно плюс балл: поддержка нескольких активных окон в одном приложении |
Спасибо большое за то, что заметили эту функцию! Как я уже говорил, я тоже пользуюсь macOS, и, к сожалению, там по умолчанию нет возможности открыть несколько окон в любом приложении, как на Windows. Поэтому я постарался реализовать эту функцию, но мои коллеги по команде не оценили её и посчитали ненужной. Очень рад, что кто-то это заметил! |
This comment was marked as resolved.
This comment was marked as resolved.
Это два интеграционных от меня. Остальные интеграционные являются по совместительству тестами UI и лежат в файле IntroWindowViewTest.kt |
Пропустил что-то, спасибо! |
|
Перед концом ревью хотели спросить. У нас тесты запускаются не напрямую в github actions, а в Docker, из-за того, что github actions не поддерживает UI(по крайней мере у нас не вышло их подружить). Насколько оптимален этот метод и как такое делать правильно в следующий раз? |
|
Эх, видимо поздно спросили. Ревью уже кончилось... |
|
Подведем итоги Работа сделана очень достойно, понравилось использование фишечек Котлина, многие юайные решения, прикольные фишечки вне тз по типу тем, доната и нескольких окон на макоси. Из недостатков: предпочтение в некоторых местах функциям верхнего уровня вместо абстракций с иерархиями. Также использование местами строковых переменных вместо классов-перечислений и вхардкодженные строки вместо констант. По итогу получился хороший проект первого курса. Но есть куда расти. Если есть желание продолжать работать с Котлин, могу посоветовать "Kotlin в действии" (ISBN:978-5-97060-497-7). А для ООП "Паттерны объектно-ориентированного проектирования" (ISBN:978-5-4461-1595-2). Итоговый балл вам сообщит Владимир Александрович |
В тз не было написано как именно стоит автоматизировать тесты, так что на это не смотрел. Предположу, что для тестов юай в Github Actions нужна эмуляция (раз уж не получилось напрямую сделать), которую вы сделали с помощью докера |
Возможно есть другие, более оптимальные средства для решения этой проблемы, но мне не приходилось с этим сталкиваться |
|
Поняли, спасибо за ревью
Поняли, спасибо за ревью |
After deleting the graph, then trying to save new graph with the name of the previously deleted graph, app didn't let user to create graph with that name. Fixed it.
Fixes of Readme instructions, false detection of similar names of graph and removed unused files.
👋! 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
mainsince the assignment started. Your teacher can see this too.Notes for teachers
Use this PR to leave feedback. Here are some tips:
mainsince 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”.main. Click a commit to see specific changes.For more information about this pull request, read “Leaving assignment feedback in GitHub”.
Subscribed: @VersusXX @AlexandrKudrya @7gambit7