주니어의 미천한 로그인 로직에 코드 리뷰를 부탁드립니다!

Question

안녕하세요. 1년이 살짝 넘은 프론트 개발자입니다.딱히 사수가 정해지지 않고 si 프로젝트를 돌다 보니개발에 대한 깊이가 늘어나지 않는 것 같아서요.개발 서적을 읽고 예전에 작성한 코드를 리팩토링하고 있습니다.말씀드렸듯 사수가 없이 프로젝트를 돌다보니gpt한테만 물어보고.. 늘 정형화된 답이 나와서요 ㅠㅜ부끄러움을 무릅쓰고 코드에 대해서 다른 개발자 선후배님들의 조언을 듣고 싶어 글을 작성합니다.제가 생각한 리팩토링의 방향은 기존 함수가 한 번에 너무 많은 기능을 담당한다고 생각해서최대한 기능별로 함수를 분할하자! 라는 생각으로 진행했습니다.아래의 코드는 vue에서 실행되는 코드이며 로그인을 담당하는 함수들입니다.요구사항은 다음과 같습니다.1. 계정과 서버를 연결시켜야만 다른 종류의 api로부터 정보를 받을 수 있다.2. 로그인은 자동 로그인과 로그인 버튼을 클릭하면 실행된다. (코드에는 없지만 렌더링 되면 로그인을 요청합니다.)```javascript // 로그인 버튼에 바인딩 + mounted 훅에 추가 async handleLoginRequest() { if (this.isMissingLoginInfo()) { return } try { await this.connectToServer() await this.decideWhetherLogin() } catch (error) { console.log('handleLoginRequest 실패', error) } }, isMissingLoginInfo() { if (!this.loginInfo.id) { alert('로그인 id가 없습니다.') return true } if (!this.loginInfo.number) { alert('로그인 number가 없습니다.') return true } return false }, async connectToServer() { try { await this.serverFactory.connect() console.log('connectToServer 성공') } catch (error) { console.log('connectToServer 실패', error) } }, async decideWhetherLogin() { const isLogged = await this.isUserLogged() if (!isLogged) { return this.loginProcess() } const answerToLogin = confirm('이미 로그인되었습니다. 연결을 끊고 다시 로그인하시겠습니까?') return answerToLogin ? this.loginProcess() : this.disconnectServer() }, async isUserLogged() { const user = await this.extractUserInfo() return user?.[0]?.state !== 'NOTLOGIN' }, async extractUserInfo() { try { const allUser = await this.getUserList() const user = allUser.filter(user => user.userId === this.loginInfo.id) console.log('extractUserInfo 성공') return user } catch (error) { console.log('extractUserInfo 실패', error) return [] } }, async getUserList() { try { const channel = this.channel.getChannel() const allUser = await channel.getUserList() console.log('getUserList 성공') return allUser } catch (error) { console.log('getUserList 실패', error) return [] } }, async loginProcess() { try { await this.performLogin() this.setUserInfoAfterLogin() } catch (error) { console.log('loginProcess 실패', error) } }, async performLogin() { try { await this.serverFactory.login([this.channel], this.loginInfo) console.log('login 성공') } catch (error) { console.log('login 실패', error) } }, setUserInfoAfterLogin() { this.isLogin = true }, 글을 읽는데 시간을 내주셔서 감사드립니다!

ssalad · Answer

일단 에디터 다시 열어서 '<>'클릭하고 ```javascript 타이핑하면 코드블록 활성화되요. 거기에 코드 넣고 인덴트하면 더 잘보일거같습니다.

캐티 · Answer

비즈니스로직에는 try catch 구문을 사용하지 않는 것이 좋지 앟더나요. 예외 처리는 서비스나 특별한 모델에 최소한으로 넣어야 좋스빈다. 사실 머 애초부터 try catch 구문이 발생하지 않도록 하여야 중요한데 c# 쪽에서는 예외처리를 할 바에야 if 문을 사용하는 전례가 있는 것 같쩌. 물론 예외처리는 아주 무거운 작업이고 if는 이나마 가벼운 작업이어요.

감독님테스트가하고싶어요 · Answer

일단 질문 가독성이 너무 최악이고요.. 본인이 보기에도 그렇지 않나요?코드 보니까 이해안가는 부분들이 너무 많아서 리팩토링하셨다고 했는데 기존 코드가 실제 서비스에 이용되고있었나요? 아니면 본인이 직접 로그인을 구현한 것인지 궁금합니다.

캐티 · Answer

이상한 얘기를 들으면 혼나빈다. try catch를 남용하면 디버깅 하기가 매우 어려워져요. 이 뿐만 아니라 예외 타입이나 그룹을 지정하여 처리하는 방법이 사용되는데요. 이렇게 하는 이유는 예외를 분리하여 효과를 보기 위함이어요. 이렇다 하더라도 로그인 같은 하찮은 기능은 분명하게 계정 잠김, 계정 정지, 보호 조치 같은 상태를 반환하여서 처리하는 수가 있쩌.그리고 예외를 남용하면 다른 회사에 코드를 보여주는 경우에 아마도 더 혼날꺼여요. 이럼 안대지 않겠나요.

신도림산당 · Answer

흠,, 일단 가독성도 떨어지구요. 함수를 굳이 너무 찢어놨다? 라는 느낌이 강하네요.

샘톨 · Answer

JSDoc을 이용하여 함수 시그니처 정보를 삽입하면 IDE에서 편리하게 힌트를 제공해 주어서, 개발에 많은 도움이 됩니다.함수 시그니처가 명확해야 합니다. 보여주신 몇몇 함수 시그니처가 모호한 경우들이 보이네요.// before async isUserLogged() { ... } // 어느 User가 로그인 했는 지 알 수 없음 isUserLogged() // after async isUserLogged({ user }) { ... } // user를 매개변수로 삽입하도록 지정 // manager user가 로그인 했는 지 확인하는 함수라고 유추할 수 있음 isUserLogged({ user: manager });불필요한 함수들이 보이네요. 기존의 함수 호출이 더 명확한 경우 그대로 이용하는 것이 좋습니다.// before async getUserList() { try { const channel = this.channel.getChannel() const allUser = await channel.getUserList() console.log('getUserList 성공') return allUser } catch (error) { console.log('getUserList 실패', error) return [] } } // User 리스트를 들고 오는 기능이다. 이렇게만 파악 가능 getUserList() // after // 특정 channel 객체를 통해 User 리스트를 들고 오는 기능이다라고 파악 가능. this.channel.getChannel().getUserList()alert 등으로 표시할 문자열은 상수로 빼두세요.// before isMissingLoginInfo() { if (!this.loginInfo.id) { alert('로그인 id가 없습니다.') return true } if (!this.loginInfo.number) { alert('로그인 number가 없습니다.') return true } return false } // after // is.. 명칭은 변수로 혼동할 수 있음. check로 변경 // 특정 대상을 로그인 했는지 체크 // alert 문자열을 상수로 지정 // alert 호출이 목적이라면 return 은 필요 없음 const NOT_ID_LOGIN_INFO = '로그인 id가 없습니다.' const NOT_NUMBER_LOGIN_INFO = '로그인 number가 없습니다.' checkLogin({ loginInfo }) { if (!loginInfo.id) { alert(NOT_ID_LOGIN_INFO) } if (!loginInfo.number) { alert(NOT_NUMBER_LOGIN_INFO) } }귀찮아서 여기까지 할께요.

カワウソ · Answer

요즘 AI 에이전트로 코드 간소화 요청 해보면 좋긴 합니다. 복수와 단일 결과를 잘 구분하는게 가독성에 좋을 듯

주니어의 미천한 로그인 로직에 코드 리뷰를 부탁드립니다!

7개의 답변

Q&A 목록