-
Notifications
You must be signed in to change notification settings - Fork 1
Cacheable 주석 처리 #33
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
Cacheable 주석 처리 #33
Changes from all commits
e72c534
1dbbc56
b91f4a7
f19aa3d
7f9a50e
0c7cd50
dd851d2
c098a0c
2a7290c
fbca9f3
4e67e9e
a4eb0a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,9 @@ | |
|
|
||
| import com.example.Jinus.dto.request.RequestDto; | ||
| import com.example.Jinus.service.v2.cafeteria.DietServiceV2; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.web.bind.annotation.PostMapping; | ||
| import org.springframework.web.bind.annotation.RequestBody; | ||
| import org.springframework.web.bind.annotation.RequestMapping; | ||
|
|
@@ -14,6 +16,9 @@ | |
| public class DietControllerV2 { | ||
| private final DietServiceV2 dietServiceV2; | ||
|
|
||
| @Autowired | ||
| private ObjectMapper objectMapper; | ||
|
|
||
|
Comment on lines
+19
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Remove unused ObjectMapper and consider consistent DI style The injected ObjectMapper is never used in this class, making it unnecessary. Additionally, the class mixes constructor injection (via @requiredargsconstructor) with field injection (@Autowired), which is inconsistent. Spring best practices generally recommend constructor injection for required dependencies. If the ObjectMapper is intended for future use, consider: - @Autowired
- private ObjectMapper objectMapper;If it will be needed later, add it through constructor injection instead: private final ObjectMapper objectMapper;
// Update the class to use @AllArgsConstructor instead of @RequiredArgsConstructor
// or create an explicit constructor |
||
| @PostMapping("/v2/dish") | ||
| public String handleRequest(@RequestBody RequestDto requestDto) { | ||
| return dietServiceV2.requestHandler(requestDto); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,12 @@ | ||
| package com.example.Jinus.controller.v2; | ||
|
|
||
| import com.example.Jinus.dto.request.RequestDto; | ||
| import com.example.Jinus.service.v2.notice.CategoryServiceV2; | ||
| import com.example.Jinus.service.v2.notice.NoticeServiceV2; | ||
| import com.example.Jinus.service.v2.userInfo.DepartmentServiceV2; | ||
| import com.example.Jinus.service.v2.userInfo.UserServiceV2; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.web.bind.annotation.PostMapping; | ||
| import org.springframework.web.bind.annotation.RequestBody; | ||
| import org.springframework.web.bind.annotation.RequestMapping; | ||
|
|
@@ -19,6 +20,8 @@ public class NoticeControllerV2 { | |
| private final NoticeServiceV2 noticeServiceV2; | ||
| private final DepartmentServiceV2 departmentServiceV2; | ||
| private final UserServiceV2 userServiceV2; | ||
| @Autowired | ||
| private ObjectMapper objectMapper; | ||
|
Comment on lines
+23
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Remove unused ObjectMapper and consider consistent DI style The injected ObjectMapper is never used in this class, making it unnecessary. Additionally, the class mixes constructor injection (via @requiredargsconstructor) with field injection (@Autowired), which is inconsistent. Spring best practices generally recommend constructor injection for required dependencies. If the ObjectMapper is intended for future use, consider: - @Autowired
- private ObjectMapper objectMapper;If it will be needed later, add it through constructor injection instead: private final ObjectMapper objectMapper;
// Update the class to use @AllArgsConstructor instead of @RequiredArgsConstructor
// or create an explicit constructor |
||
|
|
||
| // 학교 공지사항 조회 | ||
| @PostMapping("/v2/main-notice") | ||
|
|
@@ -32,6 +35,7 @@ public String getMainNotice() { | |
| @PostMapping("/v2/department-notice") | ||
| public String responseDepartmentNotice(@RequestBody RequestDto requestDto) { | ||
| String userId = requestDto.getUserRequest().getUser().getId(); | ||
|
|
||
| int departmentId = userServiceV2.getUserDepartmentId(userId); | ||
| // 학과 영문명 찾기 | ||
| String departmentEng = departmentServiceV2.getDepartmentEng(departmentId); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
|
|
||
| public class JsonUtils { | ||
| private static final ObjectMapper objectMapper = new ObjectMapper(); | ||
| public static final ObjectMapper objectMapper = new ObjectMapper(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider the implications of exposing ObjectMapper as public Changing the visibility of the ObjectMapper from private to public exposes it for external modification and direct access. While this enables reuse across controllers, it introduces risks:
Consider whether the direct access is necessary, or if all JSON processing could be encapsulated through methods in JsonUtils. If direct access is required, consider: - public static final ObjectMapper objectMapper = new ObjectMapper();
+ // Create an immutable public instance with defense against modification
+ public static final ObjectMapper OBJECT_MAPPER = createObjectMapper();
+
+ private static ObjectMapper createObjectMapper() {
+ ObjectMapper mapper = new ObjectMapper();
+ mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
+ return mapper;
+ }And remove the static block since initialization would happen in the createObjectMapper method. |
||
|
|
||
| static { | ||
| // null 값 무시 설정 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Remove unused imports
The imports for JsonProcessingException, ObjectMapper, and Autowired are added but not used in this controller. Unlike other controllers, no ObjectMapper field is injected here.
This creates inconsistency with other controllers and clutters the code.
📝 Committable suggestion