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

HW-1 네이버 영화 검색 앱 #510

Merged
merged 54 commits into from Mar 4, 2020
Merged

Conversation

mtjin
Copy link
Contributor

@mtjin mtjin commented Mar 2, 2020

HW-1 네이버 검색 영화앱
rxjava와 interceptor 사용이 처음인 점 양해 부탁드립니다.

@mtjin mtjin requested review from sooakim and sjjeong March 2, 2020 13:10
@mtjin mtjin self-assigned this Mar 2, 2020
@mtjin mtjin added Review Needed 리뷰어의 리뷰가 필요한 경우 and removed Answer Needed PR담당자의 응답이 필요한 경우 labels Mar 4, 2020
@sooakim sooakim self-requested a review March 4, 2020 06:31

override fun onDestroy() {
super.onDestroy()
movieCall.cancel()
Copy link
Contributor

Choose a reason for hiding this comment

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

movieCall이 초기화되지 않았을 경우에 대한 예외처리를 추가해주세요.

@sooakim sooakim added Answer Needed PR담당자의 응답이 필요한 경우 and removed Review Needed 리뷰어의 리뷰가 필요한 경우 labels Mar 4, 2020
@sooakim sooakim self-requested a review March 4, 2020 06:39

override fun onDestroy() {
super.onDestroy()
movieCall?.cancel()
Copy link
Contributor

Choose a reason for hiding this comment

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

스택오버플로우 답변 참조 movieCall이 lateinit var로 선언되어 있어서 ?이 의미가 없어요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

감사합니다. 확인해보겠습니다.

Copy link
Contributor

@sooakim sooakim Mar 4, 2020

Choose a reason for hiding this comment

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

@mtjin 네 이것만 처리되면 approve할게요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

수정했는데 movieCall을 lateinit말고 ?= null 로 해놓는게 더 낫나요?
fb3830f

Copy link
Contributor

Choose a reason for hiding this comment

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

@mtjin lateinit을 쓸지 Nullable을 쓸지에 대해선 제 생각에는 개인 취향일 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sooakim 넵 리뷰 감사합니다~

@sooakim sooakim self-requested a review March 4, 2020 06:57
@sooakim sooakim added Mentor Needed 멘토가 필요한 경우 Review Needed 리뷰어의 리뷰가 필요한 경우 and removed Answer Needed PR담당자의 응답이 필요한 경우 labels Mar 4, 2020
Comment on lines 11 to 15
val newRequest = request().newBuilder().run {
addHeader("X-Naver-Client-Id", "33chRuAiqlSn5hn8tIme")
addHeader("X-Naver-Client-Secret", "fyfwt9PCUN")
build()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Builder pattern이기 때문에 run으로 하지 않아도 됩니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

run코드 제거했습니다.
87f2a2e

Comment on lines 71 to 73
interface ItemClickListener {
fun onItemClick(movie: Movie)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

interface말고 kotlin higher-order function으로 처리해보세요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

higher-order function로 처리하였습니다.
15bb680

Comment on lines 59 to 61
fun setItems(items: List<Movie>) {
this.items.addAll(items)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

notify를 함수 내부에서 호출 하도록 해주세요
아래 clear도 동일하게 해주세요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

수정했습니다.
f82dc00


fun bind(movie: Movie) {
with(movie) {
Glide.with(itemView).load(image).placeholder(R.drawable.ic_default).into(ivPoster!!)
Copy link
Collaborator

Choose a reason for hiding this comment

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

load부터 개행해서 한 함수당 한 라인씩 나오도록 해주세요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

수정했습니다.
35d017a

initListener()
}

fun initView() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

private 추가해주세요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

추가했습니다.

254cb98

Comment on lines 75 to 76
if (isSuccessful && body() != null) {
body()?.movies?.let { it -> movieAdapter.setItems(it) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

if에서 body()가 null이 아닌걸 검사했는데 내부에서 ?로 사용하고 있네요
이거는 이렇게 수정하시면 됩니다.

val body = body()
if (isSuccessful && body != null) {
    body.movies.let { movieAdapter.setItems(it) }
    movieAdapter.notifyDataSetChanged()
} else {
    onToastMessage("불러오는데 실패 했습니다.")
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

수정했습니다.

e0b3a4a


override fun onDestroy() {
super.onDestroy()
if(this::movieCall.isInitialized){
Copy link
Collaborator

Choose a reason for hiding this comment

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

if와 ( 사이에 공백 추가해주세요
reformat code 하시면 됩니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

레트로핏 onResponse() 쪽 수정할때 같이 수정하여 커밋했습니다.
e0b3a4a

@sjjeong sjjeong added Answer Needed PR담당자의 응답이 필요한 경우 and removed Review Needed 리뷰어의 리뷰가 필요한 경우 labels Mar 4, 2020
@mtjin mtjin added Review Needed 리뷰어의 리뷰가 필요한 경우 and removed Answer Needed PR담당자의 응답이 필요한 경우 labels Mar 4, 2020
Copy link
Collaborator

@sjjeong sjjeong left a comment

Choose a reason for hiding this comment

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

LGTM ✊ ✌️ 👋

@sjjeong sjjeong added Merge Needed 모든 리뷰어가 Approve를 한 경우 and removed Review Needed 리뷰어의 리뷰가 필요한 경우 labels Mar 4, 2020
@mtjin mtjin merged commit 3f7f3c6 into StudyFork:2003/mtjin Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mentor Needed 멘토가 필요한 경우 Merge Needed 모든 리뷰어가 Approve를 한 경우
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants