-
Notifications
You must be signed in to change notification settings - Fork 0
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
Code Review #180
Comments
You should commit your code when all parts of an entity is ready. I see you have two other commits about violation entity.. You should avoid this style as long as you don’t have a valid reason. I will try to include those two other commits in my review. I believe your code is clear to understand but you didn’t write any comments. At least you must have documentation for the controller in order to let frontend and android teams to refer. You should carefully read the requirements before you start implementig an entity as it is much harder to change it in the future. I see that modification date field is added later than your first commit. You have no tests written for violation entity. You should complete your tests and commit them as soon as possible. I checked your code with IntelliJ Code Analyzer. Here are some comments i want you to consider. In dtos/ViolationDTO.java, there is an unused import import java.util.Date;. This is because date field once thought to be filled by the user, but then converted to automatically filled with creation date. So you can remove this import. The problem with the unused active parameter exist here also. My conclusion about the code analysis is the following. The mistakes are really easy to detect with IntelliJ Code Analyzer. It also suggest solutions and apply the suggested solution (for example removing the acvitve parameter) by just one click. |
Review the commit ad6207e by omer temel.
The text was updated successfully, but these errors were encountered: