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주차 과제 #37

Open
wants to merge 24 commits into
base: jsh00325
Choose a base branch
from

Conversation

jsh00325
Copy link

6/26~6/28동안 학회 참석으로 인해, 매니저님께 양해를 구하고 조금 늦게 과제 제출했습니다...!


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

  • 처음에 ContactRegisterActivity(연락처 정보를 입력할 수 있는 액티비티)에서 ContactsListActivity(연락처 목록을 보여주는 액티비티)로 데이터를 전달할 때, Serializable한 데이터 클래스를 처음 만들어봐서 이를 찾아보면서 코드를 작성하는 것이 어려웠습니다.

  • RecyclerView에서 아무런 값이 들어있지 않은 경우에 텍스트를 보여주는 것을 어떻게 구현해야할지 고민하는 것이 어려웠습니다. 해당 앱에서는 데이터 삭제가 없어서 연락처가 입력될 때마다 visibility를 visible로 수정해서 구현했습니다.

    • 실제 앱과 같이 데이터의 삽입과 삭제가 빈번한 경우, 매번 데이터의 리스트가 비어있는 경우를 체크해서 빈 경우에만 텍스트를 보이도록 구현하면 되는지, 아니면 다른 효율적인 방법이 존재하는지 궁금합니다.

코드 리뷰 시, 멘토님이 중점적으로 리뷰해줬으면 하는 부분

  • 코드 상에서 비효율적으로 구현되었거나, 가독성을 위해서 어떻게 수정해야 할지와 같은 부분을 중점적으로 리뷰해 주셨으면 좋겠습니다!
  • 인텐트간 정보를 주고 받을 때, null 확인을 위해서 let 구문을 사용하였는데, 코드에서 저의 사용 방법이 올바른 방법인지 확인해 주셨으면 감사하겠습니다.

jsh00325 added 16 commits June 24, 2024 21:50
- 연락처의 정보를 입력할 수 있는 레이아웃 제작
- 더보기 버튼 클릭 시 추가정보를 입력하는 레이아웃이 보이도록 설정
- clearInputFields()를 통해 입력 값들 초기화
- 취소되었음을 Toast를 통해서 알림
- 연락처의 필수 요소를 확인하는 checkRequiredFields()함수 제작
- ContactData라는 Data Class로 연락처를 저장
- RecyclerView를 통해 연락처 item을 나열하는 레이아웃 제작
- 연락처가 없는 경우를 안내하는 TextView를 배치 후, 이를 코드 상에서 조작할 예정
- 메일, 생일과 같은 상세 정보는 gone 상태로 두고, 값이 있는 경우만 보이도록 구현할 예정
- 취소 버튼 클릭 시, 등록 액티비티가 종료되도록 구현
- 직렬화 가능한 구조로 Data 클래스를 변경
- 연락처 정보를 직렬화하여 Intent에 담아 결과 전송
- ContactsListActivity에서 역직렬화하여 정보를 가져와서 저장 후 notifyItemInserted를 통해 업데이트
- Item 클릭 시 해당 연락처를 직렬화하여 Intent에 담아 전송
- Intent를 통해 얻은 ContactData의 정보로, 레이아웃을 완성함
@jsh00325
Copy link
Author

step1 커밋의 변경사항 링크입니다! - 링크

@JSpiner JSpiner self-requested a review July 1, 2024 12:33
Copy link

@JSpiner JSpiner left a comment

Choose a reason for hiding this comment

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

안녕하세요! 정수현님과 코드리뷰를 계속 진행하게될 정성민 멘토라고 합니다 🙌
가볍게 첫 리뷰를 남겨보았습니다.

더 궁금하신게 있다면 멘토링 신청을 해주시거나 슬랙으로 문의주셔도 좋아요~~

app/build.gradle.kts Show resolved Hide resolved
android:background="@drawable/backgound_input_form"
android:inputType="text" />

<LinearLayout
Copy link

Choose a reason for hiding this comment

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

view depth가 깊어지네요
ConstraintLayout도 배우셨나요? 한번 ConstraintLayout을 써보시는것도 좋을거 같아요

Copy link
Author

Choose a reason for hiding this comment

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

b45cc13 / 다음과 같이 ConstraintLayout으로 교체해보았습니다!

혹시 더 depth를 줄일 수 있는지 확인해 주시면 감사하겠습니다!

Copy link

Choose a reason for hiding this comment

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

변경하신 구조를 보면

ConstraintLayout
...LinearLayout
......ConstraintLayout

이렇게 3 depth가 있네요
특이한 케이스를 제외하곤 ConstraintLayout 내에 다른 ViewGroup이 들어가지 않아도
레이아웃 배치를 구성할수 있습니다

Copy link
Author

@jsh00325 jsh00325 Jul 2, 2024

Choose a reason for hiding this comment

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

a4cecd4 / 다음과 같이 최대한 depth를 줄였습니다!

  1. 근데 여기서 더보기 버튼에서 클릭 영역을 크게 잡아주려고 뒷 배경으로 빈 View를 두어서 진행했는데, 이와 같은 경우에는 어떻게 작성하는게 더 좋은지 궁금합니다!
  2. 그리고 성별 입력 폼도 배경을 통일하려고 빈 뷰를 두어서 진행했는데, 이것도 이런식으로 작성하는 것이 좋은가요? 아니면 다른 방법이 있나요?
  3. 이렇게 작성을 하니 visibility를 설정할 때 모든 요소를 찾아서 설정해줘야하는데, 혹시 다른 좋은 방법이 있을까요?

@JSpiner
Copy link

JSpiner commented Jul 1, 2024

실제 앱과 같이 데이터의 삽입과 삭제가 빈번한 경우, 매번 데이터의 리스트가 비어있는 경우를 체크해서 빈 경우에만 텍스트를 보이도록 구현하면 되는지, 아니면 다른 효율적인 방법이 존재하는지 궁금합니다.

네 데이터는 매번 비교해야하는것이 맞습니다. 데이터 비교 자체는 큰 리소스 소모가 아니라서 상관 없습니다.
데이터에 따라 뷰를 건들여야하는 경우엔 리소스 소모가 클 수도 있습니다.
이럴땐 같은 상태로 셋팅된 뷰를 재활용 해본다던가 하는 시도가 있을수 있을텐데, 지금 요구사항에서는 지금처럼 구현하셔도 문제 없을거 같습니다

findViewById<TextView>(R.id.contactPhone).text = it.phone

it.email?.let { email ->
findViewById<LinearLayout>(R.id.contact_mail_layout).visibility = View.VISIBLE
Copy link

Choose a reason for hiding this comment

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

인텐트간 정보를 주고 받을 때, null 확인을 위해서 let 구문을 사용하였는데, 코드에서 저의 사용 방법이 올바른 방법인지 확인해 주셨으면 감사하겠습니다.

이 부분을 말씀 해주신거 같은데 나쁘지 않은 방법으로 보입니다.
다만 요구사항이 더 복잡해져서 visibility가 여러군데에서 바뀐다면
it.email이 null일때 visibility=GONE 처리가 누락될 가능성도 있을거 같아요
항상 visibility 처리를 하도록 이렇게 바꿔보는 방법도 있을거 같습니다.

// it.email?.let { email ->  // let은 더이상 사용하지 않습니다.
findViewById<LinearLayout>(R.id.contact_mail_layout).isVisible = it.email != null

Copy link
Author

Choose a reason for hiding this comment

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

2033f07 / 리뷰 참고하여 다음과 같이 수정하였습니다!

Copy link

Choose a reason for hiding this comment

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

오 좋네요 👍
제가 보기엔 깔끔해진거 같은데 어떠세요?

Copy link
Author

Choose a reason for hiding this comment

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

가독성도 더 좋아진 것 같고, 항상 조건에 따라 상태를 지정할 수 있어 좋은 것 같습니다!!

jsh00325 added 8 commits July 1, 2024 22:06
- ContactData.CONTACT_DATA_KEY를 통해서 관리
- 미처 바꾸지 못한 상수들도 변경하였습니다.
- 기존의 saveContact()라는 이름이 실제 역할과 달라, getContactInfoFromView()라는 이름으로 변경했습니다
- 깊이가 너무 깊은 레이아웃을 ConstraintLayout으로 교체하였습니다.
- visibility를 설정할 때, 각 입력칸의 visibility를 수정하는 방식으로 교체하였습니다.
- let으로 null-check 후 사용하는 방법에서, isVisible 옵션으로 `it.email != null`을 적용해 visibility를 수정했습니다.
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.

2 participants