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

리디 안드로이드 코딩 스타일 리뷰(뷰어팀 PR 템플릿) #41

Merged
merged 3 commits into from
Jun 9, 2020

Conversation

libliboom
Copy link
Owner

환경 정보

  • branch: feature-style
  • 리뷰 방식: 최신 커밋된 작업 내역 부분에 코멘트 부탁드립니다.
  • 해당 PR은 PR을 생성하기 위한 PR입니다.
    스타일 코멘트는 최신 커밋내역에 코멘트를 부탁드리지만 상황에 따라 자유형식으로 의견을 주셔도 됩니다.

요약

  • 리디 안드로이드 코드 스타일을 적용했습니다.

목적

  • 리디 안드로이드 코드 스타일이 올바르게 반영되었는지 확인합니다.

관련 링크

후속 작업

  • 리뷰 받은 코드 스타일을 반영합니다.

주요 확인사항

  • @review + 관련 키워드 부분에 대해 올바르게 기술되었는지 확인이 필요합니다.
  • scope 연산자가 올바른 의도로 사용되었는지 확인이 필요합니다.
  • Exception 처리 방법에 문제가 없는지 확인이 필요합니다.
  • mutable, immutable 개념이 전체적으로 지켜지고 있는지 확인이 필요합니다.
  • 반환타입 형변환이 불필요한 형변환과정 없이 올바르게 지켜지는지 확인이 필요합니다.

@JSpiner
Copy link

JSpiner commented Jun 9, 2020

실행해보니 이런 에러가 나네요

     Caused by: kotlin.UninitializedPropertyAccessException: lateinit property istream has not been initialized
        at com.github.libliboom.epubviewer.main.viewmodel.BookshelfViewModel.forceInitialization(BookshelfViewModel.kt:48)
        at com.github.libliboom.epubviewer.main.viewmodel.BookshelfViewModel.initResources(BookshelfViewModel.kt:31)
        at com.github.libliboom.epubviewer.main.activity.BookshelfActivity.onCreate(BookshelfActivity.kt:19)
        at android.app.Activity.performCreate(Activity.java:7009)
        at android.app.Activity.performCreate(Activity.java:7000)
        at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1214)
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2731)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2856) 
        at android.app.ActivityThread.-wrap11(Unknown Source:0) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1589) 
        at android.os.Handler.dispatchMessage(Handler.java:106) 
        at android.os.Looper.loop(Looper.java:164) 
        at android.app.ActivityThread.main(ActivityThread.java:6494) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:807) 

초기 설정을 해줘야 할 게 있을까요?

@libliboom
Copy link
Owner Author

우선 로그상 디바이스 assets 사용시점에 문제가 발생하는것으로 보여요
우선 제가 개발한 당시에는 해당 문제 상황이 재현되지 않았는데
제가 사용한 환경 정보는 아래와 같습니다.

디바이스: Q9 / Android 9(API 29)
에뮬: Pixel 3 / API 29

실행한 환경 정보 공유해주시면 한번 확인해볼게요

@JSpiner
Copy link

JSpiner commented Jun 9, 2020

스크린샷 2020-06-09 오후 12 23 51
이러합니다.

제 에뮬 문제인거 같으니 지금 당장 수정하지 않으셔도 괜찮습니다 😄

- path with "/" doesn't work at API 27
- fixed wrong file path
@libliboom
Copy link
Owner Author

공유해주신 환경에서 assets에 접근하는 path에 "/" 문제가 있었습니다.
API 27에서는 "/" 붙은 폴더 경로를 찾지 못해 제거했습니다.

@libliboom
Copy link
Owner Author

아래 내용을 반영하겠습니다.

  • 리턴 타입은 최대한 추상화
  • 배열 변환 타입 정리하기
  • Viewmodel의 android 패키지 제거하기 (특히 인자로 들어온 부분들)
  • Lint warning 수정
  • Single expression 적용

- Remove all import of android at XXXViewModel
- Single expression
- abstract type for argument and return
- dispose observable
- fix lint warning
@libliboom
Copy link
Owner Author

금일 피드백 받은 내용 모두 적용 완료하여 Closed합니다.

@libliboom libliboom closed this Jun 9, 2020
@libliboom libliboom reopened this Jun 9, 2020
@libliboom libliboom merged commit 1d01f4d into develop Jun 9, 2020
@libliboom libliboom deleted the feature-style branch September 6, 2020 07:06
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

2 participants