Ready to merge - Database
closes #30 (closed)
Merge request reports
Activity
assigned to @c23102007
requested review from @d1634883
added In Progress label
changed milestone to %Week 9 - Sprint #2
added Ready to Merge label and removed In Progress label
Code Review: To begin, database branch that the assignee intends to merge with main is a number of commits behind the main branch and required correcting before branch is merged to main.
As per the acceptance criteria of #30 (closed), criteria 1,2,3,5 are completed. The branch has a beans error when attempting to load the remaining location and trail mock-data tables into a html page, which fails the acceptance criteria 4, of showing the data tables in a html page.
As per tasks, all but the final 2 tasks have been successfully completed, code is to a high standard, readable and easy to follow, no commenting, though class names, SQL tables and the respective table variables are self explanatory, I have no issue with understanding anything.
To rectify the above issues with location and trail data not showing, the trailsRepositoryJDBC class file must be changed to " public class trailsRepositoryJDBC implements trailsRepository", you forgot the implements, which caused the beans issue.
To solve the location table not showing up correctly, this appears to be caused by the fact that your location tables share the same name as your location. Class file in the Team5.SmartTowns.Data package. Ensuring that all the table-related "location" names are modified to locations, should also rectify that issue.
To close, sorry for the long wall of text!! Your code is very clear, good and in working order. There's just a few accidental hiccups causing issues that need to be fixed before implementing the merger. I understand that in todays lecture-session a couple of lecturers tried to help but couldn't find the error and that after a group discussion we agreed to merge the request due to the necessity for a working database, however, these fixes shouldn't take long.
Thanks for the changes, unfortunately there now seems to be a bean error with the user table. To fix this issue, you should;
- The branch is still behind main and needs to be updated using " git merge main" in the git bash branch command line
- Rename "userId" to "userID" on line 23 of the UserRepositoryJDBC java class
- Within schema.sql, the varchar(128) seems to have gone missing. You need to replace name varchar with name varchar(128) for all three tables, as it allocates the number of bytes allocated within each string ( The two above points combine to re-produce a bean error with the user table).
- Could you also rename the trail table to trails, as we already have an associated trail file and it may get a little confusing when calling the table element trail while referring to the table as trail as well.
- And finally, could you also fix the schema.sql table format, it works but the incremental tabbing of the tables looks a little funky.
- If possible could you see about adding comments to each Team5.SmartTowns.Data table files i.e., something like;
- for location.java "Holds variable data for locations table"
- locationRepository.java "Holds locations data repository"
- locationRepsoitoryJDBC.java "Implements the locations repository using JDBC"
Overall, you've done a good job fixing the code re: my last review, just these small changes and then we should be good to go, the tables print the correct mock-data and fit the assessment criteria, we just need to do these few fixes before it's ready to merge.
Edited by Rhys Nuteadded 98 commits
-
49efe83d...290d61c0 - 97 commits from branch
main - 1e184fbc - database
-
49efe83d...290d61c0 - 97 commits from branch
