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

부산대Android_이창욱_1주차_과제 #5

Open
wants to merge 33 commits into
base: ichanguk
Choose a base branch
from

Conversation

ichanguk
Copy link

@ichanguk ichanguk commented Jun 27, 2024

1단계 커밋 모음 링크

코드 작성하면서 어려웠던 점

  1. 생일 입력 폼을 구현할 때 캘린더에서 날짜를 선택해 등록하는 것을 처음 해봐서 좀 어려웠던 것 같습니다.

  2. Intent로 data class 객체를 전달하는 방법을 잘 몰라서 구글링하면서 구현하였습니다.

  3. 코딩을 시작하기 전에 디자인, 기능 요구 사항들을 완벽히 이해하고 시작해야 코드를 다시 짜는 일이 줄어들 것 같습니다.

중점적으로 리뷰해주셨으면 하는 부분

  1. 커밋 단위가 적절한지 궁금합니다.

  2. data class를 Intent로 전달하는 과정이 효율적으로 잘 구현이 됐는지 봐주시면 좋을 것 같습니다.

  3. 함수들이 적절하게 분리되어 사용되고 있는지 봐주시면 좋을 것 같습니다.

결과 화면

result1

result2

result3

ichanguk added 30 commits June 24, 2024 15:11
- repository 설명 추가
- 구현할 feature list 추가
이름 EditText 또는 전화번호 EditText가 비어있을 때 토스트 메시지 출력 기능 추가
전화번호 입력을 숫자로 제한하도록 함
이름과 전화번호 입력 외에 나머지 EditText UI 추가
- 더보기를 위한 레이아웃 추가
- 더보기를 누를 때 나오는 EditText 뷰들은 visibility의 기본값을 gone으로 설정
더보기를 누르면 나머지 입력폼들이 보여지는 기능 추가
gender text와 gender를 선택하는 radiogroup을 하나의 layout으로 묶음
저장 버튼을 누르면 저장 완료 토스트 메시지 출력
취소 버튼을 누르면 취소 토스트 메시지 출력
성별 라디오 버튼을 선택하면 gender 변수에 성별이 저장되도록 함
기능 리스트에 생일 입력 기능 추가
생일 입력 폼을 클릭하면 캘린더에서 생일을 고를 수 있도록 함
취소, 저장 버튼을 제외한 나머지 레이아웃들을 ScrollView 안에 넣어 스크롤할 수 있도록 함
2단계를 위한 기능 리스트 추가
연락처 등록 화면 UI 디자인
+ 버튼을 누르면 연락처 작성 액티비티로 넘어가도록 구현
- 연락처 목록을 표시하기 위한 리사이클러뷰 추가
- 리사이클러뷰를 위한 아이템 UI 추가
기능리스트의 3번 내용(목록 스크롤)을 2번 내용(목록에 추가)에 추가
연락처를 저장하면 목록에 추가되도록 구현
화면 전환 시 연락처 목록이 초기화되지 않도록 함수 추가
연락처 목록의 연락처를 클릭하면 상세화면이 보여지도록 함
뒤로가기 외에 취소 버튼을 눌렀을 때도 확인 팝업이 나오도록 기능 변경
뒤로가기/ 취소 버튼 클릭 시 확인 팝업이 나오도록 구현
./gradlew ktlintCheck와 ./gradlew ktlintFormat를 사용해 리팩토링
contents 이미지 추가
@ichanguk ichanguk changed the base branch from main to ichanguk June 28, 2024 08:23
Copy link

@bigstark bigstark left a comment

Choose a reason for hiding this comment

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

안녕하세요 창욱님! 앞으로 함께할 강대규 멘토입니다.
앞으로 6주동안 잘 부탁드려요 :)


커밋 단위가 적절한지 궁금합니다.

커밋단위 매우 적절하게 잘 작성해주셨어요. 걱정하시는 것보다 더욱 잘 작성해주셨어요.

data class를 Intent로 전달하는 과정이 효율적으로 잘 구현이 됐는지 봐주시면 좋을 것 같습니다.

type casting 도 잘 신경써주셨고, parcelable 로 정의한 부분도 매우 인상깊었습니다. 매우 좋습니다 👍

함수들이 적절하게 분리되어 사용되고 있는지 봐주시면 좋을 것 같습니다.

평소에 습관이신지는 모르겠지만, 매우 잘하고 계시네요! 더할나위 없이 깔끔한데요!?

@@ -1,6 +1,8 @@
plugins {
id("com.android.application")
id("org.jetbrains.kotlin.android")
id("kotlin-parcelize")
id("org.jlleitschuh.gradle.ktlint")
Copy link

Choose a reason for hiding this comment

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

오호... ktlint 설정까지 해주실 예정인걸까요?

Comment on lines 6 to 14
@Parcelize
data class Contact(
val name: String,
val phoneNum: String,
val email: String,
val birthday: String,
val gender: Int,
val memo: String,
) : Parcelable
Copy link

Choose a reason for hiding this comment

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

Parcelize 아주 좋습니다! 👍

Comment on lines +44 to +56
private fun initViews() {
detailNameTextView = findViewById(R.id.detail_name_text_view)
detailPhoneNumTextView = findViewById(R.id.detail_phone_num_text_view)
detailEmailTextView = findViewById(R.id.detail_email_text_view)
detailBirthdayTextView = findViewById(R.id.detail_birthday_text_view)
detailGenderTextView = findViewById(R.id.detail_gender_text_view)
detailMemoTextView = findViewById(R.id.detail_memo_text_view)

detailEmailLayout = findViewById(R.id.detail_email_layout)
detailBirthdayLayout = findViewById(R.id.detail_birthday_layout)
detailGenderLayout = findViewById(R.id.detail_gender_layout)
detailMemoLayout = findViewById(R.id.detail_memo_layout)
}
Copy link

Choose a reason for hiding this comment

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

기본적으로 주석과 함수가 적절히 쪼개져 있는 것이 매우 보기 좋습니다.
다만 요러한 initViews 작업은 ViewBinding 을 사용한다면 코드 작성이 따로 필요 없이 간편하게 처리될 수 있을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

클론코딩 강사님께서 일단 findViewById를 사용하라고 하셔서 사용중인데 ViewBinding도 사용해보겠습니다!

Comment on lines +62 to +67
val contact: Contact? =
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
intent.getParcelableExtra("contact", Contact::class.java)
} else {
intent.getParcelableExtra("contact")
}
Copy link

Choose a reason for hiding this comment

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

nullable, 버전 처리 매우 좋습니다!

Comment on lines 11 to 20
lateinit var detailNameTextView: TextView
lateinit var detailPhoneNumTextView: TextView
lateinit var detailEmailTextView: TextView
lateinit var detailBirthdayTextView: TextView
lateinit var detailGenderTextView: TextView
lateinit var detailMemoTextView: TextView
lateinit var detailEmailLayout: ConstraintLayout
lateinit var detailBirthdayLayout: ConstraintLayout
lateinit var detailGenderLayout: ConstraintLayout
lateinit var detailMemoLayout: ConstraintLayout
Copy link

Choose a reason for hiding this comment

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

접근제한자를 습관적으로 선언하시는 것을 추천드립니다 :)

class ContactRecyclerViewAdapter(
var contactList: ArrayList<Contact>,
var inflater: LayoutInflater,
val context: Context,
Copy link

Choose a reason for hiding this comment

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

activity 를 변수로 넘기는 것은 금기시해야할 것 중 하나입니다. inflater 또한 파라미터로 받을 필요는 없을 것 같구요.
아이템을 클릭했을 때 이동해야한다면, 아이템이 클릭되었음을 알려주는 listener 를 정의하고 그것을 파라미터로 받는 것이 좋습니다.

interface OnContactClickListener {
    fun onContactClicked(position: Int)
}

var inflater: LayoutInflater,
val context: Context,
) : RecyclerView.Adapter<ContactRecyclerViewAdapter.ViewHolder>() {
inner class ViewHolder(itemView: View) : RecyclerView.ViewHolder(itemView) {
Copy link

Choose a reason for hiding this comment

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

inner class 일 필요가 있을까요?

Comment on lines 111 to 112
val lastNameTextView: TextView
val nameTextView: TextView
Copy link

Choose a reason for hiding this comment

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

외부에서 접근가능하도록 하는 것보다는 내부에서 set text 를 해줄 수 있는 메소드를 생성하심을 추천드립니다.

@Override
override fun onSaveInstanceState(outState: Bundle) {
super.onSaveInstanceState(outState)
outState.putParcelableArrayList("contact_list", ArrayList(contactList))
Copy link

Choose a reason for hiding this comment

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

이러한 key 들은 const 로 따로 관리되는 편이 좋습니다.
해당 RegisterActivity 의 companion object 에 정의되어도 괜찮을 것 같구요.
이러한 형태로 수동으로 넣게된다면, 휴먼에러 덕분에 오류가날 가능성이 높습니다.

class RegisterActivity: AppCompatActivity() {
    ...
    companion object {
        const val KEY_CONTACT_LIST = "contact_list"
    }
}

ichanguk and others added 2 commits July 2, 2024 10:29
if 문에서 isVisible을 사용하는 방법으로 리팩토링

Co-authored-by: Victhor <[email protected]>
- Gender enum class 사용
- 가독성 및 입력 폼 로직 개선
- 접근 제한자 사용
- onBackPressedCallback 사용
- 리사이클러뷰 구조 개선
- key 관리 객체 추가
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants