refactor: Auth Feature 수직 슬라이스 전환 — AuthDomain + AuthData 분리#71
refactor: Auth Feature 수직 슬라이스 전환 — AuthDomain + AuthData 분리#71
Conversation
gomminjae
commented
Mar 27, 2026
- Auth/Domain: AuthRepository 프로토콜, Auth 전용 모델(AuthUser, AuthSimpleUser, AuthSignupResponse 등), UseCase 프로토콜
- Auth/Data: AuthRepositoryImpl + Auth 전용 DTO (UserAPI 기반)
- Auth/Sources: import Domain → import AuthDomain 전환, UserUseCase 의존 제거 → AuthRepository 직접 의존
- CompositionRoot: AuthRepositoryImpl 직접 생성으로 DI 변경
- MFA Step 3 파일럿: 공유 Domain/Data 제거를 위한 첫 Feature 수직 슬라이스
- Auth/Domain: AuthRepository 프로토콜, Auth 전용 모델(AuthUser, AuthSimpleUser, AuthSignupResponse 등), UseCase 프로토콜 - Auth/Data: AuthRepositoryImpl + Auth 전용 DTO (UserAPI 기반) - Auth/Sources: import Domain → import AuthDomain 전환, UserUseCase 의존 제거 → AuthRepository 직접 의존 - CompositionRoot: AuthRepositoryImpl 직접 생성으로 DI 변경 - MFA Step 3 파일럿: 공유 Domain/Data 제거를 위한 첫 Feature 수직 슬라이스
There was a problem hiding this comment.
Code Review
This pull request refactors the authentication feature by introducing dedicated AuthDomain and AuthData modules and implementing the AuthRepository using Moya. Key feedback includes a data integrity issue where industryId mapping loses its optionality, fragile error handling based on string matching, and a bug in the signup view model that double-increments the resend failure counter. Recommendations also cover removing redundant catch blocks and optimizing performance by reusing date formatters.
| birthYear: birthYear, | ||
| gender: gender, | ||
| createdAt: createdAt, | ||
| industryId: industryId ?? 0, |
There was a problem hiding this comment.
There is a potential data integrity issue when mapping industryId. The DTO property industryId is an Int?, and the domain model AuthUser also expects industryId to be an Int?. However, the mapping uses industryId ?? 0, which converts nil to 0. This changes the semantic meaning of the data, as nil (not provided) becomes 0 (which could be a valid industry ID). This can lead to subtle bugs downstream. The ?? 0 should be removed to preserve the optionality.
| industryId: industryId ?? 0, | |
| industryId: industryId, |
| } catch let error as NetworkError { | ||
| if case .serverError(let statusCode, let message) = error, statusCode == 400 { | ||
| let errorMessage = message ?? "" | ||
| if errorMessage.contains("비밀번호") { | ||
| throw LoginError.invalidPassword | ||
| } else if errorMessage.contains("계정") { | ||
| throw LoginError.accountNotFound | ||
| } | ||
| } | ||
| throw LoginError.networkError(error) | ||
| } catch { | ||
| throw LoginError.networkError(error) | ||
| } |
There was a problem hiding this comment.
The error handling logic relies on string matching on the error message from the server (errorMessage.contains("비밀번호")) to determine the type of login error. This is fragile and can break if the server error messages are changed, for example, for localization or just wording updates. It would be more robust to rely on specific error codes from the server API to distinguish between different failure reasons like "invalid password" and "account not found".
| if isRequestSent { | ||
| resendFailureCount += 1 | ||
| } |
There was a problem hiding this comment.
The resendFailureCount is incremented here on any resend attempt, and again in the catch block (line 235) if the network request fails. This means for a single failed resend attempt, the counter is incremented twice, causing the user to hit the resend limit of 3 attempts after only one failed resend. The counter should likely only be incremented on failure, i.e., only in the catch block.
| do { | ||
| let response: [AuthSimpleUserDTO] = try await provider.asyncRequest( | ||
| .checkPhoneNumber(phoneNumber: phoneNumber) | ||
| ) | ||
| return response.map { $0.toDomain() } | ||
| } catch let error as NetworkError { | ||
| if case .serverError(let statusCode, _) = error, statusCode == 400 { | ||
| return [] | ||
| } else { | ||
| throw error | ||
| } | ||
| } catch { | ||
| throw error | ||
| } |
There was a problem hiding this comment.
The error handling logic in this function can be simplified. The catch let error as NetworkError block is followed by a generic catch { throw error } block, but the first block's else clause already re-throws the error, making the second catch block redundant.
Additionally, assuming any 400 status code means "no users found" and returning an empty array could hide other potential client-side errors. It would be safer to confirm if the API guarantees that a 400 is only for this case.
do {
let response: [AuthSimpleUserDTO] = try await provider.asyncRequest(
.checkPhoneNumber(phoneNumber: phoneNumber)
)
return response.map { $0.toDomain() }
} catch let error as NetworkError {
if case .serverError(let statusCode, _) = error, statusCode == 400 {
return []
}
throw error
}| public var formattedCreatedAt: String { | ||
| let isoFormatter = ISO8601DateFormatter() | ||
| isoFormatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds] | ||
| guard let date = isoFormatter.date(from: createdAt) else { | ||
| return createdAt | ||
| } | ||
| let displayFormatter = DateFormatter() | ||
| displayFormatter.locale = Locale(identifier: "ko_KR") | ||
| displayFormatter.dateFormat = "yyyy.MM.dd" | ||
| return "\(displayFormatter.string(from: date)) 가입" | ||
| } |
There was a problem hiding this comment.
The formattedCreatedAt computed property creates new instances of ISO8601DateFormatter and DateFormatter every time it's accessed. These formatters are expensive to initialize and can impact performance, especially if used frequently (e.g., in a list). For better performance, they should be created once and reused by defining them as static constants.
private static let isoFormatter: ISO8601DateFormatter = {
let formatter = ISO8601DateFormatter()
formatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds]
return formatter
}()
private static let displayFormatter: DateFormatter = {
let formatter = DateFormatter()
formatter.locale = Locale(identifier: "ko_KR")
formatter.dateFormat = "yyyy.MM.dd"
return formatter
}()
public var formattedCreatedAt: String {
guard let date = Self.isoFormatter.date(from: createdAt) else {
return createdAt
}
return "\(Self.displayFormatter.string(from: date)) 가입"
}You can add the static formatters to the AuthSimpleUser struct and update formattedCreatedAt to use them. This avoids repeated initializations.