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.

Sign up for GitHub

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

[Help] 메시지 전송 로직에서 문제가 있는 부분 수정 #52

Merged
merged 8 commits into from
Nov 18, 2024
Merged

[Help] 메시지 전송 로직에서 문제가 있는 부분 수정 #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e2a8fc9
feat: 문자 임시 저장 기능 구현
rokaf6444 Nov 13, 2024
a64047a
chore: Ppurio API 설정 추가
rokaf6444 Nov 14, 2024
c1ef4e1
feat: WIP
rokaf6444 Nov 16, 2024
f6c0fa8
fix: WIP
rokaf6444 Nov 17, 2024
a3f1c6e
fix: merge conflict 해결
Enble Nov 18, 2024
cd768f4
fix: WIP
Enble Nov 18, 2024
7e91e13
fix: WIP
Enble Nov 18, 2024
fc41214
fix : 불필요 필드 삭제
lsh1215 Nov 18, 2024
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
14 changes: 14 additions & 0 deletions src/main/java/com/pictalk/global/config/RestTemplateConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package com.pictalk.global.config;

import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.web.client.RestTemplate;

@Configuration
public class RestTemplateConfig {

@Bean
public RestTemplate restTemplate() {
return new RestTemplate();
}
}
Copy link

Choose a reason for hiding this comment

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

코드를 전반적으로 살펴보았을 때 몇 가지 개선 사항을 제안할 수 있습니다.

  1. RestTemplate의 사용자 정의 설정: RestTemplate을 사용할 때 일반적으로 커스터마이징이 필요할 수 있습니다. 예를 들어, 타임아웃 설정이나 메시지 컨버터를 추가하는 방법을 고려할 수 있습니다.

    @Bean
public RestTemplate restTemplate() {
    RestTemplate restTemplate = new RestTemplate();
    restTemplate.setRequestFactory(new SimpleClientHttpRequestFactory() {{
        setConnectTimeout(5000); // 연결 타임아웃 설정
        setReadTimeout(5000); // 읽기 타임아웃 설정
    }});
    return restTemplate;
}

  2. RestTemplate의 단일 빈 사용: RestTemplate은 스레드 세이프(Thread-safe)하므로 애플리케이션 내에서 단일 인스턴스를 공유하는 것이 좋습니다. 현재 코드에서는 이 부분이 적절하게 처리되고 있습니다.

  3. 상수 사용: 타임아웃과 같은 상수 값을 사용할 때, 하드코딩 대신 상수로 선언하고 사용하는 것이 좋습니다. 이렇게 하면 유지보수성이 높아집니다.

    private static final int TIMEOUT = 5000;

@Bean
public RestTemplate restTemplate() {
    SimpleClientHttpRequestFactory factory = new SimpleClientHttpRequestFactory();
    factory.setConnectTimeout(TIMEOUT);
    factory.setReadTimeout(TIMEOUT);
    return new RestTemplate(factory);
}

  4. Exception Handling: API 호출 시 예외 처리를 좀 더 구체화할 수 있습니다. 특정 예외 발생 시 어떻게 처리할지에 대한 로직을 추가하는 것도 좋습니다.

  5. 로깅: RestTemplate을 사용할 때 요청과 응답에 대한 로깅을 추가하면 디버깅에 유용합니다. 이를 위해 ClientHttpRequestInterceptor를 구현할 수 있습니다.

    public class LoggingInterceptor implements ClientHttpRequestInterceptor {
    @Override
    public ClientHttpResponse intercept(HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException {
        // 요청 로깅
        System.out.println("Request: " + request.getURI());
        return execution.execute(request, body);
    }
}

@Bean
public RestTemplate restTemplate() {
    RestTemplate restTemplate = new RestTemplate();
    restTemplate.getInterceptors().add(new LoggingInterceptor());
    return restTemplate;
}

이와 같이 수정하면 RestTemplate의 기능성을 높이고 향후 확장성 및 유지보수성을 고려할 수 있습니다.

4 changes: 2 additions & 2 deletions src/main/java/com/pictalk/global/jwt/JwtService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public String createRefreshToken() {
.compact();
}

// saveAndFlush 사용으로 @Transactional은 없어도 될 듯
// saveAndFlush 사용으로 @Transactional은 없어도 될 듯
public String reIssueRefreshToken(User user) {
String reIssuedRefreshToken = createRefreshToken();
user.updateRefreshToken(reIssuedRefreshToken);
Expand Down Expand Up @@ -122,4 +122,4 @@ public Optional<Claims> decodeAccessToken(String accessToken) {
return Optional.empty(); // 유효하지 않은 경우 빈 값 반환
}
}
}
}
Copy link

Choose a reason for hiding this comment

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

코드를 검토한 결과, 아래와 같은 부분에서 수정이나 개선이 필요합니다.

  1. 주석의 가독성:

    • // saveAndFlush 사용으로 @Transactional은 없어도 될 듯 주석에서 불필요한 공백이 존재합니다. 일관된 스타일로 수정하면 가독성이 향상됩니다. 예를 들어, // saveAndFlush 사용으로 @Transactional은 없어도 될 듯와 같이 수정하는 것이 좋습니다.

  2. 함수의 반환값 사용:

    • Optional.empty()를 반환하는 부분에서, 주석 등을 통해 왜 빈 값을 반환하는지에 대한 설명이나 문서화가 필요할 수 있습니다. 이 방법은 이 메서드를 호출하는 개발자가 반환값의 의미를 이해하는 데 도움이 됩니다.

  3. 메서드 이름:

    • reIssueRefreshToken이라는 메서드 이름이 반환되는 값이 무엇인지 명확하게 나타내지 않을 수 있습니다. 예를 들어, generateNewRefreshTokenForUser와 같은 이름으로 변경하면 이 메서드가 어떤 동작을 수행하는지 더 잘 이해할 수 있습니다.

  4. 트랜잭션 주석:

    • 코드에서 @Transactional이 필요하지 않을 수 있다고 언급하고 있습니다. 이 경우, 해당 메서드의 비즈니스 로직을 확인하여 실제로 트랜잭션이 필요한지 확인하는 것이 좋습니다. 만약 필요 없다면, 주석 대신 이 메서드에서 트랜잭션에 대한 정보를 명확히 하는 것이 좋습니다.

  5. 파일 끝에 개행 추가:

    • 마지막 줄에 newline 문자(\n)가 없는 것은 일반적으로 좋지 않은 관례입니다. 코드의 가독성과 일관성을 위해 마지막 줄에 개행을 추가하는 것이 좋습니다.

이러한 사항들을 개선하면 코드의 가독성, 유지보수성 및 명확성이 향상될 것입니다.

47 changes: 38 additions & 9 deletions src/main/java/com/pictalk/message/controller/MessageController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
package com.pictalk.message.controller;

import static com.pictalk.message.service.MessageService.getReceiversAsString;

import com.pictalk.global.payload.response.CommonResponse;
import com.pictalk.message.domain.Message;
import com.pictalk.message.domain.MessageImage;
import com.pictalk.message.domain.Receiver;
import com.pictalk.message.dto.MessageRequestDto;
import com.pictalk.message.dto.MessageRequestDto.SendMessageRequest;
import com.pictalk.message.dto.MessageResponseDto;
import com.pictalk.message.dto.MessageResponseDto.CancelMessageResponse;
import com.pictalk.message.dto.MessageResponseDto.MessageResponse;
import com.pictalk.message.dto.MessageResponseDto.SendMessageResponse;
Expand Down Expand Up @@ -36,12 +37,13 @@ public class MessageController {
public CommonResponse<SendMessageResponse> sendMessage(@AuthenticationPrincipal UserDetails authenticatedPrincipal,
@Valid @RequestBody SendMessageRequest request) {
String userEmail = authenticatedPrincipal.getUsername();
SendMessageResponse response = messageService.sendMessage(request, userEmail);
MessageResponseDto.SendMessageResponse response = messageService.sendMessage(request, userEmail);
return CommonResponse.onSuccess(response);
}

@GetMapping
public CommonResponse<List<MessageResponse>> getMessages(@AuthenticationPrincipal UserDetails authenticatedPrincipal) {
public CommonResponse<List<MessageResponse>> getMessages(
@AuthenticationPrincipal UserDetails authenticatedPrincipal) {
String userEmail = authenticatedPrincipal.getUsername();
List<Message> messages = messageService.getMessages(userEmail);

Expand All @@ -53,28 +55,40 @@ public CommonResponse<List<MessageResponse>> getMessages(@AuthenticationPrincipa
.messageImages(getImageUrls(message.getMessageImages()))
.status(message.getStatus().toString())
.build()
).collect(Collectors.toList());
).toList();

return CommonResponse.onSuccess(messageResponses);
}

private List<String> getImageUrls(List<MessageImage> messageImages) {
return messageImages.stream()
.map(messageImage -> messageImage.getImage().getImageUrl())
.collect(Collectors.toList());
.toList();
}

@GetMapping("/{message-id}")
public CommonResponse<MessageResponse> getMessage(@AuthenticationPrincipal UserDetails authenticatedPrincipal,
@PathVariable("message-id") Long messageId) {
String userEmail = authenticatedPrincipal.getUsername();
MessageResponse response = messageService.getMessage(messageId, userEmail);
Message message = messageService.getMessage(messageId, userEmail);

MessageResponse response = MessageResponse.builder()
.messageId(message.getId())
.content(message.getContent())
.to(getReceiversAsString(message.getReceivers()))
.sendTime(message.getSentAt() != null ? message.getSentAt().toString() : null)
.status(message.getStatus().toString())
.messageImages(message.getMessageImages().stream()
.map(messageImage -> messageImage.getImage().getImageUrl())
.collect(Collectors.toList()))
.build();
return CommonResponse.onSuccess(response);
}

@PatchMapping("/{message-id}")
public CommonResponse<CancelMessageResponse> cancelScheduledMessage(@AuthenticationPrincipal UserDetails authenticatedPrincipal,
@PathVariable("message-id") Long messageId) {
public CommonResponse<CancelMessageResponse> cancelScheduledMessage(
@AuthenticationPrincipal UserDetails authenticatedPrincipal,
@PathVariable("message-id") Long messageId) {
String userEmail = authenticatedPrincipal.getUsername();
CancelMessageResponse response = messageService.cancelScheduledMessage(messageId, userEmail);
return CommonResponse.onSuccess(response);
Expand All @@ -87,4 +101,19 @@ public CommonResponse<Void> deleteMessage(@AuthenticationPrincipal UserDetails a
messageService.deleteMessage(messageId, userEmail);
return CommonResponse.onSuccess(null);
}

@PostMapping("/temp")
public CommonResponse<MessageResponseDto.TempMessageResponse> saveTempMessage(
@AuthenticationPrincipal UserDetails authenticatedPrincipal,
@Valid @RequestBody MessageRequestDto.TempMessageRequest request) {
String userEmail = authenticatedPrincipal.getUsername();
MessageResponseDto.TempMessageResponse response = messageService.saveTempMessage(request, userEmail);
return CommonResponse.onSuccess(response);
}

private String getReceiversAsString(List<Receiver> receivers) {
return receivers.stream()
.map(Receiver::getPhoneNumber)
.collect(Collectors.joining(", "));
}
}
Copy link

Choose a reason for hiding this comment

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

코드 리뷰 결과, 다음과 같은 수정 및 개선 사항을 제안합니다:

  1. 메소드 명확성 향상:

    • getReceiversAsString 메소드를 MessageController 클래스 내에 정의하여 사용하고 있으나, 이 메소드는 따로 클래스에서 분리하여 유틸리티 클래스로 만들면 재사용성이 높아질 것입니다.
    • sendMessage 메소드 내에서 MessageResponseDto.SendMessageResponse를 사용하여 명시적으로 타입을 작성한 점은 좋지만, 이와 같은 패턴이 반복된다면 일반적인 DTO를 리턴하는 메소드를 별도로 만들어 유연성을 높일 수 있습니다.

  2. 코드 중복 제거:

    • getMessages 및 getMessage 메소드에서 메시지 이미지 URL을 추출하는 로직이 중복되어 있습니다. 이 것을 getImageUrls 메소드로 추상화하여 중복을 줄일 수 있습니다.

  3. 예외 처리 개선:

    • 서비스 메소드를 호출할 때, 예외 상황을 처리하는 로직이 보이지 않습니다. 예외를 발생시킬 수 있는 부분에 대해 적절한 예외 처리를 추가함으로써 안정성을 높일 수 있습니다.

  4. 리턴 타입의 일반화:

    • CommonResponse의 리턴 타입으로 항상 onSuccess 메소드를 호출하기 전에 결과가 널인지 확인하고, 널인 경우 적절한 에러 응답을 리턴하는 것이 좋습니다.

  5. Stream API 사용 최적화:

    • map 및 collect의 남용으로 인해 가독성이 떨어질 수 있습니다. 예를 들어, toList() 호출하는 부분은 Java 16 이상에서만 사용할 수 있으며, 하위 버전에서는 collect(Collectors.toList())로 변환해주어야 합니다. 모든 경우를 고려하여 코드를 작성할 필요가 있습니다.

  6. 주석:

    • 기능에 대한 설명과 함께 주석을 추가하면 더 많은 이해를 돕는 데 도움이 됩니다. 특히 API 메소드에서 어떤 데이터를 수신하고 어떤 데이터가 반환되는지에 대한 주석이 유용합니다.

이러한 개선점을 적용하면 코드의 가독성과 유지보수성이 높아질 것입니다. 추가적으로, 모든 메소드에 대한 단위 테스트를 작성하면 코드의 신뢰성을 더욱 강화할 수 있습니다.

23 changes: 13 additions & 10 deletions src/main/java/com/pictalk/message/domain/Message.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,17 +52,19 @@ public class Message extends BaseEntity {

private String content;

@Column(nullable = false)
private LocalDateTime createdAt;

private LocalDateTime updatedAt;
private LocalDateTime sentAt;

@Builder.Default
private boolean deleted = false;
@Column(name = "external_message_id")
private String externalMessageId;

public void addReceivers(List<Receiver> newReceivers) {
this.receivers.addAll(newReceivers);
newReceivers.forEach(receiver -> receiver.associateWithMessage(this));
}

public void addReceivers(List<Receiver> receivers) {
this.receivers.addAll(receivers);
public void addReceiver(Receiver receiver) {
this.receivers.add(receiver);
receiver.associateWithMessage(this);
}

public void cancel() {
Expand All @@ -71,7 +73,8 @@ public void cancel() {
}
}

public void softDelete() {
this.deleted = true;
public Message withExternalMessageId(String externalMessageId) {
this.externalMessageId = externalMessageId;
return this;
}
}
Copy link

Choose a reason for hiding this comment

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

코드를 살펴본 결과, 다음과 같은 부분에서 수정이나 개선이 필요합니다:

  1. 필드 주석 처리 및 불필요한 필드 제거:

    • createdAt, updatedAt, deleted 필드가 주석 처리되어 있습니다. 이들이 필요하지 않다면 완전히 제거해도 됩니다. 만약 사용될 예정이라면, 해당 필드의 로직을 복원하는 것이 좋습니다.

  2. 메서드 이름:

    • addReceivers와 addReceiver 메서드가 혼동을 줄 수 있습니다. 여러 수신자를 추가하는 메서드와 단일 수신자를 추가하는 메서드를 구분하기 위해 맞춤형 이름을 고려해보세요. 예를 들어, addReceivers는 addMultipleReceivers로, addReceiver는 addSingleReceiver로 변경할 수 있습니다.

  3. softDelete 메서드:

    • softDelete 메서드가 코드에 없어진 것 같습니다. 이 메서드가 꼭 필요한 기능이라면 추가해야 합니다. 삭제 상태를 관리할 로직이 필요합니다.

  4. Builder 패턴:

    • withExternalMessageId 메서드는 설계상으로 좋지만, Builder 패턴을 사용할 때는 Builder를 활용하여 객체를 생성하는 것이 더 일관성 있고 직관적일 수 있습니다. 여기에 externalMessageId를 Builder에 추가함으로써 객체 생성 시 해당 값을 설정할 수 있도록 개선할 수 있습니다.

  5. 메서드의 일관성:

    • addReceivers 메서드와 addReceiver 메서드 간에 associateWithMessage 호출이 모두 포함되어 있는데, 만약 이 로직이 필요하다면 두 메서드 모두에서 호출하도록 유지해야 합니다. 논리적인 일관성을 유지하는 것이 중요합니다.

  6. Optional 사용 고려:

    • externalMessageId와 같은 필드는 유효성을 검사하고 필요에 따라 Optional을 사용함으로써 NPE(널 포인터 예외)를 방지하는 것도 좋은 방법입니다.

수정 후 코드는 다음과 같이 개선될 수 있습니다:

public class Message extends BaseEntity {
    private String content;

    @Column(name = "external_message_id")
    private String externalMessageId;

    private LocalDateTime sentAt;
    private List<Receiver> receivers = new ArrayList<>();

    public void addMultipleReceivers(List<Receiver> newReceivers) {
        this.receivers.addAll(newReceivers);
        newReceivers.forEach(receiver -> receiver.associateWithMessage(this));
    }

    public void addSingleReceiver(Receiver receiver) {
        this.receivers.add(receiver);
        receiver.associateWithMessage(this);
    }

    public void cancel() {
        // Cancellation logic
    }

    public Message withExternalMessageId(String externalMessageId) {
        this.externalMessageId = externalMessageId;
        return this;
    }
}

위의 제안들을 통해 코드의 가독성과 유지 보수성을 높일 수 있습니다.

3 changes: 2 additions & 1 deletion src/main/java/com/pictalk/message/domain/MessageStatus.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@
public enum MessageStatus {
SCHEDULED, // 예약됨
SENT, // 전송됨
CANCELLED // 예약 취소됨
CANCELLED, // 예약 취소됨
TEMP // 임시 저장됨
}
Copy link

Choose a reason for hiding this comment

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

코드에서 수정이 필요한 부분과 개선할 점은 다음과 같습니다.

  1. ENUM의 정의: 현재 MessageStatus 열거형에서 CANCELLED 후에 쉼표가 추가되었습니다. 일반적으로 마지막 Enum 상수에는 쉼표를 붙이지 않는 것이 관례입니다. 따라서, CANCELLED 뒤의 쉼표를 제거하는 것이 좋습니다.

  2. 주석의 일관성: 각 Enum 상수에 대한 주석을 추가하는 것은 좋지만, 주석이 일관되게 작성되도록 확인해야 합니다. 모든 상수에 대한 주석이 포함되어 있는지 확인하고, 필요하다면 추가적으로 다양한 상태에 대한 설명을 덧붙이는 것이 좋습니다.

  3. 네이밍: TEMP라는 이름이 명확하지 않을 수 있습니다. 이 변수의 의미나 사용 용도에 대한 설명이 있는 주석을 추가하거나 좀 더 명확한 이름으로 변경하는 것이 좋습니다.

개선된 코드는 다음과 같이 될 수 있습니다:

public enum MessageStatus {
    SCHEDULED, // 예약됨
    SENT,      // 전송됨
    CANCELLED, // 예약 취소됨
    TEMP;      // 임시 저장됨
}

이와 같은 방식으로 Enum의 사용 및 정의를 개선할 수 있습니다.

3 changes: 3 additions & 0 deletions src/main/java/com/pictalk/message/domain/Receiver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,7 @@ public Receiver(String nickname, String phoneNumber) {
this.phoneNumber = phoneNumber;
}

public void associateWithMessage(Message message) {
this.message = message;
}
}
Copy link

Choose a reason for hiding this comment

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

코드의 일부를 검토해보니 다음과 같은 몇 가지 개선점이 있습니다.

  1. 변수 접근 제어자: message가 어떤 접근 제어자를 가지고 있는지 코드에서 확인할 수 없으나, 만약 private 변수라면 이를 보완하는 getter 메서드가 필요할 수 있습니다. message 변수를 더 안전하게 접근하기 위해 private으로 설정하고 getter를 제공하는 것이 좋습니다.

  2. 메서드 명명: associateWithMessage라는 메서드 이름은 의미는 명확하지만, 좀 더 간결하게 setMessage처럼 명명하는 것이 일반적인 관례일 수 있습니다.

  3. Null 체크: associateWithMessage 메서드 안에서 message가 null인지 체크하고, null이라면 예외를 던지거나 적절하게 대응할 수 있는 로직을 추가하는 것이 좋습니다.

  4. 주석 추가: 메서드 및 클래스에 대해 간단한 주석을 추가하면 코드 가독성이 높아질 수 있습니다. 다른 개발자가 코드를 이해하는 데 도움을 줄 수 있습니다.

개선된 예시는 다음과 같습니다:

public class Receiver {
    private String nickname;
    private String phoneNumber;
    private Message message;

    public Receiver(String nickname, String phoneNumber) {
        this.nickname = nickname;
        this.phoneNumber = phoneNumber;
    }

    /**
     * 메시지를 설정합니다. 메시지가 null인 경우 예외를 던집니다.
     * @param message 설정할 메시지
     * @throws IllegalArgumentException 메시지가 null일 경우
     */
    public void setMessage(Message message) {
        if (message == null) {
            throw new IllegalArgumentException("메시지는 null일 수 없습니다.");
        }
        this.message = message;
    }

    public Message getMessage() {
        return message;
    }
}

이렇게 변경하면 코드의 안정성을 높이고, 가독성을 개선할 수 있습니다.

20 changes: 20 additions & 0 deletions src/main/java/com/pictalk/message/domain/Request.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package com.pictalk.message.domain;

public class Request {

private String requestUri;
private String authorization;

public Request(String requestUri, String authorization) {
this.requestUri = requestUri;
this.authorization = authorization;
}

public String getRequestUri() {
return requestUri;
}

public String getAuthorization() {
return authorization;
}
}
Copy link

Choose a reason for hiding this comment

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

코드를 살펴본 결과, 일부 개선할 수 있는 부분이 있습니다.

  1. 입력값 검증: 현재 생성자에서 requestUri와 authorization에 대한 검증이 없습니다. 입력값이 null 혹은 비어있지 않은지 확인하는 로직을 추가하는 것이 좋습니다.

    public Request(String requestUri, String authorization) {
    if (requestUri == null || requestUri.isEmpty()) {
        throw new IllegalArgumentException("requestUri는 null이거나 비어있을 수 없습니다.");
    }
    if (authorization == null || authorization.isEmpty()) {
        throw new IllegalArgumentException("authorization은 null이거나 비어있을 수 없습니다.");
    }
    this.requestUri = requestUri;
    this.authorization = authorization;
}

  2. Immutable 클래스 구현: 클래스의 필드 변수를 변경할 필요가 없다면, 필드를 final로 선언하여 불변 클래스로 구현하는 것이 좋습니다.

    private final String requestUri;
private final String authorization;

  3. 필드 접근성: Request 클래스가 특정 패키지에만 속할 경우, 접근 제어자를 protected 또는 private로 설정하여 캡슐화를 강화할 수 있습니다.

  4. toString() 메서드 추가: 객체를 쉽게 디버깅하고 출력할 수 있도록 toString() 메서드를 오버라이드하는 것이 좋습니다.

    @Override
public String toString() {
    return "Request{" +
            "requestUri='" + requestUri + '\'' +
            ", authorization='" + authorization + '\'' +
            '}';
}

이러한 개선사항을 반영하면 코드의 가독성과 안정성을 높일 수 있을 것입니다.

22 changes: 16 additions & 6 deletions src/main/java/com/pictalk/message/domain/Sender.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,25 @@
package com.pictalk.message.domain;

import com.pictalk.user.domain.User;
import jakarta.persistence.*;
import lombok.*;

import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.FetchType;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.GenerationType;
import jakarta.persistence.Id;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.OneToMany;
import jakarta.persistence.PrePersist;
import jakarta.persistence.Table;
import java.time.LocalDateTime;
import java.util.ArrayList;
import java.util.List;
import lombok.AccessLevel;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;

@Entity
@Table(name = "sender")
Expand Down Expand Up @@ -37,9 +50,6 @@ public class Sender {
@Column(updatable = false)
private LocalDateTime createdAt;

@Builder.Default
private boolean deleted = false;

@PrePersist
protected void onCreate() {
createdAt = LocalDateTime.now();
Copy link

Choose a reason for hiding this comment

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

코드를 리뷰하겠습니다. 다음과 같은 부분에서 수정이 필요하거나 개선할 수 있는 점이 있습니다.

  1. 필드에 대한 Getter 설정:

    • @Getter 어노테이션이 걸려 있는 경우, 모든 필드에 대해 Getter가 자동으로 생성됩니다. 그러나 deleted와 같은 Boolean 타입의 필드는 누락되어 있는 것 같습니다. 이를 추가하면 좋습니다.

  2. default 값의 명시적인 정의:

    • deleted 필드를 @Builder.Default를 사용하여 기본값을 설정한 부분이 주석 처리되어 있습니다. 이 필드가 필요한 경우, 주석을 제거하고 default 값을 명시적으로 지정하는 것이 좋습니다.
    @Builder.Default
private boolean deleted = false;

  3. 클래스 접근 제한자:

    • public class Sender 대신, 필요한 경우 sender 클래스에 @Getter와 다른 어노테이션들을 포함하여 클래스 접근 제한자를 조정할 수 있습니다. 만약 이 클래스의 외부 접근이 필요 없다면 package-private로 설정하는 것이 좋습니다.

  4. 코드 정리:

    • 필드의 순서를 정리하여 가독성을 높일 수 있습니다. 예를 들어, 모든 @Column 필드들을 한 곳에 모두 모아 관리하면 유지보수성이 향상될 수 있습니다.

  5. Null 체크:

    • createdAt 필드와 같은 타임스탬프 필드는 null에 대한 검사를 추가하면 더 안전하게 만들 수 있습니다.

  6. 인라인 주석:

    • 중요하지 않은 주석이나 사용하지 않는 코드 주석을 제거하여 코드의 가독성을 높일 수 있습니다.

  7. 의미 있는 메서드 이름:

    • onCreate() 메서드의 이름을 좀 더 명확하게 변경하여 메서드의 기능을 더욱 잘 설명할 수 있습니다. 예를 들어 initializeCreatedAt()와 같이 변경할 수 있습니다.

위의 점들을 고려하여 코드 개선을 진행하면 좋겠습니다.

Expand Down
15 changes: 11 additions & 4 deletions src/main/java/com/pictalk/message/dto/MessageRequestDto.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ public static class SendMessageRequest {
private int targetCount;
private List<Target> targets;
private String refKey;
private String rejectType;
private String sendTime;
private String subject;
private List<FileDto> files;
// private String rejectType;
// private String sendTime;
// private String subject;
// private List<FileDto> files;
}

@Getter
Expand Down Expand Up @@ -62,6 +62,13 @@ public static class FileDto {
private String data;
}

@Getter
public static class TempMessageRequest {
private String content;
private String to;
private String sendTime;
}

@Getter
public static class CreateAIMessageRequest {
private String situation;
Copy link

Choose a reason for hiding this comment

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

코드에서 수정이 필요하거나 리팩토링할 수 있는 부분에 대해 다음과 같은 피드백을 드립니다:

  1. 주석 처리된 필드:

    • 현재 rejectType, sendTime, subject, 및 files 필드가 주석 처리되어 있습니다. 해당 필드들이 필요하지 않다면 삭제하고, 필요하다면 재활용할 수 있도록 재정의하는 것이 좋습니다. 주석보다는 코드에서 명확한 의도를 드러내는 것이 중요합니다.

  2. 접근 제어자:

    • SendMessageRequest 및 다른 내부 클래스에 대해 기본적으로 private 필드가 사용되고 있습니다. 클래스 외부에서 접근할 필요가 있는 경우 public 혹은 적절한 접근 제어자를 사용하는 것이 좋습니다.

  3. Getter와 Setter의 통일성:

    • @Getter 애너테이션을 사용하고 있지만, 필요하다면 @Setter 애너테이션도 추가하는 것이 좋습니다. 데이터 클래스의 필드 변경이 필요한 경우를 대비하여 Setter를 추가하는 것도 좋은 방법입니다.

  4. 예외 처리:

    • 요청을 처리하는 과정에서 발생할 수 있는 예외에 대한 처리 로직이 없습니다. 예외 상황을 적절히 처리하여 안정성을 높이는 것이 필요합니다.

  5. 지속적인 객체의 불변성:

    • FileDto 클래스 같은 경우 상태가 변하지 않도록 final 키워드를 추가하여 불변 객체로 만드는 것이 좋습니다. 이로 인해 객체의 상태 변화에 따른 버그를 예방할 수 있습니다.

  6. 코드 주석 개선:

    • 클래스와 메서드에 대한 설명 주석을 추가하여 다른 개발자들이 코드를 이해하기 쉽게 만드는 것이 좋습니다. 주석은 코드의 목적과 사용법을 설명하는 데 유용합니다.

  7. 끝부분 빈 줄:

    • 코드 끝부분에 빈 줄을 추가하여 코드 스타일을 맞추고 가독성을 높이는 것도 좋은 관행입니다.

이 점들을 고려하여 코드를 수정하고 개선해보세요.

Expand Down
7 changes: 7 additions & 0 deletions src/main/java/com/pictalk/message/dto/MessageResponseDto.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ public static class CancelMessageResponse {
private String status;
}

@Getter
@Builder
public static class TempMessageResponse {
private Long messageId;
private String status;
}

@Getter
@Builder
public static class CreateAIMessageResponse {
Copy link

Choose a reason for hiding this comment

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

코드를 검토한 결과, 다음과 같은 부분들이 개선될 수 있습니다.

  1. 일관성 있는 접근 제어자 사용: TempMessageResponse 클래스의 접근 제어자가 생략되어 있습니다. 만약 이 클래스가 다른 클래스에서도 사용될 예정이라면 public 접근 제어자를 명시하는 것이 좋습니다.

    수정 예시:

    @Getter
@Builder
public static class TempMessageResponse {
    private Long messageId;
    private String status;
}

  2. 클래스 주석 추가: 각 응답 클래스를 설명하는 주석을 추가하면 나중에 코드를 유지보수하거나 다른 개발자가 이해하는 데 도움이 됩니다.

    수정 예시:

    /** 
 * 임시 메시지 응답 클래스 
 */
@Getter
@Builder
public static class TempMessageResponse {
    private Long messageId;
    private String status;
}

  3. 필드에 대한 추가 설명: status 필드의 경우 그 의미나 가능 값을 설명하는 주석을 추가하면 코드의 가독성을 높일 수 있습니다.

    수정 예시:

    @Getter
@Builder
public static class TempMessageResponse {
    private Long messageId;

    /** 
     * 메시지 상태 (예: SUCCESS, FAILURE) 
     */
    private String status;
}

  4. 타입 안전성을 위한 Enum 사용: status 필드가 특정 값만 가질 수 있다면 String 대신 enum 타입을 사용하는 것이 좋습니다. 이를 통해 잘못된 값이 들어가는 것을 방지할 수 있습니다.

    예시:

    public enum MessageStatus {
    SUCCESS,
    FAILURE
}

@Getter
@Builder
public static class TempMessageResponse {
    private Long messageId;
    private MessageStatus status;
}

이러한 개선사항을 통해 코드의 가독성과 유지보수성을 높이고, 잠재적인 오류를 줄일 수 있습니다.

Expand Down
6 changes: 2 additions & 4 deletions src/main/java/com/pictalk/message/repository/MessageRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,16 @@

import com.pictalk.message.domain.Message;
import com.pictalk.user.domain.User;

import java.util.List;
import java.util.Optional;

import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.stereotype.Repository;

@Repository
public interface MessageRepository extends JpaRepository<Message, Long> {
// 삭제되지 않은 특정 사용자의 모든 메시지 조회
List<Message> findAllByDeletedFalseAndSenderUser(User user);
List<Message> findAllBySenderUser(User user);

// 삭제되지 않은 특정 사용자의 특정 메시지 조회
Optional<Message> findByIdAndSenderUserAndDeletedFalse(Long id, User user);
Optional<Message> findByIdAndSenderUser(Long id, User user);
}
Copy link

Choose a reason for hiding this comment

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

코드에서 수정이 필요하거나 리팩토링할 수 있는 부분에 대해 제안하겠습니다.

  1. 삭제 상태 관련 메소드의 의미 변경:

    • findAllByDeletedFalseAndSenderUser(User user) 메소드를 findAllBySenderUser(User user)로 변경하셨는데, 삭제 여부를 검사하는 로직이 사라지면서 중요한 정보가 손실되었습니다. 삭제되지 않은 메시지를 조회하려면 삭제 여부에 대한 조건을 유지해야 합니다. 조건을 삭제하지 말고, 원래의 의미를 유지하는 것이 좋습니다.
    // 수정된 코드
List<Message> findAllByDeletedFalseAndSenderUser(User user);

  2. 삭제된 메시지 처리:

    • 카스쿼리에서 삭제된 메시지를 어떻게 처리할지에 대한 방침을 정해야 합니다. 만약 삭제된 메시지를 무시해야 한다면, 항상 삭제 여부를 체크하는 메소드를 사용하거나 애플리케이션 전반에 걸쳐 삭제 여부를 어떻게 처리할지를 명확히 해야 합니다.

  3. 메소드 명세:

    • 구체적으로 메소드의 역할을 구분하기 위해 명확한 네이밍을 사용하는 것이 좋습니다. 예를 들어, 삭제된 메시지와 관련된 메소드를 구분 지을 수 있는 방법을 고려해보세요.

  4. Optional 처리:

    • findByIdAndSenderUser(Long id, User user) 메소드는 특정 사용자에게 특정 메시지가 있는지 여부를 확인하는 것과 관련이 있습니다. 이 메소드는 결과가 없을 수 있기 때문에 결과를 처리할 때 Optional을 적절히 활용하는 것이 중요합니다. 호출하는 쪽에서 Optional을 안전하게 처리하도록 문서화하는 것도 고려해보세요.
// Optional을 사용하여 안전하게 처리하도록 작업
Optional<Message> message = messageRepository.findByIdAndSenderUser(id, user);
message.ifPresentOrElse(
    m -> {/* 메시지가 있을 경우 처리 */},
    () -> {/* 메시지가 없을 경우 처리 */}
);
  1. 테스트:
    • 리팩토링 후 코드가 의도한 대로 작동하는지 검증하기 위해 단위 테스트를 작성하는 것이 좋습니다. JPA 메소드를 직접 호출할 수 있는 테스트를 구성하여 삭제 여부 확인 로직이 제대로 작동하는지 확인하십시오.

위의 고려 사항을 통해 코드의 가독성과 유지보수성을 높일 수 있습니다.

72 changes: 0 additions & 72 deletions src/main/java/com/pictalk/message/service/ImmediateMessageService.java
View file
Open in desktop

This file was deleted.

Loading
Loading