김한영#4
Hidden character warning
Conversation
There was a problem hiding this comment.
- 아래 클래스파일까지 공통 내용이라 첫번째 파일에 적습니다.
.idea 폴더와 build 폴더는 .gitignore에 추가해주세요
.idea/는 IntelliJ 개인 설정 파일이고, build/는 빌드 결과물(.class 파일 등)이라 Git에 올리면 안 됩니다.
프로젝트 루트의 .gitignore에 아래 내용을 추가해주세요.
.idea/
build/
*.ser
*.ser 파일(직렬화 데이터 파일)도 이후에는 실행 환경마다 달라지므로 사용되지 않는 경우, 지우시거나 gitignore 추가하시는 게 좋을 것 같습니다.
(방법을 모르시겠으면 다음 멘토링 시간에 알려드리겠습니다)
|
|
||
| // 이름으로 채널 있는지 확인 | ||
| @Override | ||
| public Boolean findAll(String channelname) { |
There was a problem hiding this comment.
findAll(String channelname)은 이름만 보면 "전체 조회"처럼 보이지만, 실제로는 채널 이름 중복 여부를 확인하고 있습니다. 게다가 중복이 없으면 true, 있으면 false를 반환해서 직관적이지 않아요.
현재: 이름과 동작이 다르고 true/false도 반대 의미로 보여서 헷갈림
Boolean findAll(String channelName); // 이름 없으면 true??
권장:
의도가 명확한 이름으로 바꾸시거나 exists 같이 JPA에서 많이 사용하는 네이밍으로 바꾸시는 것도 권장드립니다.
boolean isChannelNameAvailable(String channelName);
// 또는
boolean existsByChannelName(String channelName); // 이 경우는 true/falser가 반대로 리턴되어야합니다.
| outer: while(true){ | ||
|
|
||
| // 사용자 정보 가져오기 | ||
| String useremail = userService.enableuser(); // -> 성공시 현제 유저 이메일 가져옴 |
There was a problem hiding this comment.
현재 readUserFromFile() / saveUserToFile() 처럼 메서드 호출할 때마다 전체 파일을 읽고 → 전체를 다시 저장하고 있습니다.
지금 단계에서 동작은 하지만, 예를 들어 enableuser() 메서드 같은 경우 221~222번째 줄에서 연속으로 두 번 호출되면서 파일을 불필요하게 두 번 읽게 됩니다.
String useremail = userService.enableuser(); // 파일 읽기 1번
if(userService.enableuser() == null){ // 파일 읽기 2번 (불필요)
이미 useremail에 결과를 담았으니, 아래처럼 바꾸면 됩니다.
javaString useremail = userService.enableuser();
if(useremail == null) { // 이미 받아온 값으로 비교
|
|
||
| login(); // 로그인하기 | ||
|
|
||
| outer: while(true){ |
There was a problem hiding this comment.
outer는 실무에서는 거의 본 적이 없는 문법입니다.
실제 서비스에서는 loop 탈출 패턴은 하기 패턴 등을 많이 사용하고 있어서
유지보수성을 생각하면 안쓰시는 걸 권장드립니다.
boolean done = false;
while (!done) {
while (...) {
if (condition) {
done = true;
break;
}
}
}
koritsu
left a comment
koritsu
left a comment
There was a problem hiding this comment.
안녕하세요 한영님! 코드 잘 봤습니다. 콘솔 기반으로 로그인/회원가입/채널/메시지까지 전체 흐름을 직접 구현하신 점이 인상적입니다. 몇 가지 피드백 남겨드리겠습니다.
일부는 파일에 직접 달았고 그 외에 전체적인 내용을 남기겠습니다.
[메서드 네이밍 컨벤션을 지켜주세요]
자바에서는 메서드 이름을 camelCase로 작성하는 게 규칙입니다. 현재 코드에서 몇 가지 이름이 이 규칙에 맞지 않는 것들이 보입니다.
findmessage() -> findMessage()
tomessage() -> toMessage() 또는 sendMessage()
finduserId() -> findUserId()
enableuser() -> findEnabledUser()
findnamebyid() -> findNameById()
findchannelbyid() -> findChannelById()
getchannelid() -> getChannelId()
creat() -> create() (오타)
IdByName() -> findNamesByIds()
작은 차이지만, 자바 개발자들 사이의 약속이라 지켜주시면 코드 가독성이 좋아질 것 같습니다.
그리고 변수명도 전부 소문자로 된 패턴이 많이 보이는데, 이런 것들도 소문자로 시작하는 camelCase 작성해보시는 게 좋을 것 같습니다.
[메소드 이름과 동작이 다름]
존재 여부를 판단하는 메소드 이름이 find인 경우가 많은데, 이것들도 is 혹은 exists 등으로 존재여부 판단한다는 걸 명시하는 게 좋아 보입니다.
[Scanner를 여러 곳에서 new로 만들고 있어요]
login(), channel(), createChannel(), inChannel() 등 거의 모든 메서드에서 new Scanner(System.in)을 각각 생성하고 있는데, System.in에 대한 Scanner는 하나만 만들어서 공유하는 게 안전합니다 (여러 개 만들면 입력 버퍼 충돌로 의도치 않은 동작이 생길 수 있습니다)
private static final Scanner input = new Scanner(System.in);
[예외 처리를 좀 더 구체적으로 해주세요]
catch(Exception e)로 모든 예외를 한번에 잡고 있는데, 어떤 에러인지 구분이 안 돼서 디버깅이 어려워집니다. IOException, ClassNotFoundException 등 구체적인 예외를 먼저 잡는 연습도 해보시면 좋을 것 같습니다.
[서비스 인터페이스에 비즈니스 로직이 섞여 있어요]
이번 미션의 핵심은 "비즈니스 로직"과 "저장 로직"을 분리하는 것인데요.
현재 FileUserService.login() 안에 로그인 검증 로직(비즈니스) + 파일 읽기/쓰기(저장) + 출력 메시지(UI)가 전부 섞여 있어요.
앞으로 Repository 패턴을 적용할 때 이 부분을 분리해야 하니, 지금부터 "이 코드는 어느 계층의 역할인가?"를 의식하면서 보시면 좋겠습니다.
[머지 타겟 브랜치가 메인으로 되어있습니다]
메인이 아니라 branch는 본인의 이름이 들어간 브랜치로 머지하도록 PR 다시 올려주세요
merge까지는 진행하지 않겠습니다.
2 participants
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게