[문자열 계산기] 김경수 제출 (1주차)#9
Conversation
| throw new IllegalArgumentException("잘못된 문자가 있는 것 같은데요?"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
함수 깊이가 너무 깊은 것 같아요! 더 분리하면 좋을 것 같아요
특히 if elseif가 많이 섞여있어서 가독성이 떨어지는 것 같아요
There was a problem hiding this comment.
정규표현식 관련 리뷰와 합쳐서 동의하는게 정규표현식으로 커스텀 구분자를 추출한다면 코드가 확실히 짧아질 수 있을 것 같아요!
| else { | ||
| throw new IllegalArgumentException("잘못된 문자가 있는 것 같은데요?"); | ||
| } | ||
| } |
There was a problem hiding this comment.
이 윗부분에서 구분자 추출하는 부분은 정규표현식으로 처리하고 밑 부분은 인덱스 접근이 아닌 원소 순회로 표현하면 가독성이 더 높을 것 같아요
There was a problem hiding this comment.
정규표현식 기억하겠습니다..! Matcher와 Pattern 말씀하시는거죠?
인덱스 접근 부분은 생각해보니 원소 순회가 가독성이 더 좋을 것 같네요 감사합니다!
| // 문자열 입력받기 | ||
| System.out.println("덧셈할 문자열을 입력해 주세요."); | ||
| Input(); | ||
| for (int i = 0; i < userInput.length(); i++){ |
There was a problem hiding this comment.
Character을 순회하면 일의자리를 제외한 수를 표현할 수 없지 않나요?
ex) 32,1 -> 3+2+1 -> 6
There was a problem hiding this comment.
저도 그렇게 생각해서 아래 if문 부분이 해결된다면 숫자 추출을 문자열 내부 함수로 처리할 수 있어 보입니다.
| a.extractNum(); | ||
| a.sumNumbers(a.getNumbers()); | ||
| } | ||
| } |
There was a problem hiding this comment.
전체적으로 역할을 분리하지 않아서 코드 흐름이 흔들리는게 있는 것 같아요! 기능을 쪼개고 기능마다 메서드를 만들어 구현한다면 좀 더 쉽게 표현될 수 있을 것 같아요!
| // 문자열 입력받기 | ||
| System.out.println("덧셈할 문자열을 입력해 주세요."); | ||
| Input(); | ||
| for (int i = 0; i < userInput.length(); i++){ |
There was a problem hiding this comment.
저도 그렇게 생각해서 아래 if문 부분이 해결된다면 숫자 추출을 문자열 내부 함수로 처리할 수 있어 보입니다.
| // 개행문자를 문자열로 넣는 과정을 추가하기보단 | ||
| // 그냥 다시 문자열을 받아도 상관 없겠다는 생각을 했어요 | ||
| // 근데 구분자를 추가하고 끝나는 경우도 있다는 생각에 | ||
| // if 문을 하나 더 넣었네요 |
There was a problem hiding this comment.
코드 외의 이야기지만 긴 문장의 주석을 작성한다면 각 줄마다 '//'로 주석을 다는 것보다 '/* */'과 같은 블록주석 표현을 쓰는 것도 좋다고 생각합니다.
| throw new IllegalArgumentException("잘못된 문자가 있는 것 같은데요?"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
정규표현식 관련 리뷰와 합쳐서 동의하는게 정규표현식으로 커스텀 구분자를 추출한다면 코드가 확실히 짧아질 수 있을 것 같아요!
| System.out.println("덧셈할 문자열을 입력해 주세요."); | ||
| Input(); | ||
| for (int i = 0; i < userInput.length(); i++){ | ||
| if ((userInput.charAt(i) == '/' && userInput.charAt(i+1) == '/')) { |
There was a problem hiding this comment.
for문의 수행 횟수가 문자열의 길이와 일치하기 때문에 'i+1'과 같은 접근 표현을 사용 시 인덱스가 오버될 수도 있어보입니다.
There was a problem hiding this comment.
아 여기 인덱스 신경을 못썼네요. 이런 곳에서 for문보다는 stream을 사용하는게 더 괜찮을까요?
| } | ||
| } | ||
| else { | ||
| throw new IllegalArgumentException("잘못된 문자가 있는 것 같은데요?"); |
| a.extractNum(); | ||
| a.sumNumbers(a.getNumbers()); | ||
| } | ||
| } |
There was a problem hiding this comment.
기능마다 나눠서 코드를 짜면 좋은 코드가 될 수 있을 것 같아요! 저랑 비슷하지만 다르게 접근하셔서 대단한 것 같습니다!
| // if 문을 하나 더 넣었네요 | ||
| if (i+2 < userInput.length()) { | ||
| separator.add(String.valueOf(userInput.charAt(i + 2))); | ||
| if(userInput.length() > i + 4) { |
There was a problem hiding this comment.
여기서는 커스텀 구분자 이후에 나오는 input을 받기 위해서 i+4로 한 건가요?
There was a problem hiding this comment.
//"커스텀구분자"\n로 커스텀 구분자가 추가되기만 하고 더이상 입력이 없을 경우에 Input()을 받게 된다면 오류가 발생하기 때문에 방지하기 위해 i+4조건을 붙여주었습니다
| private ArrayList<String> separator; | ||
|
|
||
| public Application(){ | ||
| numbers = new ArrayList<String>(); |
There was a problem hiding this comment.
Application()에 안에 numbers와 separator을 선언한 이유가 따로 있나요?
There was a problem hiding this comment.
MVC를 아직 공부하지 못해 모든 기능을 Application() 안에 다 넣어서 그렇습니다 ㅠㅜ
| public void Input(){ | ||
| userInput = Console.readLine(); | ||
| } | ||
|
|
5 participants
gradlew.bat이 안된다고 뜨네요.
java 버전 확인하려고 해도
이렇게 뜨고 안떠서 제대로 작동하는지 확인도 못해보고 제출하네요...
이번 주차는 넘어야 할 벽의 높이를 알게되는 값진 주차였던 것 같습니다 :)
피드백 해주시면 감사하겠습니다.