jwChung commented 8 years ago

5 와 관련된 작업입니다.

WIP 머릿말은 The Rules of the Open Road 에서 아래와 같이 표현하고 있습니다.

How do you let your fellow contributors know the pull request you just sent as early as possible isn’t done? Prefix the title with [WIP], e.g. [WIP] Add Cross-Platform Builds.

When the pull request is done, edit the title and remove [WIP], and add a comment to the pull request to let people know it’s ready for final review.

myeesan commented 8 years ago

구현 된 것 같은데, [WIP] 는 제거해도 되지 않을까요?

jwChung commented 8 years ago

@myeesan 네 [WIP] 머릿말 제거했습니다. case (rawInput, result) 변경 컷밋 인상적이네요. 👍 match문 축약을 어떻게 하나 의문이 있었는데 이렇게 되는군요.

def method(x: Int): Int => x match { case _ => x }
def method: Int => Int = case _ => x

jwChung commented 8 years ago

@myeesan 위에서 제게 보내주신 컷밋 도움이 많이 되었습니다. 조금 조심스러운 의견입니다만, 의도하신 내용을 리뷰 코멘트를 통해 PR을 작성한 사람이 직접 수정을 하도록 가이드하는 것이 좋지 않을까하는 생각해봤습니다. :)

younggi commented 8 years ago

DiceBot 생성자에서 Random을 전달 받을 수 있게 하신건 process method 테스트를 용이하게 하기 위해서 인것 같은데, 다른 목적도 있으신지 궁금합니다~

그리고 require 대신 case null 으로 변경하신건 코드 가독성이 더 나아서 변경하신 건지 궁금합니다~

질문 만 하네요.. --;

jwChung commented 8 years ago

@younggi

DiceBot 생성자에서 Random을 전달 받을 수 있게 하신건 process method 테스트를 용이하게 하기 위해서 인것 같은데, 다른 목적도 있으신지 궁금합니다~

주신 의견을 일고 난 후 디폴트 생성자를 제거했습니다. 혼돈을 드리다는 점 그리고 디폴트 생성자는 syntactic sugar역할에 지나지 않기 때문에 필수적이 요소가 아니라는 이유에서였습니다.

일반적으로 모든 의존성은 직접생성하는 것이 아니라, 주입 받는 것이 OCP(open closed principle) 관점에서 좋다고들 말하죠. Random 을 직접 생성하면 다른 인스턴스로 대체가 불가능하나 주입되면 코드를 고치지 않고 다른 행동을 하게 만들 수 있을 것입니다.(OCP) 가령 Random을 상속하는 NonDuplicatedRandom 클래스를 생각해볼 수 있겠습니다.

사실 OCP까지는 고민하고 싶지 않았고 랜덤을 테스트하는 목적이 가장 컸습니다.

그리고 require 대신 case null 으로 변경하신건 코드 가독성이 더 나아서 변경하신 건지 궁금합니다~

컷밋 기록까지 봐주셔서 감사합니다. 네 저는 그렇게 생각하고 리팩토링을 했습니다. 저랑 다르게 require을 선호하시는 분들 계실꺼라 생각됩니다.

질문 만 하네요.. --;

전혀 이런 생각을 안하셔도 됩니다. :) 더욱 적극적으로 질문하고 토론하는 장이 되도록 서로 노력해야겠습니다.

myeesan commented 8 years ago

@jwChung

의도하신 내용을 리뷰 코멘트를 통해 PR을 작성한 사람이 직접 수정을 하도록 가이드하는 것이 좋지 않을까하는 생각해봤습니다. :)

넵! 해당 태스크를 처리하는데 다소 방해 될 수 있는 행동일 수 있는데, 한 번 사례를 만들기 위해서 시도해 봤습니다. 친절하게 제안해 주셔서 감사합니다.

그리고 require 대신 case null 으로 변경하신건 코드 가독성이 더 나아서 변경하신 건지 궁금합니다~

null 은 좀 더 이야기 해 봐야 할 것 같습니다. 자바에서 호출 되는 것을 고려한다면 null 체크가 필요하겠지만 스칼라에서는 null을 사용하지 않는 것을 기본 관례입니다. 예를 들면, scala lint check 를 수행할 때 옵션에서 null을 disable 하는 것이 기본 설정입니다. 따라서, null체크는 배제하는 것이 코드를 간결하게 하며, null 오류는 관례에 의해 방지되도록 하는 것이 좋을 것 같습니다.

@younggi @jwChung

DiceBot 생성자에서 Random을 전달 받을 수 있게 하신건 process method 테스트를 용이하게 하기 위해서 인것 같은데, 다른 목적도 있으신지 궁금합니다~ Random을 상속하는 NonDuplicatedRandom 클래스를 생각해볼 수 있겠습니다.

테스트만을 목적으로 생성자를 추가하고 의존성을 주입받는 것이 좋은지는 잘 모르겠습니다. 비슷하게 하나의 클래스에서만 구현하는 인터페이스 분리도 비슷한 관점에서 이야기 할 수 있을 것 같습니다. NonDuplicatedRandom 같은 클래스가 필요한 상황은 논외로 하구요.

hkjlee109 commented 8 years ago

+1 I approve. I see 3 happy reviewers. Pulling a trigger!

hkjlee109 commented 8 years ago

😱

jwChung commented 8 years ago

@kwoolytech 완결된 pr 이 아닌데 머지가 되었네요 @myeesan 님이 고쳤으면 하는 의견이 있으셨습니다 pr 작성자가 머지 하더록 해야겠네요

hkjlee109 commented 8 years ago

Deng! Sorry, guys. 😭

hkjlee109 commented 8 years ago

Oh I see Revert button. Is this the thing? @jwChung

jwChung commented 8 years ago

이 pr을 다시 열수 없는게 포인트인 것 같습니다 의견이 또 다른 pr에서 진행되어야 하니까요 그래서 revert 보다는 제가 다른 pr을 통해 제기된 문제를 따로 처리하는게 어떨까합니다 너무 심려마세요

hkjlee109 commented 8 years ago

yup, thanks :)

funprog / funbot

process 메소드 구현 #22

5 와 관련된 작업입니다.