Eunsaem03 #2
Eunsaem03 #2
Conversation
525tea
left a comment
525tea
left a comment
There was a problem hiding this comment.
은샘님 코드 잘 보았습니다 😊
클래스 분리가 잘 되어 있어서 각각의 역할이 명확히 드러나서 전체 구조를 이해하는 데 도움이 됐어요
제가 생각하는 몇 가지 포인트에 대해서는 제안 방향을 코멘트 해봤습니다
마지막에 예외를 던지는 부분은 제 코드 리뷰에도 언급해주셔서, 더 깊이 공부해볼 수 있는 계기가 됐어요. 감사합니다 🙇♀️
| if (!Character.isDigit(input.charAt(0))) { | ||
| String delimiter; | ||
| delimiter = String.valueOf(input.charAt(2)); | ||
| input = input.replaceAll("^.*\\\\n", ""); | ||
| String customRegex = "[" + delimiter + ",:]"; | ||
| return input.split(customRegex); | ||
| } | ||
| return input.split("[,:]"); |
There was a problem hiding this comment.
이 부분에서 의도하신 게 입력값의 첫 글자가 숫자가 아닌 경우 커스텀 구분자가 존재한다고 판단하고,
"//"와 "\n" 사이의 문자를 커스텀 구분자로 인식한 뒤, 이후 부분을 해당 구분자 및 기본 구분자(",", ":")로 나누는 것 같아요
그런데 이 경우 2가지 문제점이 우려되는데요
(1) input = input.replaceAll("^.*\\\\n", "");에서는 "\\\\n"이 이스케이프 문자열 "\\n"만 인식하고, "\n"은 인식하지 못하게 돼요. 즉, 정규식으로는 "\\n", 문자열 리터럴에서는 "\n"이 처리되어야 하는데 이 부분이 누락이 될거 같아요.
(2) input.charAt(2)에서 2번째 문자를 고정 인덱스로 커스텀 구분자를 추출하고 있는데요, 커스텀 구분자가 2글자 이상인 경우를 커버할 수 없을거 같아요 (ex. "//;;\n1;;2") 만약 커스텀 구분자를 한 글자만 가능하도록 기획하셨다면 이 부분은 무시해주셔도 될 거 같습니다
따라서 Pattern, Matcher를 사용해서 "//"와 \n 사이의 구분자를 추출하는 정규식 방식을 한번 고려해보시면 좋을 것 같습니다
There was a problem hiding this comment.
(1) 우테코에서 제공한 테스트 케이스가 "\\n"이어서 그것만 처리하면 된다고 생각했는데 다른 분들 확인하니 "\n"도 함께 생각해서 처리하셨더라구요 조언 해주신 대로 Pattern, Matcher 사용해서 둘 다 처리할 수 있도록 다시 개선해보겠습니다!
(2) 사실 구분자가 2글자 이상인 경우를 어떻게 해야 커버가 가능할까 고민했는데요 "//"와 "\n"전까지를 묶어서 구분자로 설정하고 싶었는데 정규식이 너무 어렵더라구요.. 조금 더 고려해보겠습니다. 또한 이렇게 코드를 짰을 땐 커스텀 구분자가 1글자인 경우에만 실행 가능이라는 기획 의도를 분명하게 나타내고, 2글자 이상일 시 오류등 예외 처리를 했으면 좋았겠다는 생각이 드네요
| } catch (IllegalArgumentException e) { | ||
| System.out.println("Error " + e.getMessage()); | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
은샘님이 해주신 제 코드 리뷰와도 관련이 있어서 살펴봤는데요,
마지막 제어 담당 클래스인 Application에서 throw e로 예외를 다시 잡아서 던져주신 것 같아요.
이렇게 하면 프로그램이 비정상 종료되고, 콘솔에 에러 메시지와 스택 트레이스가 두 번 출력될 가능성이 있을 것 같습니다.
따라서 제 생각에는 예외를 다시 잡아 던지는 rethrow 대신 return으로 main()이 정상 종료 시키면 스택트레이스가 안 남아서 출력이 깔끔해질거 같아요
| if (!splitInput[i].matches("\\d+")) { | ||
| throw new IllegalArgumentException("형식을 벗어남"); | ||
| } |
There was a problem hiding this comment.
이 부분에서 예외 메시지를 더 구체적으로 작성하는건 어떨까요?
예외를 세분화하면 로직의 의도도 명확하게 표현되고, 디버깅 시에도 원인 파악이 쉬워질 것 같다고 생각합니다
There was a problem hiding this comment.
저도 그렇게 생각해요! 다음에는 더 세분화해서 작성해보도록 하겠습니다
| package calculator; | ||
|
|
||
| public class StringSplitter { | ||
| public static String[] splitInput(String input) { |
There was a problem hiding this comment.
입력값이 null이거나 빈 문자열일 경우 에러가 날 것 같아요
if (input == null || input.isEmpty()) return new String[]{"0"};로 예외 방어 해주시면 좋을 것 같습니다
There was a problem hiding this comment.
에러가 날 것 같은 부분을 더 세세하게 설정해야겠다고 생각했습니다 반영할게요!
| if (!splitInput[i].matches("\\d+")) { | ||
| throw new IllegalArgumentException("형식을 벗어남"); | ||
| } | ||
| numbers[i] = Integer.valueOf(splitInput[i]); |
There was a problem hiding this comment.
엄청 중요한 부분은 아닌데요,
Integer.valueOf()를 사용하면 래퍼 클래스 Integer로 반환이 되는데요,
Integer.parseInt() 대신 Integer.valueOf()를 사용하신 의도가 있을까요?
3 participants
No description provided.