Dvandenburg117 / cs435-Project2

0 stars 0 forks source link

Code Review #11

Open GAstraeus opened 4 years ago

GAstraeus commented 4 years ago

https://github.com/Dvandenburg117/cs435-Project2/blob/7b152a6af3e1488c8b55049bd1a316897ad72666/part1#L5

Make sure this file is saved as a python file rather than just a blob

GAstraeus commented 4 years ago

https://github.com/Dvandenburg117/cs435-Project2/blob/7b152a6af3e1488c8b55049bd1a316897ad72666/part1#L13 You should remove pass once code is filled in

GAstraeus commented 4 years ago

https://github.com/Dvandenburg117/cs435-Project2/blob/7b152a6af3e1488c8b55049bd1a316897ad72666/part1#L12 Inset line spacing between different functions to make this more readable

GAstraeus commented 4 years ago

https://github.com/Dvandenburg117/cs435-Project2/blob/7b152a6af3e1488c8b55049bd1a316897ad72666/part1#L118 It looks like you are calling the function on 3 parameters however the function declaration below takes two parameters only https://github.com/Dvandenburg117/cs435-Project2/blob/7b152a6af3e1488c8b55049bd1a316897ad72666/part1#L110

GAstraeus commented 4 years ago

https://github.com/Dvandenburg117/cs435-Project2/blob/7b152a6af3e1488c8b55049bd1a316897ad72666/part1#L118 I suggest making a helper function so that you are able to pass the visited list and keep track of that

GAstraeus commented 4 years ago

https://github.com/Dvandenburg117/cs435-Project2/blob/7b152a6af3e1488c8b55049bd1a316897ad72666/proj2%20full.py#L12 With a dictionary, you can directly access the value. You shouldn't need a loop to find and accumulate your list of neighbors you can directly access it through the key of the dictionary and return the value.

GAstraeus commented 4 years ago

https://github.com/Dvandenburg117/cs435-Project2/blob/7b152a6af3e1488c8b55049bd1a316897ad72666/proj2%20full.py#L29 This vertex object is made but is never used or saved anywhere.

GAstraeus commented 4 years ago

https://github.com/Dvandenburg117/cs435-Project2/blob/7b152a6af3e1488c8b55049bd1a316897ad72666/proj2%20full.py#L33 I noticed you were only inserting the id of the node into the adjacency list rather than the node object itself. If you pass the node you will have less "translating" to do between ids and nodes

GAstraeus commented 4 years ago

https://github.com/Dvandenburg117/cs435-Project2/blob/7b152a6af3e1488c8b55049bd1a316897ad72666/proj2%20full.py#L142 I think you can override the get neighbors function with this one to prevent having to make new functions names

GAstraeus commented 4 years ago

https://github.com/Dvandenburg117/cs435-Project2/blob/7b152a6af3e1488c8b55049bd1a316897ad72666/proj2%20full.py#L1 Splitting up this code into multiple files could help with readability

Dvandenburg117 commented 4 years ago

OK. amended in https://github.com/Dvandenburg117/cs435-Project2/blob/master/traverse_this_town.py