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

프로젝트 관리 앱 [STEP2-1] malrang, Eddy #127

Merged
merged 12 commits into from
Jul 5, 2022

Conversation

kimkyunghun3
Copy link

@kimkyunghun3 kimkyunghun3 commented Jul 5, 2022

안녕하세요 @innocarpe
2-1 PR 보냅니다! 감사합니다

배경

  • 프로젝트에 필요한 라이브러리 설치
    • UI: RxCocoa
    • 비동기 처리: Rxswift, RxRelay
    • 로컬 DB: Realm
    • 원격 DB: FireBase Realtime DataBase
  • TodoList View에서 NavigationBar 구현
  • 각 파일 폴더로 구분

작업 내용

  • AppDelegate, SceneDelegate 주석 제거

  • 각 Scene 별로 파일 분리

  • TodoListViewControlle 네비게이션바 구현
    네비게이션바에 사용될 title과 button 추가

테스트 방법

SPM(Swift Package Manager)으로 설치했으므로 따로 라이브러리 설치가 필요하지 않습니다.
시뮬레이터를 iPAD로 설정해서 실행하시면 됩니다.

리뷰 노트

Scene에 따라 폴더 분리를 하면 각 화면에 필요한 요소를 폴더로 관리할 수 있어 좋다고 생각합니다. 이 방식이 괜찮은 방향일까요?

영상

TodoListViewController 네비게이션바 구현 영상

@innocarpe
Copy link

@kimkyunghun3 결과적으로 같게 될 수도 있는데요,

폴더구조를 각 화면별로 그룹을 나누어 진행하려고 합니다!

되도록이면 도메인 별로 폴더 그룹을 나누시는 것을 추천드립니다. 그러면 도메인 규모를 얼마만큼으로 정하냐의 문제가 있을 수는 있겠지만, 같은 도메인 안에 여러 화면이 존재할 수도, 아닐 수도 있습니다. 그리도 오히려 도메인 성질에 따라 Viewless 할 수도 있구요~

정답은 없으니 앞으로 이런 기준으로 폴더링을 해 가시면서 얘기를 나누면 좋을 것 같네요!

//

import UIKit
import FirebaseCore

Choose a reason for hiding this comment

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

[P4] OS 레벨의 프레임워크와 그 외의 프레임워크는 개행으로 분리하는 것이 좋습니다. 더 구체적으로는 아래와 같은 느낌으로 분리합니다(각 영역 안에서는 알파벳 순으로 정렬합니다):

import UIKit

import AppsFlyer
import FirebaseCore

import Shared_Foundation
import Shared_UIFoundation

import AppCore_Entity
import AppCore_Service

import AppFeature_Main

Choose a reason for hiding this comment

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

네! 이런식으로 분리해야 겠다는 생각을 못해본것 같습니다!
권해주신 방법이 훨씬 정돈되어있는 느낌인것 같습니다! :)

조언해주신 방법으로 수정하였습니다! 😁


func sceneWillEnterForeground(_ scene: UIScene) {}

func sceneDidEnterBackground(_ scene: UIScene) {}

Choose a reason for hiding this comment

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

[P1] 사용하지 않는 코드들은 전부 제거해 주세요.

Copy link

@malrang-malrang malrang-malrang Jul 5, 2022

Choose a reason for hiding this comment

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

네! 사용되지 않는 코드 제거하였습니다! 😁

func application(
_ application: UIApplication,
didDiscardSceneSessions sceneSessions: Set<UISceneSession>
) {}

Choose a reason for hiding this comment

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

[P1] 사용하지 않는 코드들은 전부 제거해 주세요.

Choose a reason for hiding this comment

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

네! 사용되지 않는 코드 제거하였습니다! 😁

final class TodoListViewController: UIViewController {
override func viewDidLoad() {
super.viewDidLoad()
setUpNavigation()

Choose a reason for hiding this comment

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

팀에서 혹시 코드 컨벤션을 어떻게 가져가고 있으신지요? self. 를 붙이냐 마냐에 대한 얘기를 보통 하시는것 같던데, 리뷰어의 기준에서 보면 인스턴스 프로퍼티와 메서드는 self. 가 있는 것이 훨씬 가독성이 높습니다.

리뷰어는 각 PR 안에서 주어진(+ 많지 않은) 맥락만 가지고 리뷰를 하게 되기 때문에 최대한 많은 정보를 전달하는 것이 중요합니다. self 는 그런 측면의 의견이라 보시면 되어요.

Copy link
Author

Choose a reason for hiding this comment

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

저희가 이야기 했을 때에 self.를 붙이자고 코드 컨벤션 이야기를 미리 했습니다.
앞으로 작성할 때 인스턴스 프로퍼티와 메서드에 self. 모두 붙이도록 하겠습니다!

Choose a reason for hiding this comment

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

저희가 이야기 했을 때에 self.를 붙이자고 코드 컨벤션 이야기를 미리 했습니다. 앞으로 작성할 때 인스턴스 프로퍼티와 메서드에 self. 모두 붙이도록 하겠습니다!

아하 그러시군요 앞으로는 저도 그렇게 인지하겠습니다!

Copy link

@innocarpe innocarpe left a comment

Choose a reason for hiding this comment

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

첫 PR 고생하셨습니다!! 이런 식으로 작게 PR 나누는 것 좋은 것 같습니다 ☺️

@innocarpe innocarpe merged commit 8d53ee8 into yagom-academy:ic_5_eddy05 Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants