mhill10 / SalonSolutions

Easy Scheduling and Goal-Setting Software for Salon Owners, Stylists, and Clients
0 stars 0 forks source link

Peer review - Kelly Palese #6

Closed kpalese closed 4 years ago

kpalese commented 4 years ago

Design/Code Review 3

Project: Salon Solutions

Developer: Matt Hill

Reviewer: Kelly Palese

Category Criteria Rating/Comments
Project Overview
Which planned functionality has been met? The Google Calendar API, the authentication realm, Generic Dao
What planned functionality has not been met? Sign up, Add Clients/Schedule Reservations/Add Services with Pricing & Timing Defaults, View All Reservations, View All users (technically you can view all users now, but I think you intended for the salon owner to have a nice UI to do so), About section, Financial Goals
Describe the GitHub history and what it demonstrates about the project progress during the semester. Progress has been a little on and off, as you know, but there are some really cool things implemented, such as CI-CD.
Describe how peer and instructor feedback/recommendations were incorporated into the project. Looks like the project has been updated to meet the requirements of the checkpoints. I don't see any references to FatBikeTrailReports anymore. As you continue to build your app, keep other comments in mind, such as using the equals() method on each entity so that your DAO tests are more efficient.
Other comments/notes?
JSPs
Evaluate the JSPs for templating, business logic, data validation, overall look and feel. There is a template for the head - it might be helpful to move this to it's own folder within webapp. Then you can include future templates in there too, such as a navbar or footer or whatever you choose to include. Styling is not there yet so I can't comment on the look and feel. As you progress, keep data validation in mind for any user forms.
Other comments/notes?
Java code quality Evaluate the code quality for the following and identify specific areas for improvement (class, method or line number)
  • single-purpose methods
  • well-structured project
  • descriptive naming of packages, classes, methods, variables
  • classes appropriately-sized (no monster classes)
  • CPD (copy paste detection, meaning are the same lines of code repeated?)
  • are there candidates for super/subclass relationships, abstract classes, interfaces?
  • are any values hard-coded that should be in a properties file?
  • proper exception handling
  • proper error reporting to the user
  • code documentation
  • I'm having issues with the md file, so I'll include specific comments at the bottom of my review (I initially used Word to write my comments). However, I did use QAPlug
    Logging Evaluate the use of logging, for example:
  • appropriate use of logging statements in the code for error logging and debugging
  • logging levels used - info, debug, error
  • no occurrences of System.out.printlns or printStackTrace()
  • logging works on AWS deploy
  • I don't see any System.out.prints, but also it doesn't look like Log4J is implemented yet.
    Unit Tests Evaluate the unit tests, for example:
  • tests are truly a unit test rather than a high level functional test
  • test data is appropriately cleaned up or handled
  • there is full coverage of methods that perform business logic or utility functions
  • redundant code is eliminated by using set up and tear down methods, i.e., @Before, @After
  • Other comments/notes? Unit tests are limited so far. Your UserDao test will be more streamlined once you implement the equals() method on your entities.
    Security Evaluate authentication/authorization: Currently there is authentication but it looks like it's only for the admin role? Will there be any other roles? Also, you could put your protected jsps in a separate folder so it's easier to keep track of what requires authentication and what does not.
    Is it implemented correctly and working locally as well as on AWS?
    Other comments/notes? I could not test AWS because your instance was down (so is mine...)
    Web Service/API integration Evaluate the service/api integration, for example:
  • Which service/api is implemented?
  • How is error handling of the service implemented? For example, what happens if the service is not available?
  • Other comments/notes? The Google Calendar API is ready to be implemented. It seems that you can receive data, now it's just time to put it into your app.
    Independent research topic What is the independent research topic? CI/CD seems to already be implemented. Materialize and JavaScript are yet to some (though I don't know if you need to do ALL three prof dev topics? Maybe double check with Paula)
    Is the independent research topic/technique implemented in the project?
    Other comments/notes?
    Deployment Has the application been successfully deployed to AWS? I know it has been, but right now the instance is down for me so I can't comment on functionality.
    Is the hosted application fully functioning?
    Other comments/notes?

    Code Quality Comments

    • (From QAPlug) Your jsps have a tag at the top and an tag at the bottom which don’t match up with anything. • (From QAPlug) Your CSS style sheet is not in a separate folder, yet in head.jsp has it marked as being in a styleSheets directory. • (From QAPlug) head.jsp has a ${title} attribute that is so far empty on all of your jsps. • (From QAPlug) The “language=’java’" at the top of most of your jsps is redundant. • (From QAPlug) It doesn’t like your elements in your table…but it’s displaying properly, right? I guess if it’s not displaying the way you want it to, look into the th elements.

    kpalese commented 4 years ago

    I just noticed my comment about html tags (fifth bullet point from the bottom) doesn't appear correctly because the html tags I wrote were absorbed into the post. It's a < /head > and a < /html > tag that can be deleted.

    mhill10 commented 4 years ago

    Thank you @kpalese! Really appreciate such thoughtful feedback! FYI, I am not certain if I provided you the correct AWS link (not that there is much to see there yet). You mentioned the Instance was down, but I am not showing that on my end (although, I suppose it could have been down at the time you were writing your review). At any rate, the correct address is: mysalonsolutions.com

    Thanks again!

    kpalese commented 4 years ago

    Aha! Yes, mysalonsolutions.com works beautifully. I had tried this link: http://3.20.142.31:8080/SalonSolutions/

    Thank you, also, for your helpful feedback!