Open itsaerie opened 4 years ago
First off, it would be useful to comment your code to help people understand your code. For example, if someone didn't know what DFSRec and DFSIter meant (as your code is publicly available on Git), you'd want to assist by adding a comment about what "DFS" means.
https://github.com/angdav/cs435-p2/blob/7773ebbbc02f04d5968b58217efb4db4635591ad/p123.py#L35
When it comes to testing out your code, convention dictates that you should have a main function and throw all of your testing code into it.
https://github.com/angdav/cs435-p2/blob/7773ebbbc02f04d5968b58217efb4db4635591ad/p123.py#L133
WIth your BFT Rec Helper, wouldn't it be more space efficient to not pass the queue downwards?
https://github.com/angdav/cs435-p2/blob/7773ebbbc02f04d5968b58217efb4db4635591ad/p123.py#L92
Readme files often at least define some common acronyms which are used in the program. What does CS435 mean? What is P2? (Is it Part 2, Program 2?)
https://github.com/angdav/cs435-p2/blob/master/README.md#cs435-p2
What is "ret" in this situation? Oftentimes, any confusing and/or shortened variable names will want to have a descriptive (even inline) comment which explains what the variable means.
https://github.com/angdav/cs435-p2/blob/7773ebbbc02f04d5968b58217efb4db4635591ad/p4.py#L49
This would make more sense if your Node had its inside values set as privately accessible.
https://github.com/angdav/cs435-p2/blob/7773ebbbc02f04d5968b58217efb4db4635591ad/p4.py#L13
Same thing with the privately available values.
https://github.com/angdav/cs435-p2/blob/7773ebbbc02f04d5968b58217efb4db4635591ad/p4.py#L32
Stylistically- why are you defining a new method in the middle of what could be your main function and not outside of it?
https://github.com/angdav/cs435-p2/blob/7773ebbbc02f04d5968b58217efb4db4635591ad/p4.py#L112
Would it be simpler to combine the neighbors and weights, then create a dictionary instead of using a set with an array?
https://github.com/angdav/cs435-p2/blob/7773ebbbc02f04d5968b58217efb4db4635591ad/p5.py#L18
Why is this LL just out here? It's separated from a majority of your other code. Also, python styling guidelines encourage us to avoid using hard-to-distinguish characters as variable names, featuring "uppercase 'eye' I, lowercase 'el' l" and so on.
https://github.com/angdav/cs435-p2/blob/7773ebbbc02f04d5968b58217efb4db4635591ad/p5.py#L49
I commented the code as requested.
I added an if __name__ == "__main__":
as requested.
My BFTRecHelper
passes the queue so that the breadth-first-traversal is guaranteed to be in the same order as the iterative counterpart. Otherwise, because multiple BFT's exist, it may not match. The only way I can make them match with a recursive implementation is to pass this queue through as a parameter.
I have modified the README as requested.
I changed ret
to retList
, as I feel that's more understandable.
I have made visited
private with Nodes.
I choose not to make nodes
private in Graphs because my implementation requires it to still be accessed by certain functions
I put printvals
at the start of the main function, to clarify its function. I don't want to put it outside the main function, because then it would be loaded every time this file is imported in the future. printvals
exists solely to test in the main function, so I put it in the main function as well to suggest this.
I keep a set with an array to be consistent with the format of a set of neighbors existing for every node. If I change the structure of this dictionary, it'll be inconsistent with the rest of the code's Node structure. Also, doing so wouldn't save any explicit space anyway.
I fixed the location of ll
and its name, as requested.
All these are shown in the commit: https://github.com/angdav/cs435-p2/commit/0f8931c20ae409797a1921173f8cf032231e54cc
Hi. Lookin' heckin' cute with that uncommented code. Refer to the replies below to see stuff that can be changed for some improvements.