[CHORE] Calendar View의 가독성을 향상시키기 위해 노력했습니다. #607

MMMIIIN · 2023-12-07T09:27:12Z

🌁 Background

공통으로 사용하는 View 중 하나인 Calendar View를 더 예쁘게 다듬을 필요가 있다고 생각했습니다. 해당 뷰가 1년전에 만들어진 뷰이기도 하고, 다듬을 수 있을 부분들이 좀 있을 것 같아서 작업을 진행했습니다. 엄청 예쁘게 다듬어지진 못했지만 나름 가독성을 위해 노력했습니다.

👩‍💻 Contents

시작날짜와 종료날짜의 변수를 두개씩 사용하던 부분(temp로 사용하는 부분)을 없애고 하나로 통일시켰습니다.
시작, 종료 날짜 여부에 따라 상태가 변해야하는 버튼에 대해 대응할 수 있게 Subject를 추가했습니다. 기존 방식이었던 Delegate를 삭제했습니다.
방 생성단계에서 기간을 한번 설정했다가 다시 날짜를 수정할 때 다음 버튼이 활성화 되어 있던 문제를 해결했습니다.
TimeInterval을 Extension으로 분리해서 1일의 값인 86400과, 7일의 값인 604800(86400 x 7)를 추가했습니다.
이에 따라 테스트 코드를 비롯해 해당되는 부분의 코드를 수정했습니다.

✅ Testing

방 생성하는 부분과 방 정보를 수정하는 부분에서 해당 뷰가 사용 되어서 잘 적용되는지 체크해주시면 감사하겠습니다!

📱 Screenshot

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-12-07.at.18.18.03.mp4

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-12-07.at.18.24.44.mp4

📝 Review Note

FSCalendar 자체가 Third party이다 보니 사용하는데 조금의 이해가 필요한게 사실입니다. 내장되어 있는 함수들에 대해선 이해가 조금 필요하기 때문에 단번에 코드를 보고 이해하기는 좀 어려울 수 있습니다. 최대한 이해하기 편하게 만들려고 노력했습니다만.. 더 예쁘게 못만들어서 아쉽네요ㅠㅠ
그래도 더 예쁘게 만들수 있다는 것 자체가 작년에 비해 많이 성장한 것 같아 뿌듯하네요!

📣 Related Issue

close [REFACTOR] CalendarView를 리팩토링합니다. #604

📬 Reference

…/MC2-Team5-Firefighter into feature/604-calendar-refactor

YoonAh-dev · 2024-02-06T12:57:20Z

Manito/Manito/Screens/Detail-Wait/DetailEditViewController.swift

@@ -161,12 +156,18 @@ final class DetailEditViewController: UIViewController {
            .store(in: &self.cancellable)
    }

-    private func bindCancelButton() {
+    private func bindButtons() {


p5;

나중에 새로운 UI binding 부분이 추가될 수도 있으니 bindUI로 네이밍하는건 어떨까요?

1497c6c
반영했습니다!

YoonAh-dev · 2024-02-07T11:23:25Z

Manito/Manito/Screens/Detail-Wait/UIComponent/CalendarView.swift

    var startDateText: String = ""
    var endDateText: String = ""
-    private var tempStartDateText: String = ""
-    private var tempEndDateText: String = ""
    var isFirstTap: Bool = false


p3;
startDateText, endDateText는 CalendarView 내부에 Text를 받아와서 세팅하는 메서드를 따로 만들면 어떨까요?
변수가 외부에 노출되는게 좋지 않을 것 같습니다.

isFirstTap은 메서드로 만들어서 사용하는 게 어떨까요?

dd6c61b
접근제어를 private로 만들고 외부에서 접근할 수 있는 함수를 만들었습니다.

YoonAh-dev · 2024-02-07T11:33:38Z

Manito/Manito/Screens/Detail-Wait/UIComponent/CalendarView.swift

-        self.delegate?.detectChangeButton(hasDate)
-        // FIXME: - delegate로 통일 후 삭제해야함
-        self.changeButtonState?(hasDate)
+    private func setupButtonState() {


p3;

setupButtonState 메서드가 Text 설정에 따라서 여러번 호출되는거 같더라구요.
setup 보다는 update 라는 네이밍이 더 좋지 않을까 싶습니다.

++)
startDateText와 endDateText가 Publisher를 통해서 외부로 전달된다면 buttonStatePublisher를 만들지 않아도 나머지 두 Publisher를 통해서 button State를 설정할 수 있지 않을까요?

따로 클래스 내부 변수로 매번 startDate, endDate Text를 저장하지 않고 선택 시에 바로 Publisher를 통해서 텍스트 이벤트를 던져서 사용할 순 없을까요? 클래스 내부에서 Text를 매번 저장하는 로직이 많아서 Text를 건드는 부분들이 많은데 ⓵ 휴먼 에러를 발생시킬 확률이 높아보이고 ⓶ 불필요한 저장이 많아 보입니다.

p1;

114번 라인과 115번 라인이 줄 바꿈 없이 붙어 있습니다. 확인 부탁드려요!

5dc58de
button을 관리하던 publisher를 삭제했습니다. 114번 115번 간격도 띄워놨습니다.
하지만 startDateText와 endDateText를 미처 삭제하지는 못했습니다. 저도 듀나의 의견에 동의해서 삭제를 하려고 하니 날짜를 수정해야 하는 상황에서는 시작날짜와 끝 날짜가 지정되어 있는 상태여야 하는데 그 초기화를 어떻게 처리해야 할지 잘 모르겠습니다.
추가적으로 PassthroughSubject -> CurrentValueSubject 로 변경하려 했을 때, 다른 부분의 코드에도 영향을 미치기 때문에 수정하지 못했습니다.

@MMMIIIN

날짜를 수정해야 하는 상황에서는 시작날짜와 끝 날짜가 지정되어 있는 상태여야 하는데 그 초기화를 어떻게 처리

시작날짜와 끝 날짜를 현재는 어떤 방식으로 가져오고 있나요? 수정하는 화면에서 startDateText와 endDateText를 설정해주는 방식일까요?

네 DetailEditViewController에서 방 정보를 불러오고 해당 View를 생성하면서 설정해주고 있습니다.

YoonAh-dev · 2024-02-07T11:37:09Z

Manito/Manito/Screens/Detail-Wait/UIComponent/CalendarView.swift

    func setupDateRange() {
-        self.startDateTapPublisher.send("20\(self.startDateText)")
-        self.endDateTapPublisher.send("20\(self.endDateText)")
-        guard let startDate = self.startDateText.toDefaultDate else { return }
-        guard let endDate = self.endDateText.toDefaultDate else { return }
+        self.startDateTapPublisher.send(self.startDateText)
+        self.endDateTapPublisher.send(self.endDateText)
+        guard let startDate = self.startDateText.toDefaultDate,
+              let endDate = self.endDateText.toDefaultDate else { return }
        self.setupCalendarRange(startDate: startDate, endDate: endDate)
    }


p3;

65번 라인에 코멘트 해 둔 부분을 여기서 처리할 수 있을 거 같은데요!?

p3;
startDateText, endDateText는 CalendarView 내부에 Text를 받아와서 세팅하는 메서드를 따로 만들면 어떨까요?

해당 메서드에서 매개변수로 값을 받으면 굳이 변수를 internal로 설정하지 않아도 될 듯 합니다.

dd6c61b
윗 부분에서 private하게 수정했습니다 ^_______^

YoonAh-dev · 2024-02-07T11:42:11Z

Manito/Manito/Screens/Detail-Wait/UIComponent/CalendarView.swift

+        self.startDateTapPublisher.send(self.startDateText)
+        self.endDateTapPublisher.send(self.endDateText)
+        guard let startDate = self.startDateText.toDefaultDate,
+              let endDate = self.endDateText.toDefaultDate else { return }
        self.setupCalendarRange(startDate: startDate, endDate: endDate)


p5;

setupCalendarRange 네이밍을 setupInitialCalendarRange 로 하는게 어떨까요?
범위 설정하는 메서드인데 초기에만 사용하는 거 같아서요.

수정했습니다!
34a46d2

YoonAh-dev · 2024-02-07T12:03:09Z

Manito/Manito/Screens/Detail-Wait/UIComponent/CalendarView.swift

@@ -180,35 +165,41 @@ final class CalendarView: UIView {
        }
    }

-    func setSelecteDate(startIndex: Int, endIndex: Int) {
+    private func setSelecteDate(startIndex: Int, endIndex: Int) {


p1;

SelectedDate인데 오타가 난 거 같아요.

어머... 감사합니다... (머쓱)
25ad87d

YoonAh-dev · 2024-02-07T12:12:17Z

Manito/Manito/Screens/Detail-Wait/UIComponent/CalendarView.swift

            } else {
-                self.tempEndDateText = date.toDefaultString
+                self.endDateText = date.toDefaultString
                self.setDateRange()
                calendar.reloadData()
            }
        } else if isReclickedStartDate {
-            self.tempStartDateText = date.toDefaultString
-            self.tempEndDateText = ""
+            self.startDateText = date.toDefaultString
+            self.endDateText = ""
            (calendar.selectedDates).forEach {
                calendar.deselect($0)
            }


p3;

해당 메서드 내부에 있는 각각의 로직을 메서드로 따로 뺄 순 없을까요?

YoonAh-dev · 2024-02-07T12:14:28Z

Manito/Manito/Screens/Detail-Wait/UIComponent/CalendarView.swift

+        self.endDateText = ""
        self.isFirstTap = true
        (calendar.selectedDates).forEach {
            calendar.deselect($0)
        }
        self.selectStartDate = date
        calendar.select(date)
        calendar.reloadData()
+        self.endDateTapPublisher.send(self.getEndDate())
        self.setupButtonState()


혹시 didDeselect 메서드 로직 설명해주실 수 있나요?
로직 이해가 잘 안되어서요...👀👀

ㅋㅋㅋㅋㅋㅋㅋ 이거 저도 다시 보니까 이해가 잘 안가더라구요...
코드를 살짝 수정했습니다.

/// 이미 선택 되어있는 날짜를 클릭했을 때 실행되는 함수. func calendar(_ calendar: FSCalendar, didDeselect date: Date, at monthPosition: FSCalendarMonthPosition) { self.endDateText = "" self.endDateTapPublisher.send("") `self.isFirstTap = true` (calendar.selectedDates).forEach { calendar.deselect($0) } self.selectStartDate = date calendar.select(date) calendar.reloadData() }
설명을 드리자면 위 사진처럼 날짜들이 선택되어있는 상황에 해당 날짜들을 눌렀을 때 실행되는 함수 입니다. 사진을 예로 19, 20, 21, 22 ,23일을 눌렀을 때 실행되는 함수입니다. `self.endDateText = ""` 저 상태에서 눌렀을때는 시작날짜를 지정한 것이기 때문에 종료날짜 endDateText를 비워줍니다. self.endDateText = "" 그리고 isFirstTap << 첫 번째 탭을 했다는 의미의 변수에 true 값을 줍니다.(isFirstTap 변수의 목적이 뭔지는 까먹었습니다. 당시에 첫 탭에 버그가 있어서 필요해서 넣었던 것 같은데 잘 모르겠네요.. 좀 더 보겠습니다.)
(calendar.selectedDates).forEach { calendar.deselect($0) }

선택되어있는 날짜리스트를 비워줍니다.

self.selectStartDate = date
selectStartDate변수에 선택한 날짜의 값을 넣어줍니다.(얘도 왜 필요했는지는 기억이 잘 안나네요...)
calendar.select(date)
실제 캘린더 select함수에 선택한 날짜를 넣어줍니다.
calendar.reloadData()
캘린더를 리로드 합니다.

@MMMIIIN

자세하게 설명해주셔서 감사합니다. 🙇‍♂️
작은 바람이 있다면 조금 더 코드가 읽기 쉬우면 좋을 것 같네유(코드 이상하다는 말 아님)👍🏻

YoonAh-dev · 2024-02-07T12:16:32Z

Manito/Manito/Screens/Detail-Wait/UIComponent/CalendarView.swift

        self.setupButtonState()
    }

    func calendar(_ calendar: FSCalendar, shouldSelect date: Date, at monthPosition: FSCalendarMonthPosition) -> Bool {
-        if date < Date() - self.oneDayInterval {
+        if date < Date() - .oneDayInterval {
            self.viewController?.makeAlert(title: TextLiteral.Common.Calendar.pastAlertTitle.localized(),


p1;

해당 로직에 사용중인 viewController를 삭제할 예정이라서 다른 방식으로 구현해주시면 감사드리겠습니다..🙇‍♂️

++) showAlertOverDateSelect 메서드랑 title, message 텍스트 차이 밖에 없어서 하나의 메서드로 묶으면 좋을 것 같네요.

YoonAh-dev · 2024-02-07T12:19:48Z

Manito/Manito/Screens/Detail-Wait/UIComponent/CalendarView.swift

-        self.changeButtonState?(hasDate)
+    private func setupButtonState() {
+        let hasDate = !self.startDateText.isEmpty && !self.endDateText.isEmpty
+        self.buttonStatePublisher.send(hasDate)
    }

    private func setupDelegation() {


p1;

하단에 보니깐 dataSource에서 제공해주는 API를 사용하지 않는데, 굳이 dataSource = self를 할 필요가 있나요..?
꼭 써줘야 하는 부분일까요?

그러네용 없어도 될거같네요!

YoonAh-dev

해당 브랜치에서 시뮬레이터로 확인해봤는데, 잘 됩니다. 👍🏻

[ Combine 스트림 활용 ]
Combine에서 제공하는 스트림을 잘 사용해서 시작 날짜와 끝 날짜를 따로 변수로 저장하지 않고도 사용할 수 있는 방식을 생각해보면 좋을 것 같습니다. 현재 클래스 내부에서 해당 변수를 여러번 세팅하는데 잘못된 값이 들어갈 수도 있을 것 같고, 무엇보다도 실수로 문제가 생길 가능성이 커보입니다.

[ 공통 로직 분리 ]
현재 있는 캘린더 뷰 내부에 있는 로직을 조금 더 깔끔하게 손 볼 수 있으면 좋겠습니다.
여러 화면에서 캘린더를 사용하다보니 매 화면마다 설정되는 부분이 다르게 되는 경우가 있는 것 같습니다.

방 생성 시에는 첫 날짜와 마지막 날짜를 설정하지 않아도 되지만 날짜 변경 화면에서는 이미 설정된 날짜로 설정해야 한다 등..

이런 경우에 내부에서 진행하는 초기 세팅 로직을 internal하게 설정해서 사용하는 부분에서 날짜 세팅을 할 수 있게 하는 식으로 메서드를 변경할 수 있을 것 같습니다. 캘린더 뷰는 다양한 뷰에서 사용하는 만큼 여러 상황을 고려해서 분기가 생기는 부분들을 어떻게 수정할 수 있을까에 초점을 맞춰서 수정하면 좋을 것 같습니다.

MMMIIIN added 12 commits November 21, 2023 22:48

[ADD] TimeInterval Extension 추가 (#604)

f72b3df

[CHORE] timeInterval 수정 (#604)

81fa5cb

[CHORE] 불필요한 코드 삭제 (#604)

052eaef

Merge branch 'develop' of https://github.com/DeveloperAcademy-POSTECH…

33b43a9

…/MC2-Team5-Firefighter into feature/604-calendar-refactor

[CHORE] private 접근 제어 추가 (#604)

d2e0356

[CHORE] alert 함수 분리 (#604)

af3f5af

[CHORE] TimeInterval 변수 적용 (#604)

611e3e4

[CHORE] 가독성을 위한 수정, 타입 명시 (#604)

af5904b

[CHORE] temp 변수 삭제 (#604)

51ad98e

[CHORE] timeInterval 변수 적용 (#604)

c0723eb

[CHORE] timeInterval 변수 적용 (#604)

dd3eab6

[CHORE] 날짜 하나만 선택했을 때 버튼 활성화 되는 문제 해결 (#604)

f896045

MMMIIIN added 🔨 Refactor 코드 리팩토링 👾 케미 labels Dec 7, 2023

MMMIIIN self-assigned this Dec 7, 2023

auto-assign bot requested review from coby5502, dangsal and YoonAh-dev December 7, 2023 09:27

[CHORE] 안쓰는 함수 삭제 (#604)

4d3933c

YoonAh-dev added this to In progress in 명예소방관 Project via automation Feb 5, 2024

YoonAh-dev moved this from In progress to Review in progress in 명예소방관 Project Feb 5, 2024

YoonAh-dev reviewed Feb 6, 2024

View reviewed changes

YoonAh-dev reviewed Feb 7, 2024

View reviewed changes

YoonAh-dev requested changes Feb 7, 2024

View reviewed changes

MMMIIIN added 5 commits February 18, 2024 21:00

[CHORE] 함수명 변경 (#604)

1497c6c

[CHORE] 변수 은닉화 (#604)

dd6c61b

[CHORE] 버튼에 대한 publisher 삭제 (#604)

5dc58de

[CHORE] calendar range 함수명 변경 (#604)

34a46d2

[CHORE] 오타 수정 (#604)

25ad87d

MMMIIIN modified the milestone: Sprint 1 Mar 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CHORE] Calendar View의 가독성을 향상시키기 위해 노력했습니다. #607

[CHORE] Calendar View의 가독성을 향상시키기 위해 노력했습니다. #607

MMMIIIN commented Dec 7, 2023

YoonAh-dev Feb 6, 2024

MMMIIIN Feb 18, 2024

YoonAh-dev Feb 7, 2024

MMMIIIN Feb 18, 2024

YoonAh-dev Feb 7, 2024

YoonAh-dev Feb 7, 2024

MMMIIIN Feb 18, 2024 •

edited

Loading

YoonAh-dev Feb 27, 2024

MMMIIIN Mar 4, 2024

YoonAh-dev Feb 7, 2024

MMMIIIN Mar 4, 2024

YoonAh-dev Feb 7, 2024

MMMIIIN Mar 4, 2024

YoonAh-dev Feb 7, 2024

MMMIIIN Mar 4, 2024

YoonAh-dev Feb 7, 2024

YoonAh-dev Feb 7, 2024

MMMIIIN Feb 18, 2024

YoonAh-dev Feb 27, 2024

YoonAh-dev Feb 7, 2024

YoonAh-dev Feb 7, 2024

YoonAh-dev Feb 7, 2024

dangsal Feb 19, 2024

YoonAh-dev left a comment •

edited

Loading

[CHORE] Calendar View의 가독성을 향상시키기 위해 노력했습니다. #607

Are you sure you want to change the base?

[CHORE] Calendar View의 가독성을 향상시키기 위해 노력했습니다. #607

Conversation

MMMIIIN commented Dec 7, 2023

🌁 Background

👩‍💻 Contents

✅ Testing

📱 Screenshot

📝 Review Note

📣 Related Issue

📬 Reference

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MMMIIIN Feb 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YoonAh-dev left a comment • edited Loading

Choose a reason for hiding this comment

MMMIIIN Feb 18, 2024 •

edited

Loading

YoonAh-dev left a comment •

edited

Loading