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

Review 2 #2

Open
Catrovitch opened this issue Aug 25, 2022 · 1 comment
Open

Review 2 #2

Catrovitch opened this issue Aug 25, 2022 · 1 comment

Comments

@Catrovitch
Copy link

Catrovitch commented Aug 25, 2022

Review 2

General

For this review I have read all documentation, all code and contemplated the ideas, the execution and possible improvements. In general the code was well written and I can't say that I found any major faults. I have given some thoughts on quite minor aspects which could be nice. Mainly these concern individual functions and where they could be placed within the project structure, some ideas around the visualization and some improvements which could be made in the documentation and specifically the testing document. All in all a really interesting and well executed project!

Code

The code is written in a clear and appropriate manner following good coding practice. The doc-string documentation is also really good and covers all classes and functions.

Structure

The structure of the project is clear and intuitive. Separating the algorithms into its own folder makes sense. One thought: Would it make sense to somehow move the functionality of method "count_time" to its own class in the functionalities folder since this is really not part of the algorithm itself and more of a general functionality. This would also skip some repetition which is considered good. Anyways this is a minor problem and I mostly mentioned this to have something to give constructive criticism about :).

Visualization

I liked that the program had a full fetched graphical user interface. This made it nice to interact with. The colour scheme was also graceful and appropriate. One thing that could be nice is an indication on what the spectrum for a reasonable sized labyrinth is. I for example tried typing in a sized 1000 labyrinth and discovered this was a bit too big and had a really long execution time. Another idea which could be nice is if the visualized solving routes of the Wall follower and Tremaux algorithm would use different colours, say red and blue. It would also be nice if the routes could be left there after the other algorithm is also performed and would not be removed. This would mean that the user could see the two solutions side by side and colour coded. This would be really nice for intuitive comparison of the two different algorithms! All in all I liked the visualization a lot.

Documentation

Määrittelydokumentti

Reading your määrittelydokumentti gave the necessary information in a very condense package. It would have been nice if you would have explained a bit more specifics, but all in all the information needed was given. The toteutusdokumentti holds more specifics so I guess it is not needed to explain things twice.

Toteutusdokumentti

In the toteutusdokumentti the wallfollower algorithm is explained in a good and simple way. You explain the main idea behind the algorithm, that it always follows the right (or left) border of the labyrinth and how this produces a deterministic solution to the same maze. It was also good that you explained that this algorithm would not work for a disconnected labyrinth. It was also good that you recognized that you couldn't find an exact time complexity for the algorithm. If you however find one it would be even better! An other alternative could be that you say that you don't know what it is, but then you reason around how it works and give your thoughts about what it could be!
Your explanation of the Tremaux algorithm is also really good. It is also interesting that it is different from the wallfollower in the sense that it would be able to solve also disconnected labyrinths.
Your explanation of the project structure was also straight forward and explained what was needed without excess flutter. Good job!

Käyttöohjeet

The user guide was once again straight forward and to the point while still explaining the main steps. A question of improvement would be more explanation on how to install Poetry, but this depends on your target audience. In this case everyone who reads the code probably already is familiar with poetry, but if one wasn't it could mean problems. A link to a webpage which explains how to install poetry from scratch would suffice.
I liked that you not only explained how to install and start the program, but also explained step by step what the program does and how this is visualized. This gives the user a better understanding of how to use the program and what everything does. The addition of pictures was gold! Well done!

Testausdokumentti

The documenting on testing was in my opinion a bit lacking. I can see that you have done proper testing, but in the documentation you have only posted pictures of the testing results. What would be nice is if you explained in words what is being tested, how and what the results are. This would ensure better readability, understanding and confirmation that the testing is proper. Of course graphs themselves tell a lot, but from a documentation perspective some verbal explanation would be nice.

@JanneKarki
Copy link
Owner

Thank you for a great review and feedback! Documentation is still in progress and will be finished during this week. Good ideas also about the visualization and count_time method. We'll see if I make improvements there too.

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

No branches or pull requests

2 participants