hornunjb / Weather-Wear

Group Project for IT3048C
0 stars 0 forks source link

Meyer3cj codereview2 #16

Closed meyer3cj closed 3 years ago

meyer3cj commented 3 years ago

Analysis of the program I think it is a pretty relevant idea that people will use. I think this app could have a lot more functionality that I did not see while using it. I think the coding conventions are well used. I think more unit tests could have been used.

Was the program available in UC Github on time? Yes, the program was on github on time.

Is the program documented/commented well enough for you to understand? I think it is commented well, but I do think the group needs to be more specific about what the app is doing. I am confused about how the recommendations are being made. Are they providing the recommendations or is the user supposed to make decisions based on the weather data.

Does the program compile? Yes, the program does compile.

Rationale behind your changes. Most of the things I found were look and feel related. Though I didn’t change that much, I think I have made some changes that were helpful to the project. I made these changes based on what a user might think when they use the app or what another developer would think if they were working on it. I thought about consistency, relevance to the current state of the program, and eliminating redundancy when I was making my changes. I will explain my rationale for the changes in the pull request.

Three Technical Concepts I learned I have not coded a lot of methods for my project, so I learned how to write a method in Kotlin. I learned about how files are linked together due to comments in the code. I learned how to pull data from an api.

discospiff commented 3 years ago

I'm neutral on merging this branch. I encourage the group to review the suggestions and then determine whether or not to integrate them.

McMas1999 commented 3 years ago

After reviewing the pull request personally I do not think we should integrate. I think most changes are negligible at the moment. However I think we should remove unused variables.

hornunjb commented 3 years ago

I've gone ahead and implemented the relevant code review suggestions from this branch locally. See commit here.