publisher による構成に修正（Trying Test）

[shundroid]

This change is 

[dadaa]

Review status: 0 of 13 files reviewed at latest revision, 1 unresolved discussion.

---

test/publisher.test.js, line 28 at r1 (raw file):

    let isEqualsData = false;
    const callback = function(author, data, data2, data3) {
      isCalled = true;

まず、ここが呼ばれるかを callback の中でテストすると良いかも。 This callback function should be called. 的な。
あと、isCalled とかは使わないで、この callback の内部でテストしましょう。
dashboard.test.js も同様です。

---

Comments from Reviewable

[shundroid]

Review status: 0 of 13 files reviewed at latest revision, 1 unresolved discussion.

---

test/publisher.test.js, line 28 at r1 (raw file):
わかりました、ありがとうございます。

@dadaa

This callback function should be called. 的な。
主語が This callback function になるときも、そのまま it の第一引数にいれていいでしょうか
とりあえずそれでやってみます

---

Comments from Reviewable

[dadaa]

Review status: 0 of 22 files reviewed at latest revision, 16 unresolved discussions.

---

commandRunner.js, line 68 at r2 (raw file):

  stopLoop() {
    if (this.loopTimeoutId !== null) {
      clearTimeout(this.loopTimeoutId);

this.loopTimeoutId が null になる箇所がなさそうなので、毎回ここ呼ばれてしまいませんか？

---

commandRunner.js, line 76 at r2 (raw file):

      const timeoutId = this.customTimeoutIds[timeoutIdName];
      if (timeoutId !== null) {
        clearTimeout(timeoutId);

ここも同様かな？
this.customTimeoutIds[timeoutIdName]; を空っぽにしておきたいですね。

---

commandRunner.js, line 93 at r2 (raw file):

    this.loopTimeoutId = setTimeout(() => {
      this.loopMethod(nextIndex);
    }, currentCommand.time * 1000);

この 1000 はもう少し細かく動かしたいなどの調整のために、どこかコンフィグをまとめているようなところに出しておきたいですね。

---

componentBase.js, line 9 at r2 (raw file):

  subscribe(subjectName, observeFunction) {
    publisher.subscribe(subjectName, (author, ...data) => {
      if (author !== this) {

author の null チェックはどんな意味がありますか？

---

controllerManager.js, line 21 at r2 (raw file):

  }
  resetHp(name) {
    controllerModel.get(name).setHp(100);

この 100 も変数化してどこかに置いておきたいですね。MAX_HP 的な感じで。

---

controllerManager.js, line 73 at r2 (raw file):

          controller.client !== null &&
          orb.linkedClients.indexOf(controller.client.key) !== -1) {
        controller.setHp(controller.hp - 10);

-10 も変数化したいでですね。

---

controllerManager.js, line 82 at r2 (raw file):

      if (client !== null) {
        client.sendCustomMessage("availableCommandsCount", count);
      }

他のところもそうですけど null だった場合に何かしらログを残します？ console.warn とかで。

---

main.js, line 136 at r2 (raw file):

      connector.connect(port, rawOrb.instance).then(() => {
        rawOrb.instance.setInactivityTimeout(9999999, function(err, data) {
          console.log(err | "data: " + data);

エラーは console.error とか使ったら？

---

publisher.js, line 6 at r2 (raw file):

  }
  subscribe(subjectName, observeFunction) {
    if (typeof this.observeFunctions[subjectName] === "undefined") {

if (!this.observeFunctions[subjectName])
で良さげな気がします。

---

publisher.js, line 13 at r2 (raw file):

  publish(author, subjectName, ...data) {
    if (typeof this.observeFunctions[subjectName] !== "undefined") {

同じく

---

spheroServerManager.js, line 31 at r2 (raw file):

  initializeOrb(orb) {
    const rawOrb = orb.instance;
    rawOrb.color("white");

white も変数にー

---

virtualSpheroManager.js, line 16 at r2 (raw file):

  addSphero(key, name, isNewName) {
    this.virtualSphero.addSphero(name);

controllerModel に対して key と name の登録とかいらない？

---

test/componentBase.test.js, line 27 at r2 (raw file):

    publisher.publish(this, "test2", "test-data-2");
    component.subscribe("test3", data => {
      it("should not call function when author is same", () => {

このテストの意味がちょっと良くわからないっすー

---

test/dashboard.test.js, line 35 at r2 (raw file):

    publisher.subscribe("pingAll", author => {
      it("should publish pingAll to eventPublisher", () => {
        assert(true);

100 通っちゃうテストですねｗこれから？

---

test/virtualSpheroManager.test.js, line 9 at r2 (raw file):

    const virtualSpheroManager = new VirtualSpheroManager(8081);
    it("should initialize virtualSphero", () => {
      assert(typeof virtualSpheroManager === "object" &&

２つの assert に分けましょう

---

Comments from Reviewable

[shundroid]

Review status: 0 of 22 files reviewed at latest revision, 16 unresolved discussions.

---

commandRunner.js, line 68 at r2 (raw file):

Previously, dadaa wrote…

this.loopTimeoutId が null になる箇所がなさそうなので、毎回ここ呼ばれてしまいませんか？

最初、コマンドが呼ばれていないときは、初期値として null が入っているので、
その時は clearTimeout しないようにしていましたが、
ここで、this.loopTimeoutId = null をするようにします。

---

commandRunner.js, line 76 at r2 (raw file):

Previously, dadaa wrote…

ここも同様かな？
this.customTimeoutIds[timeoutIdName]; を空っぽにしておきたいですね。

これは空っぽにします

---

commandRunner.js, line 93 at r2 (raw file):

Previously, dadaa wrote…

この 1000 はもう少し細かく動かしたいなどの調整のために、どこかコンフィグをまとめているようなところに出しておきたいですね。

わかりました。 config.js があって、そこでポートの調整などをしているので、
そこでできるように改善します。

---

componentBase.js, line 9 at r2 (raw file):

Previously, dadaa wrote…

author の null チェックはどんな意味がありますか？

自分で publish したイベントを、自分でsubscribe で受け取らないようにしています。

そうしないと、socket に送るとき、socket から送られてきたデータを送り返すみたいになっちゃうんです・・

---

Comments from Reviewable

[shundroid]

Review status: 0 of 22 files reviewed at latest revision, 16 unresolved discussions.

---

controllerManager.js, line 21 at r2 (raw file):

Previously, dadaa wrote…

この 100 も変数化してどこかに置いておきたいですね。MAX_HP 的な感じで。

config.js で行きますー

---

controllerManager.js, line 73 at r2 (raw file):

Previously, dadaa wrote…

-10 も変数化したいでですね。

これも config.js で行きますー

---

controllerManager.js, line 82 at r2 (raw file):

Previously, dadaa wrote…

他のところもそうですけど null だった場合に何かしらログを残します？ console.warn とかで。

うーん、この場合は client がないという状態なのですが、
間違えて client をリロードしてしまったときなど、そこまでのエラーでないときもあるので、大丈夫かと思うんですが、、、

そこまでのエラーでないときだから、 console.warn にすることにしますｗ

---

main.js, line 136 at r2 (raw file):

Previously, dadaa wrote…

エラーは console.error とか使ったら？

あーー、これは土曜日急いだやつでした
そうします

---

publisher.js, line 6 at r2 (raw file):

Previously, dadaa wrote…

if (!this.observeFunctions[subjectName])
で良さげな気がします。

わかりました

---

publisher.js, line 13 at r2 (raw file):

Previously, dadaa wrote…

同じく

わかりました

---

spheroServerManager.js, line 31 at r2 (raw file):

Previously, dadaa wrote…

white も変数にー

config.js でいきやす

---

virtualSpheroManager.js, line 16 at r2 (raw file):

Previously, dadaa wrote…

controllerModel に対して key と name の登録とかいらない？

その処理は controllerManager のほうで行っているのですが、どうでしょう、それでいいですか？

---

Comments from Reviewable

[shundroid]

Review status: 0 of 22 files reviewed at latest revision, 16 unresolved discussions.

---

test/componentBase.test.js, line 27 at r2 (raw file):

Previously, dadaa wrote…

このテストの意味がちょっと良くわからないっすー

あ、、、
author が同じ場合は subscribe されたコールバックを呼ばないようにしたく、
それをテストしていたのですが、
これは publish 側のテストだったので、そっちに移動します。

---

test/dashboard.test.js, line 35 at r2 (raw file):

Previously, dadaa wrote…

100 通っちゃうテストですねｗこれから？

そうっすねｗｗ

dashboard.publishPingAll したら subscribe してた関数が呼ばれるかをチェックしたかったのですが、
こういうテストっていりますか？

---

test/virtualSpheroManager.test.js, line 9 at r2 (raw file):

Previously, dadaa wrote…

２つの assert に分けましょう

わかりました。

---

Comments from Reviewable

[shundroid]

Review status: 0 of 22 files reviewed at latest revision, 16 unresolved discussions.

---

test/componentBase.test.js, line 27 at r2 (raw file):

Previously, shundroid wrote…

あ、、、
author が同じ場合は subscribe されたコールバックを呼ばないようにしたく、
それをテストしていたのですが、
これは publish 側のテストだったので、そっちに移動します。

あ、いや、subscribe 側であってました

「コールバックが呼ばれなかったらOK」というテストをしたいんですけど、
どうやるのがいいでしょうか

---

Comments from Reviewable

[shundroid]

Review status: 0 of 25 files reviewed at latest revision, 16 unresolved discussions.

---

commandRunner.js, line 93 at r2 (raw file):

Previously, shundroid wrote…

わかりました。 config.js があって、そこでポートの調整などをしているので、
そこでできるように改善します。

@dadaa 今気づいたのですが、これって controller 側から指定された秒数を
ミリ秒に変換するのに使っている 1000 なのですが、
これ調整することってありますか？

---

Comments from Reviewable

[dadaa]

Review status: 0 of 33 files reviewed at latest revision, 16 unresolved discussions.

---

commandRunner.js, line 93 at r2 (raw file):

Previously, shundroid wrote…

@dadaa 今気づいたのですが、これって controller 側から指定された秒数を
ミリ秒に変換するのに使っている 1000 なのですが、
これ調整することってありますか？

逆にいうと、controller から渡す値を ms にするのが良いかと思います。

---

commandRunner.js, line 67 at r3 (raw file):

  stopLoop() {
    if (this.loopTimeoutId !== null) {

if (this.loopTimeoutId) {
これで問題無い気がしますー

---

commandRunner.js, line 76 at r3 (raw file):

    Object.keys(this.customTimeoutIds).forEach(timeoutIdName => {
      const timeoutId = this.customTimeoutIds[timeoutIdName];
      if (timeoutId !== null) {

同様です

---

Comments from Reviewable

[dadaa]

Review status: 0 of 33 files reviewed at latest revision, 12 unresolved discussions.

---

orbController.js, line 126 at r3 (raw file):

          this.publish("log", "connected orb.", "success");

          rawOrb.instance.configureCollisions({

この間のように、このコンフィグは結構キモになるので、これも config.js などに書いておきましょう。

---

Comments from Reviewable

[dadaa]

Review status: 0 of 33 files reviewed at latest revision, 9 unresolved discussions.

---

virtualSpheroManager.js, line 16 at r2 (raw file):

Previously, shundroid wrote…

その処理は controllerManager のほうで行っているのですが、どうでしょう、それでいいですか？

そしたら、ここはすべて name だけを扱うようにしたらいかがかしら？
removeSphero も引数は name で。

---

Comments from Reviewable

[dadaa]

Review status: 0 of 33 files reviewed at latest revision, 10 unresolved discussions.

---

model/uuidModel.js, line 12 at r3 (raw file):

    if (!appModel.isTestMode) {
 /*     noble.on("stateChange", state => {

このコメントは何かしら？
もしこれから実装するのであれば、console.warn とかで "not implemented yet" 的なメッセージを。

---

Comments from Reviewable

[dadaa]

Review status: 0 of 33 files reviewed at latest revision, 11 unresolved discussions.

---

test/commandRunner.test.js, line 7 at r3 (raw file):

  const commandRunner = new CommandRunner();
  describe("#stopLoop()", () => {
    const timeoutId = setTimeout(() => {}, 1000);

なるべく、1000 とかは定数化しておきたいです。

---

Comments from Reviewable

[dadaa]

Review status: 0 of 33 files reviewed at latest revision, 11 unresolved discussions.

---

test/componentBase.test.js, line 27 at r2 (raw file):

Previously, shundroid wrote…

あ、いや、subscribe 側であってました

「コールバックが呼ばれなかったらOK」というテストをしたいんですけど、
どうやるのがいいでしょうか

この下の assert までは、publisher.publish の前にしないと意味がない？

---

Comments from Reviewable

[dadaa]

Review status: 0 of 33 files reviewed at latest revision, 11 unresolved discussions.

---

test/dashboard.test.js, line 35 at r2 (raw file):

Previously, shundroid wrote…

そうっすねｗｗ

dashboard.publishPingAll したら subscribe してた関数が呼ばれるかをチェックしたかったのですが、
こういうテストっていりますか？

あー、そういうことか。しておきたいですね。

---

Comments from Reviewable

[shundroid]

Review status: 0 of 33 files reviewed at latest revision, 9 unresolved discussions.

---

commandRunner.js, line 93 at r2 (raw file):

Previously, dadaa wrote…

逆にいうと、controller から渡す値を ms にするのが良いかと思います。

あーー、そうします
そうすると、onigo-controller のほうも変えなければいけないので、
Issue にして、今度 controller 側と同時にやります

---

Comments from Reviewable

[shundroid]

Review status: 0 of 33 files reviewed at latest revision, 9 unresolved discussions.

---

virtualSpheroManager.js, line 16 at r2 (raw file):

Previously, dadaa wrote…

そしたら、ここはすべて name だけを扱うようにしたらいかがかしら？
removeSphero も引数は name で。

あー、ここは 名前が決まっていなかった client の名前が決まった段階で呼ばれるので、
key -> name にする必要があり、ここでは key は必要です。

それ以外のところは、name で受け取るようにしました。

また、 #54 で、
今後、sphero-websocket から socket.io にするので、
controller の管理がまた変わるかもしれません

---

Comments from Reviewable

[shundroid]

Review status: 0 of 33 files reviewed at latest revision, 9 unresolved discussions.

---

commandRunner.js, line 67 at r3 (raw file):

Previously, dadaa wrote…

if (this.loopTimeoutId) {
これで問題無い気がしますー

Done.

---

commandRunner.js, line 76 at r3 (raw file):

Previously, dadaa wrote…

同様です

Done.

---

orbController.js, line 126 at r3 (raw file):

Previously, dadaa wrote…

この間のように、このコンフィグは結構キモになるので、これも config.js などに書いておきましょう。

直しました

---

model/uuidModel.js, line 12 at r3 (raw file):

Previously, dadaa wrote…

このコメントは何かしら？
もしこれから実装するのであれば、console.warn とかで "not implemented yet" 的なメッセージを。

あー、ここ、
Travis CI などの noble 非対応の環境だと、
noble を import した段階ですぐにエラーが出てしまうので、
テストができるように、コメントアウトにしています。

どうすればいいと思いますか・・？
noble を import するタイミングをずらすのかな

---

test/componentBase.test.js, line 27 at r2 (raw file):

Previously, dadaa wrote…

この下の assert までは、publisher.publish の前にしないと意味がない？

あ、assert は publish の後でもおｋだと思います

---

test/dashboard.test.js, line 35 at r2 (raw file):

Previously, dadaa wrote…

あー、そういうことか。しておきたいですね。

author で比較するように直しました。

---

Comments from Reviewable

[shundroid]

Review status: 0 of 33 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

test/commandRunner.test.js, line 7 at r3 (raw file):

Previously, dadaa wrote…

なるべく、1000 とかは定数化しておきたいです。

Done.

---

test/componentBase.test.js, line 27 at r2 (raw file):

Previously, shundroid wrote…

あ、assert は publish の後でもおｋだと思います

Done.

---

Comments from Reviewable

[dadaa]

Review status: 0 of 33 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

config.js, line 20 at r4 (raw file):

  damage: 10,
  defaultColor: "white",
  collision: {

この collision のコンフィグだと、spark+ は当てはまらないけど、良いですか？

---

Comments from Reviewable

[dadaa]

Review status: 0 of 33 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

controllerManager.js, line 88 at r4 (raw file):

  }
  damage(orb) {
    Object.keys(this.controllerModel.controllers).forEach(controllerName => {

Object.keys は新しい配列を作っちゃうので for (let controllerName in this.controllerModel.controllers) { で回した方が有利かもです。
他にも似たようなところがあるので、そこも。

---

Comments from Reviewable

[dadaa]

Review status: 0 of 36 files reviewed at latest revision, 1 unresolved discussion.

---

controllerManager.js, line 71 at r6 (raw file):

      const controller = this.controllerModel.get(controllerName);
      if (this.appModel.gameState === "active" && !controller.isOni &&
          controller.client !== null &&

基本的に null チェックは
if (controller.client) で良いと思います。いっぱいあるけど。

---

Comments from Reviewable