Feature/jaino/#93

[jeongjaino]

1. 📄 관련된 이슈 및 소개

#93 설문 엔티티 수정 및 설문 형식 수정 기능 구현

2. 🔥변경된 점

1. Survey, SurveyForm 엔티티에 SurveyFormId 추가

• 기존 eventId로 설문 및 응답에 접근하던 방식을 surveyFormId로 변경

3. SurveyForm 수정 기능 추가

4. 설문 형식으로 네비게이션 구현

파일 리네이밍

• registerSurveyUseCase -> postSurveyFormUseCase

• surveyRegstrationConent -> SurveyFormContent

  • Edit에서도 사용해서 위와 같이 수정

• SurveyRegistrationScreen -> SurveyFormRegistrationScreen

3. 📸 스크린샷(선택)

4. 💡알게된 혹은 궁금한 사항들

weight는 상대적인 View 간의 가중치로도 사용이 가능하지만 ,

레이아웃에서 단일로 weight를 설정하면, constraintLayout의 0dp와 같이 부모 레이아웃의 나머지 부분의 크기로 설정할 수 있다.

weight을 알기 전까지 customLayout으로 구현할 수는 없을까 고민하여, 태규상의 커스텀 레이아웃을 정독했다 캬캬

[tgyuuAn]

고생하셨습니다!

이번 PR도 마찬가지로 간단한 코멘트 남겨봤어요!

[tgyuuAn]

별 건아닌데,

파이어베이스 사용할 때 default value주는 거랑 부 생성자로 선언하는 거랑 차이점이 있나요 ?

[jeongjaino]

정확히 내부적으로는 어떻게 차이점이 있는지는 잘 모르겠습니다 ㅠ.ㅠ

제가 Default가 아닌 따로 생성자를 사용하는 이유는

단순히 디폴트보다 부 생성자가 코드상 깔끔해보여서, 매번 이렇게 작성하고 있습니다 .. !

코드 량으로 봤을 떄는 디폴트가 더 적게 작성되긴 하겠네용 하하

[jeongjaino]

그래도 아마 코틀린에서 부 생성자가 디폴트 값의 역할도 하기 때문에,

내부적으로는 아마 같게 동작하지 않을까요 ?

[tgyuuAn]

파라미터를 2개로 받는 이유가 있을까요? 확장성의 이유 때문인가요 ?.?

[jeongjaino]

ViewModel에서 로직을 변경해서, 그에 따라 파라미터를 2개로 전달받도록 구현했습니다 !

기존에는 아래와 같이 ViewModel에서
setSurveyRegistrationState (다음 상태로 전환하는 함수) 내에서 Validation을 진행했습니다.

 fun setSurveyRegistrationState(surveyRegistrationState: SurveyRegistrationState) {
        when (surveyRegistrationState) {
            SurveyRegistrationState.EVENT_SELECTION -> { /* initial value */
            }

            SurveyRegistrationState.INFORMATION -> {
                if (isNotValidEventSelection()) {
                    emitValidationErrorMessage("행사를 선택해 주세요.")
                    return
                }
            }

            SurveyRegistrationState.QUESTION -> {
                if (isNotValidInformation()) {
                    emitValidationErrorMessage("제목과 내용을 확인해 주세요.")
                    return
                }
            }

            SurveyRegistrationState.DEADLINE -> {
                if (isValidSurveyQuestion()) {
                    addSurveyQuestion() // 현재 작성한 질문, 질문 리스트에 삽입
                } else {
                    emitValidationErrorMessage("질문 내용을 확인해 주세요.")
                    return
                }
            }
        }
        _currentRegistrationState.value = surveyRegistrationState
    }

해당 함수가 다음과 같은 문제가 있다고 판단해서 변환했어요 !

1. 함수가 가독성이 좋지 않음

• 상태에 따라 다른 로직을 수행해, 예측이 힘듦

2. 함수 네이밍이 setSurveyRegistrationState인데 valid한 상태에서만 다음 상태가 설정됨
3. 다른 함수에서 Validation이 필요한 경우, 또 다시 Validation 코드를 작성해야 함.

그래서 모든 Validation 코드를 함수 밖으로 빼서, 아래와 같이 따로 함수로 만들었어요 !

fun validateSurveyForm(currentState: SurveyFormState): Boolean {
        when (currentState) {
            SurveyFormState.EVENT_SELECTION -> {
                if (isNotValidEventSelection()) {
                    emitValidationErrorMessage("행사를 선택해 주세요.")
                    return false
                }
            }

            SurveyFormState.INFORMATION -> {
                if (isNotValidInformation()) {
                    emitValidationErrorMessage("제목과 내용을 확인해 주세요.")
                    return false
                }
            }

            SurveyFormState.QUESTION -> {
                if (isNotValidSurveyQuestion()) {
                    emitValidationErrorMessage("질문 내용을 확인해 주세요.")
                    return false
                }
            }

            SurveyFormState.DEADLINE -> {
                if (isNotValidDeadline()) {
                    emitValidationErrorMessage("최소 하루 이상 설문 날짜를 지정하세요.")
                    return false
                }
            }
        }

        return true
    }

    fun setSurveyFormState(nextState: SurveyFormState) {
        _currentSurveyFormState.value = nextState
    }

이렇게 ViewModel 내에서 로직을 구성해서,
View 단위에서는 두 개의 상태를 전달해야 해요. (현재 상태, 다음 상태)
현재 상태 -> validation() 함수의 인자로 전달
다음 상태 -> setNextState() 함수의 인자로 전달

태규님께서 말씀하신 확장성의 이유도 있어요 !
사실 현재 상태만 보고 다음 상태가 예측이 가능하니까요. (현재 상태가 질문이면, 다음 상태는 당연히 마감기한이겠죠 !? 하하)

하지만, 제가 작성한 #95 이슈를 살펴보시면,
이전 페이지 설문 내용도 작업을 한다고 하면, 현재 상태에 따라 다음 상태가 예측이 안돼요
(다음 페이지가 아닌 이전 페이지로 돌아갈 수 있으니까요 !)

따라서 다음과 같이 두 개의 상태를 받도록 구현했습니다 !

[tgyuuAn]

 fun setEventStartDate(eventDate: LocalDate) {
        if (!isValidStartDate(eventDate)) {
            emitValidationErrorMessage("최소 하루 이상 일정 날짜를 지정하세요.")
            return
        }
        _eventStartDate.value = eventDate
    }

    fun setEventStartTime(eventTime: LocalTime) {
        _eventStartTime.value = eventTime
    }

    fun setEventEndDate(eventDate: LocalDate) {
        if (!isValidEndDate(eventDate)) {
            emitValidationErrorMessage("종료 날짜는 시작 날짜와 같거나 더 늦어야 합니다.")
            return
        }
        _eventEndDate.value = eventDate
    }

    fun setEventEndTime(eventTime: LocalTime) {
        if (!isValidEndTime(eventTime)) {
            emitValidationErrorMessage("종료 날짜는 시작 날짜와 같거나 더 늦어야 합니다.")
            return
        }
        _eventEndTime.value = eventTime
    }

    fun setEventRegistrationState() {
        if (_currentRegistrationState.value == EVENT_DETAILS) {
            if (!isValidTitle()) {
                emitValidationErrorMessage("행사 이름을 입력하세요.")
                return
            }
            if (!isValidContent()) {
                emitValidationErrorMessage("행사 내용을 입력하세요.")
                return
            }
            _currentRegistrationState.value = EVENT_SCHEDULE
        }
    }

제가 짠 일정 등록 로직에서는 아예 stateFlow에 set할 때 유효성 검증을 하는데요,

하나로 통일 시키는 것이 보기가 좋을 것 같아서 어떤 것으로 통일하는 것이 좋을까요 ?.?

[jeongjaino]

태규님이 제안해주신 방법도 정말 좋은 방법인 것 같아요 !

완료 버튼을 누르지 않고도, 잘못되었는지 확인할 수 있으니까요 !

하지만, 컴포넌트가 Text Field의 경우에는 값이 변경될 때마다,
setText를 호출해 계속 검증을 하고, 그에 따라 snackBar로 에러를 계속 뱉는 상황이 발생할 것 같아요.

그점을 태규님도 숙지하셔서, 따로 텍스트 필드인 경우는 State 변경에 검증을 하도록 구현하신 것 같고요 !

또한, 함수 내부에서 검증 로직이 있는 걸 외부에서는 알기 어려운 점도 있고요 ..

저는 오히려 설문과 일정 로직을 통일 안하는 게 나을 것 같아요.
개발자도 다르고, 서로의 상황과 도메인이 다 다르니까요 ,,, !

설문 도메인은 상대적으로 TextField 컴포넌트가 많아서,
set 과정에 validation을 포함시킨다면, event 선택과 데드라인 설정에만 포함되고 나머지는 이전 로직과 동일할꺼 같아요.

만약 TextField의 실시간 검증 로직을 만든다고 하면,
기존 snackbar가 아니라 TextField의 error를 활용하면 더 좋을 것 같아요 !

[tgyuuAn]

저는 오히려 설문과 일정 로직을 통일 안하는 게 나을 것 같아요.

개발자도 다르고, 서로의 상황과 도메인이 다 다르니까요 ,,, !

도메인이 다르다에는 동의를 하지만, 개발자도 다르고 ,서로의 상황이 다르다라는 점에서는 약간은 의견이 다른 것 같아요...!

조금 키워드가 쎘나요...? 그렇다면 죄송합니다..

이 부분도, 변수 명 혹은 들여쓰기 규칙과 같이 컨벤션의 일종에 들어갈 수 있을 것 같아요.

여러 개의 Major한 프로젝트를 봤을 때 클린 코드에서 강조하는 것 처럼

여러 명에서 작업했을 때에도 한사람이 짠 것 처럼 팀에서는 규칙을 맞춰 작업 해야 한다. 라는 것을 잘 지키고 있음을 볼 수 있어요.

https://monsterb.tistory.com/entry/Clean-Code-클린-코드

저희는 분명 대기업 프로젝트도 아니고 Major한 프로젝트도 아니라서 이런 디테일한 부분에 신경쓰는 것은 MVP 배포를 지연시키게만 할 뿐이지만,

추후에, 충분한 시간이 주어졌을 때에는 리팩토링 할 여지가 있다고 생각이 들어요.

(지금 당장 할 필요없다는 소리와 동치입니다.)

---

설문 엔티티는 상대적으로 TextField 컴포넌트가 많아서,

set 과정에 validation을 포함시킨다면,

event 선택과 데드라인 설정에만 포함되고 나머지는 이전 로직과 동일할꺼 같아요.

확실히 설문 등록은 TextField 부분이 많아서 set과정에 포함 시킬 수 없는 점이 존재하네요.

이런 부분에서는 위에 제가 말한 것 처럼 일관성 있게 코드를 짤 수는 없지만,

위와 같은 인사이트도 존재함을 알아주셨으면 해요...!

진호님 말도 위에 구문으로 인해 저를 설득시키는데 충분했어요.

[jeongjaino]

태규님 너무 재밌습니다 캬캬

태규님 덕분에 코드리뷰 하면서 개발의 새로운 맛(?)을 알아가는 느낌이에요

코드를 작성하면서도 개발을 알아갈 수 있지만, 내가 작성한 코드를 이렇게 자연어로 설명하는 것 자체도
엄청 저를 성장시키고 있다는 생각이 드네요

오이시 데스 !!

[tgyuuAn]

매직 넘버나 스트링 제거할 때 항상 고민인데요,

""를 EMPTY라는 단어로 치환했을 때 얻는 이점이 있을까요?

항상 고민입니다.

심한 사람들은 개행 삽입하는 것도 함수화해서 하더라고요.

[jeongjaino]

가독성 때문에 사용했어요 !

해당 EMPTY는 단순히 StateFlow의 상태를 첫 상태를 초기화 하는 목적도 있지만,

아래의 함수에서도 사용돼요 !

    private fun clearSurveyQuestionState() { _surveyQuestion.value = EMPTY }

현재는 "" = EMPTY로 상수를 만들었지만,

추후 따로 의미있는 수를 상수화시키지 않고 그대로 사용한다면,
다른 팀원이나 추후 본인이 알아보기 힘들지 않을까요 히히

제가 상수화 시키는 기준은
보통 다 상수화 하려고 하는데, StateFlow나 다른 컬렉션의 첫 시작은 따로 상수를 안 만든 것 같아요 .

상수화 하면, 작성되는 코드의 양은 늘어나긴 하지만, 유지보수 측면이나 가독성의 이유로 좋지 않을까요 ?

[tgyuuAn]

@jeongjaino

매직넘버를 제거하는 것에는 저도 동의를 합니다.

하지만 "과연 String값을 변수로 빼는 것이 옳을까?" 라는 고민을 계속하곤 합니다.

""를 EMPTY라는 변수로 치환했을 때, 초기 값이 ""인지 혹은 또 다른 별도의 약속된 값인지 알 수 없으니깐요.

Int형이나 Float형과 같은 Number타입의 3.141592라는 값을 PI라는 변수로 상수화해서 사용하는 것은 확실히 이점만 있다고 생각합니다.

이러한 숫자들은 사람의 눈으로 읽어지지가 않으니깐요.

하지만 String타입을 굳이 치환해서 빼야할까?, 어차피 잘 읽히는데?

라는 생각이 한번 씩 들곤해요.

예시를 들자면,

fun startNewLine(){
    println()
}

혹은 joinToString()을 사용할 때, 사이사이 들어갈 문자를 아래와 같이 빼서 사용하는 등.

const val SEPERATOR= "-"

이와 관련해서 아래와 같은 글을 퍼왔씁니다!

[jeongjaino]

태규님 좋은 인사이트 감사합니다 !

제시한 의견을 들어보니, 단순히 EMPTY보다는 ""이 더 좋을 수도 있다라는 생각이 드네요 ..

이런 RawValue를 서로 컨벤션을 만들어서 지켜야할 것 같다라고 생각이들어요!

따로 0, ""과 같이 당연시 되는 값은 따로 상수화 하지말고,

(질문 리스트의 시작은 0이 아닌 1에서 부터 와 같이)
당연시 여기는 것과 다르게 생각되는 것, 의미를 가지는 값으로 하는건 어때요 ?

[tgyuuAn]

@jeongjaino

제가 생각하던 부분이랑 완전히 일치합니다!

이 부분 또한 컨벤션으로 정의하는 게 좋다고 생각해요.

너무 당연시 생각되는 부분보다는, 그렇게 독특한 아이들(?) 에 대해서만 추출하는 것이 좋다고 생각합니다!

[tgyuuAn]

위 코멘트와 비슷한데,

이 부분은 상수화 안시킨 이유가 있으신 지 궁금합니다!

[jeongjaino]

참조형 타입은 상수가 안되는 걸로 알고있어요 !

해당 EVENT_SELECTION_INIT은 위와 마찬가지로, StateFlow의 초기 상태값이기도 하지만,
아래의 이벤트 검증 로직에서도 사용하기 때문에 따로 변수로 만들었어요 !

private fun isNotValidEventSelection() =
        _surveyEventSelection.value.eventId == EVENT_SELECTION_INIT.eventId

해당 코드를 보니, 굳이 초기 상태값에 현재 시간을 넣을 필요가 없는 것 같아요 !
LocalDateTime의 상수 MIN, MAX로 변경해놓겠습니다잇 ~ !!

[tgyuuAn]

아아 제 코멘트는, "" 부분이 EMPTY 가 들어가지 않은 이유에 대한 의문이었습니다!

[jeongjaino]

아예 생각을 못한 부분이네요 ,, !

해당 값에 ""대신 EMPTY가 들어가면 오히려 EMPTY가 정말 ""이 맞는지 확인해볼꺼 같아요.

추후 수정해보도록 할게요 !