[산군] 뷰 챌린지 미션 1단계 제출합니다. by s9hn · Pull Request #5 · woowacourse/android-paint

s9hn · 2023-09-26T16:54:03Z

안녕하시오 스쌤 반갑습니다 :))
빡빡하고스트레스한가득이였던노답테스트콘솔원툴DI미션하다가
애뮬레이터에 내 세상을 펼칠 수 있는 캔버스 미션하니까 속이 뻥!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!뚫리네요 ㅎㅎ
구현하는데 재밌었읍니다. 빨리 다음단계 열려서 지우개랑 리셋이랑 이것저것 넣어보고싶습니다.

캔버스 코드에 주석 설명 첨부해놓았습니다. 더욱 더 최적화시켜주십시오.
리사이클러뷰로 팔레트 구현한 이유는 나중에 색상이 추가될 수도있고 가로스크롤이 가능해야했기 때문입미다

보고싶습니다 스캇))))) 잘부탁드립니다b

chws0508

안녕하세요 산군!!
같이 페어한지가 엊그제 같은데, 시간이 진짜 훅훅 지나갔네요...
정말 오랜만에 산군의 코드를 봤는데, 정말 많이 발전한 것이 느껴집니다...!
코드를 보며 궁금한 점, 개선할 점 소신있게 적어보았습니다!
이해가 되지 않는다면 언제든 DM 주시고, 즐거운 추석 되십쇼!!!

chws0508 · 2023-09-29T11:13:45Z

+import woowacourse.paint.Color.YELLOW
+
+class ColorAdapter(
+    private val onClick: (color: Int) -> Unit,


저는 onClick 이라는 네이밍을 보고는 어떤 함수인지 정확히 알기 힘들 것 같아요. 좀더 명시적인 네이밍이 필요해 보입니다! 그리고 저는 개인적으로 클릭했을 때 어떤 행위를 하는지에 대한 네이밍을 선호하는데요, 산군은 어떻게 생각하시나요?

음 생각해보니 스캇말이 맞네요! 지금까지 별 생각없이 단순히 클릭되었을때라는 네이밍의 onClick을 사용했습니다만
리사이클러뷰 아이템에 여러 클릭리스너들이 달릴 수 있으니, 어떤것을 클릭했는 지 정도는 구체화해주어도 괜찮을 것 같습니다!
onClick -> onPaletteClick

후자의 경우, 클릭했을 때 일어날 이벤트에 대해선 어댑터는 굳이 알 필요가 없다고 생각합니다!
어댑터의 관심사는 어떤것을 클릭했냐 정도, 클릭했을때 무엇을 넘겨주어야하는지 정도만 알 책임이 있고 해당 클릭에 대한 구현방식은
액티비티에서 자세한 네이밍 ex) changeColor 등으로 명시해줄 것 같아요!!

chws0508 · 2023-09-29T11:17:58Z

+class ColorViewHolder(
+    private val binding: ItemColorBinding,
+    private val onClick: (color: Int) -> Unit,
+    private val context: Context,


context를 넘겨받지 않고 해결할 수 있을 것 같아요~

chws0508 · 2023-09-29T11:28:53Z

+    private fun Path.addDot(event: MotionEvent) {
+        moveTo(event.x, event.y)
+        lineTo(event.x, event.y)
+    }
+


개인적으로는 addDot 이라는 함수 명이 , moveTo 와 lineTo를 모두 포함하는 단어라고는 생각되지 않네요. 저는 path를 move하고 line 을 그린다는 행위가 함수명에 드러나야 할 것 같아요. 산군은 어떻게 생각하시나요??

함수는 호출만으로 해당 네이밍으로 어떤 역할을 하는지 알려줘야한다고 생각해요!

when (event.action) { MotionEvent.ACTION_DOWN -> pathRecords.last().addDot(event) MotionEvent.ACTION_MOVE -> pathRecords.last().lineTo(event.x, event.y) else -> super.onTouchEvent(event) }

addDot()을 사용하는 부분에선 해당 함수의 이름만으로 어떤역할을 하는지 명시하는게 더욱 가독성을 높인다고 생각했습니다!
해당 행위를 구현하는 방식은 개발자가 직접 타고들어가서 해당 함수를 확인할 때 비로소 구현부를 알 수 있겠죠

크루들의 코드를 보았을 때, 점찍는 방식을

moveTo - lineTo

addCircle, add~

등으로 구현한 것 같습니다. 두가지 방식 모두 행위의 목적은 점을 찍는것으로 createDot, addDot 등 행위의 추상화된 이름으로 표기할 수 있을 것 같고, 구현방식은 개발자마자 함수본문 내에서 달라질 것 같다는게 제 생각입니다!

다시 생각해보니, 산군의 의견이 더 가독성이 좋은 것 같네요! 성실한 답변 감사합니다~

chws0508 · 2023-09-29T11:30:15Z

+        for (record in pathRecords.indices) {
+            val path = pathRecords[record]
+            val paint = paintRecords[record].setBrush()
+
+            canvas.drawPath(path, paint)
+        }
+    }


개인적으로 path 와 paint를 따로 관리하는 점이 조금 불편한 것 같아요. 객체를 활용하여 같이 관리하는것은 어떠신가요??

지금 구조를 유지하면서, 나중에 이전으로 되돌리기 기능이 생긴다면, 대처가 가능할까요??

map과 forEach로 리팩터링할 생각했는데 객체로 분리할 생각은 못해봤네요! 감삼다

chws0508 · 2023-09-29T11:36:13Z

+    }
+
+    companion object {
+        @SuppressLint("NonConstantResourceId")


혹시 SupperessLint를 사용하신 이유를 물어봐도 될까요?? 특별히 어떤 경우에 사용하시나요??

IDE가 시켜서..

chws0508 · 2023-09-29T11:36:35Z

+    }
+
+    // 누를 때 점이 찍히며, 드래그 시 선이 그려짐
+    @SuppressLint("ClickableViewAccessibility")


혹시 SupperessLint를 사용하신 이유를 물어봐도 될까요?? 특별히 어떤 경우에 사용하시나요??

IDE가 시켜서..

chws0508 · 2023-09-29T11:41:40Z

@@ -0,0 +1,18 @@
+package woowacourse.paint
+
+enum class Color(val colorRes: Int) {


Enum을 활용한 점 인상깊네요~~ 잘 먹겠습니다~

s9hn · 2023-09-30T17:51:25Z

스캇 리뷰 참고하여 객체분리 열심히 쪼개보았읍니다. 레벨1 생각나네요
거의 화가의 객체지향사회 만들어보았습니다 재밌게봐주십쇼

chws0508

이전 코드에 비해 가독성이 정말 미친듯이 올라간 것 같습니다...!
Painter와 Line 객체 네이밍이 센스있고 이해가 잘되네요!!
이번 미션은 지금으로도 충분할 것 같아서 이만 머지해도 될 것 같습니다!
개인적으로 궁금한점, 개선할 점 코멘트로 남겨놓았습니다.
다음 단계를 진행할 때 참고하셨으면 좋겠어요!
다음 미션도 잘 부탁 드려요~

chws0508 · 2023-10-02T06:25:51Z

+    fun changeBrush(width: Float) {
+        brush = brush.changeBrush(width)
+
+        painting.add(Line(brush = brush))
+    }
+
+    fun changeBrush(color: Int) {
+        brush = brush.changeBrush(color)
+
+        painting.add(Line(brush = brush))
+    }
+


brush를 변경하는데, painting에 계속 추가하는 이유가 있나요????

오와우 굿포인트 지적이군요!
해당 리뷰 반영하여 몇가지 수정사항이 생겼습니다.

현재 클릭 후 ACTION_UP 되었을 때, add해주면서 저장을 해주고 있었는데요. 그러다보니
그림을 그림 -> Action_Up (저장이됨)(현재 brush add) -> 색을 바꿈 -> 다시 그림을그림(바뀐 brush add안됨) -> Action_Up(바뀐 brush add 됨)
이렇게 바뀐 브러시가 한턴 늦게 적용되는 문제가 생겨서, 아예 Action_DOWN 될 때 저장하는 식으로 바꾸었습니다.
어차피 페인터가 생성될때 가지고 있는 Painting 리스트에 Line초기값을 넣어놓고 있었는데 이를 제거하고
mutableListOf(Line()) -> mutableListOf()
처음부터 액션다운될 때 저장(add) 되도록 수정하였습니다 감사합니다

도움이 됐다니 뿌듯하네요 ㅎㅎ 다음 단계 때 확인해 보겠습니다!! ㅎㅎ

chws0508 · 2023-10-02T06:27:27Z

+    override fun onTouchEvent(event: MotionEvent): Boolean {
+        when (event.action) {
+            MotionEvent.ACTION_DOWN -> painter.drawDot(event)
+            MotionEvent.ACTION_MOVE -> painter.drawLine(event)
+            MotionEvent.ACTION_UP -> painter.savePainting()
+            else -> super.onTouchEvent(event)
+        }
+


개인적으로 event자체를 넘기는 것 보단, event.x,event,y 를 넘기는 편이 자연스러워 보입니다! 라인을 그리거나, 점을 찍는 행위에는 event 보다는 좌표가 관심사 일 것 같아서요!

chws0508 · 2023-10-02T06:29:41Z

+    private var brush = Brush()
+    private val painting: MutableList<Line> = mutableListOf(Line())
+


생성자에 선언하는 것은 어떤가요? 개인적으로 저는 비록 테스트를 작성하지는 않더라도, 되도록이면 테스트 가능한 코드로 짜는 것이 좋다고 생각합니다!

s9hn added 5 commits September 26, 2023 14:54

feat: view 구현

f4877ce

feat: canvas 구현

3caaac8

feat: 색상변경 리사이클러뷰 구현

3859e7e

feat: 색상변경 적용

868d52d

refactor: 코드 리팩터링 및 주석설명 추가

ebe5f2f

s9hn changed the title ~~[산군] 1단ㄱ~~ [산군] 뷰 챌린지 미션 1단계 제출합니다. Sep 26, 2023

s9hn requested a review from chws0508 September 26, 2023 16:55

s9hn added 2 commits September 27, 2023 02:02

refactor: 코드 리팩터링 및 주석설명 추가

78cfb1e

Merge branch 'step1' of https://github.com/s9hn/android-paint into step1

e3ab948

s9hn self-assigned this Sep 26, 2023

chws0508 requested changes Sep 29, 2023

View reviewed changes

s9hn added 4 commits September 30, 2023 16:16

refactor: 어댑터 클릭메서드명 수정

26fd6d6

refactor: context 삭제

eab0fe3

refactor: annotation 수정

5b1aa12

refactor: customView 객체분리

97be5d7

chws0508 approved these changes Oct 2, 2023

View reviewed changes

chws0508 merged commit cfdfdd9 into woowacourse:s9hn Oct 2, 2023

		@@ -0,0 +1,18 @@
		package woowacourse.paint

		enum class Color(val colorRes: Int) {

		private var brush = Brush()
		private val painting: MutableList<Line> = mutableListOf(Line())

Conversation

s9hn commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chws0508 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s9hn commented Sep 30, 2023

Uh oh!

chws0508 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s9hn Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

s9hn commented Sep 26, 2023 •

edited

Loading

s9hn Oct 2, 2023 •

edited

Loading