회원가입에 대한 유닛 테스트 코드를 작성하다, 회원가입 코드가 정말 남이 알아보기 힘들게 작성하였다고 생각했다.
회원가입하는 함수가 회원가입만 로직만 수행하는 것이 아니고 유저조회, 유저생성, 토큰 생성등 여러 기능을 한번에 수행하고 있었다. 이는 단일 책임 원칙(SRP)을 지키지 않는 프로그래밍 방식이었다. 디자인패턴을 무조건 지켜야하는 것은 아니지만, 당장 지키지 않으니 나부터 알아보기가 매우 힘들었다.
또한 테스트코드를 작성하는 부분에서 유닛테스트는 해당 함수가 그 기능을 제대로 수행하는지 확인하는 용도여야하는데, 하나의 함수가 여러기능을 하니 테스트를 하는 의미가 있나 싶을 정도였다.
기존의 회원가입 코드
async signUp(data: SignupRequestDto): Promise<Tokens> {
const {
snsId,
name,
nickname,
birthday,
bank,
account,
fcmId,
profileImageSrc,
} = data;
// snsId로 유저 식별
const snsIdExist = await this.userRepository.findOne({
where: { snsId: snsId, deletedAt: null },
});
if (snsIdExist) {
throw new BadRequestException({
message: '이미 가입된 유저입니다!',
code: customErrorCode.USER_ALREADY_EXIST,
});
}
// fcmId로 유저 다시 식별
const fcmIdExist = await this.userRepository.findOne({
where: { fcmId: fcmId, deletedAt: null },
});
if (fcmIdExist) {
throw new BadRequestException({
message: '이미 가입된 유저입니다!',
code: customErrorCode.USER_ALREADY_EXIST,
});
}
// validation이 있지만 닉네임 중복 다시 체크
const nicknameCheck = await this.userRepository.findOne({
where: { nickname: nickname, deletedAt: null },
});
if (nicknameCheck) {
throw new BadRequestException({
message: '이미 사용중인 닉네임 입니다!',
code: customErrorCode.DUPLICATE_NICKNAME,
});
}
// 유저 저장 (profileImage등록 안하면 기본 이미지로 설정
const newUser = new UserEntity();
newUser.snsId = snsId;
newUser.name = name;
newUser.nickname = nickname;
newUser.bank = bank;
newUser.account = account;
newUser.fcmId = fcmId;
newUser.birthday = birthday;
newUser.profileImgSrc =
profileImageSrc === null ? 'basic.png' : profileImageSrc;
newUser.alarm = true;
await this.userRepository.save(newUser);
const slackClient = new SlackApiClient();
await slackClient.newUser();
return this.createToken(newUser.id);
return { accessToken, refreshToken };
}회원가입 함수이다. 일단 보기에도 너무 좋지않고, 주석을 써놨다지만 너무 스파게티 소스 코드같아서 읽기가 힘들다.
우리 서비스가 큰 규모는 아니지만 이렇게 작은 서비스에서도 읽기가 힘든 서비스 함수라면 큰 규모의 서비스 함수를 이렇게 작성한다면 정말 읽기가 힘든 코드가 될 것이다.
async createUser(userDAO: UserDAO): Promise<User> {
const {
snsId,
name,
nickname,
birthday,
bank,
account,
fcmId,
profileImgSrc,
} = userDAO;
const newUser = new UserEntity();
newUser.snsId = snsId;
newUser.name = name;
newUser.nickname = nickname;
newUser.bank = bank;
newUser.account = account;
newUser.fcmId = fcmId;
newUser.birthday = birthday;
newUser.profileImgSrc =
profileImgSrc === null ? 'basic.png' : profileImgSrc;
newUser.alarm = true;
return await this.userRepository.save(newUser);
}일단 user를 생성하는 로직 부분은 userService함수에 정의해 주었다.
async signUp(data: SignupRequestDto): Promise<Tokens> {
const {
snsId,
name,
nickname,
birthday,
bank,
account,
fcmId,
profileImageSrc,
} = data;
// snsId로 유저 식별
const snsIdExist = await this.userRepository.findOne({
where: { snsId: snsId, deletedAt: null },
});
if (snsIdExist) {
throw new BadRequestException({
message: '이미 가입된 유저입니다!',
code: customErrorCode.USER_ALREADY_EXIST,
});
}
// fcmId로 유저 다시 식별
const fcmIdExist = await this.userRepository.findOne({
where: { fcmId: fcmId, deletedAt: null },
});
if (fcmIdExist) {
throw new BadRequestException({
message: '이미 가입된 유저입니다!',
code: customErrorCode.USER_ALREADY_EXIST,
});
}
// validation이 있지만 닉네임 중복 다시 체크
const nicknameCheck = await this.userRepository.findOne({
where: { nickname: nickname, deletedAt: null },
});
if (nicknameCheck) {
throw new BadRequestException({
message: '이미 사용중인 닉네임 입니다!',
code: customErrorCode.DUPLICATE_NICKNAME,
});
}
const user = this.userService.createUser(data);
const slackClient = new SlackApiClient();
await slackClient.newUser();
return this.createToken(user.id);
return { accessToken, refreshToken };
}유저 생성하는 부분만 빼주어도 코드가 읽기 그나마 편해졌다.
const snsIdExist = await userService.findUserBySnsId(snsId);
if (snsIdExist) {
throw new BadRequestException({
message: '이미 가입된 유저입니다!',
code: customErrorCode.USER_ALREADY_EXIST,
});
}
// fcmId로 유저 다시 식별
const fcmIdExist = await userService.findUserByFcmId(fcmId);
if (fcmIdExist) {
throw new BadRequestException({
message: '이미 가입된 유저입니다!',
code: customErrorCode.USER_ALREADY_EXIST,
});
}
// validation이 있지만 닉네임 중복 다시 체크
const nicknameCheck = userService.findUserByNickname(nickname);
if (nicknameCheck) {
throw new BadRequestException({
message: '이미 사용중인 닉네임 입니다!',
code: customErrorCode.DUPLICATE_NICKNAME,
});
}조회부분도 각각 userService로 함수를 넘겨주었다.
이제 회원가입은 회원가입 기능만 수행하고, 다른 기능들은 다른 서비스들에서 도맡아하게 되었다.
SRP를 지키며 간결하게 리팩토링
async findUserByNickname(nickname: string): Promise<User | null> {
const user = await this.userRepository.findOne({
where: { nickname: nickname, deletedAt: null },
});
if (!user) {
return null;
}
ModelConverter.user(user);
}
async findUserBySnsId(snsId: string): Promise<User | null> {
const user = await this.userRepository.findOne({
where: { snsId: snsId, deletedAt: null },
});
if (!user) {
return null;
}
ModelConverter.user(user);
}
async findUserById(id: number): Promise<User | null> {
const user = await this.userRepository.findOne({
where: { id: id, deletedAt: null },
});
if (!user) {
return null;
}
ModelConverter.user(user);
}
async findUserByFcmId(fcmId: string): Promise<User | null> {
const user = await this.userRepository.findOne({
where: { fcmId: fcmId, deletedAt: null },
});
if (!user) {
return null;
}
ModelConverter.user(user);
}어.. 하지만 이렇게 특정 column마다 유저를 찾는 함수를 따로따로 작성하니, 코드가 너무 길어진다.
유저를 찾는다는 같은 기능을 하는 함수들이므로 하나의 함수로 리팩토링 해주자.
async findUser(
property: string,
value: string | number,
): Promise<User | null> {
const user = await this.userRepository.findOne({
where: { [property]: value, deletedAt: null },
});
if (!user) {
return null;
}
return ModelConverter.user(user);
}이렇게 함수를 리팩토링 해주면 단일책임원칙을 위배하지 않으며 코드를 간결하게 작성할 수 있다.
항상 짧은 코드가 좋은 코드는 아니지만 해당 경우에는 훨씬 이점이 많은 것 같다.
async signUp(data: SignupRequestDto): Promise<Tokens> {
const { snsId, nickname, fcmId } = data;
const snsIdExist = await this.usersService.findUser('snsId', snsId);
if (snsIdExist) {
throw new BadRequestException({
message: '이미 가입된 유저입니다!',
code: customErrorCode.USER_ALREADY_EXIST,
});
}
const fcmIdExist = await this.usersService.findUser('fcmId', fcmId);
if (fcmIdExist) {
throw new BadRequestException({
message: '이미 가입된 유저입니다!',
code: customErrorCode.USER_ALREADY_EXIST,
});
}
const nicknameExist = await this.usersService.findUser(
'nickname',
nickname,
);
if (nicknameExist) {
throw new BadRequestException({
message: '이미 사용중인 닉네임 입니다!',
code: customErrorCode.DUPLICATE_NICKNAME,
});
}
const user = await this.usersService.createUser(data);
await this.slackClient.newUser();
const accessToken = this.createAccessToken(user.id);
const refreshToken = this.createRefreshToken(user.id);
return { accessToken, refreshToken };
}회원가입 코드가 정말 알아보기 쉽게 변경되었다. 서비스 함수 이름들도 직관적으로 바뀌어 해당 줄이 어떤 역할을 수행하는 지도 바로바로 알 수 있다.
class MockUserService {
#data = [
{
id: 1,
snsId: 'test_snsId',
nickname: 'test_nickname',
name: '김민우',
birthday: '19980914',
bank: 'hana',
account: '1234-5678',
profileImgSrc: 'basic.png',
fcmId: 'test_fcmId',
alarm: true,
deletedAt: null,
},
];
findUser(property: string, value: string | number) {
const data = this.#data.find(
(v) => v[property] === value && v.deletedAt === null,
);
if (data) {
return data;
}
return null;
}
}
beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
providers: [
AuthService,
{
provide: JwtService,
useClass: MockJwtService,
},
{
provide: UsersService,
useClass: MockUserService,
},
],
}).compile();
authService = module.get<AuthService>(AuthService);
});이렇게 하니 AuthService를 단위테스트할때 UserService를 mocking하기 정말 편해졌다.
이렇게 AuthService.spec.ts에서는 AuthService에서 사용하는 다른 서비스 함수들은 mocking하여 테스트하고 mocking한 함수들은 해당 서비스에서 테스트 해주면 되는 것이다.
이점?
때문에 내가 SRP원칙을 지키도록 코드를 수정하며 느낀점은
코드 가독성 향상: 함수가 하나의 기능만 수행하면 해당 함수의 목적이 명확해지며, 코드를 읽고 이해하기가 쉬워집니다.
재사용성: 단일 책임을 갖는 함수는 다른 부분에서 더 쉽게 재사용될 수 있습니다. 재사용성이 높아지면 코드 중복이 감소하고, 개발 시간을 단축할 수 있습니다.
테스트 용이성: 단일 책임을 갖는 함수는 테스트하기가 더 쉽습니다. 특정 기능에 대한 테스트를 작성하고 유지보수하는 것이 간단해지므로 품질을 향상시킬 수 있습니다.
위 세가지를 느낄 수 있었다. 아직 코드를 확장하는 일은 없었기 때문에 확장성에 대한 용이성은 느끼지 못하였다. 나머지 코드들도 SRP 원칙을 지키도록 수정해야겠다.
https://www.youtube.com/watch?v=__9qLP846JE