-
Notifications
You must be signed in to change notification settings - Fork 0
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
非同期処理プログラムの提出 #3
base: main
Are you sure you want to change the base?
Conversation
03.asynchronous/c1_callback.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
順番を管理する必要があるならしょうがないですが、必要がないならファイル名に prefix を付けるのはやめたほうがよいと思います。c1 が何を指しているのかもよく分からないです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
承知しました!
03.asynchronous/c1_callback.js
Outdated
`CREATE TABLE books ( | ||
id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
title TEXT NOT NULL UNIQUE | ||
)`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
複数行のテンプレート文字列でインデントすると、以下のようにインデント分も空白として含まれてしまいますよ。
CREATE TABLE books (
id INTEGER PRIMARY KEY AUTOINCREMENT,
title TEXT NOT NULL UNIQUE
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
余計な空白を含ませないようにSQLを1行で書くように修正しました!
03.asynchronous/c1_callback.js
Outdated
bookTitles.forEach((title) => { | ||
db.run("INSERT INTO books (title) VALUES (?)", [title], () => { | ||
console.log(`ADD TITLE: ${title}`); | ||
|
||
db.get("SELECT * FROM books WHERE title = ?", [title], (_, book) => { | ||
console.log(`GET TITLE: ${book.title}, ID: ${book.id}`); | ||
index++; | ||
|
||
if (index >= bookTitles.length) { | ||
db.run("DROP TABLE books", () => { | ||
db.close(); | ||
}); | ||
} | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コールバックベースの非同期処理は、ループと組み合わせてその実行順を制御することはできないです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コメントありがとうございます。なるほど、これがコールバック形式の問題点なのですね!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ループ処理を除外しました!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ファイル名は名詞形になるようにしてください。文のように見えます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正しました!
03.asynchronous/c1_callback.js
Outdated
|
||
bookTitles.forEach((title) => { | ||
db.run("INSERT INTO books (title) VALUES (?)", [title], () => { | ||
console.log(`ADD TITLE: ${title}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
自動採番された ID を出力してください。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正しました!
|
||
bookTitles.forEach((title) => { | ||
db.run("INSERT INTO books (title) VALUES (?)", [title], () => { | ||
console.log(`ADD TITLE: ${title}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
エラーメッセージを標準エラー出力に出力してください。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
実装漏れ失礼しました🙇♂️
標準エラー出力の処理を追加しました。
console.log(`ADD TITLE: ${title}`); | ||
|
||
db.get("SELECT * FROM books WHERE title = ?", [title], (_, book) => { | ||
console.log(`GET TITLE: ${book.title}, ID: ${book.id}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
エラーメッセージを標準エラー出力に出力してください。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
実装漏れ失礼しました🙇♂️
標準エラー出力の処理を追加しました。
03.asynchronous/promise.js
Outdated
bookTitles.forEach((title) => { | ||
promise = promise | ||
.then(() => | ||
run(db, "INSERT INTO books (title) VALUES (?)", [title]).then( | ||
(result) => { | ||
console.log(`ADD TITLE: ${title}, ID: ${result.lastID}`); | ||
}, | ||
), | ||
) | ||
.then(() => | ||
get(db, "SELECT * FROM books WHERE title = ?", [title]).then( | ||
(book) => { | ||
console.log(`GET TITLE: ${book.title}, ID: ${book.id}`); | ||
}, | ||
), | ||
); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promise の実行順もループでは制御できませんよ。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ご指摘ありがとうございます。
- forEachのインデックスで起動順は保証されるが、待機しないので結果が返ってくる順序の制御できない
- 今回の実行結果は毎回同じ順序になるのですが、それは処理時間にばらつきがでない処理だからだった
と理解しました。齟齬あればご指摘お願い致します
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach
forEach()同期関数を想定しており、Promise を待機しません。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「制御できない」の具体的な中身はそうですが、今回に限ってはもっと簡単な話で、「前の処理が終わってから次の処理を行う必要がある」というだけのことです。
03.asynchronous/promise.js
Outdated
return promise; | ||
}) | ||
.then(() => run(db, "DROP TABLE books")) | ||
.then(() => db.close()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
close が Promise として扱えるようになっていないです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
今回のコードだと後続処理がないので問題はないかと思いますが、後続処理がある場合もよくあるという意図かと解釈しました。齟齬あればご指摘をお願い致します。
db.closeをPromise化するヘルパー関数 close を追加しました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
それはそうですが、コールバックの項目で使った node-sqlite3 の関数を Promise の形式で扱えるようにする、というのがそもそもの要件です。
03.asynchronous/sqlite_utils.js
Outdated
@@ -0,0 +1,23 @@ | |||
export const run = (db, sql, params = []) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
質問ですが、params
にデフォルト引数を設定しているのはなぜですか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
申し訳ありません。意図はなく確認漏れでした💦
不要なデフォルト値と思いますので削除しました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
意図なく設定していることはないと思うのですが...。なぜ設定していたのかと、なぜ削除していいのかをコメントしてほしいです。なぜ設定していたのかが本当にないのであれば、それはコメントしなくてもよいですが、それはそれで進め方がまずいと思います。自分で書いたコードのいずれの部分についても理由が説明できる状態でないといけません。でないと、コードの要不要が適切に判断できなくなってしまうし、コードを後から変更することになっても現状どうしてこのようなコードになっているのか・変更してよいのかどうかの判断がつかなくなってしまうからです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
書いたコードのすべてを説明できる状態ではなかったです。申し訳ございません🙇♂️
デフォルト引数は、以下のAPIガイドをみてrunメソッドの引数の挙動を確認していたときのコードの名残りでした。(デフォルト引数をいじる必要はなかったのですが..)
https://github.com/TryGhost/node-sqlite3/wiki/API#runsql--param---callback
削除理由は、動作確認結果が期待通りの出力だったためです。(FBC内の方へキャプチャは貼っています)
改めてAPIガイドを読み返したところ、以下のように書かれていました🤔
コールバックを3番目のパラメータとして保持したい場合は、問題#116に従って、パラメータを「[]」(空の配列)に設定する必要があります
リンク先のIssueのエラー は.then(() => run(db, "DROP TABLE books", null))
とすれば再現し、undefined
では発生しないことを確認できました。
今回の実装ではnull
がセットされることはありませんので、現状のままでよいと考えます。
もし、ヘルパー関数 run
に const safeParams = params === null ? [] : params;
を追記するような修正をした方がよければコメントお願い致します
03.asynchronous/promise.js
Outdated
.then(() => | ||
run(db, "INSERT INTO books (title) VALUES (?)", [bookTitle]).then( | ||
(result) => { | ||
console.log(`ADD TITLE: ${bookTitle}, ID: ${result.lastID}`); | ||
}, | ||
), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コールバックでネストが深くなる問題を解消するために Promise を導入しているのに、その Promise でネストが発生してしまっているのはおかしいですね。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ネストが深くならないように修正しました!
03.asynchronous/promise.js
Outdated
bookTitles.forEach((title) => { | ||
promise = promise | ||
.then(() => | ||
run(db, "INSERT INTO books (title) VALUES (?)", [title]).then( | ||
(result) => { | ||
console.log(`ADD TITLE: ${title}, ID: ${result.lastID}`); | ||
}, | ||
), | ||
) | ||
.then(() => | ||
get(db, "SELECT * FROM books WHERE title = ?", [title]).then( | ||
(book) => { | ||
console.log(`GET TITLE: ${book.title}, ID: ${book.id}`); | ||
}, | ||
), | ||
); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「制御できない」の具体的な中身はそうですが、今回に限ってはもっと簡単な話で、「前の処理が終わってから次の処理を行う必要がある」というだけのことです。
03.asynchronous/promise.js
Outdated
return promise; | ||
}) | ||
.then(() => run(db, "DROP TABLE books")) | ||
.then(() => db.close()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
それはそうですが、コールバックの項目で使った node-sqlite3 の関数を Promise の形式で扱えるようにする、というのがそもそもの要件です。
03.asynchronous/sqlite_utils.js
Outdated
@@ -0,0 +1,23 @@ | |||
export const run = (db, sql, params = []) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
意図なく設定していることはないと思うのですが...。なぜ設定していたのかと、なぜ削除していいのかをコメントしてほしいです。なぜ設定していたのかが本当にないのであれば、それはコメントしなくてもよいですが、それはそれで進め方がまずいと思います。自分で書いたコードのいずれの部分についても理由が説明できる状態でないといけません。でないと、コードの要不要が適切に判断できなくなってしまうし、コードを後から変更することになっても現状どうしてこのようなコードになっているのか・変更してよいのかどうかの判断がつかなくなってしまうからです。
03.asynchronous/promise.js
Outdated
.then((book) => { | ||
console.log(`GET TITLE: ${book.title}, ID: ${book.id}`); | ||
run(db, "DROP TABLE books"); | ||
}) | ||
.then(() => close(db)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この書き方では DROP TABLE ...
の実行を待たずに close の処理に進んでしまいます。なぜかは自分で考えてみてください。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ご指摘ありがとうございます。文法の理解があやふやでした💦
上記のように短縮形を使わない場合、return しないと run(db, "DROP ..
が戻すPromiseを次のthenに渡せていないから待機できてないと考えます。
return を追記しました!
https://developer.mozilla.org/ja/docs/Web/JavaScript/Guide/Using_promises#%E9%80%A3%E9%8E%96
コールバック関数から処理結果を返すのを忘れないでください。さもないと後続のコールバック関数からその処理結果を利用することができなくなります(アロー関数を使った () => x は () => { return x; } の短縮形です)
03.asynchronous/promise_error.js
Outdated
.then(() => { | ||
return get(db, "SELECT * FROM NotExistTable WHERE title = ?", [bookTitle]); | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここだけアロー関数の簡潔文体が使われていないです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正しました!
2点、質問させてください🙏
1.文体の使い分けについて、
MDN Web docs: アロー関数式をみたのですが使い分けには特に言及がなく..
console.log()
やconsole.error()
も1行なので簡潔文体で書けるかと思います。
戻り値がない式文の場合、どちらでもよく(簡潔文体orブロック文体)
戻り値がある式文の場合、簡潔文体にするという使い分けでしょうか?
2.promise_error.js の待機制御について
console.log()
やconsole.error()
はPromiseを返さない(undefined)なので、後続処理(SELECT、DROP)が待機せず実行されているかと思います。
が、INSERT → SELECT → DROP の実行順を制御できている(console...を待つ必要がない)から問題なしという理解でよいですか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.文体の使い分けについて、 MDN Web docs: アロー関数式をみたのですが使い分けには特に言及がなく..
console.log()
やconsole.error()
も1行なので簡潔文体で書けるかと思います。 戻り値がない式文の場合、どちらでもよく(簡潔文体orブロック文体) 戻り値がある式文の場合、簡潔文体にするという使い分けでしょうか?
違います。アロー関数の中身の処理が 1 行しかなく、かつその実行結果を return したい場合に簡潔文体を使います。アロー関数で return する必要がない場合はブロック文体を使います。理由は、アロー関数が戻り値を持つかどうかを明確にするためです。式文に戻り値があっても、アロー関数の戻り値としてそれを return しない可能性もあるのだから、式文に戻り値があるかどうかだけでは決められないです。
2.promise_error.js の待機制御について
console.log()
やconsole.error()
はPromiseを返さない(undefined)なので、後続処理(SELECT、DROP)が待機せず実行されているかと思います。 が、INSERT → SELECT → DROP の実行順を制御できている(console...を待つ必要がない)から問題なしという理解でよいですか?
console.log
や console.error
は非同期処理ではなく同期処理です。なので、質問自体が成り立っていないです。
03.asynchronous/async.js
Outdated
const bookTitles = ["booktitle_01", "booktitle_02", "booktitle_03"]; | ||
const db = new sqlite3.Database(":memory:"); | ||
|
||
const main = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async 関数を定義するのではなく Top-level await を使ってください。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
修正しました!
03.asynchronous/async.js
Outdated
await run(db, "DROP TABLE books"); | ||
await close(db); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
他の処理間には空行があるのにここだけないです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
承知しました!
03.asynchronous/async_error.js
Outdated
]); | ||
console.log(`ADD TITLE: ${title}, ID: ${result.lastID}`); | ||
} catch (err) { | ||
if (err.code === "SQLITE_CONSTRAINT") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err
にプロパティアクセスできない値が入っていると err.code
の部分でエラーが起きます。また、エラーでない以下のようなオブジェクトが捕捉された場合にもこの条件が満たされてしまいます。
try {
throw { code: "SQLITE_CONSTRAINT" }
} catch (...) {
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw { code: ...
を console.log の下に追加してcatch内でフックされること確認しました💦
(JSはErrorオブジェクト以外もthrowできるんですね..📝)
if文に&条件を追加しました!
- codeプロパティを持つこと >
"code" in err
- Errorオブジェクトであること > instanceof
for (const title of bookTitles) { | ||
const result = await run(db, "INSERT INTO books (title) VALUES (?)", [title]); | ||
console.log(`ADD TITLE: ${title}, ID: ${result.lastID}`); | ||
|
||
const book = await get(db, "SELECT * FROM books WHERE title = ?", [title]); | ||
console.log(`GET TITLE: ${book.title}, ID: ${book.id}`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コールバックや Promise のプログラムと処理内容が異なっています。前のプログラムを順に書き換えていく課題なので、処理内容はプログラム間で統一してください。
} catch (err) { | ||
if ( | ||
err instanceof Error && | ||
"code" in err && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
質問ですが、"code" in err
のチェックは何のために必要なんでしょうか?
03.asynchronous/promise_error.js
Outdated
.then(() => { | ||
return get(db, "SELECT * FROM NotExistTable WHERE title = ?", [bookTitle]); | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.文体の使い分けについて、 MDN Web docs: アロー関数式をみたのですが使い分けには特に言及がなく..
console.log()
やconsole.error()
も1行なので簡潔文体で書けるかと思います。 戻り値がない式文の場合、どちらでもよく(簡潔文体orブロック文体) 戻り値がある式文の場合、簡潔文体にするという使い分けでしょうか?
違います。アロー関数の中身の処理が 1 行しかなく、かつその実行結果を return したい場合に簡潔文体を使います。アロー関数で return する必要がない場合はブロック文体を使います。理由は、アロー関数が戻り値を持つかどうかを明確にするためです。式文に戻り値があっても、アロー関数の戻り値としてそれを return しない可能性もあるのだから、式文に戻り値があるかどうかだけでは決められないです。
2.promise_error.js の待機制御について
console.log()
やconsole.error()
はPromiseを返さない(undefined)なので、後続処理(SELECT、DROP)が待機せず実行されているかと思います。 が、INSERT → SELECT → DROP の実行順を制御できている(console...を待つ必要がない)から問題なしという理解でよいですか?
console.log
や console.error
は非同期処理ではなく同期処理です。なので、質問自体が成り立っていないです。
お手数ですがレビューよろしくお願い致します🙏