중복없는 dice 구현 #34

curry-ing · 2016-07-12T03:49:46Z

#6 중복없는 dice구현을 위한 작업입니다.

눈팅만 하다 영원히 참여 못할 것 같아,
WIP라벨 시작도 끊어 볼 겸 일단 한 줄이라도 작성 해봤습니다. (이렇게 하면 되나요;;)

추가 구현되는대로 커밋 예정입니다.

jwChung · 2016-07-12T03:53:37Z

src/main/scala/DiceBot.scala

@@ -4,6 +4,7 @@ class DiceBot(val random: Random) {
  require(random != null, "the 'random' value cannot be null.")

  private lazy val regex = """(?i)\s*roll\s+(\d+)d(\d+)\s*""".r
+  private lazy val regex_for_non_dup = """(?i)\s*roll\s+(\d+)n(\d+)\s*""".r


필드 naming convention이 어떻게 되나요? scala에서는 snake notation 이 일반적인가요?

아 스칼라에서는 camelCase를 쓰죠? 수정해야겠습니다 :)

jwChung · 2016-07-12T03:55:13Z

제목 [WIP] 머릿말은 제거하셔도 되지 않을까요?

jwChung · 2016-07-12T06:47:08Z

src/main/scala/DiceBot.scala

@@ -22,6 +23,8 @@ class DiceBot(val random: Random) {
          "the 'input' value cannot be null.")
      case regex(count, max) =>
        Some(makeOrderString(count.toInt, max.toInt))
+      case regex_for_non_dup(count, max) =>


이 이슈을 내가 구현 한다면 아래 두 가지 방법 중 어떤 것을 택할까에 대해서 고민을 많이 했습니다.

@masunghoon 님이 작성하신 것과 같이 case 문을 추가하는 방법

NonDuplicatedDiceBot 과 같은 새로운 클래스를 만드는 방법

두 가지 방법은 아래와 같은 장단점이 있어서 선택이 쉽지가 않네요.

장점: 구현이 쉽다, 단점: OCP를 위배함으로 코드가 수정되면 복잡도가 증가

장점: OCP 를 유지, 단점: 코드조각이 많아져 전체를 한눈에 보기 어렵다

말로하니 이해가 안되네요 -_-;;; 제 생각을 담은 [WIP] PR 작성해보겠습니다.

OCP가 무슨 Principle이었죠? ㅎㅎ

라고 쓰고 보니 Open-Closed Principle이군요 ㅎㅎ 자문자답

~~Open-Closed Principle 입니다.~~ 이걸 여기에서 고민하는게 오버인가 라는 생각도 드네요.

제 생각을 잘 대변하는 글이 있어 링크 남깁니다. 읽으시라고 강요하는 거 아니닌깐 부담가지지 마세요. 기록의 목적입니다. :)

http://joelabrahamsson.com/a-simple-example-of-the-openclosed-principle/

어차피 학습 목적의 프로젝트이므로 저같이 잘 모르는 사람에게는 이렇게 던져주시는 이슈가 도움이 많이 됩니다. 도움이 된다면 쉬운 길 돌아가는것도 문제 안 된다고 생각해요~

myeesan · 2016-07-14T12:59:57Z

현재 n과 d 이상의 커맨드가 확정 되지 않은 상태이고, n의 필요성은 명확한 상태입니다. 두 요구사항을 구현 한 상태에서 코드를 이해하기는 case문을 활용하는 것이 맞다고 생각됩니다.

의견을 교환하기 위한 글을 쓰거나 찾아보도록 하겠습니다. :)

jwChung · 2016-07-14T13:02:12Z

@masunghoon 저도 @myeesan님 말씀에 동의합니다. 제가 괜히 일을 복잡게 만든 것 같습니다. 현재 의도하시는 방향으로 구현하시는 것이 좋다는 생각입니다.

# Conflicts: # src/main/scala/com/github/funprog/funbot/DiceBot.scala (resolved)

curry-ing · 2016-07-17T04:06:25Z

몇 가지 테스트케이스 추가 후 WIP 떼도록 하겠습니다~

- 실제 동작을 확인해보는 테스트가 변경된 코드에 맞지 않아서 삭제 - 가능한 모든 출력을 미리 열거한 후 process 함수의 리턴과 일치하는 값이 해당 결과에 있는지 확인하는 케이스 추가

jwChung · 2016-07-17T13:45:17Z

src/test/scala/com/github/funprog/funbot/DiceBotTest.scala

+    ("roLL 2D3", List("1, 1", "1, 2", "1, 3", "2, 1", "2, 2", "2, 3", "3, 1", "3, 2", "3, 3")),
+    ("roll 2n2", List("1, 2", "2, 1")),
+    ("RoLl 1n2", List("1", "2")),
+    ("roLL 2N3", List("1, 2", "1, 3", "2, 1", "2, 3", "3, 1", "3, 2"))


roll 2nd 에 대한 대소문자 구분 없는 테스트는 위에서 진행되었습니다. 제 생각에는 불필요해 보입니다.

의도는 대소문자 구분 테스트가 아니었고, 삭제한 테스트에 대한 대체 테스트를 만들어 보려 했습니다. (위 테스 트를 C&P해서 만들다보니... ;;) 각 테스트에 대한 description을 좀 더 명확히 적어보겠습니다.

@masunghoon 그렇군요.
테스트를 보니, 랜덤한 결과값에 대한 결과값 후보들을 모두 나열하고, process 결과 값이 후보군에 들어 있는지 확인 하는 방식으로 테스트 하는 것 같습니다.

위 테스트에서는 result 라는 변수명이 적절하지 않아보입니다. cadidates 같은 단어를 선택하시거나, Random의 seed 값을 지정해서 테스트 하는 것은 어떨까요?

jwChung · 2016-07-17T14:02:13Z

질문 및 의견 몇 가지 남겼습니다. @masunghoon 님 의견을 듣기 위해 -1 로 표시해두었습니다.

jwChung · 2016-07-17T14:06:23Z

src/main/scala/com/github/funprog/funbot/DiceBot.scala

+    }
+  }
+
+  def parseInput(input: String): (Boolean, Int, String, String) = input match {


접근자를 지정하지 않으면 public으로 동작하지 않나요? private 멤버로 표시하는 것은 어떨까요? 아래 메소드들에 대해서도 이 의견을 동일하게 적용할 수 있다고 생각합니다.

네, process 제외한 메소드는 모두 private 으로 지정하는게 낫겠네요. 동의합니다.

jwChung · 2016-07-17T14:41:30Z

@masunghoon 위 제 의견을 다시 읽어 보니 좀 강한 어조가 느껴지는군요. 불필요한 인삿말 (eg. thanks) 군더더기를 제거하고 제 생각을 담백한게 적은 것 뿐입니다. 모든 문장 끝에 ^^에 있다고 보시면 됩니다 ㅎ

myeesan · 2016-07-17T15:07:14Z

저는 작동에 결함이 없는 만큼 merge 하는게 좋다고 생각됩니다.

현재 이 브랜치에 의존성을 가진 이슈들이 몇 개 있습니다.
@jwChung 님께서 제기해 주신 의문은 충분히 가치 있다고 생각합니다. 그래서, 현재 쓰레드에서 계속 진행 되는 것 보다, merge 후에 새로운 이슈를 생성해서 논의를 이어나간 다면 좀 더 집중 할 수 있지 않을까 합니다.

hkjlee109 · 2016-07-17T15:12:37Z

src/main/scala/com/github/funprog/funbot/DiceBot.scala

+    case _ => (1 to 6).toList.map(_.toString)
+  }
+
+  @annotation.tailrec


hkjlee109 · 2016-07-17T15:27:28Z

Great work! @masunghoon Hope you sleep well tonight. I am oooooofff!

curry-ing · 2016-07-17T15:51:45Z

@jwChung 어조는 전혀 문제되지 않습니다! 저기 사이사이에 ^^있으면 더 이상할 듯... ㅎㅎ

jwChung · 2016-07-19T09:02:14Z

@myeesan

저는 작동에 결함이 없는 만큼 merge 하는게 좋다고 생각됩니다.

저도 같은 생각입니다. 그러나 위에서 제가 드린 의견이 반영되든 아니든 종결된 뒤에 -1 이모지를 제거하겠습니다.

현재 이 브랜치에 의존성을 가진 이슈들이 몇 개 있습니다.

@jwChung 님께서 제기해 주신 의문은 충분히 가치 있다고 생각합니다. 그래서, 현재 쓰레드에서 계속 진행 되는 것 보다, merge 후에 새로운 이슈를 생성해서 논의를 이어나간 다면 좀 더 집중 할 수 있지 않을까 합니다.

네 다른 이슈를 통해서 처리되는 것도 방법이겠습니다. 다만 이런 사항에 대해 동의가 있어야 되고 가능하다면 이슈까지 만들어 놓고 병합되는 것이 바람직하다고 생각합니다.

- `2e4b7f6` Remove unnecessary guard clauses

curry-ing added 2 commits July 12, 2016 00:52

중복없는 dice의 구분자로 n 입력받을 수 있도록 테스트케이스 추가

b2a1716

중복없는 주사위를 위한 구분자로, d외에도 n문자를 입력받을 수 있도록 처리

b8b334a

curry-ing added the WIP label Jul 12, 2016

jwChung reviewed Jul 12, 2016
View reviewed changes

curry-ing changed the title ~~[WIP] 중복없는 dice 구현~~ 중복없는 dice 구현 Jul 12, 2016

jwChung reviewed Jul 12, 2016
View reviewed changes

jwChung mentioned this pull request Jul 12, 2016

CompositeBot 클래스를 만듭니다. #35

Closed

curry-ing added 3 commits July 16, 2016 10:35

snake case -> camel case

b810ed6

implementation of non-duplication dice

6fc4264

Merge branch 'master' into non_dup_dice

fe34df6

# Conflicts: # src/main/scala/com/github/funprog/funbot/DiceBot.scala (resolved)

테스트 케이스 일부 수정

b66c6d9

- 실제 동작을 확인해보는 테스트가 변경된 코드에 맞지 않아서 삭제 - 가능한 모든 출력을 미리 열거한 후 process 함수의 리턴과 일치하는 값이 해당 결과에 있는지 확인하는 케이스 추가

curry-ing removed the WIP label Jul 17, 2016

jwChung reviewed Jul 17, 2016
View reviewed changes

hkjlee109 reviewed Jul 17, 2016
View reviewed changes

src/main/scala/com/github/funprog/funbot/DiceBot.scala

case _ => (1 to 6).toList.map(_.toString)

}

@annotation.tailrec

Copy link

Member

hkjlee109 Jul 17, 2016

Choose a reason for hiding this comment

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

👍

curry-ing closed this Jul 17, 2016

curry-ing reopened this Jul 17, 2016

change methods to private

d5090fc

curry-ing added 4 commits July 20, 2016 12:22

삭제한 테스트 복원 및 회귀 테스트 보장

d89e01c

shuffle을 안 쓰고 rollNormalDice와 비슷하게 수정

671ce85

재귀함수 대신에 주사위를 한 번 shuffle후 결과에서 take로 가져오는 방법으로 변경

88bd113

master 병합 과정에서 제대로 반영되지 않았던 부분 수정

e47241e

- `2e4b7f6` Remove unnecessary guard clauses

jwChung merged commit 522527a into master Jul 20, 2016

jwChung deleted the non_dup_dice branch July 20, 2016 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

중복없는 dice 구현 #34

중복없는 dice 구현 #34

curry-ing commented Jul 12, 2016 •

edited

Loading

jwChung Jul 12, 2016

curry-ing Jul 12, 2016 •

edited

Loading

jwChung commented Jul 12, 2016

jwChung Jul 12, 2016

curry-ing Jul 12, 2016

curry-ing Jul 12, 2016

jwChung Jul 12, 2016 •

edited

Loading

jwChung Jul 12, 2016

curry-ing Jul 12, 2016

myeesan commented Jul 14, 2016

jwChung commented Jul 14, 2016

curry-ing commented Jul 17, 2016

jwChung Jul 17, 2016

curry-ing Jul 17, 2016

myeesan Jul 17, 2016

jwChung commented Jul 17, 2016 •

edited

Loading

jwChung Jul 17, 2016

curry-ing Jul 17, 2016

jwChung commented Jul 17, 2016

myeesan commented Jul 17, 2016 •

edited

Loading

hkjlee109 Jul 17, 2016

hkjlee109 commented Jul 17, 2016

curry-ing commented Jul 17, 2016

jwChung commented Jul 19, 2016 •

edited

Loading

중복없는 dice 구현 #34

중복없는 dice 구현 #34

Conversation

curry-ing commented Jul 12, 2016 • edited Loading

Choose a reason for hiding this comment

curry-ing Jul 12, 2016 • edited Loading

Choose a reason for hiding this comment

jwChung commented Jul 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwChung Jul 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myeesan commented Jul 14, 2016

jwChung commented Jul 14, 2016

curry-ing commented Jul 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwChung commented Jul 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwChung commented Jul 17, 2016

myeesan commented Jul 17, 2016 • edited Loading

Choose a reason for hiding this comment

hkjlee109 commented Jul 17, 2016

curry-ing commented Jul 17, 2016

jwChung commented Jul 19, 2016 • edited Loading

curry-ing commented Jul 12, 2016 •

edited

Loading

curry-ing Jul 12, 2016 •

edited

Loading

jwChung Jul 12, 2016 •

edited

Loading

jwChung commented Jul 17, 2016 •

edited

Loading

myeesan commented Jul 17, 2016 •

edited

Loading

jwChung commented Jul 19, 2016 •

edited

Loading