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

Tens D10などの詳細なダイスロール一覧 #134

Merged
merged 7 commits into from
Mar 19, 2020

Conversation

ysakasin
Copy link
Member

@ysakasin ysakasin commented Mar 12, 2020

新クトゥルフでTens D10を複数ダイスロールするようになった。現方式だとD10とTens D10の区別がつかないので、 @detailedRandResultsを新設する。

Ref. #133

仕様

@detailedRandResults には以下のような値が入る

[
  [49, "100"],
  [40, "tens_d10"],
  [0, "d9"]
]

ダイスのタイプは文字列であらわし、現状では以下の3種類である

  • "tens_d10"
    • Tens D10
  • "d9"
    • ガンドックなどで用いられるダイスで、10面ダイスを振って0~9の乱数を得るもの
  • 単なる整数の文字列
    • その数分の面を持ったダイスを振ったことを表す

TODO

  • 新クトゥルフでTens D10を判別できるのかテストケースを作る

@ysakasin ysakasin requested a review from ochaochaocha3 March 12, 2020 11:45
@ysakasin ysakasin changed the title Tens D10などの詳細なダイスロール一覧 [WIP] Tens D10などの詳細なダイスロール一覧 Mar 12, 2020
@ysakasin ysakasin added the 新機能 新機能の実装やリクエスト label Mar 12, 2020
@ccfolier
Copy link

[IMO] 新設のプロパティになって互換性をある程度考えないとして 命名規則を {specialty}_d{range} とするとしたら 100 > d100 とした方が綺麗かも... と思いました。が、これは決めの問題で、どちらでも大丈夫だと思います。

@ysakasin
Copy link
Member Author

100 > d100 とした方が綺麗かも...

N面ダイスを dN と表現してしまうと、9面ダイスのd9とガンドック式のd9 が衝突してしまうので、現在の仕様にしていました。

第2引数を必ず文字列にする理由もないので、通常のダイスを振った場合には従来通り、文字列化していない数値を入れるのもありかもしれません

@ochaochaocha3
Copy link
Member

ochaochaocha3 commented Mar 13, 2020

命名規則を {specialty}_d{range} とするとしたら

対称性や、TypeScriptでの実装を容易にするための型の統一(必ず String へ)を考えると、例えば speciality を「normal」(通常)として、"normal_dX"(X:面数)とするのはいかがでしょうか?

また、JSONで渡す場合は種類が文字列で問題ないと思いますが、Rubyで扱う内部データとしては、2つの情報が含まれた文字列を分けて管理できると扱いやすいと考えました。通常の出目の情報に項目が加わっていることを考慮すると、@detailedRandResults を以下の構造体の配列にするのも一案です。

# 出目を表す構造体
# @!attribute [rw] value
#   @return [Integer] 値
# @!attribute [rw] sides
#   @return [Integer] ダイスの面数
# @!attribute [rw] type
#   @return [Symbol] ダイスの種類。:normal, :d9, :tens_d10など
RandResult = Struct.new(:value, :sides, :type) do
  # 種類を表す文字列を返す
  # @return [String]
  def type_str
    type == :normal ? "normal_d#{value}" : type.to_s
  end

  # JSON表現用の配列を返す
  # @return [(Integer, String)] [値, 種類]
  def to_array_for_json
    [value, type_str]
  end
end

@codecov-io
Copy link

codecov-io commented Mar 14, 2020

Codecov Report

Merging #134 into master will increase coverage by 0.05%.
The diff coverage is 98.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   86.30%   86.36%   +0.05%     
==========================================
  Files         192      193       +1     
  Lines       22263    22347      +84     
==========================================
+ Hits        19214    19299      +85     
+ Misses       3049     3048       -1     
Impacted Files Coverage Δ
src/bcdiceCore.rb 72.30% <96.87%> (+0.84%) ⬆️
src/diceBot/Cthulhu7th.rb 99.20% <100.00%> (ø)
src/test/test_detailed_rand_results.rb 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d71424...22a38d7. Read the comment docs.

@ysakasin
Copy link
Member Author

下記の構造体で持つようにしてみました。

DetailedRandResult = Struct.new(:kind, :sides, :value)

DetailedRandResult#kind が種類、#sidesが実際に振ったサイコロの面数、 #value が出目です。

構造体で保持できるので、種類をパースすることで情報を分解できるような形式にする必要はないため、このようになっています。Struct.newはto_jsonも自動生成してくれるので、従来の形式と合わせる必要もないと判断しました。

@ysakasin ysakasin changed the title [WIP] Tens D10などの詳細なダイスロール一覧 Tens D10などの詳細なダイスロール一覧 Mar 14, 2020
Copy link
Member

@ochaochaocha3 ochaochaocha3 left a comment

Choose a reason for hiding this comment

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

機能面はこれで大丈夫だと思います。

インターフェースの面では、#91 (comment) の方針に従い、新しく追加するインスタンス変数を @detailed_rand_results とsnake_caseにしておきたいと感じました(@randResults とちぐはぐな状態となっていますが、統一するならば、今後の方向性を考えるとむしろ @randResults を変える方が望ましいです)。また、getterを attr_reader で作るとRubyらしくなると思いました。

src/bcdiceCore.rb Outdated Show resolved Hide resolved
src/bcdiceCore.rb Outdated Show resolved Hide resolved
src/test/test_detailed_rand_results.rb Outdated Show resolved Hide resolved
src/bcdiceCore.rb Outdated Show resolved Hide resolved
Copy link
Member

@ochaochaocha3 ochaochaocha3 left a comment

Choose a reason for hiding this comment

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

これで問題ないと思います!

@ysakasin ysakasin merged commit 3563245 into master Mar 19, 2020
@ysakasin ysakasin deleted the detailed_rand_results branch March 19, 2020 16:31
ysakasin added a commit that referenced this pull request Aug 30, 2020
Tens D10などの詳細なダイスロール一覧
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
新機能 新機能の実装やリクエスト
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants