Conversation
- GET /api/v2/admin/lockers 페이지네이션 API 추가 (위치/활성/사용중 필터링) - LockerLocation에 description 컬럼 추가 및 기존 데이터 마이그레이션 (SECOND→2층, THIRD→3층, FOURTH→4층) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- 사물함 배정, 연장, 회수 기능을 구현합니다. - 사물함 목록 조회 시 상태 및 만료일시를 함께 반환합니다. - 페이지네이션 응답을 위한 PageResponse DTO를 추가합니다. - LockerStatus enum을 추가하여 사물함의 상태를 관리합니다.
Summary of ChangesHello @bingle625, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 관리자가 사물함을 효율적으로 관리할 수 있도록 새로운 API와 관련 기능을 구현합니다. 사물함 목록을 다양한 조건으로 조회하고, 사용자에게 사물함을 배정하거나, 사용 중인 사물함의 만료일을 연장하고, 사물함을 회수하는 기능을 제공합니다. 이를 통해 사물함 관리의 편의성과 유연성을 크게 향상시킵니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
...va/net/causw/app/main/domain/asset/locker/api/v2/controller/admin/LockerAdminController.java
Show resolved
Hide resolved
...main/src/main/java/net/causw/app/main/domain/asset/locker/service/v2/LockerAdminService.java
Outdated
Show resolved
Hide resolved
app-main/src/main/resources/db/migration/V20260213120000__AddDescriptionToLockerLocation.sql
Outdated
Show resolved
Hide resolved
V2 API를 통해 사용자가 사물함을 신청, 반납, 연장할 수 있는 기능을 구현합니다. - LockerService, LockerAdminService 분리 및 v2 구현 - Flag, TextField 기반 정책 조회 기능 추가 - LockerLogWriter, LockerPolicyReader, LockerReader, LockerValidator 구현 feat/#1082/locker
- todo: 해당 처리는 flagField 의 사용성을 저하시키는 요인이므로 별도의 이슈 생성이 필요(서로의 상태를 몰라야함)
|
/gemini summary |
|
/gemini review |
Summary of Changes이 PR은 사물함 관리 시스템의 기능을 대폭 확장하고 구조를 개선합니다. 사용자에게는 사물함 신청, 반납, 연장 기능을 제공하고, 관리자에게는 사물함의 상세 현황 조회, 배정, 정책 설정 등 포괄적인 관리 도구를 제공하여 사물함 운영의 효율성과 유연성을 크게 향상시킵니다. 기존 V1 API는 V2로의 전환을 위해 리팩토링되었으며, 새로운 V2 API는 QueryDSL을 활용한 고급 검색 기능과 세분화된 정책 관리를 지원합니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
|
There was a problem hiding this comment.
Code Review
This PR refactors and extends the locker management function to the V2 API, including the addition of administrator locker management APIs (LockerAdminController, LockerPolicyAdminController), separation of V2 service logic, implementation of related DTOs and QueryDSL repositories, and database schema changes. While the functional separation and structuring are generally well-done, a security audit identified several critical logic errors and potential denial-of-service vulnerabilities. Key issues include missing audit logs for administrative actions, incompatible date formatting between writers and readers that could lead to system crashes, a fragile database query in the V1 service that may cause a 500 error if multiple feature flags are active, and a functional bug where new locker locations are created but not saved to the database. Furthermore, the code review highlighted opportunities to improve code duplication and unnecessary logic. Please also ensure the db-change label is applied as per repository style guidelines, given the database schema changes.
app-main/src/main/resources/db/migration/V20260213120000__AddDescriptionToLockerLocation.sql
Outdated
Show resolved
Hide resolved
...main/src/main/java/net/causw/app/main/domain/asset/locker/service/v2/LockerAdminService.java
Outdated
Show resolved
Hide resolved
...main/src/main/java/net/causw/app/main/domain/asset/locker/service/v2/LockerAdminService.java
Outdated
Show resolved
Hide resolved
...ava/net/causw/app/main/domain/asset/locker/service/v2/implementation/LockerPolicyWriter.java
Outdated
Show resolved
Hide resolved
...va/net/causw/app/main/domain/asset/locker/api/v2/controller/admin/LockerAdminController.java
Show resolved
Hide resolved
...domain/asset/locker/api/v2/controller/admin/dto/request/LockerPolicyExtendPeriodRequest.java
Show resolved
Hide resolved
...domain/asset/locker/api/v2/controller/admin/dto/request/LockerPolicyExtendPeriodRequest.java
Show resolved
Hide resolved
...main/asset/locker/api/v2/controller/admin/dto/request/LockerPolicyRegisterPeriodRequest.java
Show resolved
Hide resolved
...main/asset/locker/api/v2/controller/admin/dto/request/LockerPolicyRegisterPeriodRequest.java
Show resolved
Hide resolved
- 사물함 관리 API에 대한 접근 권한을 강화하고, 관리자 ID를 로깅합니다. - 이를 통해 사물함 관리 작업의 보안성을 높이고, 누가 어떤 작업을 수행했는지 추적할 수 있습니다.
- isActive의 경우 v1에서와 다르게 신청/반납에 의해 변경되는 것이 아닌 활성화 비활성화 자체로 보기로 함. - 사물함의 소유에 대해 이미 locker.user 존재 여부로 확인이 가능한데, isActive에 변경을 줄 이유가 없다고 봄
- 상태 && 기간 만족 이 둘다 이뤄져야 신청이 가능한 것으로 정책 변경
|
/gemini review |
...ava/net/causw/app/main/domain/asset/locker/service/v2/implementation/LockerPolicyReader.java
Outdated
Show resolved
Hide resolved
...sw/app/main/domain/asset/locker/api/v2/controller/admin/dto/request/LockerAssignRequest.java
Outdated
Show resolved
Hide resolved
...n/domain/asset/locker/api/v2/controller/admin/dto/request/LockerPolicyExpireDateRequest.java
Outdated
Show resolved
Hide resolved
- 신청/연장 flag가 상호배타적이므로, 해당 flag 기준으로 조건문 분기
- 로직수정 x 오직 이동만 진행함 - 사용하지 않고 있는 TimePassedValidator 클래스는 삭제
|
/gemini review |
There was a problem hiding this comment.
Code Review
이 PR은 사물함 신청 및 관리 로직을 V2 API로 대대적으로 리팩토링하고, 관리자 기능을 추가하는 중요한 변경사항을 담고 있습니다. 전체적으로 책임 분리가 잘 이루어졌고, 새로운 기능들이 체계적으로 구현되었습니다. 몇 가지 코드 스타일 가이드 준수 및 일관성 개선을 위한 제안 사항을 코멘트로 남겼습니다. 추가로, 이 PR에는 DB 스키마 변경이 포함되어 있으므로 Flyway 마이그레이션 CI가 실행되려면 db-change 라벨을 추가해주세요.
| @ModelAttribute @Validated LockerListRequest request) { | ||
|
|
||
| int page = request.page() != null ? request.page() : 0; | ||
| int size = request.size() != null ? request.size() : 10; |
There was a problem hiding this comment.
getLockers 메소드에서 페이지 크기 기본값으로 10이 하드코딩되어 있습니다. getLockerLogs 메소드에서는 StaticValue.DEFAULT_PAGE_SIZE 상수를 사용하고 있는데, 일관성을 위해 getLockers에서도 상수를 사용하도록 수정하는 것이 좋겠습니다.
| int size = request.size() != null ? request.size() : 10; | |
| int size = request.size() != null ? request.size() : StaticValue.DEFAULT_PAGE_SIZE; |
| import jakarta.validation.constraints.NotNull; | ||
|
|
||
| @Schema(description = "사물함 만료일 연장 요청") | ||
| public record LockerExtendRequest( |
There was a problem hiding this comment.
코드 리뷰 가이드라인(19라인)에 따라 DTO 클래스명은 Dto 접미사를 포함해야 합니다. LockerExtendRequestDto로 변경하는 것을 제안합니다. 이 문제는 새로 추가된 다른 요청 DTO 파일들에도 동일하게 적용됩니다.
| public record LockerExtendRequest( | |
| public record LockerExtendRequestDto( |
References
- DTO 클래스는 PascalCase 표기법을 따르며 'Dto' 접미사를 붙여야 합니다. (예:
CalendarResponseDto) (link)
| import io.swagger.v3.oas.annotations.media.Schema; | ||
|
|
||
| @Schema(description = "사물함 전체 층 리스트 응답") | ||
| public record LockerFloorListResponse( |
There was a problem hiding this comment.
코드 리뷰 가이드라인(19라인)에 따라 DTO 클래스명은 Dto 접미사를 포함해야 합니다. LockerFloorListResponseDto로 변경하는 것을 제안합니다. 이 문제는 새로 추가된 다른 응답 DTO 파일들(LockerLocationResponse, MyLockerResponse, LockerListItemResponse 등)에도 동일하게 적용됩니다.
| public record LockerFloorListResponse( | |
| public record LockerFloorListResponseDto( |
References
- DTO 클래스는 PascalCase 표기법을 따르며 'Dto' 접미사를 붙여야 합니다. (예:
CalendarResponseDto) (link)
KEEKE132
left a comment
KEEKE132
left a comment
There was a problem hiding this comment.
수고하셨습니다.
설계상에 생각의 차이가 많이 보이네요
좋은코드 많이 배웠습니다.
...app/main/domain/asset/locker/api/v2/controller/admin/dto/request/LockerAssignRequestDto.java
Outdated
Show resolved
Hide resolved
| @Component | ||
| @RequiredArgsConstructor | ||
| public class LockerLocationReader { |
There was a problem hiding this comment.
[P2]
Transactional(readonly = true) 빠진 것 같습니다.
app-main/src/main/java/net/causw/app/main/domain/asset/locker/util/LockerMapper.java
Show resolved
Hide resolved
| @Component | ||
| @RequiredArgsConstructor | ||
| @Transactional(readOnly = true) | ||
| public class LockerPeriodResolver { | ||
|
|
||
| private final LockerPolicyReader lockerPolicyReader; | ||
|
|
There was a problem hiding this comment.
[P2]
저는 implementation이 다시 implementation을 의존하는 형태가 되면 계층을 분리한 이유중 하나인 순환참조 해소가 의미 없어진다고 생각합니다.
더불어 해당 클래스는 판별에대한 기능을 수행하고 있어 유틸적인 책임이 크다고 생각합니다.
의존을 제거하고 가능한 service에서 인자로 정보를 넘겨주어 사용하는 형태로 바꿀 수 있으면 좋을 것 같습니다.
There was a problem hiding this comment.
의견 감사합니다.
1. 순환참조 해소에 대하여
순환 참조는 엄격하게 고려되어야 하는 사항은 맞으나, 순환참조는 A → B → A처럼 양방향 의존이 형성될 때 문제가 되는데,현재 구조에서는 해당하지 않아서 동일 계층 내 참조 자체가 계층 분리의 취지를 훼손하지는 않는다고 생각합니다.
오히려 고려해야 할 것은 동일 계층내의 참조인데, PeriodResolver-> reader 참조하는 상황에 대해서는 각각의 책임이 명확하기에 문제 될것은 없다고 생각합니다. 도리어 해당 periodResolver 에서도 repository를 통한 조회를 하게 될 경우, 다시 특정 데이터 조회에 대한 규칙지정에 대해서 해당 클래스에서 책임을 가지게 되기 때문에 책임과 테스트 측면 모두에서 불리해질 것이라고 생각합니다.
2. 유틸적인 책임에 대하여
이 부분은 타당한 지적 같습니다. 해당 클래스의 경우 순수 논리적인 판단에 대한 로직이 주를 이루기 때문에, 데이터 조회를 통해 주입을 받는 상황을 만들 수도 있을거 같습니다. 다만, 그렇게 될 경우 서비스 로직이 조금 두꺼워지지 않을까 싶기도 한데요.
예를 들어,
boolean canApplyPolicy = lockerPeriodResolver.isRegisterActive(LocalDateTime.now());
의 코드가
// Service에서 직접 조회 후 넘겨줘야 함
boolean flag = lockerPolicyReader.getLockerAccessStatusFlag();
LocalDateTime start = lockerPolicyReader.findRegisterStartDate();
LocalDateTime end = lockerPolicyReader.findRegisterEndDate();
boolean canApplyPolicy = periodResolver.isRegisterActive(now, flag, start, end); // 인자로 전달
로 달라지는데, 이렇게 될 경우 해당 resolver 클래스를 독립적으로 재사용하기에 호출 할때의 책임이 늘어나 응집도가 낮아질 수 있다. 요런 단점이 있을 수 있을거 같습니다.
그래서, 저는 해당 resolver 자체는 다양하게 쓰일 수 있는 도구 클래스라는 생각이 들어서, 유지를 해도 괜찮지 않을까 하는 생각입니다!
| @Component | ||
| @RequiredArgsConstructor | ||
| @Transactional(readOnly = true) | ||
| public class LockerValidator { | ||
|
|
||
| private final LockerPeriodResolver lockerPeriodResolver; | ||
| private final LockerReader lockerReader; |
There was a problem hiding this comment.
[P2]
Resolver에 남긴 리뷰와 같은 의견이에요.
더욱이 Validator->Resolver->Reader로 implementation 내에서 연속적인 의존이 발생하고 있는데, 유지보수에 어려움이 있을 수 있다고 생각합니다.
게다가 이 클래스는 Resolver 보다도 의존이 필요없는 메서드가 더 많아서 유틸적인 면모가 강해 보여요.
There was a problem hiding this comment.
validator의 경우는 맞습니다. util적인 면모가 강하기 때문에, 데이터를 주입받는 static 클래스로 만드는게 좋을거 같네요
There was a problem hiding this comment.
코드를 조금 다시 읽어보면서 고민해보니, 약간 다시 생각이 달라지는 거 같습니다,,
util 클래스 전환의 경우, 결국 데이터 조회를 외부에서 진행하는 static 클래스로의 전환이라고 생각하는데,
validateRegisterPeriod, validateUserNotHavingLocker 등 일부 메서드가 LockerPeriodResolver, LockerReader에 의존하고 있어서 완전한 static 전환을 하려면 해당 조회 책임을 서비스 레이어로 올려야 합니다.
이렇게 되면 결국 서비스 코드가 두꺼워지고, 현재 LockerValidator가 담당하는 "조회 후 검증" 흐름이 서비스로 분산되는 문제가 있을 것 같습니다.
/**
* 유저 사물함 중복 보유 검증 (관리자용)
*/
public void validateUserNotHavingLocker(String userId) {
if (lockerReader.existsByUserId(userId)) {
throw LockerErrorCode.LOCKER_USER_ALREADY_HAS_LOCKER.toBaseException();
}
}
// service 에서
-> validator.validateUserNotHavingLocker(userId)에서
// service 에서
boolean userExist = lockerReader.existsByUserId(userId);
validator.validateUserNotHavingLocker(userExist)와 같이 되지 않을까 하는데, 이건 오히려 검증 책임을 서비스 쪽에 넘기는 느낌이 강해지지 않을까 싶긴합니다.
또한, LockerValidator → LockerPeriodResolver → LockerPolicyReader 연속 참조의 경우도, 사이클 없는 단방향 의존이라 순환참조 위험은 없고 각 클래스의 책임이 잘 분리되어 있어서 문제가 없다고 판단했습니다.
물론, 연속참조가 너무 깊어지게 될 경우에 변경범위 추적에 문제가 생길 수는 있을 거 같긴 하지만, 해당 경우에는 비즈니스 로직 검증 -> 비즈니스 로직 적용된 조회 -> 기본적인 데이터 조회 인 내부적인 논리 계층구조가 어느정도 말이 되기 때문에, 유지해도 되지 않을까 하는 생각입니다.
다른 분들의 생각도 조금 궁금해지네요.
There was a problem hiding this comment.
저는 사실 implementation 계층 자체가 비즈니스 로직에서 잘 써먹기위한 '재사용 가능한 단위의 도구들' 이라는 생각을 가지고 있어서, 도구가 다른 도구를 조합해서 쓰는 것이고, 사이클만 없으면 문제없다고 생각하긴 합니다.
이 연장 선상에서 util 을 굳이 분리해야할 필요도 없다고 생각합니다.
There was a problem hiding this comment.
사실 저도 개발하면서 implementation이 책임을 얼마나 가져가야 하는지 고민을 좀 했습니다. 처음에는 가능한 가볍게 유지하다 보니 repository 메서드를 한 번 감싸는 정도로 그치는 게 맞나? 싶기도 했고요.
일단 service가 비대해지는 문제는 공감합니다. 제가 개발한 postService도 지금 너무 두꺼워서 책임을 분산할 필요가 있다고 느끼고 있어요.
다르게 생각하는 부분에서는 선입견이 있을 수 있지만, Validator / Resolver는 파라미터로 주어진 정보만으로 동작하는 로직이라고 보기 때문에 util 클래스가 적합하다고 생각합니다.
또 Validator나 Resolver가 Reader를 의존하는 구조는 개인적으로 논리 흐름이 자연스럽지 않게 느껴집니다.
"검증 메서드인 줄 알았는데 조회 쿼리가 나가네?" 처럼 예상치 못한 쿼리가 숨어 있기 좋은 구조라고 생각해요.
순환참조 언급은 해당 코드 자체의 문제라기보다, 처음에 이 아키텍처를 설계할 때 service → implementation 계층 구조로 순환 참조 발생 가능성을 차단하자는 의도가 있었기 때문에 말씀드렸습니다. 같은 계층 내에서 서로 참조하는 상황은, implementation 계층이 없던 구조에서 service끼리 참조하다가 순환 의존이 터지던 상황과 본질적으로 같다고 봐서요.
implementation이 충분한 책임을 가져갈 수 있는 계층이라는 점은 동의합니다. 다만 무분별한 의존을 막기 위해 아래 같은 정도의 제약은 두는 게 좋다고 생각합니다.
- implementation 계층 내 의존은 동일 도메인 내에서만 허용
(단일 도메인 내에서 발생한 순환 의존은 해결하기가 쉽기 때문)
OR - implementation 계층 내 의존 대상은 Reader / Writer로만 제한
(Reader와 Writer는 repository만 참조하기에 순환 참조가 발생하지 않기 때문)
정리하자면
- Service의 책임이 과도해질 수 있으므로 implementation으로 위임하는 방향은 좋습니다.
- 단, implementation 계층 내 의존에는 명시적인 제약이 필요하다고 생각합니다.
3. Validator/Resolver는 util 클래스로 분류하는 것이 맞다고 봅니다.
There was a problem hiding this comment.
예시로들어주신
// Service에서 직접 조회 후 넘겨줘야 함
boolean flag = lockerPolicyReader.getLockerAccessStatusFlag();
LocalDateTime start = lockerPolicyReader.findRegisterStartDate();
LocalDateTime end = lockerPolicyReader.findRegisterEndDate();
boolean canApplyPolicy = periodResolver.isRegisterActive(now, flag, start, end); // 인자로 전달
이런 코드가 발생하기 쉽긴 한데. 이럴 때가 이제 application dto가 필요한 부분이라고 생각합니다.
LockerPolicyInformation 같은 dto를 만들어서 lockerPolicyReader가 해당 dto를 반환해주는 식입니다.
조금 생각할 거리가 많고 번거로울 수는 있다고 생각합니다.
제가 개발한 Post에 저도 머리 아파서 적용을 못하고 Service가 매우 비대해졌습니다..
There was a problem hiding this comment.
implementation과 util 클래스의 구분 기준에 대해 몇 가지 관점을 정리해봤습니다.
클래스의 기능적 성격에 따라 구분 ❌
저는 처음에는 기능이 유틸적인 성격(검증, 판별 등)인지, 아니면 CRUD에 필요한 비즈니스 프로세스를 수행하는지(Aggregator 등)를 기준으로 구분할 수 있다고 생각했습니다.
그런데 Validator처럼 비즈니스 프로세스를 알아야 하면서도 유틸적인 성격을 띠는 경우가 생겨 혼동이 발생했습니다.
이 기준은 명확하지 않으므로 사용하지 않는 것이 좋을 것 같습니다.
추가적인 의존성 필요 여부에 따라 구분
클래스가 외부 의존성을 필요로 하는지에 따라 구분하는 방법입니다.
다만 단순히 "의존성이 있는지 없는지"만 보면 service에서 모든 정보를 조회해 파라미터로 넘기는 방식으로 의존성을 우회할 수 있어 기준이 모호해집니다.
따라서 "해당 메서드를 호출하기 위해 추가적인 조회나 정보가 필요한가" 를 기준으로 생각하는것이 좋아보입니다. 추가 조회가 필요한 경우, 의존성을 상위 계층(service)으로 떠넘긴 것에 불과하므로 implementation 계층으로 본다는 시각입니다.
예를 들어, Mapper는 호출 시 다양한 정보가 필요한 것처럼 보이지만 해당 조회들은 응답 명세를 위해 반드시 필요한 정보들 이므로 추가 정보로 생각하지 않아 util에 해당합니다.
반면 이번 PR의 Validator와 Resolver는 모두 flag 조회라는 추가 정보가 필요하므로 implementation이 맞습니다.
비즈니스 로직 포함 여부에 따라 구분
비즈니스 로직이 없으면 util, 있으면 implementation으로 구분하는 방법입니다.
이 경우 "비즈니스 로직"가 무엇인지 명확히 하는 과정이 필요할 것 같습니다.
비즈니스 로직을 엄격하게 정의할 경우(Resolver에서 start < now < end를 검증하는 것도 비즈니스 로직으로 보는 수준), util 패키지에 들어갈 수 있는 클래스가 거의 없어집니다. 해당 패키지가 도메인 내부의 특화 유틸 클래스를 담는 곳인 만큼, 비즈니스 로직을 포함하지 않으면서 도메인 특화이기도 한 클래스는 Mapper나 Converter 정도뿐일 것 같습니다.
이 기준으로도 Validator와 Resolver는 모두 비즈니스 로직을 포함하므로 implementation으로 분류됩니다.
결국 아키텍처를 처음 설계할 때 util과 implementation의 구분 기준을 명확히 정하지 않아 발생한 문제인 것 같습니다.
지금까지의 내용을 3줄로 요약하면 다음과 같습니다.
1. implementation 계층 내 의존은 제한적으로 허용하여 service의 책임을 가져가는 방향은 좋다고 생각한다.
2. util과 implementation을 구분하는 기준으로 "추가 의존 여부"와 "비즈니스 로직 포함 여부" 두 가지를 생각했다.
3. 두 기준 모두 Validator / Resolver를 implementation으로 분류하므로, 현재 구현을 수정할 필요는 없다.
추가로, LockerMapper가 현재 implementation 패키지에 들어가있는데 확인해주십쇼.
There was a problem hiding this comment.
util과 implementation 구분 기준에 대해서 깊게 생각해보지 못했는데, 의견공유 감사합니다!
비즈니스 로직을 어떻게 구분할 것인가 가 아직도 어려운 주제인거 같기는 한데, 그래도 구분의 방향성을 제시해줄 수 있는 좋은 관점인 것 같습니다.
추가적으로 생각해보니, 뭔가 validator, resolver 라는 네이밍상에서 뭔가 util 적인 냄새를 풍길 수도 있겠다는 생각이 드네요.
(validator나 resolver 클래스에서 조회 책임을 넣지 않는 패턴으로 보통 사용해왔기 때문에)
네이밍 변경에 대해서도 고려해보겠습니다. (추후 리팩토링 과정에서)
LockerMapper는 위치 수정하겠습니다!
| locker.getUser().ifPresent( | ||
| user -> { | ||
| locker.returnLocker(); | ||
| lockerLogWriter.logAdminRelease(locker, admin); | ||
| }); |
There was a problem hiding this comment.
[P4]
취향 차이이긴 한데
ifPresent()는 보다는 if(~~.isPresent){..} 가 가독성이 훨씬 좋아보여요.
개인적으로는 ifPresent() 와 유사 메서드의 사용을 지양하는 컨벤션을 만들어도 좋다고 생각합니다.
There was a problem hiding this comment.
해당 코드의 경우 user를 쓰지도 않고, 람다안에서 여러개를 처리하는게 약간 이질적이긴 하네요!
수정하겠습니다!
다만, 메서드 자체의 사용을 지양하게 하는 것은 약간 생각의 방향성을 한정지을 수도 있겠다라는 생각이 들어서 조심스러워지긴 합니다!
There was a problem hiding this comment.
아 제한하는건 문제가 있겠네요, 생각해보니 해당 정보가 필요한 경우 매번 .get()을 하는게 더 가독성이 떨어질 것 같습니다.
3 participants
🚩 관련 이슈
Close #1083
📢 PR 개요
사물함 신청 정책 및 신청/연장 로직을 전면 정리했습니다.
책임 분리를 고려하여 정책/상태/행위를 명확히 구분했습니다.
사물함 관련 정책
1. 사용자 액션
일반 사용자는 다음 3가지 액션을 수행할 수 있습니다.
2. 사물함 상태 정의
사물함은 두 가지 독립적인 상태를 가집니다.
locker.user != nulllocker.isActive == true상태 정리
locker.user != nulllocker.user == nulllocker.isActive == truelocker.isActive == false3. 신청 규칙 (APPLY)
사용자는 아래 조건을 모두 만족할 경우에만 신청 가능합니다.
4. 기존 점유 상태에서 재신청
이미 사물함을 사용 중인 사용자가 다른 사물함을 신청하면:
→ 중복 점유는 허용하지 않습니다.
5. 연장 규칙 (EXTEND)
연장은 아래 조건을 만족해야 합니다.
6. 정책 구성
사물함 정책은 신청 정책과 연장 정책으로 분리됩니다.
신청 정책
연장 정책
7. Phase 개념 정리
8. 기간 판정 기준
신청 및 연장 기간은 시작/종료 시각을 포함하여 판단합니다.
📃 구현 범위
📸 스크린샷
⚙️ 기타