-
Notifications
You must be signed in to change notification settings - Fork 85
[BCSD Lab 임아리] 1차시 미션 제출합니다. #105
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
Conversation
if (b == 0) { | ||
throw new ArithmeticException("0으로 나눌 수 없습니다."); | ||
} | ||
if (a == Integer.MIN_VALUE && b == -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오... int 형의 음수와 양수의 절댓값이 음수가 더 크니까 해당 케이스에서 오버플로우가 발생할 수 있네요.
하나 배워갑니다!
build.gradle
Outdated
@@ -12,6 +12,8 @@ repositories { | |||
dependencies { | |||
testImplementation platform('org.junit:junit-bom:5.9.1') | |||
testImplementation('org.junit.jupiter:junit-jupiter') | |||
testImplementation platform('org.assertj:assertj-bom:3.25.1') | |||
testImplementation('org.assertj:assertj-core') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 관련 사항은 아니지만 코인 프로젝트에서는 빌드 환경 관련 커밋은 feat이 아닌 build를 쓴다고 합니다!
아래 링크 참고하시면 될 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감사합니다! 참고하겠습니다!
@Test | ||
void errorTestOverFlowMinus() { | ||
SimpleCalculator calc = new SimpleCalculator(); | ||
assertThatThrownBy(() -> calc.minus(Integer.MAX_VALUE, -3)).isInstanceOf(ArithmeticException.class); | ||
} | ||
|
||
@Test | ||
void errorTestOverFlowMultiply() { | ||
SimpleCalculator calc = new SimpleCalculator(); | ||
assertThatThrownBy(() -> calc.multiply(Integer.MAX_VALUE, 3)).isInstanceOf(ArithmeticException.class); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
덧셈과 나눗셈에서도 오버플로우가 발생하는지 테스트 해볼 수 있을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 그부분을 빼먹었네요. 감사합니다~!
src/main/java/StringCalculator.java
Outdated
} | ||
|
||
//커스텀 구분자가 있는지 확인 | ||
public int checkDelimiter(String checkedInput) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 메소드는 구분자에 따라 문자열을 파싱하기 위한 인덱스 값을 구하는 것으로 보입니다.
check~~ 메소드의 경우 보통 검증의 의미가 강하다보니 메소드 명만 보고 구분자의 형태를 확인하는 메소드라고 해석할 경우가 더 많아 보여요.
getSplitIdx 또는 getDelimiterIdx 와 같은 get~~ 로 바꾸는 것에 대해 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
굉장히 날카로운 지적이네요! 확실히 check라는 의미는 검증이니 해석이 달라질 수 있겠네요. 이에 관해서 백엔드 스레드에 기존 레귤러 분들이 고민하신 흔적이 있는 것 같은데 참고해봐야겠습니다! 의견 감사합니다~
해당 메소드는 get이 더 맞는 표현인 것 같으니 get으로 변경하도록 하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 여기서 이야기 나누셨군요
위에 댓글에 링크 남겨놨으니 참고해주세용
src/main/java/StringCalculator.java
Outdated
public int calculate(String input){ | ||
String checkedInput = checkInput(input); | ||
int idx = checkDelimiter(checkedInput); | ||
String[] strings = splitInput(checkedInput, idx); | ||
return sum(strings); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메소드 시작부분에서 파라미터인 input이 공백만 들어있는 문자열이면 바로 0을 return 하는건 어떻게 생각하시나요?
그렇게 된다면 다른 메소드에서 문자열이 공백인 경우는 고려하지 않아도 될 것 같아 로직이 더 단순해지고 불필요한 코드를 더 실행해도 되지 않아 보입니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 의견입니다! 반영하겠습니다~
assertThatThrownBy(() -> calc.calculate("-1,2")).isInstanceOf(RuntimeException.class); | ||
assertThatThrownBy(() -> calc.calculate("2147483647,10")).isInstanceOf(RuntimeException.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasMessageContaining 메소드를 추가로 사용해서 정확히 무슨 예외가 발생했는지 확인하는 것이 더 좋아보입니다.
같은 타입의 예외라도 예외가 발생한 경우는 다르니 에러 메시지도 추가적으로 확인한다면 내가 의도한 곳에서 터진 예외인지 예상치 못한 곳에서 터진 예외인지 디버깅하기 더 수월할 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그것도 좋은 방법입니다! 원래는 그런식으로 되어있었는데, 너무 앞서나간 것 같아서 지웠네요... 의견 감사합니다!
저는 반대로 displayname을 통해 테스트에 대한 설명을 줘야 한다고 생각했었습니다.
그러나 웬만해서 테스트 코드의 경우는 메소드에 한글 사용을 허용하다보니 1, 2번 내용은 크게 신경쓰지 않아도 되지만 3번 내용이 제일 핵심일 것 같아요. 구글링을 해보면 대부분은 테스트 메소드를 한글로 해야 한다는 의견이 많아 저도 그렇게 따라가긴 했습니다. 대부분의 프로젝트에서는 컨벤션이 정해져 있다보니 해당 컨벤션을 따라가면 되지 않을까? 싶습니다. |
맞는 말씁이십니다! 다만 저는 메소드 명까지 한글로 적지 않고 영어로 지어도 이 테스트가 어떤 테스트인지 이해가 되도록 작고 간단하게 만들어야하지 않는가? 라는 의미에서 궁금했습니다! 동훈님의 의견 감사합니다. 확실히 협업에는 컨벤션이 중요하죠. 알려주신 내용들 참고하겠습니다!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
첫번째 미션 고생하셨습니다!
질문에 대한 내용은 저도 동훈님과 비슷한 의견입니다
거기에 더해서 메서드명을 한글로 해도 결국 공백이 허용되지 않아 _
를 사용해야 해서 가독성이 DisplayName
보다 안좋을 수 있다고 생각합니다
public int add(int firstNum, int secondNum){ | ||
return firstNum + secondNum; | ||
public int add(int a, int b) { | ||
return Math.addExact(a, b); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addExact()
나 subtractExact()
함수들은 일단 +
, -
랑 어떤점이 다른가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
내부적으로 long으로 연산한 후 int 범위를 벗어나면 ArithmeticException을 던집니다! 예전 코드에서는 그저 +, - 만 사용하여 계산하였기 때문에 오버플로우 처리가 되지 않았습니다. 그래서 이번에는 추가했습니다!
src/main/java/StringCalculator.java
Outdated
import java.util.regex.Pattern; | ||
|
||
public class StringCalculator { | ||
|
||
//입력 문자열이 null인지 확인 | ||
public String checkInput(String input){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

최근에 트랙내에서 이야기를 나눈 컨벤션에 대한 내용입니다!
슬랙
메서드 네이밍에 참고하면 좋을것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵! 저도 이 스레드를 봤는데 반영하지를 못했네요 ㅠㅠ 반영하겠습니다!
src/main/java/StringCalculator.java
Outdated
} | ||
|
||
//커스텀 구분자가 있는지 확인 | ||
public int checkDelimiter(String checkedInput) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 여기서 이야기 나누셨군요
위에 댓글에 링크 남겨놨으니 참고해주세용
src/main/java/StringCalculator.java
Outdated
public int checkDelimiter(String checkedInput) { | ||
if(checkedInput.isEmpty()) return -2; //문자열이 빈 경우 | ||
|
||
|
||
if (checkedInput.startsWith("//")) { //커스텀 구분자를 설정하는가? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주석이 많은것 같아서 주석이 없어도 이해가 가능하도록 코드를 짜보면 좋을것 같아요!
지금 코드에서는 idx
라는 변수명이 어떤뜻을 하는지 알기 어려워서 수정해보면 좋겠다는 생각입니당
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
의견 감사합니다! 수정하겠습니다!
src/main/java/StringCalculator.java
Outdated
@@ -54,7 +44,6 @@ public int sum(String[] strings){ | |||
|
|||
int num = checkException(n); | |||
|
|||
//예외 처리 | |||
if(sum > Integer.MAX_VALUE - num || sum < Integer.MIN_VALUE + num) throw new RuntimeException("int 범위를 벗어났습니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(sum > Integer.MAX_VALUE - num || sum < Integer.MIN_VALUE + num) throw new RuntimeException("int 범위를 벗어났습니다."); | |
if(sum > Integer.MAX_VALUE - num || sum < Integer.MIN_VALUE + num) { | |
throw new RuntimeException("int 범위를 벗어났습니다."); | |
} |
이렇게 하면 가독성이 좀 더 좋을 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 수정하겠습니다!
public class SimpleCalculatorTest { | ||
|
||
private final SimpleCalculator calc = new SimpleCalculator(); | ||
|
||
@Nested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 어떤 역할을 하는 어노테이션인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
중첩된 테스트 클래스를 정의할 때 붙이는 어노테이션입니다! Junit5랑 AssertJ 테스트로 나누고 싶어서 어노테이션을 쓰고 나누려고 했는데... 지금 보니 코드가 그 부분을 제대로 설명하지 못하고 있어보이네요. 이 부분도 수정하겠습니다!
class CalculatorTest{ | ||
@Test | ||
void simpleCalcTest(){ | ||
assertThat(calc.add(1, 2)).isEqualTo(3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertThat()
방식과 assertEquals()
방식을 사용하신것 같은데 둘의 차이가 뭔가요?
각각의 장단점이 있나요? (궁금)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://velog.io/@doolchong/assertEqual%EA%B3%BC-assertThat
이런 장점들이 존재합니다! 이번 미션에서는 테스트 코드를 학습하는 것을 중점으로 두었기 때문에 AssertJ, Junit5 모두 사용해보고 싶어서 이런식으로 작성해보았습니다. 이 부분도 장단점에 맞게 수정해보겠습니다!
궁금점
displayname을 통해 함수에 대한 설명도 작성을 할 수 있는 것으로 알고있습니다. 그러나 제 생각에는 메소드 이름을 통해 이 메소드가 어떤 테스트를 진행하는 것인지 설명하는 편이 맞다고 생각하는데 다들 어떻게 생각하시는지 궁금합니다! 즉, displayname이 굳이 필요한가에 대한 의문이 들었습니다.
그 외로는 다양한 예외에 대해서 생각해보고 테스트를 작성하였는데 놓친 부분이 있을지 궁금합니다!
그리고 다음 미션부터는 어떤 식으로 테스트 코드를 작성하면 좋을지도 피드백 남겨주시면 감사하겠습니다!