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

Feat 191 #192

Merged
merged 3 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@ public class FilterEntity extends BaseEntity {
@Column(nullable = false)
private String filterColor;

// @OneToOne(fetch = FetchType.LAZY)
// @JoinColumn(name = "member_id")
// private MemberEntity member;

@Column(nullable = false)
private Long memberId;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package com.example.demo.schedule.application.dto;

import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;

import java.time.LocalDateTime;

@Getter
@AllArgsConstructor
@NoArgsConstructor
@Builder
public class SemesterScheduleResponse {
private String eventName;
private String eventStartDate;
private String eventEndDate;
Comment on lines +15 to +17
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using proper date types instead of Strings for date fields

Using String for date fields (eventStartDate and eventEndDate) can lead to:

  • Inconsistent date formats
  • Difficulty in date comparisons and calculations
  • Potential parsing errors

Consider using LocalDateTime or LocalDate:

public class SemesterScheduleResponse {
    private String eventName;
-   private String eventStartDate;
-   private String eventEndDate;
+   private LocalDateTime eventStartDate;
+   private LocalDateTime eventEndDate;
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private String eventName;
private String eventStartDate;
private String eventEndDate;
private String eventName;
private LocalDateTime eventStartDate;
private LocalDateTime eventEndDate;

}
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,12 @@ public List<AllPrivateCalendarResponse> getPrivateSchedule() {
return responseConverter.toPrivateModel(model);
}

public List<SemesterScheduleResponse> getSemesterSchedule() {
LocalDateTime requestNow = LocalDateTime.now();
List<ScheduleEntity> semesterScheduleResponses = scheduleJpaRepository.findOneSemesterPublicSchedule(requestNow);
return responseConverter.toSemesterScheduleResponse(semesterScheduleResponses);
}
Comment on lines +76 to +80
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling and documentation for getSemesterSchedule

The method lacks:

  1. Error handling for empty results
  2. Documentation explaining the business logic
  3. Separation of concerns (entity to DTO conversion should be in converter)

Consider this improvement:

+   /**
+    * Retrieves public schedules for the current semester.
+    * @return List of semester schedules, empty list if none found
+    */
    public List<SemesterScheduleResponse> getSemesterSchedule() {
        LocalDateTime requestNow = LocalDateTime.now();
        List<ScheduleEntity> semesterScheduleResponses = scheduleJpaRepository.findOneSemesterPublicSchedule(requestNow);
+       if (semesterScheduleResponses.isEmpty()) {
+           // Consider if this should throw a custom exception or return empty list
+           return Collections.emptyList();
+       }
        return responseConverter.toSemesterScheduleResponse(semesterScheduleResponses);
    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public List<SemesterScheduleResponse> getSemesterSchedule() {
LocalDateTime requestNow = LocalDateTime.now();
List<ScheduleEntity> semesterScheduleResponses = scheduleJpaRepository.findOneSemesterPublicSchedule(requestNow);
return responseConverter.toSemesterScheduleResponse(semesterScheduleResponses);
}
/**
* Retrieves public schedules for the current semester.
* @return List of semester schedules, empty list if none found
*/
public List<SemesterScheduleResponse> getSemesterSchedule() {
LocalDateTime requestNow = LocalDateTime.now();
List<ScheduleEntity> semesterScheduleResponses = scheduleJpaRepository.findOneSemesterPublicSchedule(requestNow);
if (semesterScheduleResponses.isEmpty()) {
// Consider if this should throw a custom exception or return empty list
return Collections.emptyList();
}
return responseConverter.toSemesterScheduleResponse(semesterScheduleResponses);
}



public List<ScheduleEntity> findWeekendSchedule() {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.example.demo.filter.persistence.FilterRepository;
import com.example.demo.schedule.application.dto.*;
import com.example.demo.schedule.domain.model.ScheduleModel;
import com.example.demo.schedule.infrastructure.persistence.ScheduleEntity;
import org.springframework.stereotype.Component;

import java.time.LocalDateTime;
Expand Down Expand Up @@ -64,6 +65,20 @@ private AllPublicCalendarResponse toPublicCalendarResponse(ScheduleModel model)
.build();
}

public List<SemesterScheduleResponse> toSemesterScheduleResponse(List<ScheduleEntity> entities) {
return entities.stream()
.map(this::buildSemesterResponse)
.collect(Collectors.toList());
}

private SemesterScheduleResponse buildSemesterResponse(ScheduleEntity entity) {
return SemesterScheduleResponse.builder()
.eventName(entity.getEventName())
.eventStartDate(entity.getEventStartDate().toString())
.eventEndDate(entity.getEventEndDate().toString())
.build();
}


public List<AllPrivateCalendarResponse> toPrivateModel(List<ScheduleModel> models) {
return models.stream()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.example.demo.filter.persistence.FilterEntity;
import com.example.demo.schedule.domain.ScheduleRepository;
import jakarta.persistence.EntityManager;
import jakarta.persistence.TypedQuery;
import org.springframework.data.jpa.repository.Query;
import org.springframework.data.repository.query.Param;
import org.springframework.stereotype.Repository;
Expand Down Expand Up @@ -32,6 +33,38 @@ public ScheduleEntity save(ScheduleEntity entity) {
}
}

public List<ScheduleEntity> findOneSemesterPublicSchedule(LocalDateTime requestDate) {
int month = requestDate.getMonthValue();
int year = requestDate.getYear();

LocalDateTime startDate;
LocalDateTime endDate;

if (month >= 2 && month <= 7) {
// 2월 ~ 7월
startDate = LocalDateTime.of(year, 2, 1, 0, 0);
endDate = LocalDateTime.of(year, 7, 31, 23, 59, 59);
} else if (month >= 8 && month <= 12) {
// 8월 ~ 다음해 1월
startDate = LocalDateTime.of(year, 8, 1, 0, 0);
endDate = LocalDateTime.of(year + 1, 1, 31, 23, 59, 59);
} else { // month == 1
// 전년도 8월 ~ 해당 연도 1월
startDate = LocalDateTime.of(year - 1, 8, 1, 0, 0);
endDate = LocalDateTime.of(year, 1, 31, 23, 59, 59);
}

String jpql = "SELECT s FROM ScheduleEntity s " +
"WHERE s.eventStartDate BETWEEN :startDate AND :endDate";

TypedQuery<ScheduleEntity> query = em.createQuery(jpql, ScheduleEntity.class);
query.setParameter("startDate", startDate);
query.setParameter("endDate", endDate);

return query.getResultList();
}
Comment on lines +36 to +65
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Several improvements needed for robustness and performance.

The implementation has the following concerns:

  1. Missing filter for public schedules
  2. No pagination for potentially large result sets
  3. Hardcoded date ranges could be constants
  4. No sorting specified

Consider this improved implementation:

+ private static final int FIRST_SEMESTER_START_MONTH = 2;
+ private static final int FIRST_SEMESTER_END_MONTH = 7;
+ private static final int SECOND_SEMESTER_START_MONTH = 8;

 public List<ScheduleEntity> findOneSemesterPublicSchedule(LocalDateTime requestDate) {
     int month = requestDate.getMonthValue();
     int year = requestDate.getYear();

     LocalDateTime startDate;
     LocalDateTime endDate;

-    if (month >= 2 && month <= 7) {
+    if (month >= FIRST_SEMESTER_START_MONTH && month <= FIRST_SEMESTER_END_MONTH) {
         // 2월 ~ 7월
-        startDate = LocalDateTime.of(year, 2, 1, 0, 0);
-        endDate = LocalDateTime.of(year, 7, 31, 23, 59, 59);
+        startDate = LocalDateTime.of(year, FIRST_SEMESTER_START_MONTH, 1, 0, 0);
+        endDate = LocalDateTime.of(year, FIRST_SEMESTER_END_MONTH, 31, 23, 59, 59);
-    } else if (month >= 8 && month <= 12) {
+    } else if (month >= SECOND_SEMESTER_START_MONTH && month <= 12) {
         // 8월 ~ 다음해 1월
-        startDate = LocalDateTime.of(year, 8, 1, 0, 0);
+        startDate = LocalDateTime.of(year, SECOND_SEMESTER_START_MONTH, 1, 0, 0);
         endDate = LocalDateTime.of(year + 1, 1, 31, 23, 59, 59);
     } else { // month == 1
         // 전년도 8월 ~ 해당 연도 1월
-        startDate = LocalDateTime.of(year - 1, 8, 1, 0, 0);
+        startDate = LocalDateTime.of(year - 1, SECOND_SEMESTER_START_MONTH, 1, 0, 0);
         endDate = LocalDateTime.of(year, 1, 31, 23, 59, 59);
     }

     String jpql = "SELECT s FROM ScheduleEntity s " +
-            "WHERE s.eventStartDate BETWEEN :startDate AND :endDate";
+            "WHERE s.scheduleType = :scheduleType " +
+            "AND s.eventStartDate BETWEEN :startDate AND :endDate " +
+            "ORDER BY s.eventStartDate ASC";

     TypedQuery<ScheduleEntity> query = em.createQuery(jpql, ScheduleEntity.class);
+    query.setParameter("scheduleType", "PUBLIC");
     query.setParameter("startDate", startDate);
     query.setParameter("endDate", endDate);
+    query.setMaxResults(1000); // Add pagination to prevent memory issues

     return query.getResultList();
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public List<ScheduleEntity> findOneSemesterPublicSchedule(LocalDateTime requestDate) {
int month = requestDate.getMonthValue();
int year = requestDate.getYear();
LocalDateTime startDate;
LocalDateTime endDate;
if (month >= 2 && month <= 7) {
// 2월 ~ 7월
startDate = LocalDateTime.of(year, 2, 1, 0, 0);
endDate = LocalDateTime.of(year, 7, 31, 23, 59, 59);
} else if (month >= 8 && month <= 12) {
// 8월 ~ 다음해 1월
startDate = LocalDateTime.of(year, 8, 1, 0, 0);
endDate = LocalDateTime.of(year + 1, 1, 31, 23, 59, 59);
} else { // month == 1
// 전년도 8월 ~ 해당 연도 1월
startDate = LocalDateTime.of(year - 1, 8, 1, 0, 0);
endDate = LocalDateTime.of(year, 1, 31, 23, 59, 59);
}
String jpql = "SELECT s FROM ScheduleEntity s " +
"WHERE s.eventStartDate BETWEEN :startDate AND :endDate";
TypedQuery<ScheduleEntity> query = em.createQuery(jpql, ScheduleEntity.class);
query.setParameter("startDate", startDate);
query.setParameter("endDate", endDate);
return query.getResultList();
}
private static final int FIRST_SEMESTER_START_MONTH = 2;
private static final int FIRST_SEMESTER_END_MONTH = 7;
private static final int SECOND_SEMESTER_START_MONTH = 8;
public List<ScheduleEntity> findOneSemesterPublicSchedule(LocalDateTime requestDate) {
int month = requestDate.getMonthValue();
int year = requestDate.getYear();
LocalDateTime startDate;
LocalDateTime endDate;
if (month >= FIRST_SEMESTER_START_MONTH && month <= FIRST_SEMESTER_END_MONTH) {
// 2월 ~ 7월
startDate = LocalDateTime.of(year, FIRST_SEMESTER_START_MONTH, 1, 0, 0);
endDate = LocalDateTime.of(year, FIRST_SEMESTER_END_MONTH, 31, 23, 59, 59);
} else if (month >= SECOND_SEMESTER_START_MONTH && month <= 12) {
// 8월 ~ 다음해 1월
startDate = LocalDateTime.of(year, SECOND_SEMESTER_START_MONTH, 1, 0, 0);
endDate = LocalDateTime.of(year + 1, 1, 31, 23, 59, 59);
} else { // month == 1
// 전년도 8월 ~ 해당 연도 1월
startDate = LocalDateTime.of(year - 1, SECOND_SEMESTER_START_MONTH, 1, 0, 0);
endDate = LocalDateTime.of(year, 1, 31, 23, 59, 59);
}
String jpql = "SELECT s FROM ScheduleEntity s " +
"WHERE s.scheduleType = :scheduleType " +
"AND s.eventStartDate BETWEEN :startDate AND :endDate " +
"ORDER BY s.eventStartDate ASC";
TypedQuery<ScheduleEntity> query = em.createQuery(jpql, ScheduleEntity.class);
query.setParameter("scheduleType", "PUBLIC");
query.setParameter("startDate", startDate);
query.setParameter("endDate", endDate);
query.setMaxResults(1000); // Add pagination to prevent memory issues
return query.getResultList();
}





public List<ScheduleEntity> findWeekPublic(LocalDateTime startDate, LocalDateTime endDate) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ public ApiResponse<SuccessBody<List<AllPrivateCalendarResponse>>> getPrivate() {
return ApiResponseGenerator.success(response, HttpStatus.OK, MessageCode.GETALL);
}

@GetMapping("semester")
public ApiResponse<SuccessBody<List<SemesterScheduleResponse>>> getSemesterSchedule() {
List<SemesterScheduleResponse> response = scheduleService.getSemesterSchedule();
return ApiResponseGenerator.success(response, HttpStatus.OK, MessageCode.GET);
}
Comment on lines +77 to +81
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add API documentation and consider pagination

The endpoint would benefit from:

  1. Swagger/OpenAPI documentation
  2. Pagination support for large result sets

Consider these improvements:

+   /**
+    * Retrieves public schedules for the current semester.
+    * @return List of semester schedules wrapped in ApiResponse
+    */
+   @Operation(summary = "Get semester schedule", description = "Retrieves all public schedules for the current semester")
+   @ApiResponse(responseCode = "200", description = "Successfully retrieved semester schedule")
    @GetMapping("semester")
-   public ApiResponse<SuccessBody<List<SemesterScheduleResponse>>> getSemesterSchedule() {
+   public ApiResponse<SuccessBody<Page<SemesterScheduleResponse>>> getSemesterSchedule(
+       @RequestParam(defaultValue = "0") int page,
+       @RequestParam(defaultValue = "20") int size) {
-       List<SemesterScheduleResponse> response = scheduleService.getSemesterSchedule();
+       Page<SemesterScheduleResponse> response = scheduleService.getSemesterSchedule(PageRequest.of(page, size));
        return ApiResponseGenerator.success(response, HttpStatus.OK, MessageCode.GET);
    }

Committable suggestion skipped: line range outside the PR's diff.


@GetMapping("slack/test")
@Scheduled(cron = "0 0 9 * * MON")
public void sendSlackMessage() {
Expand Down
Loading