shundroid commented 7 years ago

This change is

dadaa commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

shundroid commented 7 years ago

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

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

Previously, dadaa wrote…

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

@dadaa すみません、古いコンフィグっていうことですか？

Comments from Reviewable

shundroid commented 7 years ago

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):

Previously, dadaa wrote…

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

へえ～～～、そうなんですね知らなかった・・・、そうします

Comments from Reviewable

dadaa commented 7 years ago

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

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

Previously, shundroid wrote…

@dadaa すみません、古いコンフィグっていうことですか？

そうですね。これって何のコンフィグですか？

Comments from Reviewable

shundroid commented 7 years ago

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

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

Previously, dadaa wrote…

そうですね。これって何のコンフィグですか？

@dadaa configureCollisions の引数で渡すコンフィグです

新しいコンフィグを見つけたので修正します

Comments from Reviewable

dadaa commented 7 years ago

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

dashboard.js, line 78 at r4 (raw file):

      console.log("a dashboard connected.");
      this.socket = socket;
      this.log("accepted a dashboard.", "success");

this.log と console.log の使い分けってなんですっけ？

Comments from Reviewable

dadaa commented 7 years ago

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

orbController.js, line 25 at r4 (raw file):

  addOrbToModel(name, orb) {
    if (this.orbModel.has(name)) {
      throw new Error(`追加しようとしたOrbは既に存在します。 : ${name}`);

こういうメッセージも、いまは日本語だけなので良いけど、おそらく本来はロケールを見て変更する必要があるので、そのような仕組みを組めたら良いですね。少なくとも、メッセージは別クラスなりでまとめておいた方が良いような気がします。

orbController.js, line 117 at r4 (raw file):

          rawOrb.instance.setInactivityTimeout(9999999, function(err, data) {
            if (err) {
              console.error(err);

ここで return する必要は無いですか？

Comments from Reviewable

shundroid commented 7 years ago

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

orbController.js, line 25 at r4 (raw file):

Previously, dadaa wrote…

こういうメッセージも、いまは日本語だけなので良いけど、おそらく本来はロケールを見て変更する必要があるので、そのような仕組みを組めたら良いですね。少なくとも、メッセージは別クラスなりでまとめておいた方が良いような気がします。

そうします

Comments from Reviewable

shundroid commented 7 years ago

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

orbController.js, line 117 at r4 (raw file):

Previously, dadaa wrote…

ここで return する必要は無いですか？

@dadaa ありますｗ

あと、 throw new Error("..."); と、 console.error("..."); を使う場所があるのですが、どっちのそろえたほうがいいですか？

test 的には、assert.throws でチェックできる throw のほうが便利なのかな

Comments from Reviewable

shundroid commented 7 years ago

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

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

Previously, shundroid wrote…

へえ～～～、そうなんですね知らなかった・・・、そうします

直しました

Comments from Reviewable

shundroid commented 7 years ago

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

dashboard.js, line 78 at r4 (raw file):

Previously, dadaa wrote…

this.log と console.log の使い分けってなんですっけ？

@dadaa this.log は dashboard の client 側に送る log で、 console.log は送らない log です

Comments from Reviewable

dadaa commented 7 years ago

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

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

Previously, shundroid wrote…

@dadaa `configureCollisions` の引数で渡すコンフィグです新しいコンフィグを見つけたので修正します

　うん、あと、このコンフィグの設定方法は、どっかにドキュメントとして残したいですね。spark 使う場合はこうしてくださいとか。

orbController.js, line 117 at r4 (raw file):

Previously, shundroid wrote…

@dadaa ありますｗあと、 `throw new Error("...");` と、 `console.error("...");` を使う場所があるのですが、どっちのそろえたほうがいいですか？ test 的には、assert.throws でチェックできる `throw` のほうが便利なのかな

そうですね、throw で表面まで持ってこれたら良いと思います。

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

Previously, shundroid wrote…

あー、ここは名前が決まっていなかった client の名前が決まった段階で呼ばれるので、 key -> name にする必要があり、ここでは key は必要です。それ以外のところは、name で受け取るようにしました。また、 #54 で、今後、sphero-websocket から socket.io にするので、 controller の管理がまた変わるかもしれません

key と name の紐付けはまだ未実装ということでいいのかな？

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

Previously, shundroid wrote…

あー、ここ、 Travis CI などの noble 非対応の環境だと、 noble を import した段階ですぐにエラーが出てしまうので、テストができるように、コメントアウトにしています。どうすればいいと思いますか・・？ noble を import するタイミングをずらすのかな

まだ調べてないけど Travis CI の方で設定できないのかしら。

test/dashboard.test.js, line 41 at r5 (raw file):

    });

    publisher.subscribe("gameState", (author, state) => {

ここのテストは分けた方が良いかと思います。少なくともテスト内で、ロジックに関わるところはなるべく書かない方が良いです。e.g. registeredListener = listener; とか

Comments from Reviewable

shundroid commented 7 years ago

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

orbController.js, line 117 at r4 (raw file):

Previously, dadaa wrote…

そうですね、throw で表面まで持ってこれたら良いと思います。

わかりました、そうします

Comments from Reviewable

shundroid commented 7 years ago

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

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

Previously, dadaa wrote…

　うん、あと、このコンフィグの設定方法は、どっかにドキュメントとして残したいですね。spark 使う場合はこうしてくださいとか。

62

Comments from Reviewable

shundroid commented 7 years ago

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

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

Previously, dadaa wrote…

key と name の紐付けはまだ未実装ということでいいのかな？

いや、

最初 controller が connect されたときは、 key だけが入ります

そのあと、ユーザーによって key に紐づけられる name がきまって、その決まった段階で publish されるのが named っていう subject ですだから、named では、 key と name の両方を引数で受け取ります

Comments from Reviewable

shundroid commented 7 years ago

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

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

Previously, dadaa wrote…

まだ調べてないけど Travis CI の方で設定できないのかしら。

あー、できそうですね。

noble を optional package にしたらできるかなあ

Comments from Reviewable

dadaa commented 7 years ago

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

dashboard.js, line 78 at r4 (raw file):

Previously, shundroid wrote…

@dadaa this.log は dashboard の client 側に送る log で、 console.log は送らない log です

なるほど、そしたら this.log の方、asClientMessage とか少しわかりやすい名前にしておきたいですね。

Comments from Reviewable

shundroid commented 7 years ago

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

dashboard.js, line 78 at r4 (raw file):

Previously, dadaa wrote…

なるほど、そしたら this.log の方、asClientMessage とか少しわかりやすい名前にしておきたいですね。

わかりました、そうします

Comments from Reviewable

dadaa commented 7 years ago

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

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

this.virtualSphero.addSphero(name); 処理がこれしか書いてないので、key はどのように処理されているのかよくわからなかったのです。実質これで良いとするならば引数に key は要らないように思えます。

Comments from Reviewable

dadaa commented 7 years ago

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

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

Previously, shundroid wrote…

あー、できそうですね。 noble を optional package にしたらできるかなあ

少なくともコメントにしておくのは避けたいですね。clone して、それからコメント外してください、と言うのはちょっとわかりづらいです。

Comments from Reviewable

shundroid commented 7 years ago

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

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

Previously, dadaa wrote…

> this.virtualSphero.addSphero(name); 処理がこれしか書いてないので、key はどのように処理されているのかよくわからなかったのです。実質これで良いとするならば引数に key は要らないように思えます。

@dadaa

いったん間に関数をはさんだほうがわかりやすいですかね onNamed とかで name だけ渡すようにするのかな

Comments from Reviewable

shundroid commented 7 years ago

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

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

Previously, dadaa wrote…

少なくともコメントにしておくのは避けたいですね。clone して、それからコメント外してください、と言うのはちょっとわかりづらいです。

アプリのオプションで、 isTestMode と同様、 noble を使うかの isUseNoble みたいなの作って、そこで分岐させます

Comments from Reviewable

dadaa commented 7 years ago

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

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

Previously, shundroid wrote…

@dadaa いったん間に関数をはさんだほうがわかりやすいですかね onNamed とかで name だけ渡すようにするのかな

key と name のマッピングをするクラスは一つにしておきたいけど、それを model がやってるのであれば、model だけそこを意識して、他には波及しないのが良いかと思います。

Comments from Reviewable

shundroid commented 7 years ago

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

test/dashboard.test.js, line 41 at r5 (raw file):

Previously, dadaa wrote…

ここのテストは分けた方が良いかと思います。少なくともテスト内で、ロジックに関わるところはなるべく書かない方が良いです。e.g. `registeredListener = listener;` とか

わかりました、そうします

Comments from Reviewable

shundroid commented 7 years ago

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

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

Previously, dadaa wrote…

key と name のマッピングをするクラスは一つにしておきたいけど、それを model がやってるのであれば、model だけそこを意識して、他には波及しないのが良いかと思います。

わかりました、そうします

Comments from Reviewable

shundroid commented 7 years ago

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

test/dashboard.test.js, line 41 at r5 (raw file):

Previously, shundroid wrote…

わかりました、そうします

@dadaa そのままだと分割しにくかったので、変えました。

https://semaphoreci.com/community/tutorials/getting-started-with-node-js-and-mocha

あと、it の中で処理をしている人が多かったので、 it の中で処理をするようにして、beforeEach とか before とかを使うようにしたのですが、どうでしょうか

Comments from Reviewable

shundroid commented 7 years ago

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

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

Previously, shundroid wrote…

わかりました、そうします

Done.

Comments from Reviewable

shundroid commented 7 years ago

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

dashboard.js, line 78 at r4 (raw file):

Previously, shundroid wrote…

わかりました、そうします

Done.

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

Previously, shundroid wrote…

アプリのオプションで、 `isTestMode` と同様、 noble を使うかの `isUseNoble` みたいなの作って、そこで分岐させます

Done.

Comments from Reviewable

shundroid commented 7 years ago

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

orbController.js, line 117 at r4 (raw file):

Previously, shundroid wrote…

わかりました、そうします

Done.

Comments from Reviewable

dadaa commented 7 years ago

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

comozilla / onigo-server

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

62