-
Notifications
You must be signed in to change notification settings - Fork 0
[♻️ Refactor] user 코드 개선 - 지현 #4
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
base: main
Are you sure you want to change the base?
Conversation
aahreum
left a comment
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.
리뷰 남겼습니다~~!!!
| const [email, setEmail] = useState(''); | ||
| const [password, setPassword] = useState(''); | ||
| const { login, isLoading, error } = useLogin(); | ||
|
|
||
| const handleSubmit = async (e: React.FormEvent) => { | ||
| e.preventDefault(); | ||
|
|
||
| if (!email || !password) { |
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.
필드별로 각각 state를 작성하시는군요!
필드가 많아져도(닉네임, 이메일, 비밀번호, 비밀번호 확인...등등등.....) 각각 작성하시는 편이신가요?!
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개의 필드만 있어서 각 필드별로 state를 작성했는데,
회원가입 페이지처럼 3개 이상이라면 객체 형태로 묶어서 state를 관리하는 방법을 고려할 것 같습니다!
| try { | ||
| const item = localStorage.getItem(key); | ||
| return item ? JSON.parse(item) : null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| set<T>(key: string, value: T): void { | ||
| if (typeof window === 'undefined') return; | ||
|
|
||
| try { | ||
| localStorage.setItem(key, JSON.stringify(value)); | ||
| } catch (error) { | ||
| console.error('로컬스토리지 저장 에러 :', error); | ||
| } | ||
| } |
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.
와 로컬스토리지도 에러처리를 해볼 수 있겠네요...
저는 한번도 고려를 못해봤던 것 같아요
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.
클래스 활용 잘하시는거 부럽습니다.... 저는....클래스 너무 어려워요...
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.
사실 클래스 부분은 AI의 도움을 조금 받았습니다ㅎㅎ
Java로는 어떻게 작성하긴 했는데 TypeScript로는 조금 어려워서 결국 도움을 받았습니다...
| "tabWidth": 2, | ||
| "semi": true, | ||
| "singleQuote": true, | ||
| "bracketSameLine": true, |
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.
오 지현님도 true 파신가요??!!
저도 >태그 아래로 떨어지는거 싫어해서 꼭 넣는 편입니다ㅋㅋㅋㅋㅋ
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.
저도 아름님처럼 똑같이 싫어서 넣었습니다ㅎㅎ
| const message = | ||
| err instanceof Error ? err.message : '로그인에 실패했습니다'; | ||
| setError(message); | ||
| throw err; |
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.
여기서 던진 에러는 어디에서 사용되나요??!
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.
useLogin 훅에서 에러를 상태로 처리한 후 다시 throw하고는 있지만, login/page.tsx의 catch 블록에서 하는 처리가 없어서 실질적으로는 에러를 무시하고 있습니다.
원래 코드를 작성할 때는 login/page.tsx의 catch 블록에서 처리하게 하려고 했는데 작성하다보니까 useLogin 훅에서 처리를 해버려서 의미없는 코드가 되어버렸어요....ㅎㅎ😓
| api.interceptors.response.use( | ||
| (response: AxiosResponse) => { | ||
| return response.data; | ||
| }, | ||
| (error: AxiosError) => { | ||
| let errorMessage = ''; | ||
|
|
||
| if (error.code === 'ECONNABORTED') { | ||
| errorMessage = '요청 시간이 초과되었습니다. 다시 시도해 주세요.'; | ||
| } else if (error.request && !error.response) { | ||
| errorMessage = | ||
| '네트워크 오류가 발생했습니다. 인터넷 연결을 확인해 주세요.'; | ||
| } else if (error.response?.status && error.response.status >= 500) { | ||
| errorMessage = '서버 오류가 발생했습니다. 잠시 후 다시 시도해 주세요.'; | ||
| } else if (error.response?.status === 401) { | ||
| errorMessage = '로그인이 필요합니다.'; | ||
| } | ||
|
|
||
| if (errorMessage) toast.error(errorMessage); |
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.
에러 인터셉터가 ui로직도 함께 관리하고 있어서
이 부분도 분리해보면 좋을 것 같아요!
api.interceptors.response.use(
(response) => response.data,
(error: AxiosError) => {
const status = error.response?.status;
if (status === 401) {
error.message = '로그인이 필요합니다.';
} else if (status === 500) {
error.message = '서버 오류가 발생했습니다.';
}
return Promise.reject(error);
}
);인터셉터는 에러 메세지만 던져주고
UI 레이어에서 toast.error를 관리해보는건 어떨까요?!
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.
말씀해주신 부분은 생각해보지 못했는데 한번 시도해보겠습니다!
| import { tokenStorage } from '@/shared/lib/storage/tokenStorage'; | ||
|
|
||
| // API 베이스 URL 환경 변수 | ||
| const BASE_URL = process.env.NEXT_PUBLIC_API_BASE_URL; |
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.
하나 궁금한 점이 있는데요!
더줄게 작업하실 때 서버 api 인스턴스를 별도로 만드셨나요?!
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.
네 더줄게에서도 지금 리펙토링할 때 작성한 것처럼 별도로 작성했습니다!
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개가 필요하군요... 저도 미션할때 server, client 별도로 만들어야겠어요! 감사합니다!!
| if (!token) return false; | ||
|
|
||
| try { | ||
| const payload = JSON.parse(atob(token.split('.')[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.
atob... 이런것도 있군요..
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.
Base64 디코딩 함수입니다! (ASCII to Base64)
JWT의 payload가 Base64로 인코딩되어 있기 때문에, 안의 정보를 읽으려면 디코딩이 필요해서 사용했습니다.
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.
아하😲 친절한 설명 감사합니다!!!!
| const [isAuthenticated, setIsAuthenticated] = useState(false); | ||
| const [isLoading, setIsLoading] = useState(true); | ||
|
|
||
| useEffect(() => { | ||
| const checkAuth = () => { | ||
| const authenticated = authService.isAuthenticated(); | ||
| setIsAuthenticated(authenticated); | ||
|
|
||
| if (!authenticated) { | ||
| router.push(redirectTo); | ||
| } | ||
|
|
||
| setIsLoading(false); | ||
| }; |
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.
제가 생각했을 때는 isAuthenticated라는 state가 없어도 되지 않을까? 싶은데요!
혹시 추가하신 이유가 있으실까요?!
저는 이렇게 생각했습니다
아래 코드처럼 별도로 상태를 두지 않아도 리다이렉트를 구현할 수 있지 않을까 싶었습니다!!
혹시 제가 놓친 부분이 있다면 말씀해주세요!!!
'use client';
import { useEffect, useState } from 'react';
import { useRouter } from 'next/navigation';
import { authService } from '../services/authService';
export const useAuth = (redirectTo: string = '/login') => {
const router = useRouter();
const [isLoading, setIsLoading] = useState(true);
useEffect(() => {
const authenticated = authService.isAuthenticated();
if (!authenticated) {
router.push(redirectTo);
return;
}
setIsLoading(false);
}, [router, redirectTo]);
return { isLoading };
};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.
말씀하신대로 없어도 되는 state입니다.
처음에 코드를 작성하면서 혹시 나중에 사용할까 싶어서 추가한 state인데 결국에는 사용하지 않아서 불필요한 state가 되어버렸습니다...
제가 지웠어야 했는데 미쳐 확인을 못하고 그냥 남겨 두었어요ㅎㅎ
📊 리팩토링 작업 내용
⚖️ Before vs After 비교
Before: 혼재된 로직
After: 계층별 분리
🎓 적용된 설계 원칙
1. Single Responsibility Principle (SRP)
원칙: 각 모듈은 하나의 책임만 가져야 한다
적용 예시:
2. Dependency Inversion Principle (DIP)
원칙: 상위 계층은 하위 계층의 구체적 구현이 아닌 추상화에 의존해야 한다
적용 예시:
의존성 흐름:
3. Open/Closed Principle (OCP)
원칙: 확장에는 열려있고 수정에는 닫혀있어야 한다
적용 예시:
4. Separation of Concerns (관심사 분리)
원칙: 서로 다른 관심사는 분리되어야 한다
적용 예시: