ftranghese / EC601-Fall2017-Seamless-Track-Detection

16 stars 8 forks source link

Code Review2 by Chunxi Wang #27

Open tracycxw opened 6 years ago

tracycxw commented 6 years ago

Code Review 2

Overview

Hey guys, I am so excited to look through your codes. Since last code review, you have made a great progress and solve some problems that we suggested in the previous code review. Well done and Congratulations!

I find out you upload a new folder called main code, so I believe in this folder, the codes are all your codes that needs to run your program.

Problems

There're some problems when I check your codes.

1.Instruction When I try to follow the instruction to run this python file, it goes wrong. I have no idea about how to fix it, maybe you have come across the same problem and should write in your readme.md. Also, I believe there are many other solutions of track detection, so in the README.md, you can show the difference between your project and others.

2. Code formatting 2.1 code quality I have checked every file and the problems are almost the same. Take line_fit_video.py file for example. I checked it by pep8 and pylint, the results are shown as follows. I do not care about the tab errors. So we can see it is good that pep8 has little problems about the length of code lines. But there are more issues should be taken care of in pylint. As you can see, invalid constant name and unused variables are too much. Pylint and pep8 are very important in developing, they can improve the elegance and make others easily to get your points. They can also help you save the storage memory and improve the efficiency of your codes. I think it is not difficult for you to change these issues.

(pep8:)

2017-12-03 3 39 42

(pylint:)

2017-12-03 3 36 03

2.2 command lines In the files, I notice that you have add lots of comments, this is really good and clear for me to read your codes. Well Done!!! 👍

2.3 code style In the Line.py, line19-25, if you define the variables in one line, such as: self.A, self.B, self.C = [], [], [] self.A_arg, self.B_arg, self.C_arg = 0, 0, 0 In this way, the code can become more concise.

In the dilation_try.py, line 7-22, I believe annotations in these lines are unused codes and should be deleted, they are unnecessary. If you want to go back to the previous codes, maybe you should use git diff if you prefer command line.

In the dilation_try_new.py, there are many repeat lines in line41-56, if you write a def function, the code will be shorter and clearer. (There are some same problems like pep8 and unused codes should be removed, I will not mention it again in the following). Also, I do not understand the difference between dilation_try and dilation_try_new. What the differences between them? Which file do you use at the end? Maybe you should declare it in the README.md.

In the perspective_transform.py, there is no comment.

3. Architecture Apparently you have followed SOLID principles and the architecture is good. But it will be better if you develop an iOS or Android application, this will make target people use it more easily instead of downloading your python files. Setting environment may take a lot of time and easily make target people give up.

Output Video

I have watched the video on Youtube. It is really nice, you can well determine the lane boundaries, the lane curvature and vehicle position. It is really impressive. I notice that in the video, your car drives in one lane continuously, but in the real world cars always change the lanes. So I hope in the future, you can improve your codes which can make changing lanes possible.

Summary

In summary, your hardworking really impresses me, hope to see your iOS or Android application some day. And I am also looking forward to your Sprint 4 and final presentation! Keep pushing yourselves.