[FEAT] 기존 기기 등록 관련 api 수정 #171
Conversation
| import java.util.Optional; | ||
|
|
||
| public interface DeviceRepository { | ||
| void save(Device device); | ||
|
|
||
| //해당 mac을 포함하는 device를 찾는 메서드 | ||
| Optional<Device> findByMac(String mac); | ||
| Optional<Device> findByDeviceId(DeviceId deviceId); | ||
| } |
There was a problem hiding this comment.
이 코드 패치에서는 DeviceRepository 인터페이스에 findByDeviceId() 메서드가 추가되었습니다. 이 메서드는 DeviceId를 기반으로 디바이스를 검색하는 기능을 제공합니다. 이는 좋은 추가라고 볼 수 있습니다.
코드의 변경 사항은 DeviceId 클래스를 임포트하도록 수정하고 findByDeviceId() 메소드를 추가하였습니다. 이 코드에서 버그나 위험한 부분은 보이지 않습니다. 그러나 findByMac() 메소드와 findByDeviceId() 메소드 간의 일관성을 유지하는 것이 좋을 것입니다. 또한 findByMac() 메소드에서는 대소문자 구분을 고려해야 할 수도 있습니다.
개선을 위해 코드의 일관성을 유지하고 findByDeviceId() 메소드의 구현을 충분히 테스트하여 안정성을 확인하는 것이 좋습니다.완료된 경우 리뷰를 해도 좋겠습니다.
|
|
||
| public final class DeviceInfoRegistered extends DomainEvent { | ||
|
|
||
| } |
There was a problem hiding this comment.
- package 문이 적절한 위치에 있는지 확인하세요.
- DeviceInfoRegistered 클래스가 final 키워드를 사용하고 있는데, 상속할 필요가 있는 경우에는 final 키워드를 제거해야 합니다.
- DomainEvent를 확장하는데 어떤 additional fields나 methods가 필요한지 고려하세요.
- DeviceInfoRegistered에 대한 설명(comment)을 추가하여 다른 개발자가 코드를 이해하기 쉽도록 하세요.
| private DeviceAlreadyRegisteredException() { | ||
| super("3030", "이미 등록된 기기입니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드 패치는 DeviceAlreadyRegisteredException이라는 예외 클래스를 생성하는 것입니다. BusinessException을 확장하고 "3030", "이미 등록된 기기입니다."라는 메시지를 전달합니다. 이 클래스는 이미 등록된 기기에 대한 예외를 처리하는 데 사용됩니다.
물론 특정한 컨텍스트에 따라 필요한 추가 기능이나 개선 사항이 있을 수 있지만, 주어진 코드 조각 자체에는 큰 문제가 없어 보입니다. 디자인 패턴이나 요구 사항에 따라 적합한지를 확인해야 합니다.
| public static InvalidDeviceOwnerException of(String ownerName){ | ||
| return new InvalidDeviceOwnerException(String.format("이 기기는 %s 회원의 기기입니다. 해당 회원에게 기기 삭제를 부탁하세요.", ownerName)); | ||
| } | ||
| } |
There was a problem hiding this comment.
- 이 코드는 예외 처리를 위한 InvalidDeviceOwnerException 클래스를 정의하고 있습니다. BusinessException 클래스를 상속받아 사용하고 있습니다.
- 생성자에서 에러 메시지를 받아 부모 클래스의 생성자를 호출하여 예외를 생성하는데, 생성자 접근 제어자가 private이므로 외부에서 직접 생성할 수 없습니다.
- static 메서드인 of() 메서드를 통해 예외를 생성하고 있습니다. ownerName을 받아 해당 문자열을 포맷팅하여 반환하고 있습니다.
- 개선 제안: 생성자의 접근 제어자를 public으로 변경하여 외부에서 직접 생성할 수 있도록 변경하는 것이 좋을 것 같습니다. 추가적으로 ownerName이 null 또는 빈 문자열인 경우에 대한 처리를 추가하는 것이 좋습니다.
- 잠재적인 버그 위험은 ownerName이 null인 경우 String.format 부분에서 NullPointerException이 발생할 수 있습니다. 이에 대한 null 체크를 추가하는 것이 좋습니다.
There was a problem hiding this comment.
도메인 내에서 사용되므로 null일 경우가 없음
| @@ -3,7 +3,8 @@ | |||
| import com.whoz_in.domain.shared.BusinessException; | |||
|
|
|||
| public class InvalidIPv4AddressException extends BusinessException { | |||
| public InvalidIPv4AddressException() { | |||
| public static final InvalidIPv4AddressException EXCEPTION = new InvalidIPv4AddressException(); | |||
| private InvalidIPv4AddressException() { | |||
| super("3006", "유효하지 않은 IPv4 주소입니다."); | |||
| } | |||
| } | |||
There was a problem hiding this comment.
이 코드 패치에서는 InvalidIPv4AddressException 클래스를 수정하여 싱글톤 패턴을 사용하도록 변경하였습니다. 이것은 새로운 객체를 생성할 때마다 메모리 낭비를 줄일 수 있습니다. 그러나 이 코드에는 몇 가지 문제가 있을 수 있습니다.
-
수정된 생성자가 private로 지정되어 있어 외부에서 직접 객체를 생성할 수 없습니다. 이것은 의존성 주입과 같은 경우에 문제를 일으킬 수 있습니다. 생성자를 public으로 변경하거나 getInstance() 메서드를 추가하여 외부에서 객체를 얻을 수 있도록 할 수 있습니다.
-
메시지 리소스를 하드 코딩하여 사용하고 있습니다. 이러한 메시지들은 외부 리소스 파일로 이동하여 유지보수가 용이하도록 변경할 수 있습니다.
-
예외 코드 "3006"도 하드 코딩되어 있습니다. 예외 코드도 외부 리소스 파일로 이동하여 변경할 수 있습니다.
-
상수로 정의된 EXCEPTION 필드는 대문자로 작성해야 합니다. 예를 들어, EXCEPTION 대신 EXCEPTION으로 작성해야 합니다.
이러한 문제를 수정하여 코드를 개선할 수 있습니다.
| super(errorMessage); | ||
| this.errorCode = errorCode; | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드 패치는 ApplicationException이라는 추상 클래스를 정의하고 있는 것으로 보입니다. 이 클래스는 lombok의 @Getter 어노테이션을 사용하여 errorCode 필드에 대한 getter 메소드를 자동으로 생성하고 있습니다.
잠재적인 버그 위험은 errorCode 필드가 private으로만 선언되어 있어 외부에서 값을 설정할 수 없기 때문에 없어 보입니다.
개선 제안으로는 errorCode 필드가 반드시 필요한 경우에만 사용되므로 생성자 파라미터로 받지 않고, 오버라이딩 가능한 setter 메소드를 추가하여 필요할 때만 값을 설정할 수 있도록 하는 것이 좋을 수 있습니다. 또한, errorMessage 필드가 필요한 경우도 고려해보면 좋을 것 같습니다.
| if (!unregisteredSsids.isEmpty()) { | ||
| throw new IllegalArgumentException("다음 wifi에 대해 맥을 등록하지 않았습니다. " + String.join(", ", unregisteredSsids)); | ||
| } | ||
| return (deviceInfos != null) && deviceInfos.stream().anyMatch(di->di.equals(deviceInfo)); | ||
| } | ||
|
|
||
| //DeviceInfo 추가 |
There was a problem hiding this comment.
코드 리뷰를 해보겠습니다.
RoomSsidConfig클래스 import 및 해당 필드 선언이 제거되었습니다. 해당 필드가 사용되지 않는 것으로 보입니다.exists메서드에서mustNotExist메서드 이름이exists로 변경되었고, 반환 타입이 void에서 boolean으로 변경되었습니다. 이로써 해당 메서드는 해당 장치 정보가 이미 존재하는지 여부를 확인하고, 존재하면 true를 반환하게 되었습니다.- 주석으로 달린
//TODO: deviceInfo들에 room이 있는데 room을 굳이 받아야 하나가 제거되었습니다. verifyAllAdded메서드가 모두 삭제되었습니다. 해당 기능은 더 이상 사용되지 않는 것으로 보입니다.
개선 제안:
- 불필요하게 남아있는 주석이나 코드는 정리할 필요가 있습니다.
verifyAllAdded메서드가 왜 필요 없어졌는 지 추가적인 검토가 필요할 수 있습니다.
이상입니다. 추가 질문이 있으면 언제든지 물어보세요.
| .map(this::getIpList) | ||
| .flatMap(Collection::stream) | ||
| .toList(); | ||
| } | ||
| } |
There was a problem hiding this comment.
룸 내부 아이피를 저장하는 PrivateIpStore 클래스에 대한 간단한 코드 리뷰와 개선 제안을 드리겠습니다.
- 문제점:
put메서드에서는 이미 존재하는 키로 값을 덮어쓰기 때문에 예기치 않은 동작이 발생할 수 있습니다. 값이 이미 존재하는지 확인하고 설정하는 것이 더 안전합니다.get메서드에서store.getIfPresent(room)의 결과가 null일 경우 IllegalStateException을 던지는데, 이것은 좋은 방법이 아닙니다. 에러 핸들링을 더 세부적으로 처리하는 것이 좋습니다.getIpList메서드의 메서드 오버로딩이 필요없이 파라미터로 room을 받아서 처리할 수 있습니다.- getIpList() 메서드 내에서 중복 코드가 발생하고 있습니다.
- 개선 제안:
put메서드에서 이미 존재하는 키인지 확인하고, 존재한다면 오버라이딩하지 않도록 조건을 추가합니다.get메서드에서 예외 처리를 더 명확하게 하세요. 예를 들어, Optional을 사용하여 값이 없을 경우 처리할 수 있습니다.getIpList메서드를 한 곳에서 처리할 수 있도록 중복 코드를 제거하고 메서드 시그니처를 최적화합니다.- 캐싱된 정보를 유지하는 데 사용되는 Cache에 대한 설명이 없으니, 추가적인 설명을 주석으로 포함하는 것이 좋습니다.
이러한 문제점을 개선하고 개선 제안 사항을 고려하여 코드를 업데이트하면 좀 더 성능적이고 안정적인 PrivateIpStore 클래스를 구현할 수 있을 것입니다.
| } | ||
|
|
||
| // 나머지 예외 | ||
| @ExceptionHandler(Exception.class) | ||
| public ResponseEntity<FailureBody> handleException(Exception e) { | ||
| return ResponseEntityGenerator.fail("UNEXPECTED_ERROR", e.getMessage(), HttpStatus.INTERNAL_SERVER_ERROR); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
MainApiGlobalExceptionHandler 클래스는 전반적으로 안정적으로 보이지만 몇 가지 개선이 필요합니다.
-
@RestControllerAdvice 어노테이션을 사용하여 전역 예외 처리기를 만들었습니다. 그러나 @requiredargsconstructor 어노테이션이 추가되어 있지만 해당 클래스에서 사용되지 않는 것으로 보입니다. 따라서 @requiredargsconstructor 어노테이션을 제거해도 됩니다.
-
handleIllegalStateException 메소드에는 IllegalStateException을 처리하는데, HttpStatus 코드가 HttpStatus.BAD_REQUEST로 설정되어 있습니다. 일반적으로 IllegalStateException은 시스템 오류를 나타내므로 HttpStatus.INTERNAL_SERVER_ERROR로 변경하는 것이 더 적절할 수 있습니다.
-
handleException 메소드는 Exception을 처리하는데, HttpStatus 코드가 HttpStatus.INTERNAL_SERVER_ERROR로 설정되어 있습니다. 모든 예외에 대해 마지막으로 처리하는 메소드이므로 예외 코드를 더 구체적으로 설정하는 것이 좋습니다.
-
TODO: mapper 주석이 있는 부분을 요청 매핑 정보를 처리하는 Mapper 클래스를 만들어 분리하는 것이 좋을 수 있습니다.
-
코드 라인의 일관성을 위해 handleIllegalArgumentException 및 handleIllegalStateException과 같이 메소드 이름을 일관적으로 작성하는 것이 좋습니다.
이러한 개선 제안을 고려하여 코드를 리팩토링하면 더 효율적이고 안정적인 예외 처리기를 구성할 수 있을 것입니다.
| @@ -43,7 +43,7 @@ public boolean write(String room, Map<String, String> privateIps) { | |||
|
|
|||
| try { | |||
| restTemplate.exchange( | |||
| mainApiBaseUrl + "/api/v1/private-ip", | |||
| mainApiBaseUrl + "/internal/api/v1/private-ip", | |||
| HttpMethod.PUT, | |||
| requestEntity, | |||
| Void.class | |||
There was a problem hiding this comment.
변경 사항이 'internal'로 될 때 URL이 /internal/api/v1/private-ip로 변경되었으므로 이것은 올바를 듯합니다. 그러나 여전히 주에 대한 검증이 완전히 이루어지지 않았을 수 있습니다. 입력 값의 유효성을 확인하고, 에러 처리 메커니즘을 구현하는 것이 좋습니다. 또한, 예외 상황 처리가 부족할 수 있으므로 추가적인 예외 처리를 고려할 필요가 있습니다.
📌 관련 이슈
close #158
✨ PR 내용
이제 나머지 기기 등록 관련 api를 추가하겠습니다. (#162 #161 #160)