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

feat(ajaxapp): Promiseを活用する を追加 #116

Merged
merged 16 commits into from
Aug 7, 2016
Merged

Conversation

lacolaco
Copy link
Collaborator

#9

@lacolaco
Copy link
Collaborator Author

    })
    .then((userInfo) => {

が認められてないのか

@azu
Copy link
Collaborator

azu commented Jul 31, 2016

ESLint 3.2.0のバグ(間違って入ったBreaking Change)ですね
http://qiita.com/mysticatea/items/706f4bb024d7ceb78f1d#1801-indent
eslint/eslint#6795

### エラーハンドリング

このままではPromiseに置き換えた意味がないので、エラーハンドリングを行いましょう。
Promiseのコンテキスト内で発生したエラーは、`Promise.catch`を使って一箇所で受け取れます。
Copy link
Collaborator

Choose a reason for hiding this comment

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

Promise.catch

Promise#catch ? static methodっぽく見える

Copy link
Collaborator

Choose a reason for hiding this comment

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

#110
表記についてのIssue

@lacolaco
Copy link
Collaborator Author

lacolaco commented Jul 31, 2016

  • ボタンのクリックイベントで起動するエントリポイントと、Promiseを生成する関数を分ける

@lacolaco
Copy link
Collaborator Author

lacolaco commented Aug 6, 2016

@azu 指摘された点を直してみたので、もう一度レビューしてもらってもいいでしょうか

そこで、HTML文字列を組み立てる`createView`関数とHTMLを表示する`displayView`関数を作り、
処理を分割します
そこで、HTML文字列を組み立てる`createView`関数とHTMLを表示する`displayView`関数を作り、処理を分割します。
さらに、エントリポイントとして新しく`main`関数を作り、`getUserInfo`関数を呼び出すようにします
Copy link
Collaborator

Choose a reason for hiding this comment

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

やや唐突なので、「なぜエントリーポイントが必要なの?」って前触れか、後述する〜のためとかがあると良さそうな気がします。
(エントリーポイントって何?はさすがになくて大丈夫か)

@@ -143,14 +148,21 @@ function getUserInfo(userId) {
}
```

### Promiseチェーンへの置き換え
### thenチェーンへの置き換え
Copy link
Collaborator

@azu azu Aug 6, 2016

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

すごく微妙な感じですがPromiseチェーンのままでよかったっぽいですね…


Promiseチェーンを使って処理を分割する利点は、同期処理と非同期処理を区別せずに連鎖できることです。
ひとつの処理として書いてしまうと、非同期処理の結果を同期的に扱うことは難しくなります。
そういう場合に`then`で区切っておけば、前の処理が非同期処理であっても、その値はコールバック関数の引数として同期的に扱えます。
Copy link
Collaborator

@azu azu Aug 7, 2016

Choose a reason for hiding this comment

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

その値はコールバック関数の引数として同期的に扱えます。

この辺の同期処理と非同期処理って言葉が何か違和感がある。
実際には結局非同期で動いてるはずなので、"その値はコールバック関数の引数として同期的に扱えます"というのは何を言ってるのかがよくわからない感じがします。
これは"コードを見た目上直列に書くことができるので同期的に書いているかのように扱える" ってことだと思うけど、それは"同期的に扱える"なのかが違和感がある感じ。
(この部分が省かれてる or 非同期がまるで同期処理として動くような感じに読める違和感がある)

async/awaitとかならもっと直列的な感じだけど、実際に処理を待つ(await)ので、処理が同期的であるワケではない気がします。

"区別せずに"ってフレーズを既に使ってるので、それに寄せていくのがいい気がする。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

「同期処理のように書くことができる」っていう表現がよさそうですね

ひとつの処理として書いてしまうと、非同期処理の結果を同期的に扱うことは難しくなります
そういう場合に`then`で区切っておけば、前の処理が非同期処理であっても、その値はコールバック関数の引数として同期的に扱えます
ひとつの処理として書いてしまうと、非同期処理の結果を扱うためにコードは複雑になります
そういう場合に`then`で区切っておけば、前の処理が非同期処理であっても、値を引数として受け取るコールバック関数はまるで同期処理のように書くことができます
Copy link
Collaborator

@azu azu Aug 7, 2016

Choose a reason for hiding this comment

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

少し大げさな感じがするので、

- はまるで
+
or
+

ぐらいでいい気がします。

@azu azu mentioned this pull request Aug 7, 2016
4 tasks
そういう場合に`then`で区切っておけば、前の処理が非同期処理であっても、値を引数として受け取るコールバック関数はまるで同期処理のように書くことができます。
それぞれの関数はどんな型の値を受け取り、どんな型の値を返すかだけに注意すればよいので、処理が簡潔になりコードの見通しがよくなるでしょう。
一般に、同期的に書かれた処理を後から非同期処理へと変更することは、全体を書き換える必要があるため難しいです。
そのため、最初から処理を分けておき、それを`then`を使って繋ぐことで、変更に強いコードを書くことができます。
Copy link
Collaborator

@azu azu Aug 7, 2016

Choose a reason for hiding this comment

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

- それを`then`を使って繋ぐことで
+ 処理を`then`で繋ぐことで

とかかな。
代名詞を無くしたかった感じ

@azu
Copy link
Collaborator

azu commented Aug 7, 2016

LGTM. 後はページとして見た時に違和感ないかをチェックして直していくといいと思います。
読んでいく流れとしてバランスがいいかどうかという視点で見ていくとまた直したくなる感じはします。

@lacolaco
Copy link
Collaborator Author

lacolaco commented Aug 7, 2016

レビューありがとうございました。すごく助かりました。

@lacolaco lacolaco merged commit 5e0bcc5 into master Aug 7, 2016
@lacolaco lacolaco deleted the ajaxapp-promise branch August 7, 2016 13:37
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.

None yet

2 participants