Feedback

[github-classroom]

👋! 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: @kar1mgh @spisladqo @qrutyy

[TimPushkin]

Hi, I'm an (almost) former SE student who was asked to review and grade your code. Since the whole project uses English I'll also stick to that tradition.

I was told that your deadline was 23:59 May 29 but I see several significant commits made afterwards. I started my review on commit ff30eef, so this is the state I will be grading you for (that commit was actually made several hours past the deadline already).

General comments:

• Code style is pretty decent but it could have been better. Pay attention to your IDE (IntelliJ IDEA is recommended for Kotlin): there is a lot of warnings (unused variables, code duplication, unrecommended use of Kotlin's features...), you can even see some of them in your CI.
• There are problems with architecture. On higher level you chose to use MVVM but the layers are mixed it your code: there are pieces of UI in model, view model's logic in UI. You also sometimes violate abstraction and encapsulation principles of OOP, especially in UI-related code (e.g. a lot of properties and methods that could have been private). Code duplication is also a huge problem: there are quite a few classes, tests and even whole files that duplicate each other symbol by symbol.
• UI looks cute but it could use some refactoring: split huge functions onto separate elements and reuse them to get rid of code duplication, remove unused variables, replace strings with enums, make better use of Compose's features
• Would be nice to see more documentation: in README (what algorithms exactly are used, how the IO formats look internally), in code (links to articles/papers describing the algorithms with their pseudo-code), in UI (what is an "SCC", how key vertices are defined)
• Great commit messages and PR descriptions 👏

In the comments below I use "💡" for comments/suggestions and "❗️" for more critical issues.

[TimPushkin]

Issues I've found while using the app (some are just UX-related wishes):

• ❗️ When the app starts, if I press "Import" then don't select anything and press "Import" again I get java.sql.SQLException: Graph with ID 0 not found. in the terminal and end up with a blank screen with no way to do anything, the only way from there is to restart the app
• 💡 After opening a graph there is no way to return to the main menu except to close the app
• 💡 When the window is resized the GUI gets stretched for a few seconds before coming back to normal
• 💡 If the window is resized to the smallest size possible the left menus are cropped (especially noticeable on "Analyze" and "Save & Load" tabs), it would help if they were vertically scrollable
• 💡 It would be nice to have bigger graph as examples, a few hundred nodes at least. When I was doing this task 3 years ago we had a requirement to support full graphs with up to 10000 nodes (you don't have this requirement)
• "General" tab:
  • 💡 When creating a vertex I need to input some data to be stored in it but there seems to be no way to view that data anywhere in the app
  • 💡 If a vertex ID is too long (even two digits long) it is not possible to see the whole ID in the GUI, it gets cropped on the vertex borders
  • When I add a vertex the app asks me to input a second vertex for the first one to be connected to it:
    • 💡 Is there no way to create a stand-alone vertex? I actually check what will happen if I manually add such vertices in the SQLite database and it seems to be displayed correctly.
    • 💡 Even if I select "Integer" as the data to be stored I can input arbitrary strings like "asdasdsa" and they are accepted
    • ❗️ If there already exist some vertices then I'm asked to input an ID of the second vertex. If I don't input anything and press "Connect" I get java.lang.NumberFormatException: For input string: "" and the app crashes
  • 💡 When adding an edge if one of the vertices doesn't exist the related error message will only appear for the first time and if the same situation happens again it just won't appear until the tab is reopened
    • 💡 Actually, the same seems to happen with all such messages, e.g. in "Analyze" tab too
  • 💡 There is no way to specify the edge weight in a weighted graph
• "Analyze" tab:
  • ❗️ For the layout algorithm, the buttons don't seem to work, nothing happens when I press them. 💡 And what do each of the sliders represent? Some meaningful names would really help.
  • 💡 The algorithm for finding key vertices gives strange results for your graph named "test_graph", I cannot really understand how the vertices it chooses can be defined as the key vertices in any meaningful way. It would be nice to have some documentation in README or in the app itself explaining what exactly the algorithm is supposed to find.
  • ❗️ The algorithm for finding SCCs crashes the app with java.lang.ArrayIndexOutOfBoundsException: Only 15 colors supported on "test_graph"
  • 💡 There should be two algorithms for finding graphs implemented: Dijkstra's and Bellman-Ford's, but in the UI it's just "Find shortest path" — it would be nice to know when each of them is used
• "Save & Load" tab:
  • SQLite:
    • 💡 Just as in the main menu, if I press "Load" then don't select anything and press "Import" I get an exception in the terminal (though the app doesn't crash)
    • ❗️ Loading a graph doesn't work: I can select a graph but it is not imported and after that pressing "Load" again does nothing until I reopen the tab (this is probably the same problem as with the error messages appearing only once)
    • 💡 For some reason, "test_graph" has vertex type "Integer" in the database but it is imported (from the main menu) as having type "String", at the same time graph "hahahahahha" has type "Int" but is saved as having type "Integer" — to sum up, data types are not treated correctly
  • ❗️ Support for Neo4j and JSON is not implemented
  • 💡 It would be nice if positions and colors of vertices were also saved and restored — this was actually a requirement when I was doing this task (but for you it is not)

Ultimately, the UI looks cute but there are quite a few bugs, vertex data seems to be broken (is not used anywhere, data type is not checked), saving/loading only works for SQLite (partially, since loading is only possible via the main menu).

[qrutyy]

• When the window is resized the GUI gets stretched for a few seconds before coming back to norma

Hm, strange, i haven't found that)

• It would be nice to have bigger graph as examples, a few hundred nodes at least. When I was doing this task 3 years ago we had a requirement to support full graphs with up to 10000 nodes (you don't have this requirement)

You are totally right!

• When creating a vertex I need to input some data to be stored in it but there seems to be no way to view that data anywhere in the app

Yep, only ID's. It would be good to implement (f.e. user needs to see what data have nodes inside the community))

• The algorithm for finding SCCs crashes the app with java.lang.ArrayIndexOutOfBoundsException: Only 15 colors supported on "test_graph"

Seems to be and UI integration issue, cant say anything about it. SCC was totally tested as a unit

• For the layout algorithm, the buttons don't seem to work, nothing happens when I press them. 💡 And what do each of the sliders represent? Some meaningful names would really help.

I repied to this issue in the comments below. Sliders represent the parameters of the T-FDP model, there is an explanation of it in the code. #toaddfaqbutton ;)

[TimPushkin]

💡 More edge-cases are needed. You basically have three variations of the same graph with most of the graph tests only using them. E.g. what if something breaks on a graph with a single vertex, with a single edge, with two parts not connected to each other...

[TimPushkin]

💡 I know the algorithm is not implemented by you but still would be nice to have at least a single test with a more complex graph (with more than one vertex), just to ensure it works at all

[TimPushkin]

❗️ I would say its just a bad test where several things are combined and its hard to say what exactly is tested.

First, you put a graph into a view model and then check that FAQBox displays the right graph name. I won't say this is an integration test since you actually put a string into the view model and just check that this string is displayed. The sole purpose of this string is to be displayed so I would argue this is a single component even though several system levels (view model and view) are involved.

Second, you put an undirected graph into the view model and check that it correctly determined the type of edges of the graph. I would also argue that this is a single component: you test the functionality of the view model, that it is able to read the graph.

Some examples of what I would consider as integration tests:

1. Import a graph from some of your supported formats and then run an algorithm on that graph: this way you would check that the graph importing component of your system correctly interacts with the implemented algorithms.
2. Closer to what you did, if you imported a graph instead of creating it manually in the test's code and then checked that FAQBox displays the correct graph name, I would also accept that as an integration test (the components interacting are import system and view model / UI).

[TimPushkin]

💡 There is a problem with your test setup. In all tests but this you use JUnit 5 (org.junit.jupiter:junit-jupiter-api:5.10.2) but for this UI test you use JUnit 4 (org.jetbrains.compose.ui:ui-test-junit4:1.6.2), probably because Android and, by extension, Compose doesn't support JUnit 5. Because of that, to make Gradle run all of the other tests I have to specify tasks.test { useJUnitPlatform() } in build.gradle.kts but than this test is not being run and vice versa. There probably is a way to make them work together but it is out of my concern.

[qrutyy]

Yep, i found out that only at the end, bc i couldn;t find any examples of UI testing in case of compose using the JUnit4, so i managed to delete the task , that caused problems with unit-tests)

[qrutyy]

Btw, only recommendation i saw - to delete the task)

[TimPushkin]

❗️

1. Why do you expect edges of an undirected graph to be directed? I think this works only because you have used an empty graph and for an empty collection of edges all { ... } returns true.
2. This assert is the one from Kotlin's stdlib, it will for only when JVM is launched with assertions enabled (-ea flag, it seems to indeed be enabled in tests) and then throw AssertionError on failure. What you need here is assertTrue() from JUnit.

[TimPushkin]

Now, I need to grade you. Each of you had to implement a "main" algorithm, two "classical" algorithms, implement saving/loading of graphs into one of the formats, write unit tests for all the functionality you have implemented and write at least a single integration test for the app.

• Karim Shakirov:

  • Main algo: communities (works, but I have some doubts about results translation from JetBrains API to your format)
  • Classical algos: cycles (inefficient subgraph creation), Bellman-Ford (ok)
  • Saving/loading: JSON (not implemented)
  • Unit tests: for all your algos (not enough tests for communities, but ok since the implementation is not yours) + for general model functionality (a lot of duplication), no tests for JSON IO (because there is no JSON IO, yeah)
  • Integration test: -

• Mike Gavrilenko:

  • Main algo: layout (works automatically when modifying the model but there is no way to control its parameters through UI)
  • Classical algos: SCCs (ok), Dijkstra (ok)
  • Saving/loading: SQLite (saving works, even though there are some bugs; loading only works from the main menu, also has bugs; the whole implementation belongs to the model layer but it is partially directly tied to UI; overall it was the only IO format implemented when I reviewed the code, so you'll have some bonus points for that)
  • Unit tests: SCCs (ok), Dijkstra (ok), no tests for layout algo and SQLite IO
  • Integration test: a poor one but at least you've tried 💪

• Daniel Vlasenco:

  • Main algo: key vertices (strange results for directed graphs but it's probably a problem of the chosen centrality itself; somewhat inefficient, has a redundant step)
  • Classical algos: bridges (ok), MST (many data copies)
  • Saving/loading: Neo4J (implemented after the deadline, so I haven't reviewed it; probably will be considered unimplemented but we'll see)
  • Unit tests: for all your algos (ok), no tests for Neo4J IO
  • Integration test: -

I'll give you some time to correct me if I'm wrong somewhere in my comments (focus on the problems I've highlighted in the general review message and in this message since they are the main ones). Let's say, I'll wait until 21:00 today and then I'll tell the grades to your teachers.

[shakareem]

Hello!! Actually, Mike worked with SqLite and I worked with JSON.

[TimPushkin]

I wanted to try out Neo4J but there are no instructions on how to set this thing up (I have no idea what AuraDB is). Of course I could ask Google but it is your responsibility to explain how to use your app. My point is that there should be instructions in README or even in UI.

@spisladqo The code looks like it should do something meaningful but I cannot verify it since there are no tests and no instructions. Maybe I'll add you some points for this but not much since the implementation came after the deadline and I cannot verify whether it actually works.

[TimPushkin]

💡 A singleton with a complex state is usually not a good programming pattern since you introduce a global state available from everywhere in the program and it's not clear where it is used

[TimPushkin]

💡 You don't need this as a separate property with its own backing field, you can just check neo4jRepo != null

Suggested change

	var isRepoInit = false
	private set
	val isRepoInit: Boolean
	get() = neo4jRepo != null

[TimPushkin]

💡 I thought, Neo4jRepository is responsible for database interaction. You could've created a companion object inside it and place this function there to make it available even when the repository is not instantiated.

[spisladqo]

Thanks, I thought about this exact problem and didn't come up with something good, so ended up with something easy.

[TimPushkin]

❗️ I think sessions should be closed after use but this one is not. You should've used it as driver.session().use { ... } to make sure it is always closed.

[TimPushkin]

❗️ Actually, the same also goes for the driver itself.

[TimPushkin]

❗️ Does this ever get called? It looks like once it has been used it stays opened forever which is not right. The right way would probably be to close everything after the graph has been saved/loaded. This way you won\t need Neo4jRepositoryHandler as a global singleton since the database connection would be opened and closed locally in the code that handles saving/loading. If you want to let the user to load/save multiple graphs without re-entering their credentials, you should either just cache the credentials or leave the connection open but find some other way to close it at some moment.

[TimPushkin]

💡 If you wanted to do logging, there are special libraries for that. You even have some of them imported in Gradle (org.slf4j:slf4j-api:1.7.36, ch.qos.logback:logback-classic:1.4.12) but I don't think I've seen you using them anywhere.

[spisladqo]

Sorry, I think I just forgot to remove this piece. Yes, we imported a logger into Gradle, but we just ended up with throwing exceptions/error messages.

[qrutyy]

• Mike Gavrilenko:

  • Main algo: layout (works automatically when modifying the model but there is no way to control its parameters through UI)
  • Classical algos: SCCs (ok), Dijkstra (ok)
  • Saving/loading: SQLite (saving works, even though there are some bugs; loading only works from the main menu, also has bugs; the whole implementation belongs to the model layer but it is partially directly tied to UI; overall it was the only IO format implemented when I reviewed the code, so you'll have some bonus points for that)
  • Unit tests: SCCs (ok), Dijkstra (ok), no tests for layout algo and SQLite IO
  • Integration test: a poor one but at least you've tried 💪

Good evening, Tim!


I wanna say few words about the project and our team work in whole.

First of all, thank you for your review. I didn’t expect this much of comments, thats very cool! 


I apologise, I had no time to get to the end of your comments, bc 1 hour is not enough to check them all and send some replies. 
Our project could have been more successful, and i wanna share some development details(

1. Start

At the start of the project, we have a lot of time developing the model. In my opinion, that stopped us a lil)

2. UI

I started developing the UI, while my team was dealing with other tasks. So when they finalized it, they were struggling a lot, diving in the world of Compose. -> UI wasn’t refactored, and really checked by other. (Thats my fault)


3. Layout + SQL

Layout also caused some issues. I couldnt test it, bc i was dealing with the stupidest bug of my life, while implementing the SQL connection. -> I couldn’t test the layout -> Till the pretty much last day I thought that it worked fine. Well, i managed to implement it the other way, using FFT, but that cut my ability to connect it with the UI (so i could set different parameters)


4. Integration tests

Integration tests were also implemented before i managed to fix the bug) So…. i couldnt make a “real” integration test and try to understand the topic of using TestContainers (as i understood, there is really no need to use them in case of SQLite). So i tried to use only backend-frontend connection, relying on definition of the “integration tests” that referred only to the relationship between two different layers of the system (backend, frontend, db in our example)

5. Some thoughts

Btw, I tried and was successful in saving the generics from deleting from project, but i had really no time, to handle exceptions, errors (type handling)


This project was a great experience. Thanks to my team!

[qrutyy]

• , no tests for layout algo and SQLite IO

Was it required? (the layout testing)

As i understood, SQLite IO should be tested only as a part of integration test)

[spisladqo]

Sorry, my fault that I didn't drop explanation on what Neo4j AuraDB is. You can try these properties, which are for a test database I used:

URI = neo4j+s://c7593bad.databases.neo4j.io
USER = neo4j
PASSWORD = 1UmNjK0StlwYP47tKIdWxPsx6rlC3ucXIXaGOkHISAQ

(and I didn't manage to do graph import yet, so only save functionality is there)

And also thank you for your review! It is very informative and helpful, these small (and sometimes big) things that were left unnoticed really have a very big impact on the whole project.

[TimPushkin]

@qrutyy

, no tests for layout algo and SQLite IO
Was it required? (the layout testing)

As i understood, SQLite IO should be tested only as a part of integration test)

Yes, of course, it's just a functionality the same as any other. You need to check that your SQL statements work at all. If in your test, for example, you just call a function that executes an SQL statement and creates a piece of model from it, that's a unit test since only the functionality of the SQLite IO is tested. If then you also run an algorithm on a graph you've imported from SQLite, then it would be an integration test since the algorithm is a piece of functionality completely independent from SQLite IO (you can unit-test it independently).

First of all, thank you for your review. I didn’t expect this much of comments, thats very cool! 


I apologise, I had no time to get to the end of your comments, bc 1 hour is not enough to check them all and send some replies.

Yeah, I wanted to give you more time but the project is pretty big so I've spent more time reviewing it than I initially anticipated. You don't have to answer all of my comments, most of them are just recommendations. I even would recommend you to answer them only if you have some questions or don't agree with me.

And regarding the fact that you haven't got enough time, this is absolutely normal. Working in teams is hard, especially in the first year when you have almost no experience. Everybody have some problems with these tasks every year.

[TimPushkin]

@spisladqo

Sorry, my fault that I didn't drop explanation on what Neo4j AuraDB is. You can try these properties, which are for a test database I used:

Tried it, saving indeed seems to work, though there is not much to verify visually :)

Anyway, I'll now go figure out what grades to give you all.