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 구현하기 - 1단계] 땡칠(박성철) 미션 제출합니다. #377

Merged
merged 2 commits into from
Sep 15, 2023

Conversation

0chil
Copy link

@0chil 0chil commented Sep 13, 2023

안녕하세요 도기 ✋🏻 반갑습니다

리플렉션을 직접 써보는 것은 처음이라 학습 테스트를 추가로 작성해보았습니다.

그 외엔 간단해서 특이사항은 없었습니다. 리뷰 잘부탁드려요!!

Copy link

@kdkdhoho kdkdhoho left a comment

Choose a reason for hiding this comment

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

안녕하세요 땡칠! 떙칠의 코드를 리뷰할 수 있어서 영광이에요 😃
1단계의 요구사항은 충분히 만족한 것 같아 바로 Approve&Merge 할게요.

질문과 사소한 리뷰를 남겨두었으니 답변 부탁드립니다.
리뷰하면서 진짜 많이 배웠어요. 감사합니다 👍


public class AnnotationHandlerMapping {

private static final Logger log = LoggerFactory.getLogger(AnnotationHandlerMapping.class);

private final Object[] basePackage;
private final Object[] basePackages;

Choose a reason for hiding this comment

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

너무 꼼꼼하네요.. 👍

Choose a reason for hiding this comment

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

�그런데 basePackages가 String배열이 아닌, Object 배열인 이유가 무얼까요? 🧐

Copy link
Author

Choose a reason for hiding this comment

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

String이면 더 좋겠지만, 라이브러리 스펙때문에 그렇다고 생각합니다.
라이브러리 생성자 스펙은 Object... == Object[] 기준이라서,
String[]을 넣으면 가변인자로 다뤄야할지 말아야할지 모르겠다는 경고가 뜨네요.

String[]도 Object 객체이기 때문에 하나의 인자로 다뤄야할지 다수의 인자로 다뤄야할지 구분하지 못하는 것 같습니다~

image 왜 이렇게 만들었는지는 모르겠지만, `ClassCastException` 예외가 있을지 모르니 그냥 놔두겠습니다 ㅎㅎ

return getInstanceOf(declaringClass);
}

private Object safeInstantiate(Class<?> clazz) {

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.

감사합니다 ㅎㅎ
도기도 한 네이밍 하지 않나요

Comment on lines +26 to +32
try {
return clazz.getDeclaredConstructor().newInstance();
} catch (NoSuchMethodException exception) {
throw new IllegalArgumentException("Class " + clazz.getSimpleName() + "에 기본 생성자가 없습니다");
} catch (ReflectiveOperationException exception) {
throw new IllegalStateException("Class " + clazz.getSimpleName() + "초기화에 실패했습니다");
}

Choose a reason for hiding this comment

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

예외 처리 진짜 꼼꼼하시네요 👍


public class PrintTestUtil {

public static AbstractStringAssert<?> assertThatOutputOf(IgnoringExceptionRunnable runnable) {

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.

ㅋㅋㅋㅋ

}

@Test
void getFields는_자식에서_선언된_필드도_가져온다() {

Choose a reason for hiding this comment

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

자식에서 선언된 필드가 무엇인가요? 테스트에서는 단일 Student 객체일 뿐인 것 같아서요~

Copy link
Author

Choose a reason for hiding this comment

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

원래는 다음과 같은 실험을 하고싶었는데, 잘못 작성한 것 같네요:
image

자식인 AStudentStudent에 대입하면 자식과 부모의 필드를 모두 가져오는지 궁금했던 건데 의미 없었습니다.
결국 인스턴스가 AStudent이기 때문에 student.getClass() -> AStudent가 나오고, 자식의 필드만을 찾게 됩니다.

그럼 부모 클래스 정보는 어떻게 찾을까요?
getSuperclass()라는 메서드가 있다고 하네요.

@kdkdhoho kdkdhoho merged commit 8ff4261 into woowacourse:0chil Sep 15, 2023
1 check failed
Copy link
Author

@0chil 0chil left a comment

Choose a reason for hiding this comment

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

도기 리뷰 너무 감사합니다. 다음 단계에서 뵙겠습니다!


public class AnnotationHandlerMapping {

private static final Logger log = LoggerFactory.getLogger(AnnotationHandlerMapping.class);

private final Object[] basePackage;
private final Object[] basePackages;
Copy link
Author

Choose a reason for hiding this comment

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

String이면 더 좋겠지만, 라이브러리 스펙때문에 그렇다고 생각합니다.
라이브러리 생성자 스펙은 Object... == Object[] 기준이라서,
String[]을 넣으면 가변인자로 다뤄야할지 말아야할지 모르겠다는 경고가 뜨네요.

String[]도 Object 객체이기 때문에 하나의 인자로 다뤄야할지 다수의 인자로 다뤄야할지 구분하지 못하는 것 같습니다~

image 왜 이렇게 만들었는지는 모르겠지만, `ClassCastException` 예외가 있을지 모르니 그냥 놔두겠습니다 ㅎㅎ

return getInstanceOf(declaringClass);
}

private Object safeInstantiate(Class<?> clazz) {
Copy link
Author

Choose a reason for hiding this comment

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

감사합니다 ㅎㅎ
도기도 한 네이밍 하지 않나요


public class PrintTestUtil {

public static AbstractStringAssert<?> assertThatOutputOf(IgnoringExceptionRunnable runnable) {
Copy link
Author

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋ

}

@Test
void getFields는_자식에서_선언된_필드도_가져온다() {
Copy link
Author

Choose a reason for hiding this comment

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

원래는 다음과 같은 실험을 하고싶었는데, 잘못 작성한 것 같네요:
image

자식인 AStudentStudent에 대입하면 자식과 부모의 필드를 모두 가져오는지 궁금했던 건데 의미 없었습니다.
결국 인스턴스가 AStudent이기 때문에 student.getClass() -> AStudent가 나오고, 자식의 필드만을 찾게 됩니다.

그럼 부모 클래스 정보는 어떻게 찾을까요?
getSuperclass()라는 메서드가 있다고 하네요.

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