-
Notifications
You must be signed in to change notification settings - Fork 67
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
신재한 1주차 과제 1차 수정본 #45
Conversation
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.
고생하셨습니다 👍
몇 가지 코멘트 남겼으니, 확인 부탁드립니다.
추가 피드백 드립니다 ~
- 각 단계 미션에 작성된
요구사항
을 꼼꼼하게 읽어주시고 반영해주세요 ! - 커밋은 각 단계 미션이 끝날 때마다 날려주시면 좋을 거 같아요.
- PR을 작성하실 때, 어려웠던점, 궁금한점 혹은 알게된 점을 작성해주시면 리뷰어도 참고해서 리뷰를 남기고 나중에 본인의 PR를 확인했을 때 되돌아 보는 시간도 가질 수 있을 거 같아요 😄
아직 이번 미션 주차가 남았기 때문에 개인적으로 추가 학습을 진행하면 좋을 거 같습니다 !
@@ -0,0 +1,92 @@ | |||
public class Calculator { |
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.
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.
관련해서 2단계 - 초간단 문자열 테스트
도 진행해주세요 ~
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.
커밋을 확인해보니, 구현하시고 다른 미션 적용을 위해 지운 거 같아요 !
1단계 - 초간단 계산기 구현
과 3단계 - 문자열 계산기 구현
은 별도의 기능이기 때문에 남겨주시면 좋을 거 같아요 ~
src/main/java/Calculator.java
Outdated
@@ -0,0 +1,92 @@ | |||
public class Calculator { | |||
public static int add(String nums) { |
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.
메인 메서드를 만들지 않는다
라는 조건때문에 static 메서드로 선언하신 거 같아요.
테스트 코드에서는 해당 클래스를 선언하고 내부 메소드를 정상적으로 호출했으니, static 메서드로 선언한 의미가 사라진 거 같아요.
static
키워드에 대한 추가 학습을 진행하시면 좋을 거 같아요 👍
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.
추가로 메소드 파라미터 변수명에 대해서 한 번 생각해보면 좋을 거 같아요 ! nums
은 숫자들이라는 의미인데, 들어오는 값에는 숫자이외에도 특수문자들이 많이 껴있어서 고민해보면 좋을 거 같아요.
src/main/java/Calculator.java
Outdated
@@ -0,0 +1,92 @@ | |||
public class Calculator { | |||
public static int add(String nums) { | |||
if (nums == null || nums.isEmpty()) return 0; |
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/Calculator.java
Outdated
public class Calculator { | ||
public static int add(String nums) { | ||
if (nums == null || nums.isEmpty()) return 0; | ||
String[] tokens = new String[nums.length()]; |
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.
입력받은 문자열의 숫자를 저장하는 배열인 거 같아요.
만약 1;2;12314124235235
이라는 값이 들어왔다고 가정을 하면, 배열에 저장할 숫자의 개수는 3개이지만, tokens 배열은 3개 이상으로 크기가 할당 될 거 같아요.
split()
의 반환값이 무엇인지 한번 확인 하면 좋을 거 같아요 ~
src/main/java/Calculator.java
Outdated
for (int i = 0; i < tokens.length; i++) { | ||
if (!tokens[i].matches("-?\\d+")) { | ||
throw new RuntimeException("양수가 아닌 값이 입력되었습니다."); | ||
} | ||
} |
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.
3단계 - 문자열 계산기 구현
의 앞의 기본 구분자(쉼표, 콜론)외에 커스텀 구분자를 지정할 수 있다. 커스텀 구분자는 문자열 앞부분의 “//”와 “\n” 사이에 위치하는 문자를 커스텀 구분자로 사용한다.
의 요구사항을 적용하면 에러가 터지네요..!
다시 한 번 확인해주세요 ~
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/Calculator.java
Outdated
} | ||
return sum; | ||
} | ||
public static int sub(String nums) { |
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.
add, sub, mul, div 총 4개의 메소드에서 동일한 로직의 코드가 반복되고 있어요. 중복은 어쩔 수 없는 상황이 아니면 피하는 게 좋다고 개인적으로 생각합니다 !
중복된 코드를 줄이기 위해서는 어떻게 코드를 작성하면 될까요 ? 한 번 고민하면 좋을 거 같아요.
src/main/java/Calculator.java
Outdated
return sum; | ||
} | ||
|
||
public static int div(String nums) { |
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.
0
으로 나누면 어떻게 될까요 ??
src/main/java/Calculator.java
Outdated
if (nums.contains("//")) { | ||
nums = nums.substring(nums.indexOf("\n") + 1); | ||
tokens = nums.split("[,|;]"); | ||
} |
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.
다음과 같은 케이스를 생각하면 좋을 거 같아요.
//
이 있지만,\n
이 없다면 ?//
은 없지만,\n
이 있다면 ?
src/test/java/calTest.java
Outdated
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
||
public class calTest { |
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.
4단계 - 리펙터링
요구사항이 반영되지 않은 거 같습니다.
확인 후 진행 부탁드립니다 ~
assertj를 import하는데 시간이 오래 걸렸다. 처음gardle를 써봐서 그런것 같다. 코드 간략화와 변수명을 보다 명확하게 바꿨다고 생각한다. 향산된 반복분을 적용하는데 약간의 문제가 생겨 Arrays를 사용했는데 그럴바에는 그냥 for문을 쓰는건 어떨까 생각했다. |
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.
고생하셨습니다 ~
코멘트 확인 부탁드립니다 😄
@@ -0,0 +1,17 @@ | |||
public class Calculator { | |||
public int add(int a, int 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.
a, b
보다 더 의미 있는 변수 명에 대해서 생각해보면 좋을 거 같아요.
간단한 부분이지만, 습관을 가지는 것이 좋다고 생각합니다 !
return a * b; | ||
} | ||
|
||
public int div(int a, int 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.
0
으로 나누면 어떻게 될까요 ?
} | ||
|
||
|
||
public static int sub(String String_nums) { |
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.
static 메소드로 선언한 이유가 궁금합니다..!
이전 리뷰 확인 부탁드릴께요
@@ -0,0 +1,68 @@ | |||
import java.util.Arrays; | |||
|
|||
public class String_Calculator { |
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 static int[] check(String String_nums) { | ||
String[] tokens = new String[String_nums.length()]; |
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.
초기 값 설정을 위해서 다음과 같이 선언하셨다면, 아래 내용에 대해서 조사해보면 좋을 거 같아요.
- String은 자료형인가? 클래스인가?
- 자바의 자료형에는 어떤 게 있을까?
- 자료형과 클래스는 무슨 차이가 있을까?
if (String_nums.contains("//")) { | ||
String_nums = String_nums.substring(String_nums.indexOf("\n") + 1); | ||
tokens = String_nums.split("[,|;]"); | ||
} |
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.
커스텀 문자열 적용을 하고 있지 않는 거 같아요.
요구 사항 다시 확인 부탁드립니다 !
for (int token : Arrays.copyOfRange(nums, 1, nums.length)) { | ||
sum -= token; | ||
} | ||
return sum; |
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.
Arrays.copyOfRange
를 사용하신 이유가 있을까요 ??
해당 메소드를 사용했을 때, 어떤 복사가 일어나는 지 확인하고 사용하면 좋을 거같아요.
간단한 for문으로 nums 배열을 순회할 수 있을 거 같아요 !
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.assertj.core.api.Assertions.assertThat; | ||
|
||
public class StringCal_AssertJTest { |
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.
약어에 대해서 한 번 생각해보면 좋을 거 같아요.
다른 사람이 봤을 때, StringCal
가 무슨 뜻일까 라는 생각이 들 수 있을 거 같아요. 내가 보기 편한 코드보다는, 누가 봐도 읽기 좋은 코드를 작성해보려고 노력하는 모습도 좋답니다 !
@@ -0,0 +1,29 @@ | |||
### IntelliJ IDEA ### |
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.
.gitinore
에 대해서 한 번 조사하고, 이를 잘 적용해보면 좋을 거 같아요.
지금은 해당 파일이 두 개나 존재하고 있어요 ! 깃 공부도 병행하면 좋습니다 👍
No description provided.