Skip to content
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

[team-07] Login - Home 화면 IssueCard 출력 흐름 구현 #267

Open
wants to merge 8 commits into
base: team-07
Choose a base branch
from

Conversation

wnsxor1993
Copy link
Collaborator

안녕하세요 롤로!
3주 동안 리뷰 정말 감사했습니다! 덕분에 헤맸던 Clean Architecture 의Usecase, Repository 의 의미와 역할, 단위를 정하는 것에 조금더 자신이 생긴것같습니다.

🧑🏼‍💻 작업 목록

  • 리뷰 사항 반영

    • ViewModel Initializer 수정
    • URLConfiguration 겹치는 host, path 수정
  • IssueCard 받아오는 로직 구현

    • FetchIssueCardUsecase , DefaultIssueCardRepository 구현
    • LoginViewModel 구현
    • LoginViewModel viewController 바인딩 작업 완료
  • 전반적인 Usecase, repository Naming 수정

    • DefaultLoginUsecase → LoginUsecase
    • GitHubAuthorizationUsecase → GithubLoginUsecase
    • AppleAuthorizationUsecase → AppleLoginUsecase
    • DefaultUserInfoUsecase → RequestTokenInfoUsecase
    • RequestUserInfoUsecase → DefaultRequestTokenInfoUsecase
    • DefaultUserInfoRepository → RequestTokenInfoRepository
    • UserInfoRepository → DefaultRequestTokenInfoRepository
      수정필요
    • FetchIssueCardUsecase → ViewIssueCardUsecase
    • FetchOpenIssuCardUsecase → ViewFilteredIssueCardUsecase
    • DefaultIssueCardRepository → ViewIssueCardRepositoy
    • FetcthIssueCardRepository → ViewFilteredIssueCardRepositoy

🤔 고민과 해결

1.0 UseCase, Repository 단위및 네이밍 정리

→ 기존 Defualt~Usecase/Repository 의 네이밍을 Protocol 에 사용했었습니다.

Default 라는 프로토콜네이밍은 범용적인 의미로 사용되었었고, 프로토콜을 따르는 usecase 의 네이밍을 사용자의 세부적인 요청에 따라 정의 했었습니다.

→ 이는 프로토콜이 가지고 있는 함수도 범용적으로 사용되어야한다는 생각을 초래 하게되었고, 문법상 구현이 안돼는 문제를 겪었습니다(generic 문제).

→ 따라서 Defualt~ 로 시작하게 되는 usecase 네이밍에 default 라는 단어를 빼고 단순히 유저가 원하는 행위 ex) ViewIssueCardUsecase, 이슈카드를 보고싶다 라는 행위를 프로토콜로 명명했습니다.

→ 사용자가 “이슈카드를본다” 라는 하나의 큰 Usecase 는 IssueCardDTO 라는 공통적인 DTO 를 서버로 부터 받게 됩니다.

→ 어떤 이슈카드를 보고싶은지, 예를들어 닫힌, 특정한 마일스톤 을가진, 특정한 label 을 가진 이슈 카드를 보는 사용자의 요청은 ViewIssueCardUsecase 를 채택하고있는 구체 Usecase, ex) ViewFilteredIssueCardUsecase 를 구현 하여, 메소드 execute(”어떠한 Usecase 를 실행한다" 는 의미에서 execute 를 사용함) 매개변수로 사용자가 원하는 filter 값을 넘깁니다. 이후,repository 에서 queryItem 에 매개변수로 들어온 filter 값을 할당하여 서버로 요청을 보낼수있게되고 필터링이 적용된 IssueCard 정보를 서버로부터 받을수 있게 됩니다.

Repository 또한 같은 네이밍 컨벤션으로 진행하면 되는데 사용자가 원하는 메인 요청의 repository 네이밍, ViewIssueCardRepository, ViewFilteredIssueCardRepository 등으로 컨벤션을 정했습니다.

→ 이때 repository 메소드명은 서버에 어떠한 동작을 요청한다는 의미로 네이밍을 “시스템 입장” 에서 지었습니다. ex) fetchIssueCard .

정리하면 Usecase, repository 네이밍은 사용자의 입장에서 어떠한 목적을 위해 실행하는 행동 의 일반적인 (큰 틀 단위) 의 행위를 Protocol 명으로 지어주었다. 이를 채택하는 구체타입들은 유저의 행동에 따르는 변수 단위의 네이밍으로 지어주었습니다.

useCase 는 어떠한 행동을 실행한다 는 의미의 execute 메소드, repository 는 시스템 입장 에서 서버로부터 요청하는 의미 fetch/load~ 등의 메소드명을 가지도록 하였습니다.

2.0 IssueCard 홈 화면 출력 흐름 구현

How?

  1. 홈 화면에서 IssueCard를 API로 fetch하고 View로 출력해주는 흐름을 MVVM + 클린 아키텍처 구조로 설계 및 도식화

  2. IssueCard를 fetch 해오는 Usecase를 확립하고 관련된 Repository와 EndPoint 구현(ViewIssueCardUsecase 프로토콜은 어떠한 종류의 IssueCard들을 가져와서 보여주는 역할만 하는 Usecase이므로 Entity나 DTO를 구체 타입으로 명시하여 사용 ⇒ IssueCard 외의 Entity나 DTO를 받아올 상황이 없으므로 제네릭과 같이 범용적으로 사용하는 방향으로 구현하지 않았음.)

  3. Usecase에서 DTO를 Entity로 mapping 후, VM에 클로저로 전달하고 VM은 이를 Observable로 VC까지 전달

  4. VC에서 dataSource에 갱신된 데이터를 전달하고 이를 토대로 TableView를 reload하여 받아온 IssueCard를 화면으로 출력

📺 작업 결과물

@wnsxor1993 wnsxor1993 added the review-iOS Extra attention is needed label Jul 1, 2022
@wnsxor1993 wnsxor1993 requested a review from eeeesong July 1, 2022 03:24
@wnsxor1993 wnsxor1993 self-assigned this Jul 1, 2022
Copy link

@eeeesong eeeesong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

마지막까지 프로젝트 진행하시느라 정말 고생 많으셨습니다 👏🏻 👏🏻 👏🏻
UseCase나 Repository, ViewModel 등 레이어들의 역할과 네이밍까지 깊게 고민해보시고 배운 점을 꼼꼼하게 정리하신 게 정말 멋지네요!
Repository - UseCase - ViewModel 세 계층을 거쳐서 데이터를 전달해보시니 어땠나요? 🙂
초반에도 한번 말씀드린 적이 있는데, 이 경험을 통해 real로 느낀 점이 뭔지 잘 기억해두시면 정말 많은 도움이 될 것 같아요!
저도 클린 아키텍처에 대해 한번 더 생각해볼 수 있던 기회라 너무 유익했습니다. 감사해요!
너무 너무 고생하셨습니다👍

Comment on lines +11 to 13
struct IssueCardArrayDTO: Codable {
let issues: [IssueCardDTO]
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 네이밍에서는 Array와 같은 자료구조 이름을 직접 사용하기 보다는
복수형태로 사용해주는 게 더 자연스럽다는 생각입니다 :) (IssueCardsDTO)

Comment on lines +19 to +25
tokenInfoRepository?.fetchTokenInfo(completion: { userInfo in
guard let userInfo = userInfo else {
completion(nil)
return
}
completion(userInfo)
})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

옵셔널을 전달할 수 있으니 guard문은 불필요해보이네요ㅎㅎ

Comment on lines +42 to 54
extension IssueCardDTO {

func toDomain() -> IssueCardEntity {
return .init(id: issueID, title: title, content: content, isSelected: false, mileStone: milestoneName, labels: labels.map { $0.toDomain() })
}
}

extension LabelDTO {

func toDomain() -> LabelEntity {
return .init(id: labelID, labelName: labelName, labelColor: labelColor)
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사소한 부분이긴 하지만 extension 작성 시엔
작성하는 타입의 선언부와 최대한 가까운 위치에 작성하면 더 직관적입니다 :)
그리고 아무래도 스코프가 달라지기 때문에, 사실 개수가 몇 개 안 되고 이 정도로 간단한 메소드들은 굳이 분리하지 않아도 될 수 있어요!
객체도 그렇고 뭐든지 분리하게 될 경우엔 나름의 장점도 있지만 복잡도와 트레이드오프가 발생한다는 점을 기억해주세요!

Comment on lines +11 to +13
var scheme: String = "https"
var host: String = "0e1f525b-4045-4a86-b2d7-b782850ccb9f.mock.pstmn.io"
var path: String = "/issue-tracker/api/issues"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 값들은 사실 상 변경될 일이 없지 않나요?
let으로 작성해도 될 것 같네요ㅎㅎ

Comment on lines +18 to +29
func fetchIssueCard(completion: @escaping (IssueCardArrayDTO?) -> Void) {
NetworkService.request(endPoint: endPoint) { result in
switch result {
case .success(let data):
let decoder = Decoder<IssueCardArrayDTO>()
let issuecards = decoder.decode(data: data)
completion(issuecards)
case .failure(let error):
print(error)
}
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번 프로젝트의 Repository에서는 사실 상 데이터를 decode해주는 역할만 하고 있긴 하네요 :)
이 역할이 조금 아쉽지는 않으셨나요?
만약 Repository가 많은 쓸모를 가지려면 어떤 상황이 주어져야 할까요?
지금은 하나의 API를 통해 필요한 모든 걸 가지고 오지만, 만약 API가 여러개거나 또는 네트워크 서비스 이외의 곳 (로컬 저장소라든가)
에서 데이터를 가져올 수 있고 또 로컬과 리모트 데이터 간의 싱크업이 필요한 프로젝트라면 Repository가 훨씬 많은 역할을 할 수 있지 않을까 싶어요.

Comment on lines +18 to +25
func execute(completion: @escaping ([IssueCardEntity]) -> Void) {
fetchIssueCardRepository.fetchIssueCard { issueCards in
guard let issueCards = issueCards else { return }

let issueEntities: [IssueCardEntity] = issueCards.issues.map { $0.toDomain() }
completion(issueEntities)
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

몇몇 UseCase들 역시 객체를 매핑하거나 전달하는 역할 외엔 하는 일이 많이 없죠ㅎㅎ
그런데 로그인 관련한 UseCase들은 비교적 많은 일을 하고 있는 것 같아요.
그렇다면 어떤 곳에서는 계층을 덜 쓰고, 어떤 곳에서는 더 많은 계층을 나누는 건 어떨까요? 이것도 정말 어려운 문제 같아요.
프로젝트의 모든 곳에서 같은 규칙을 지키는 건 통일감 측면에서 이점이 있지만 (별 고민 없이 무조건 다 만들면 되니 고민할 시간이 줄겠죠?)
또 이렇게 기능이 상대적으로 적은 부분에서는 보일러 플레이트가 늘어난다는 단점이 있을 거예요.
당연히 저에게도 답은 전혀 없고요. 늘 고민해보면 좋은 문제 같습니다ㅎㅎ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-iOS Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants