Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feedback #1

Open
wants to merge 51 commits into
base: feedback
Choose a base branch
from
Open

Feedback #1

wants to merge 51 commits into from

Conversation

github-classroom[bot]
Copy link
Contributor

@github-classroom github-classroom bot commented May 14, 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: @bikakuka @oleg01228 @SecretPersona5

github-classroom bot and others added 16 commits May 14, 2024 21:41
So i don't know what's file must be on git, later i can delete it
also add example code in Main.kt with simple window application
also add algorithm to find shorted way between 2 nodes is weighted graph.
Small update
small update
didn't merge from UI+algos becouse idk
Described what functionality is in the application, as well as what is not available to the user in the current version
Copy link

@Almazis Almazis left a comment

Choose a reason for hiding this comment

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

  • Option 2 и 3 можно было все же перименовать.

  • После вызова settings окно с выбором темы невозможно закрыть, все проложение кроме ползунка темы становится неотзывчивым, в том числе выход из приложения (закрыти окно). Хотя сама по себе темная тема классная, лучше чем у многих других команд.

  • Вызов алгоритма поиска ключевых вершин меняет цвет не у ключевой вершины, а у остальных

  • В интерфейсе нельзя создать ребро с весом

  • Очень длинный метод в main

  • Нет тестов. Ни юнит, ни интеграционных

  • Загрузки и сожранения ни в текст, ни в бд

  • .idea лежит на гите

Итоговые баллы.
Наздрюхин Матвей 20. (10/20 за GUI+сохранение+интеграционные тесты т.к. только гуи и с косяками, 10/20 за алгоритмы. Кажется, там все ок, но юнит тесты надо допилить)
Руслан Нафиков 7 (ничего не сделано из первого блока 0/20, и два алгоритма без тестования 7/20)
Чабыкин Олег 0 (по первой части ничего 0/20, по второй части два недоделанных согласно README алгоритма без тестов, еще и не подключены к gui, что делает невозможным их проверку 0/20)

import androidx.compose.ui.window.application
import kotlin.math.min

data class nodeParameters(val colorNode: Color, val int2: Int)
Copy link

Choose a reason for hiding this comment

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

ide ругается на неиспользующееся значение

import kotlin.math.sqrt

open class Graph {
val nodes = mutableListOf<Int>()
Copy link

Choose a reason for hiding this comment

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

Плохо, что только int, тут можно было бы сделать generic

adjacencyList[n]?.remove(m)
adjacencyList[m]?.remove(n)
}
fun findBridges(): List<Pair<Int, Int>> {
Copy link

Choose a reason for hiding this comment

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

Алгоритмы над графами лучше хранить отдельно от самих графов т.к. их слишком много. У вас в решении получается, что какие-то алгоритмы в классе графа, а какие-то вовне

Comment on lines +10 to +11
weights[Pair(n, m)] = weight
weights[Pair(m, n)] = weight // Если граф ненаправленный
Copy link

Choose a reason for hiding this comment

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

Гораздо лучшей имплементацией все же является хранить вес вместе с ребрами. То есть тут плохо была продумана архитектура классов

package algos

fun getStronglyConnectedComponents(graph: DGraph): MutableList<MutableList<Int>> {
val g: MutableMap<Int, MutableList<Int>> = graph.adjacencyList
Copy link

Choose a reason for hiding this comment

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

Почему не обращаться полным именем, будет понятно, к чему обращение


fun getStronglyConnectedComponents(graph: DGraph): MutableList<MutableList<Int>> {
val g: MutableMap<Int, MutableList<Int>> = graph.adjacencyList
val gr: MutableMap<Int, MutableList<Int>> = mutableMapOf()
Copy link

Choose a reason for hiding this comment

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

двубуквенное непонятное название

gr[j]!!.add(i)
}
}
fun dfs1(v: Int) {
Copy link

Choose a reason for hiding this comment

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

что за dfs 1 и 2? Совершенно непонятно

Comment on lines 59 to 95
fun app() {
val windowState = rememberWindowState()
val density = LocalDensity.current
var windowSize by remember { mutableStateOf(windowState.size) }
var windowHeight by remember { mutableStateOf(windowState.size.height) }
var windowWidth by remember { mutableStateOf(windowState.size.width) }
var circlesToDraw by remember { mutableStateOf(mutableMapOf<Int, Pair<Dp, Dp>>()) }
var colorsForBeetweenes by remember { mutableStateOf(mapOf<Int, Float>()) }
var colorsForClusters by remember { mutableStateOf(mapOf<Int, Color>()) }
var isColorsForClusters by remember { mutableStateOf(false) }
var linesToDraw by remember { mutableStateOf(mutableMapOf<Pair<Int, Int>, Pair<Pair<Dp, Dp>, Pair<Dp, Dp>>>()) }
val wdgraph = remember{ WGraph() }

val dgraph = remember{ DGraph() }
val graph = remember{ Graph() }
val bridges = remember { mutableStateOf(listOf<Pair<Int,Int>>())}
val isNodesToFindWay = remember { mutableStateOf(false) }
val isNodesToFindWayD = remember { mutableStateOf(false) }
val shortestWay = remember { mutableStateOf(listOf<Int>())}
val shortWayKeysLines = remember { mutableStateOf(listOf<Int>())}

var selectedCircle by remember { mutableStateOf<Int?>(null) }
var isDragging by remember { mutableStateOf(false) }
var selectedCircleToMove by remember { mutableStateOf<Int?>(null) }
var startConnectingPoint by remember { mutableStateOf<Int?>(null) }
var endConnectingPoint by remember { mutableStateOf<Int?>(null) }
var circleRadius by remember { mutableStateOf(20.dp) }
var expanded by remember { mutableStateOf(false) }
var moveBack by remember { mutableStateOf<Pair<Dp,Dp>?>(null) } // доделать попозже, лень
var additionalOptionsGroup1 by remember { mutableStateOf(false) }
var additionalOptionsGroup2 by remember { mutableStateOf(false) }
var additionalOptionsGroup3 by remember { mutableStateOf(false) }
var openSettings by remember { mutableStateOf(false) }
var isColorsForBeetweenes by remember { mutableStateOf(false) }
var switchState by remember { mutableStateOf(false) }
var turnBack by remember { mutableStateOf(false) }
val colorStates by remember { mutableStateOf(mutableListOf(Color.White, Color.Red, Color.Blue, Color.Gray, Color.Black)) }
Copy link

Choose a reason for hiding this comment

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

Такое хранение и такие длинные функции это прямо плохо. И скорее всего стрельнет в ногу при добавлении хранения в бд и текстовых файлах

}
}
DropdownMenuItem(onClick = {
println("Option 2 clicked")
Copy link

Choose a reason for hiding this comment

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

Логгирования через println в итоговой версии быть не должно

bikakuka and others added 13 commits June 1, 2024 04:17
also add realisation for finding  cycles by node in graph
also add findings cycles by node in graphs
Forgot add this to gitignore, sorry
add in build.gradle.kts logger plugin
it's not working and i don't understand why
Now, when you click on main window of application additional window will be closed without any problems!
also realize revert delete (?). It posible to back removed node
add realization of saving graph in .json
I think it's needed in this code. All for my teammates
:)
SecretPersona5 and others added 19 commits September 11, 2024 00:43
… the GUI

- Fixed the cycle search algorithm, which incorrectly processed graphs with loops.
- Fixed errors in the GUI related to the display of cycle search results.
Got rid of unnecessary implementation of a directed graph
Changes: Added a graph selection window.
Added drawing of an orientation graph.
Reworked the algorithm for finding cycles for a directional graph.

Rewrote tests for the current version of the application.

Fix: fixed bugs that appeared in an attempt to draw a directed graph when using the cycle search algorithm
Fix: now shortest path algorithms work adequately in a directed graph
…nts to the application.

Fix: Fixed bugs with navigation bar scaling
…h layouts, identifying strongly connected components
SecretPersona5 changes ;) aka Ruslan Nafikov
Fix: I forgot to add the resources folder to the commit. I'm correcting. adding icons
…ation-in-the-GUI

Implementation of conservation in the GUI and a lot of things
…ation-in-the-GUI

Fix: Repaired the tests
@Almazis
Copy link

Almazis commented Sep 25, 2024

Нафиков Руслан

Ревью до зачета

Итоговый балл
13 за алгоритмы (Основные косяки: не работает поиск цикла, ориентированный граф отдельно реализован под этот аглоритм)
5 за GUI+сохранение+интеграционные тесты (так оцениваю багофикс в имеющийся код, а кроме багофикса ничего не сделано, да и багует все еще порядком)

Логгирования через println
В решении какие-то алгоритмы в классе графа, а какие-то вовне
Вершины графа могут хранить только инт
Вес хранится отдельно от ребер
Слишком длинная функция main в gui

Новые:
Поиск циклов не работает, считает ребро ненаправленного графа циклом
В GUI нет направленного графа, а соответственно нет и алгоритма поиска компонент сильной связности
После запуска алгоритмов (проверено для поиска цикла и компонент сильной связности, при попытке редактировать граф, часто сыпется unknown error

На зачете были исправлены

  1. Отдельный ориентированный граф, поиск циклов в неориениированном графе. Итого за алгоритмы полный балл 20.
  2. Отображение ориентированного графа в GUI +3

На зачет хватает, оценка Е

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