search

GlynisF / WeatherTracker

Advanced Java team project -- Glynis Fisher, Kevin Grau & Peter Sokor
0 stars 1 forks source link

Team Project Ready for Review #10

Open GlynisF opened 9 months ago

GlynisF commented 9 months ago

Hi Paula-- we made changes to our project and are ready for review.

We don't have a database implemented, but I think/hope the changes we made demonstrates implementing rest.

@pawaitemadisoncollege

pawaitemadisoncollege commented 9 months ago

Thanks @GlynisF and @misterSokor. Can you please post your presentation that demonstrates the items listed on this page: http://paulawaite.com/education/java113/Module3/Week12/index.html.

This helps me understand the project and where to focus my time when reviewing code. Thanks!

pawaitemadisoncollege commented 9 months ago

Hi @GlynisF and @misterSokor. After watching your team's presentation, I have a number of questions which I posted in slack and am copying here since slack messages will archive after 90 days and I don't want to lose that information. Please respond to the questions either here and in Slack, or simply here.

Presentation comments and questions

  1. I think you mentioned JavaFX. Can you share more about what that is, and why you chose to use it in this project? What benefit did it provide over the structure for java applications that we have been using?

  2. From the code review and demo, it looks like the code in your project consumes a weather service and has a web front end as an interface to that service. Did I understand correctly that this is a web application?

  3. Is there an aspect of this project that is a RESTful api? Meaning we could access it using soapui or postman, sending in a request and receiving a response? Creating a RESTful api was the goal of the team project.

A few things I was looking for in your presentation that were missing:

a. User documentation - this is the documentation somebody consuming a service would use to understand how to use your service. The petstore api has an excellent example of user documentation: https://petstore.swagger.io/ It shows the resources, such as pet, store, etc., and the various “operations” it supports. b. Demonstration of calling your restful api. Use soapui, postman, curl, etc. b. Unit tests? c. Challenges and Key learning points? Ideally each team member shares this.

Project Code Review

In the interest of time, I'm mostly going to focus on areas for improvement so that you can use this learning in your indie project. Note that I reviewed the main branch only.

Kudos:

  1. Good use of properties for your api key.
  2. It looks like you made good use of branching.
  3. Home page is a good-looking landing page.
  4. Good use of the properties loader interface.

Areas for improvement:

  1. Missing project plan, timelog/journal, documentation of the resources, methods, and data format supported/provided.
  2. Problem statement does not really describe the problem your api solves. Why should somebody use your api versus another weather api?
  3. use log4j rather than printlns and stacktraces.https://github.com/GlynisF/WeatherTracker/blob/0c02b29d255560e803e2cdc36b47435ea561e5d3/src/test/java/com/weathertracker/persistence/WeatherDaoTest.java#L56-L58, and here, https://github.com/GlynisF/WeatherTracker/blob/0c02b29d255560e803e2cdc36b47435ea561e5d3/src/test/java/com/weathertracker/persistence/WeatherDaoTest.java#L74, for example.
  4. There's some duplicate lines in the two jsps. Create smaller, jsps that can be included rather than copy/paste code.
  5. Properties file has your api key in it, this is publicly visible on github right now. Better not to push files like this to github.
  6. You have the base.url in the properties which is the right idea. Use it here: https://github.com/GlynisF/WeatherTracker/blob/0c02b29d255560e803e2cdc36b47435ea561e5d3/src/main/java/com/weathertracker/persistence/WeatherDao.java#L49
  7. It's good that you are using a logger here. Rather than debug, this should be error: https://github.com/GlynisF/WeatherTracker/blob/0c02b29d255560e803e2cdc36b47435ea561e5d3/src/main/java/com/weathertracker/persistence/WeatherDao.java#L67
  8. Missing javadoc on most classes and methods.
  9. Remove dead code (don't worry git will keep track of this for you if you want to retrieve it later). https://github.com/GlynisF/WeatherTracker/blob/0c02b29d255560e803e2cdc36b47435ea561e5d3/src/main/java/com/weathertracker/controller/servlets/WeatherTrackerServlet.java#L97-L151
  10. Some commits include a lot of changes. Review best practices: http://paulawaite.com/education/java113/ProgrammingBestPractices.html#commit-early-and-often
  11. I'm really curious about the choice to use JavaFX here. I've heard of this being used to create desktop applications, but not so much for web applications.
  12. It looks like properties are loaded every time the WeatherDao is instantiated? How might you load properties a single time for the application instead - remember the servlet context and the application start up in Adv Java project 4?