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

Panel#takeAndPlaceをStageに移動 #223

Merged
merged 7 commits into from
Jan 9, 2017
Merged

Panel#takeAndPlaceをStageに移動 #223

merged 7 commits into from
Jan 9, 2017

Conversation

cookie-s
Copy link
Member

@cookie-s cookie-s commented Jan 4, 2017

fix #159

あと、PanelがStageを参照しなくなった。

Panelのselectedがちょっと変わった。
もともとの動作では、
「選択されているblockが最後の一個であるとき、これがtakeAndPlaceによって取られると、(Panel#updateの効果により)selectedはundefinedにな」り、
これを踏まえて #74 に対するパッチ #77 では、selectedがundefinedのとき回転するようにしていた。(現masterのpanel.js:97)

しかしこのpatchでは、PanelのコンストラクタおよびPanel内のblockがクリックされたときのみselectedが変化するように変更され、(おそらく)selectedがundefinedになりえなくなった。

一応、人の手によるstage"四則演算5"での検証の結果、#74 もうまくいく気がしている。
(この #74 に対するfunctional test作りたい...けど #199 が先行するかなぁ。)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.578% when pulling 255d853 on refactor-panel into 8f52887 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.578% when pulling 702f4c6 on refactor-panel into 8f52887 on master.

Copy link
Member

@hakatashi hakatashi left a comment

Choose a reason for hiding this comment

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

みました

}
this.parts.set(itemName, currentCount + count);
return true;
Copy link
Member

Choose a reason for hiding this comment

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

消していい気がする

Copy link
Member Author

Choose a reason for hiding this comment

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

あっせやな

rotate = (oldBlock.rotate + 1) % 4;
const currentCount = this.parts.get(blockName);
if (currentCount !== null) { // null means infinity
if (currentCount - 1 === 0) { // take the last block
Copy link
Member

Choose a reason for hiding this comment

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

currentCountが0のときに return false する必要がありそう

Copy link
Member Author

Choose a reason for hiding this comment

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

多分今のところ、currentCountが0という状況は発生し得ないのですが(そうなりそうな場合partsからkeyごと消される)、対応したほうがいいもんですかね

Copy link
Member

Choose a reason for hiding this comment

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

それを言うなら39行目の !this.parts.has(blockName) も発生し得ないような気が⋯⋯。それなら「正しくblockを減らせたかどうか」をreturnする仕様を消して、アサーションに置き換えたほうがよさそう

Copy link
Member Author

Choose a reason for hiding this comment

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

45行目を見てください。(これはもとからの仕様です)

Copy link
Member

Choose a reason for hiding this comment

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

あー、selectedがundifinedにならなくなったのか⋯⋯。それならむしろここでselectedにundefinedをセットして、takeAndPlaceにundefinedなら回転する仕様を追加して return false を撲滅するほうがよさそう。

Copy link
Member

Choose a reason for hiding this comment

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

(いずれにせよreturnされた値は使ってないんだし)

Copy link
Member Author

Choose a reason for hiding this comment

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

あ、39行目なんて通らないんじゃないかの意味がようやく分かった。(だーめ。)
undefinedにする案よいかもしれませんね。直前に操作した以外のブロックも回転するようになりますが、まあ。

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.578% when pulling 61316e7 on refactor-panel into 8f52887 on master.

@cookie-s
Copy link
Member Author

cookie-s commented Jan 8, 2017

あ、だめ。めっちゃこわれてる。

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.578% when pulling efb7236 on refactor-panel into 8f52887 on master.

*/
take(blockName) {
if (!this.parts.has(blockName)) {
Copy link
Member

Choose a reason for hiding this comment

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

消すんじゃなくてassertにしたいかなー。 currentCount === 0 の場合も同様。

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.578% when pulling 41026a5 on refactor-panel into 8f52887 on master.

@hakatashi
Copy link
Member

LGTM

お疲れさまです、ありがとう!

@hakatashi hakatashi merged commit 5561ecf into master Jan 9, 2017
@hakatashi hakatashi deleted the refactor-panel branch January 9, 2017 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panel#takeAndPlaceをStageに移行
3 participants