levitesuo / algoritmit-harjoitusty-

0 stars 0 forks source link

Peer review 2 #2

Open jakubgrad opened 4 months ago

jakubgrad commented 4 months ago

Hi! I downloaded your repository on 26th of April 6:30pm. Here are my thoughts

Testing

-Testausdokumentti is empty! Even though it's a perfect place to explain how do you the test files in /src/tests, or describe what you plan/have tested so far. The visualization is very usable, so even commenting on the fact that you "visually checked" that the programme works is valuable for the document. Also a simple thing to add would be to write:

poetry shell
pytest src

which makes it more obvious what to do. At first I assumed there is no tests because poetry run invoke test didn't work and there was no such task in tasks.py. -The course materials provided a lot of useful testing tools. I can't imagine developing my code without tests, and I run tests almost every time I make a major change to my programme. They are super useful! Definitely check out the course materials on testing, and definitely implement at least some unit tests! Making a coverage-report, lint, and format tasks could also help, feel free to check out my tasks.py file.

Documentation

Naming

height_mapping_function "determines the cost of moving from one cell to another based on their position and height difference", so perhaps it could be simply called cost. Similarly, heuristic_function could simply be heuristic. Other method and function names are intuitive :+1:

Lack of UI

Judging by the code, the outcome, and the pictures in documentation I'm convinced the programme works, but it lacks an interface through which I could change the parameters of the algorithm, and according to the course materials, it's super important! Even a simple CLI interface like this or the would work. The parameters could be as simple as start and end points and the seed. A cli.py is also a perfect way to refactor some of the stuff that otherwise lands in index.py and making it easier to read.

Comments and docstrings

It's nice to see comments in your code like

        # Linked list has a default start node at size ** 2

But on a higher level, it would be nice to know what each function does using docstrings. E.g. in a_star.py I can find both find_path and a_star functions:

def find_path(goal, nodes, size):
....
def a_star(start_cord, goal_cord, grid, heurestic_function=heurestic_function, height_mapping_function=height_mapping_function):

It would be easier to read the code if find_path and a_star had docstrings that explain what each function does and what kind of input they take. After all, it's not obvious at first if a_start returns the shortest path, produces a path, draws the map or if that's the responsibility of find_path.

Positive things

The visualization is very cool :sunglasses: ! Choosing the different option from Dijkstra, Fringe and A Star I can see what paths the algorithms have taken, turn the map around, make a picture and easily get the grasp of what the application does