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

[MVC 구현 - 3단계] 베로(김은솔) 미션 제출합니다. #553

Merged
merged 13 commits into from
Sep 29, 2023

Conversation

Cyma-s
Copy link

@Cyma-s Cyma-s commented Sep 24, 2023

안녕하세요, 루쿠! MVC 3단계는 구현이 좀 늦었습니다 😅

ApplicationContext 는 한 번에 자동으로 만들어져야 하는 패키지들의 클래스들을 한 번에 읽어와서 bean 처럼 등록하는 역할을 합니다. ApplicationContext 를 사용해서 빈들을 가져와야 하는 클래스들은 ApplicationContext 를 필드로 가지고 있는 ApplicationContextAware 를 상속 받아 빈들을 가져올 수 있도록 했습니다. 최대한 리플렉션은 ApplicationContext 에서만 일어나는 것을 신경써서 구현해 봤습니다 😄 루쿠가 구현하셨을 내용도 궁금하네요 ㅎㅎ

초반에는 ViewResolver 를 구현해서 ViewResolver 가 각각 View 들의 특징을 구분해주도록 만들려고 했는데, 미션 코드에서 컨트롤러 단에서 직접 View 를 생성하는 것을 보고 다 삭제했습니다. 이 부분은 루쿠 의견이 궁금해요. 미션 코드와는 조금 달라지더라도 ViewResolver 로 분리하는 것이 좋았을까요?

패키지를 바꾸고 이것저것 수정하다보니 커밋 기록이 좀 많이 더러워졌습니다 죄송해요 😭 커밋 단위로 보기는 어려우실 것 같아 미리 사과드립니다 🙇‍♀️

시간 되실 때 리뷰해주세요! 감사합니다 :)

@Cyma-s Cyma-s self-assigned this Sep 24, 2023
Copy link

@aiaiaiai1 aiaiaiai1 left a comment

Choose a reason for hiding this comment

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

안녕하세요 베로!
3단계 미션도 잘 구현해주셨군요!
ApplicationContextAware를 통해서 빈을 가져와서 사용한게 인상깊었네요 ㅎㅎ
의존성과 객체 분리에 대해 고민한 흔적들이 코드에 잘 담겨있었던것 같아요

ViewResolver와 관련해서는 저는 ViewResolver 로 분리하는 것도 좋을거같다는 생각이에요
또 해봐야 어떤게 문제가 있고 더 좋은지에 대해서 알 수 있다고 생각해 미션코드와는 조금 멀어지더라도 충분해 시도해볼만하다고 생각합니다! 리뷰하면서 많이 배우고 가네요 👍
이번 3단계 미션도 고생하셨습니다~!

Comment on lines 26 to 29
@Override
public String getName() {
return viewName;
}

Choose a reason for hiding this comment

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

View 인터페이스에서 존재하는 해당 메서드는 구현체에서도 이제 사용하지 않는것 같은데 따로 그대로 두신 이유가 있나요??

Copy link
Author

Choose a reason for hiding this comment

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

삭제하는 것을 잊었네요😅 바로 삭제했습니다 👍

Comment on lines 32 to 37
public ApplicationContext(final String externalPackage) {
this.beans = new HashMap<>();
registerBeans(INTERNAL_BASE_PACKAGE);
registerBeans(externalPackage);
initialize();
}

Choose a reason for hiding this comment

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

public으로 registerBeans 메서드와 initalilize 메서드를 열어두었더라고요!
그래서인지 생성자의 파라미터를 가변인자로 받아도 좋을거같다는 생각이 들었네요!

Copy link
Author

Choose a reason for hiding this comment

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

이 부분은 내부 패키지와 외부 패키지를 분리하려고 가변 인자로 받지 않고 각각 받았습니다!
그런데 스프링 코드를 보니 실제로는 가변인자로 가져오고 있네요 🧐

AnnotationConfigServletWebApplicationContext 를 보면, scan 이라는 함수가 있습니다.

	@Override
	public final void scan(String... basePackages) {
		Assert.notEmpty(basePackages, "At least one base package must be specified");
		this.basePackages = basePackages;
	}

이런 것처럼 basePackages 들을 가변인자로 받는 걸 보면, 가변 인자로 받아도 상관 없을 것 같네요!!

Comment on lines 7 to 15
public class JsonWriterObjectMapper {

private static final ObjectMapper objectMapper = new ObjectMapper();

public static String writeJson(final Map<String, ?> objects) {
try {
return objectMapper.writeValueAsString(objects);
} catch (JsonProcessingException e) {
throw new RuntimeException("Json 객체 변환 과정에서 예외가 발생했습니다.");

Choose a reason for hiding this comment

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

따로 클래스 분리를 하신 이유가 있으실까요??

Copy link
Author

Choose a reason for hiding this comment

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

변환 과정은 다른 객체에서 따로 하는 게 좋아보여서 분리했었는데, 지금 보니 굳이 분리되지 않아도 되는 동작 같네요. JsonView 자체가 뷰를 렌더링 해주는 역할을 하니까 ObjectMapper 도 클래스 내부에 있어도 될 것 같아요 :)

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class RedirectView implements View {

Choose a reason for hiding this comment

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

redirectView 좋은거같아요 👍

Copy link

@aiaiaiai1 aiaiaiai1 left a comment

Choose a reason for hiding this comment

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

머지가 늦었네요! mvc 미션 고생하셨어요~
다음 미션도 화이팅입니다!

@aiaiaiai1 aiaiaiai1 merged commit 018aaf5 into woowacourse:cyma-s Sep 29, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants