Skip to content
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

My code review changes #2 #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

My code review changes #2 #5

wants to merge 1 commit into from

Conversation

RIA03B
Copy link
Collaborator

@RIA03B RIA03B commented Oct 24, 2021

Analysis of the program:

Fast-Food application is an application where customers will be able to see a list of food items available within a restaurant, add their desired items into a cart, and check out..

Was the program available on UC GitHub on time?

Yes, The program was available on time.

Is the program easy for you, as an outsider, to understand?

Yes, The application was easy to understand considering they had documentation that helped explain the project.

Does the program compile?

Yes, The program compiles.

Changes I made with explanation:

  1. I added the necessary dependencies. The way it works is that it allows you to define dependencies to external libraries that your project needs, and when you use Maven to build your project it will fetch these libraries from the web (external repositories) and add them to your built project. So it's an automatic handling of dependencies. I added these dependencies based on the errors that were in the program because of the lack of the necessary dependencies.

  2. I created a controller package and moved the FastFoodController.java to that package. While Java doesn't enforce any project structure, it's always useful to follow a consistent pattern to organize our source files, tests, configurations, data, and other code artifacts. If your application gets larger in size proper project structure will be helpful in managing the code and distributing the work in your team.

  3. I fixed the search bar on the navigation bar in the controller file, so that when the user clicks "search" it works properly. I used ResponseEntity to represent the whole HTTP response: status code, headers, and body. As a result, we can use it to fully configure the HTTP response. If we want to use it, we have to return it from the endpoint; Spring takes care of the rest.

  4. I added @EnableCaching annotation in FastFoodApplication.java. It is the annotation-driven cache management feature in the spring framework. If such an annotation is found, a proxy is automatically created to intercept the method call and handle the caching behavior accordingly.

  5. I added @SerializedName to Food.java. @SerializedName annotation indicates the annotated member should be serialized to JSON with the provided name value as its field name.

  6. I added some items in the application.properties file to help when you want to use database, you must define the connection attributes in the application.properties file.

What I learned from this code review:

  1. I learned how to create clean code in Java.
  2. I learned more about ResponseEntity.
  3. I learned more about @EnableCaching annotation.
  4. I learned more about @SerializedName annotation.

Comment on lines +39 to +74
<exclusions>
<exclusion>
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-web</artifactId>
</dependency>
<dependency>
<groupId>com.google.code.gson</groupId>
<artifactId>gson</artifactId>
<version>2.8.5</version>
</dependency>
<dependency>
<groupId>com.squareup.retrofit2</groupId>
<artifactId>retrofit</artifactId>
<version>2.3.0</version>
</dependency>
<dependency>
<groupId>com.squareup.retrofit2</groupId>
<artifactId>converter-gson</artifactId>
<version>2.3.0</version>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-data-jpa</artifactId>
<version>2.3.4.RELEASE</version>
</dependency>
<dependency>
<groupId>org.springframework.kafka</groupId>
<artifactId>spring-kafka</artifactId>
<version>2.5.7.RELEASE</version>
</dependency>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the necessary dependencies. The way it works is that it allows you to define dependencies to external libraries that your project needs, and when you use Maven to build your project it will fetch these libraries from the web (external repositories) and add them to your built project. So it's an automatic handling of dependencies. I added these dependencies based on the errors that were in the program because of the lack of the necessary dependencies.
Source: https://www.wrike.com/project-management-guide/faq/why-should-i-use-dependency-management-in-project-management-software/
https://www.youtube.com/watch?v=793-O43F-ng

Comment on lines +5 to +8
import org.springframework.cache.annotation.EnableCaching;

@SpringBootApplication
@EnableCaching
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added @EnableCaching annotation in FastFoodApplication.java. It is the annotation-driven cache management feature in the spring framework. If such an annotation is found, a proxy is automatically created to intercept the method call and handle the caching behavior accordingly.
Source: https://javabeat.net/enablecaching-spring/

@@ -1,4 +1,4 @@
package com.enterprise.fastfoodapplication;
package com.enterprise.fastfoodapplication.controller;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a controller package and moved the FastFoodController.java to that package. While Java doesn't enforce any project structure, it's always useful to follow a consistent pattern to organize our source files, tests, configurations, data, and other code artifacts. If your application gets larger in size proper project structure will be helpful in managing the code and distributing the work in your team.
Sources: https://www.baeldung.com/java-clean-code

Comment on lines +69 to +86
}
/**
* This is for search bar on the navigation bar.
* This happens when the user click "search".
*
* @return
* */
@GetMapping("/SearchFood")
public String searchFood(@RequestParam(value="searchTerm",required = false,defaultValue = "None") String searchTerm, Model model) {
try {
List<Food> allFood = foodService.getAllFoodItems(searchTerm);
model.addAttribute("allFood", allFood);
return "allFood";
} catch (Exception e) {
e.printStackTrace();
return "error";
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the search bar on the navigation bar in the controller file, so that when the user clicks "search" it works properly. I used ResponseEntity to represent the whole HTTP response: status code, headers, and body. As a result, we can use it to fully configure the HTTP response. If we want to use it, we have to return it from the endpoint; Spring takes care of the rest.

Source: https://github.com/discospiff/SpringBootMicroservicesWithIntelliJIDEA/blob/master/src/main/java/com/myplantdiary/enterprise/PlantDiaryController.java

Comment on lines +8 to +16
@SerializedName("id")
private int foodId;
@SerializedName("foodName")
private String foodName;
@SerializedName("foodCategory")
private String foodCategory;
@SerializedName("foodDescription")
private String foodDescription;
@SerializedName("foodPrice")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added @SerializedName to Food.java. @SerializedName annotation indicates the annotated member should be serialized to JSON with the provided name value as its field name.
Source: https://www.tutorialspoint.com/what-to-use-serializedname-annotation-using-gson-in-java

Comment on lines +1 to +19
spring.jpa.hibernate.ddl-auto=update
spring.datasource.url=jdbc:mysql://localhost:3306/----
spring.datasource.username=-------
spring.datasource.password=-------

spring.profiles.active=dev

spring.servlet.multipart.max-file-size=1024KB
spring.servlet.multipart.max-request-size=1024KB

spring.kafka.consumer.bootstrap-servers=localhost:9092
spring.kafka.consumer.group-id=fastfoodapplication
spring.kafka.consumer.auto-offset-reset=earliest
spring.kafka.consumer.key-deserializer=org.apache.kafka.common.serialization.StringDeserializer
spring.kafka.consumer.value-deserializer=org.apache.kafka.common.serialization.StringDeserializer

spring.kafka.producer.bootstrap-servers=localhost:9092
spring.kafka.producer.key-serializer=org.apache.kafka.common.serialization.StringSerializer
spring.kafka.producer.value-serializer=org.apache.kafka.common.serialization.StringSerializer
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to help when you want to use database, you must define the connection attributes in the application.properties file.
Source: https://www.youtube.com/watch?v=6mrP1CoIfZ8

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bit of new/unused functionality here. Database and Kafka. Only add these when they are required: the feature is complete. Otherwise, it is an incomplete feature, which is clutter, which is technical debt. In other words, don't bloat code for something that might be used in the future.

Comment on lines +83 to +84
e.printStackTrace();
return "error";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception handling is always a good idea.

Recommendation: Use proper logging instead of printStackTrace().

  • Logging has the concept of priority (error, debug, warn, etc.), which printStackTrace doesn't have.
  • No one will see printStackTrace(), as it goes to console.
  • Log messages can be aggregated by several commercial tools. Not so easy with printStackTrace.

@discospiff
Copy link

There are several good suggestions in this branch.

I caution against bloating code with features that are incomplete. Thus, I'd consider each change in isolation, instead of merging the entire branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants