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

[refactor] 이미지 저장 방식 변경 : S3 도입 (#453) #551

Open
wants to merge 12 commits into
base: develop-backend
Choose a base branch
from

Conversation

Combi153
Copy link
Collaborator

📌 관련 이슈

📁 작업 설명

  • S3를 도입했습니다.
  • 트랜잭션 관리를 위해 Facade 패턴을 도입했습니다.

관련 문서들을 남겨두었습니다.

  1. s3를 도입하는 이유 : s3를 도입하는 이유 #488
  2. 트랜잭션 관리 : 이미지 파일 업로드에서 트랜잭션 #549
  3. s3 인프라 설정 방법 : S3 이미지 업로드 #550

Copy link
Member

@greeng00se greeng00se left a comment

Choose a reason for hiding this comment

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

꼼꼼하게 잘 작성해주셨네요 👍
작게 코멘트 남긴 부분이 있는데 확인 부탁드립니다~

Comment on lines 22 to 23
multipartConfigFactory.setMaxRequestSize(DataSize.ofMegabytes(20));
multipartConfigFactory.setMaxFileSize(DataSize.ofMegabytes(20));
Copy link
Member

Choose a reason for hiding this comment

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

👍👍
추가로 20을 MAX_SIZE와 같이 상수화해도 좋을 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

상수화 진행하겠습니다!

Comment on lines 33 to 34
String name = values[0];
String contentType = values[1];
Copy link
Member

Choose a reason for hiding this comment

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

name과 contentType을 originalFilename을 가공해서 반환하는건 어떠신가요?

Copy link
Collaborator Author

@Combi153 Combi153 Nov 20, 2023

Choose a reason for hiding this comment

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

int indexOfDelimiter = originalFilename.indexOf(DELIMITER);
String name = originalFilename.substring(BEGIN_INDEX, indexOfDelimiter);
String contentType = originalFilename.substring(indexOfDelimiter + LENGTH_OF_DELIMITER);

정도로 수정하면 괜찮을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Override
public String getName() {
    int indexOfDelimiter = originalFilename.indexOf(DELIMITER);
    return originalFilename.substring(BEGIN_INDEX, indexOfDelimiter);
}

@Override
public String getContentType() {
    int indexOfDelimiter = originalFilename.indexOf(DELIMITER);
    return originalFilename.substring(indexOfDelimiter + LENGTH_OF_DELIMITER);
}

오프라인에서 말씀하신대로 코드를 짠다면 이렇게 되겠네요.

그런데, 이러면 getName(), getContetnType() 메서드를 호출할 때마다 substring 로직을 실행해야 할텐데 비효율적이지 않을까요?
물론 현재 코드에서는 이러한 메서드들을 호출할 일이 거의 없긴 하지만요.

Copy link
Member

Choose a reason for hiding this comment

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

답변이 많이 늦었네요 죄송합니다 🙇
저는 클래스의 복잡도나 가독성을 중시해서, 효율성 면에서 생각해본다면 작은 비효율적일 수도 있지만 크게 문제가 되지 않을 것 같습니다!
만약 분리를 두 번 하는 부분이 마음에 걸린다면 name, contentType을 필드로 가지고, getOriginalFilename() 메서드에서 name + contentType을 반환하는 방법도 있겠네요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants