Skip to content

Conversation

@taichihub
Copy link
Owner

@taichihub taichihub commented Sep 10, 2024

プラクティス

クラス(JavaScript)

実行結果

メモ追加(TTY)

chiroru@MacBook-Pro-4 04.class % ./memo.js   
メモの内容を入力してください(保存するにはCtrl+Dを押してください):
これはTTYで追加したメモです
メモを追加しました。

メモ追加中にSIGINT(Ctrl+C)の入力時

chiroru@MacBook-Pro-4 04.class % ./memo.js
メモの内容を入力してください(保存するにはCtrl+Dを押してください):
Ctrl+Cが入力された為メモの作成を中止しました

メモ追加(NotTTY)

chiroru@MacBook-Pro-4 04.class % echo アイウエオ | ./memo.js
メモを追加しました。

空文字のみ/改行文字のみ/空文字&改行文字のみの場合

chiroru@MacBook-Pro-4 04.class % echo | ./memo.js
メモの内容が空です。

メモ一覧

メモが存在する場合

chiroru@MacBook-Pro-4 04.class % ./memo.js -l
メモ一覧:
・これはNotTTYで追加したメモです
・これはTTYで追加したメモです

メモが存在しない場合

chiroru@MacBook-Pro-4 04.class % ./memo.js -l
メモが存在しません。

メモ参照

メモが存在する場合

メモ選択画面
chiroru@MacBook-Pro-4 04.class % ./memo.js -r                                   
? 表示するメモを選んでください: (Use arrow keys)
❯ これはNotTTYで追加したメモです
メモ選択画面中にSIGINT(Ctrl+F)またはEOF(Ctrl+D)の入力時
chiroru@MacBook-Pro-4 04.class % ./memo.js -r                                   
? 表示するメモを選んでください: (Use arrow keys)
❯ これはNotTTYで追加したメモです
Ctrl+C/Dが入力された為処理を終了させました% 
メモ選択後画面
chiroru@MacBook-Pro-4 04.class % ./memo.js -r                                   
✔ 表示するメモを選んでください: これはNotTTYで追加したメモです
これはNotTTYで追加したメモです% 

メモが存在しない場合

chiroru@MacBook-Pro-4 04.class % ./memo.js -r
メモが存在しません。

メモ削除

メモが存在する場合

メモ選択画面
chiroru@MacBook-Pro-4 04.class % ./memo.js -d
? 削除するメモを選んでください: (Use arrow keys)
❯ これはNotTTYで追加したメモです
メモ選択画面中にSIGINT(Ctrl+F)またはEOF(Ctrl+D)の入力時
chiroru@MacBook-Pro-4 04.class % ./memo.js -d
? 削除するメモを選んでください: (Use arrow keys)
❯ これはNotTTYで追加したメモです
Ctrl+C/Dが入力された為処理を終了させました% 
メモ選択後画面
chiroru@MacBook-Pro-4 04.class % ./memo.js -d
✔ 削除するメモを選んでください: これはNotTTYで追加したメモです
メモを削除しました。

ESlint実行

image

Prettier実行

image

クラス図

classDiagram
    class MemoDatabase {
        - database
        - databasePath
        + constructor(databasePath)
        + connect()
        + createTable()
        + insert(content)
        + fetchAll()
        + delete(id)
        - openDatabase(path)
        - createMemosTable()
    }

    class Settings {
        <<Module>>
        + DATABASE_PATH: String
        + OPTIONS: Object
    }

    class AddMemo {
        <<Function>>
        + addMemo(database)
        - getInputLines(input)
    }

    class ListMemos {
        <<Function>>
        + listMemos(database)
    }

    class ReadMemo {
        <<Function>>
        + readMemo(database)
    }

    class DeleteMemo {
        <<Function>>
        + deleteMemo(database)
    }

    class MemoHelpers {
        <<Module>>
        + selectMemo(database, message)
    }

    class Main {
        <<Script>>
        + main()
    }

    MemoDatabase o-- AddMemo : uses
    MemoDatabase o-- ListMemos : uses
    MemoDatabase o-- ReadMemo : uses
    MemoDatabase o-- DeleteMemo : uses

    Main o-- MemoDatabase : instantiates
    Main o-- Settings : uses
    Main o-- AddMemo : invokes
    Main o-- ListMemos : invokes
    Main o-- ReadMemo : invokes
    Main o-- DeleteMemo : invokes

    AddMemo o-- MemoHelpers : uses
    DeleteMemo o-- MemoHelpers : uses
Loading
classDiagram
    class MemoDatabase {
        - database
        - databasePath
        + constructor(databasePath)
        + connect()
        + createTable()
        + insert(content)
        + fetchAll()
        + delete(id)
        - openDatabase(path)
        - createMemosTable()
    }

    class Settings {
        <<Module>>
        + DATABASE_PATH: String
        + OPTIONS: Object
    }

    class AddMemo {
        <<Function>>
        + addMemo(database)
        - getInputLines(input)
    }

    class ListMemos {
        <<Function>>
        + listMemos(database)
    }

    class ReadMemo {
        <<Function>>
        + readMemo(database)
    }

    class DeleteMemo {
        <<Function>>
        + deleteMemo(database)
    }

    class MemoHelpers {
        <<Module>>
        + selectMemo(database, message)
    }

    class Main {
        <<Script>>
        + main()
    }

    MemoDatabase o-- AddMemo : uses
    MemoDatabase o-- ListMemos : uses
    MemoDatabase o-- ReadMemo : uses
    MemoDatabase o-- DeleteMemo : uses

    Main o-- MemoDatabase : instantiates
    Main o-- Settings : uses
    Main o-- AddMemo : invokes
    Main o-- ListMemos : invokes
    Main o-- ReadMemo : invokes
    Main o-- DeleteMemo : invokes

    AddMemo o-- MemoHelpers : uses
    DeleteMemo o-- MemoHelpers : uses

使用方法

ローカル環境にクローンする

git clone [email protected]:taichihub/js-practices.git

当ブランチに移動

git checkout 04-my-class

メモアプリディレクトリに移動

cd 04.class

パッケージインストール

npm install

パッケージを最新状態にアップデート

npm update

実行

実行コマンドは実行結果欄の各種コマンドをご覧ください。

@taichihub taichihub changed the title 20240909 クラス(JavaScript) 20240910 クラス(JavaScript) Sep 11, 2024
@taichihub taichihub changed the base branch from 03-my-asynchronous to main October 4, 2024 09:25
"type": "module"
"type": "module",
"dependencies": {
"inquirer": "^10.2.2",

Choose a reason for hiding this comment

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

最新のバージョンを使ってください。そうできない理由がああるのであれば教えてください。

Copy link
Owner Author

@taichihub taichihub Oct 24, 2024

Choose a reason for hiding this comment

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

@cafedomancer

最新のバージョンを使ってください。そうできない理由がああるのであれば教えてください。

特に理由があるわけでは無いので、最新バージョンに修正しました。
npm outdatedコマンドで現在使用しているパッケージ情報を確認したところ、12.0.0が最新ということが分かったので、12.0.0にupdateしました。

chiroru@MacBook-Pro-4 04.class % npm outdated     

Package     Current   Wanted   Latest  Location                 Depended by
@eslint/js    9.9.0   9.13.0   9.13.0  node_modules/@eslint/js  04.class
eslint        9.9.0   9.13.0   9.13.0  node_modules/eslint      04.class
globals      15.9.0  15.11.0  15.11.0  node_modules/globals     04.class
inquirer     10.2.2   10.2.2   12.0.0  node_modules/inquirer    04.class

npm updateコマンドを叩いた後、もう一度npm outdatedコマンドを叩いたところ何も出力されなくなったので、これで現在使用しているパッケージ全てで最新バージョンが使用されていることが確認できました。

chiroru@MacBook-Pro-4 04.class % npm outdated     

chiroru@MacBook-Pro-4 04.class % 

"type": "module",
"dependencies": {
"inquirer": "^10.2.2",
"readline-sync": "^1.4.10",
Copy link

@cafedomancer cafedomancer Oct 7, 2024

Choose a reason for hiding this comment

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

この NPM パッケージはどこでも import されていませんよ。また、ノンブロッキングであるという点で非同期処理のほうが好ましいので、この NPM パッケージは使うべきでないと思います。また、このパッケージのリポジトリはすでにアーカイブされており、最後のリリースが 5 年前であるという点でも使うのを避けたほうがよいです。リポジトリの README にも以下のように書いてありますよ。

Now, ECMAScript supports Promise, await and async.

https://github.com/anseki/readline-sync

Copy link
Owner Author

@taichihub taichihub Oct 24, 2024

Choose a reason for hiding this comment

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

@cafedomancer

この NPM パッケージはどこでも import されていませんよ。......... また、このパッケージのリポジトリはすでにアーカイブされており、最後のリリースが 5 年前であるという点でも使うのを避けたほうがよいです。リポジトリの README にも以下のように書いてありますよ。

以後、パッケージを使用する際はそのパッケージのリポジトリを確認し、そのパッケージの更新状況やREADMEに書かれてある現状を読んでから使用するようにします。

こちらについては、package.jsonからreadline-syncのパッケージを削除し、npm updateコマンドを叩くことで修正対応しました。


const database = new Database();
const memoApp = new MemoApp(database);
const args = process.argv.slice(2);

Choose a reason for hiding this comment

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

理解度を確認するために slice(2) を実行している理由を教えてください。また、カレンダーの提出物では、コマンドライン引数を処理するためにパッケージが使われていたはずですが、ここではそうしていない理由を教えてください。

Copy link
Owner Author

@taichihub taichihub Oct 24, 2024

Choose a reason for hiding this comment

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

@cafedomancer
カレンダーのプラクティスでは、年と月の指定オプションで最大4要素含まれる仕様でしたが、今回のメモアプリはオプションが-l-r-dのように1つで固定だったため、sliceメソッドでオプション以外の部分を削ることで簡単にオプションだけを抜き取ろうとしました。

こちら、カレンダーのプラクティスと同様にオプション周りの実装はcommanderを使用した方が良いでしょうか?

Comment on lines 2 to 7
export const CREATE_TABLE_MEMOS = `
CREATE TABLE IF NOT EXISTS memos (
id INTEGER PRIMARY KEY AUTOINCREMENT,
memo TEXT
)
`;

Choose a reason for hiding this comment

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

テンプレート文字列を使って複数行の文字列を定義し、かつそれをインデントすると、インデント分の余分な空白が先頭に挿入されてしまいます。SQL に余分な空白が含まれていて実行上の問題はないものの、不要なものなので好ましくはないです。

Copy link
Owner Author

@taichihub taichihub Oct 24, 2024

Choose a reason for hiding this comment

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

@cafedomancer
全てのSQL文を1行にまとめて余分な空白が含まれないように修正しました。

export const CREATE_TABLE_MEMOS = `CREATE TABLE IF NOT EXISTS memos (id INTEGER PRIMARY KEY AUTOINCREMENT, memo TEXT)`;
.......

@@ -0,0 +1,27 @@
// メモのテーブルを作成するクエリ

Choose a reason for hiding this comment

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

コードを読めば分かることをコメントとして残すのはやめましょう。コードやコメントの変更にともなって両者の乖離が発生すると、どちらの内容が正しいのか分からなくなってしまうからです。コードからは分からないことのみ、コメントとして残すようにしてください。

Copy link
Owner Author

@taichihub taichihub Oct 24, 2024

Choose a reason for hiding this comment

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

@cafedomancer
今回使用するSQL文においては全て読めばわかる範囲のものだと判断しましたので、コメントアウトについては全て削除しました。

export const CREATE_TABLE_MEMOS = `CREATE TABLE IF NOT EXISTS memos (id INTEGER PRIMARY KEY AUTOINCREMENT, memo TEXT)`;
export const INSERT_MEMO = `INSERT INTO memos (memo) VALUES (?)`;
export const SELECT_ALL_MEMOS = `SELECT id, memo FROM memos`;
export const SELECT_MEMO_BY_ID = `SELECT * FROM memos WHERE id = ?`;
export const DELETE_MEMO_BY_ID = `DELETE FROM memos WHERE id = ?`;

コードを読めば分かることをコメントとして残すのはやめましょう。

コードからは分からないことだったり、どうしても可読性が低くなってしまう場合や視認性が低いコードを書かなければいけない場合にのみ使用していきます。

Comment on lines 2 to 9
export function logMessage(message) {
process.stdout.write(message);
}

// ログ出力関数(エラー出力)
export function logError(message) {
process.stderr.write(message);
}

Choose a reason for hiding this comment

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

console.logconsole.error をそのまま使えばよいだけな気がしたので、これらの関数が何のために定義されているのか分かりませんでした。理由を教えてください。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
ただただprocess.stderr.write(message);が何回も出てくると煩わしいかな、と思いからこのような行いました。

Comment on lines 11 to 17
export function handleError(err, logMessage) {
if (err) {
logError(`${logMessage}${err.message}\n`);
return true;
}
return false;
}

Choose a reason for hiding this comment

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

非同期処理のプラクティスで例外処理について触れているのだから、エラーは boolean ではなく例外として取り扱うべきでしょう。handleError という関数名から boolean の戻り値を持つことも推測できないです。

Copy link
Owner Author

@taichihub taichihub Oct 25, 2024

Choose a reason for hiding this comment

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

@cafedomancer
以下2点の理由からhandleError関数を無くし、元々handleError関数が呼び出されていた箇所についてはtry/catchで例外処理を実装したり、エラーメッセージを出力させてreturnするように修正しました。

  • 現状handleError関数でエラーをbooleanで扱っていることがまず不適切
  • この関数を呼び出しても、それが例外処理をしているかどうかは呼び出す側では読み取りづらい

また、runメソッド・allメソッド・getメソッドについてはコールバックベースのメソッドなのでPromiseでラップしawaitを使用してPromiseに基づく非同期実装をする形に修正いたしました。

Choose a reason for hiding this comment

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

ファイル名とクラス名を一致させてください。他にも処理が定義されているのなら appManager.js という名前でよい可能性もありますが、現状は MemoApp クラスしか定義されていないからです。

Copy link
Owner Author

@taichihub taichihub Oct 25, 2024

Choose a reason for hiding this comment

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

@cafedomancer

修正内容

ファイル名をappManager.jsからmemoApp.jsに変更しました。

質問

  • 他ファイルの命名をキャメルケースで行なっているのでmemoApp.jsにしたのですが、クラス名と全く同じMemoAppのようにパスカルケースにした方が良いでしょうか?
  • また、JavaScriptを用いての開発においてファイル名・変数名・クラス名の命名ケースのデファクトスタンダードを教えていただきたいです。

Comment on lines 7 to 25
constructor(database) {
this.db = database.getDb();
}

addMemo() {
addMemo(this.db);
}

listMemos() {
listMemos(this.db);
}

readMemo() {
readMemo(this.db);
}

deleteMemo() {
deleteMemo(this.db);
}

Choose a reason for hiding this comment

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

これらのコードが Database クラスのインスタンスに依存しているのならまだしも、Database クラスのフィールドに依存しているのはカプセル化の原則に反していると思います。db フィールドの API にもし変更があれば、これらの処理はすぐに壊れてしまいます。

Copy link
Owner Author

@taichihub taichihub Oct 25, 2024

Choose a reason for hiding this comment

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

@cafedomancer
こちら、どのような修正を依頼されているのかを正確に汲み取ることができなかったのですが以下のように推測しました。

指摘内容の推測

  • memoApp.js(修正前はappManager.js。以後memoApp.jsと記述する)で無駄なカプセル化・クラス化を行なっている。
  • memoApp.jsの責務がないに等しく、存在している理由が無い。

自分が推測した上記2点の理由から、以下のような対応を行いました。

推測をもとに行った修正

  • memoApp.jsで行なっていた処理をmemo.jsに移行した。
    • addMemo``listMemos``readMemo``deleteMemoをinport
    • DBInstanceMemoDatabaseのインスタンスを格納する
    • connectメソッドでSQLiteデータベースとメモテーブルの作成を行う
    • ゲッターメソッドdatabaseConnectionの実行でconnectメソッドの処理で作成されたデータベースを取得。それをdatabseに格納
    • listMemos(database);のように、それぞれの機能で作成したメソッドの引数にdatabaseを渡して実行

memo.js

#!/usr/bin/env node

import { MemoDatabase } from "./memoApp/db/database.js";
import { addMemo } from "./memoApp/actions/addMemo.js";
import { listMemos } from "./memoApp/actions/listMemos.js";
import { readMemo } from "./memoApp/actions/readMemo.js";
import { deleteMemo } from "./memoApp/actions/deleteMemo.js";
import { OPTIONS } from "./memoApp/config/settings.js";

const DBInstance = new MemoDatabase();
await DBInstance.connect();
const database = DBInstance.databaseConnection;
const args = process.argv.slice(2);

switch (args[0]) {
  case OPTIONS.LIST:
    listMemos(database);
    break;
  case OPTIONS.READ:
    readMemo(database);
    break;
  case OPTIONS.DELETE:
    deleteMemo(database);
    break;
  default:
    addMemo(database);
    break;
}

そもそもなぜ無駄なカプセル化を行ったのか?

現状はメモ作成・メモリスト表示・各メモ内容表示・メモ削除の4つの機能だが、もし今以上に機能が増えることを仮定すると、このメモアプリがどのような機能で構成されているものなのかが見えずらいと感じ、MemoAppというクラスを作成してその中に各機能ごとに関数を呼び出して「メモアプリ内の機能の一覧化」を行いたかったということになります。

質問・依頼

  • このフィードバックに対する修正は自分の推測をもとに行ったので、求められている修正かどうかがわからないのでまずはその点の評価を行なっていただきたいです。
  • また、db フィールドの API にもし変更があれば、これらの処理はすぐに壊れてしまいます。とコメントしていただいたと思うのですが、なぜdb フィールドの API にもし変更があれば、これらの処理はすぐに壊れてしま雨のかが検討もつかない状態なので、根本の部分から解説していただきたいです。

Comment on lines 1 to 25
import { addMemo } from "./addMemo.js";
import { listMemos } from "./listMemos.js";
import { readMemo } from "./readMemo.js";
import { deleteMemo } from "./deleteMemo.js";

export class MemoApp {
constructor(database) {
this.db = database.getDb();
}

addMemo() {
addMemo(this.db);
}

listMemos() {
listMemos(this.db);
}

readMemo() {
readMemo(this.db);
}

deleteMemo() {
deleteMemo(this.db);
}

Choose a reason for hiding this comment

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

import している関数に処理を委譲するような形で実装しているのはなぜでしょう? 理由を教えてください。また、これだとクラスから単に他の関数を呼び出しているだけで、オブジェクトが状態を持っていないので、クラスを定義する意味がないですね。

Copy link
Owner Author

@taichihub taichihub Oct 25, 2024

Choose a reason for hiding this comment

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

@cafedomancer
以下のコメントでこちらのフィードバックに対する返信も一緒にしてますのでご確認よろしくお願いいたします🙇‍♂️
#5 (comment)

@taichihub
Copy link
Owner Author

taichihub commented Dec 18, 2024

@cafedomancer
下記はフィヨルドの提出物ページでご指摘いただいた内容についての対応です。

config配下のファイルについて

config/ 配下に諸々を置くのをやめてください。出力しているメッセージが出力の処理を実際に行っている箇所を見ても分からず、config/ 配下のファイルをいちいち見に行かなければならず、非常に分かりにくいです。SQL についても同様です。

ログ・SQLについてもconfig配下のファイル内で定義してそれを呼び出す形で使用していましたが、そのようにせず直接ファイル内に記述することで対応しました。

process.exitについて

あちこちで process.exit() するのをやめてください。プログラム全体の処理の流れの見通しが悪くなるからです。どこでプログラムが終了するのか分からなくなります。

SIGINT/EOF周りの処理以外の部分でprocess.exitを削除しました。
SIGINT/EOFが入力後は処理の中止が第一優先なのでprocess.exitを残しました。

@@ -0,0 +1,7 @@
export const DATABASE_PATH = "db/memo.db";
export const FILE_ENCODING = "utf-8";

Choose a reason for hiding this comment

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

使われていない設定が残っています。

Copy link
Owner Author

@taichihub taichihub Dec 30, 2024

Choose a reason for hiding this comment

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

@cafedomancer
FILE_ENCODING を削除しました。

#database;
#databasePath;

constructor({ databasePath = "db/memo.db" }) {

Choose a reason for hiding this comment

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

引数が一つしかないなら区別できる必要もないのでオブジェクトではなく普通の引数として受け取ればよい気がします。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
オブジェクト引数ではなく通常の引数を使用するよにしました。

  constructor(databasePath = "db/memo.db") {
    this.#databasePath = databasePath;
  }
async function main() {
  const memoDatabase = new MemoDatabase(DATABASE_PATH);

Comment on lines 15 to 28
switch (err.code) {
case "SQLITE_CANTOPEN":
console.error(
`データベースを開けません: (databasePath)${this.#databasePath}, (エラーメッセージ)${err.message}`,
);
break;
case "SQLITE_NOTADB":
console.error(
`データベースファイルではありません: (databasePath)${this.#databasePath}, (エラーメッセージ)${err.message}`,
);
break;
default:
console.error(`データベース接続エラー(openDatabase): ${err.message}`);
}

Choose a reason for hiding this comment

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

このクラスの役割はデータベースとやり取りすることであり、エラーがあったときにどんなメッセージを出力するかやそもそもメッセージを出力するかどうかを決めるのはこのクラスを使う側です。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
エラーメッセージ出力の責務をmemoDatabase.jsに移しました。

memoDatabase.jsのconnect関数

  async connect() {
    try {
      this.#database = await this.#openDatabase(this.#databasePath);
    } catch (err) {
      if (err.code === "SQLITE_CANTOPEN" || err.code === "SQLITE_NOTADB") {
        throw err;
      }
    }

    try {
      await this.#createMemosTable();
    } catch (err) {
      if (err.code === "SQLITE_ERROR") {
        throw err;
      }
    }
  }

memo.jsのmain関数の一部

  try {
    await memoDatabase.connect();
  } catch (err) {
    switch (err.code) {
      case "SQLITE_CANTOPEN":
        console.error(`データベースを開けません: ${err.message}`);
        break;
      case "SQLITE_NOTADB":
        console.error(`データベースファイルではありません: ${err.message}`);
        break;
      case "SQLITE_ERROR":
        console.error(`SQLエラー: ${err.message}`);
        break;
    }
  }

}

try {
await this.#createMemosTable();

Choose a reason for hiding this comment

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

connect() と言いつつ CREATE TABLE まで実行しているのは違和感があります。CREATE TABLE は接続処理ではないからです。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
接続処理とテーブル作成処理でメソッドを別々にしました。

memoDatabase.jsの一部

  async connect() {
    try {
      this.#database = await this.#openDatabase(this.#databasePath);
    } catch (err) {
      if (err.code === "SQLITE_CANTOPEN" || err.code === "SQLITE_NOTADB") {
        throw err;
      }
    }
  }

  async createTable() {
    try {
      await this.#createMemosTable();
    } catch (err) {
      if (err.code === "SQLITE_ERROR") {
        throw err;
      }
    }
  }

memo.jsの一部

  try {
    await memoDatabase.connect();
  } catch (err) {
    switch (err.code) {
      case "SQLITE_CANTOPEN":
        console.error(`データベースを開けません: ${err.message}`);
        break;
      case "SQLITE_NOTADB":
        console.error(`データベースファイルではありません: ${err.message}`);
        break;
    }
    return;
  }

  try {
    await memoDatabase.createTable();
  } catch (err) {
    if (err.code === "SQLITE_ERROR") {
      return console.error(`テーブルを作成できません: ${err.message}`);
    }
  }

}
}

insertMemo(content) {

Choose a reason for hiding this comment

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

このクラスは MemoDatabase というクラスでメモを扱うことは自明なので、メソッド名に Memo を含める必要はない気がします。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
以下のようにメソッド名を簡潔にしました。

  • insertMemoinsert
  • fetchAllMemosfetchAll
  • deleteMemoByIddelete (引数で受け取るidを使用して削除処理をすることはメソッドを見ればわかることなので、ById部分も削除した。)

return;
}

const isBlank = contents.every((item) => /^\s*$/.test(item));

Choose a reason for hiding this comment

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

contentsitem で名前が対応していないです。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
lineslineで名前を統一させました。

  const isBlank = lines.every((line) => /^\s*$/.test(line));

});

rl.on("close", () => {
resolve(lines);

Choose a reason for hiding this comment

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

配列で resolve する必要は特にないような。欲しいのは入力された文字列ではないですかね。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
配列でresolveせず、改行ごとにjoinした文字列でresolveするようにしました。

    rl.on("close", () => {
      lines = lines.join("\n");
      resolve(lines);
    });

Comment on lines 5 to 26
try {
const memos = await database.fetchAllMemos();
if (memos.length === 0) {
return console.log("メモが存在しません。");
}

const choices = memos.map((memo) => ({
name: memo.content.split("\n")[0],
value: memo,
}));

const answer = await inquirer.prompt([
{
type: "list",
name: "selectedMemo",
message,
choices,
},
]);

return answer.selectedMemo;
} catch (error) {

Choose a reason for hiding this comment

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

try で囲む範囲が広すぎてどこで例外が送出される想定なのか分かりづらいです。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
try...catchブロックの範囲を狭めてどこで例外が創出されることが想定されているのかを明確にしました。

import inquirer from "inquirer";
import { ExitPromptError } from "@inquirer/core";

export async function selectMemo(database, message) {
  const memos = await database.fetchAll();
  if (memos.length === 0) {
    console.log("メモが存在しません。");
    process.exit(0);
  }

  const choices = memos.map((memo) => ({
    name: memo.content.split("\n")[0],
    value: memo,
  }));

  try {
    const answer = await inquirer.prompt([
      {
        type: "list",
        name: "selectedMemo",
        message,
        choices,
      },
    ]);

    return answer.selectedMemo;
  } catch (error) {
    if (error instanceof ExitPromptError) {
      process.stdout.write("Ctrl+C/Dが入力された為処理を終了させました");
      process.exit(0);
    }
  }
}

Comment on lines 14 to 17
} catch (err) {
process.stderr.write(`入力中にエラーが発生しました: ${err.message}`);
return;
}

Choose a reason for hiding this comment

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

例外が握り潰されています。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
想定するエラー(登録しようとしているメモの長さが長すぎてシステムのメモリ制限を超過した場合)のみを捕捉するようにしました。

addMemo.jsの一部

  try {
    if (process.stdin.isTTY) {
      console.log(
        "メモの内容を入力してください(保存するにはCtrl+Dを押してください):",
      );
    }
    lines = await getInputLines(input);
  } catch (err) {
    if (err.code == "ENOMEM") {
      throw err;
    }
  }

main関数の一部

  switch (args[0]) {
    case OPTIONS.LIST:
      await listMemos(memoDatabase);
      break;
    case OPTIONS.READ:
      await readMemo(memoDatabase);
      break;
    case OPTIONS.DELETE:
      await deleteMemo(memoDatabase);
      break;
    default:
      try {
        await addMemo(memoDatabase);
        break;
      } catch (err) {
        if (err.code === "ENOMEM") {
          console.error(`システムのメモリ制限を超鹿しました: ${err.message}`);
          process.exit(1);
        }
      }
  }

const lines = [];

rl.on("SIGINT", () => {
console.log("Ctrl+Cが入力された為メモの作成を中止しました");

Choose a reason for hiding this comment

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

この場合にどんなメッセージを出力するかは getInputLines() の呼び出し側 (Promise を使う側) が決めることです。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
SIGINTをキャッチした際に新しいエラーオブジェクトを作成してrejectし、それをもとにgetInputLines関数の使用側で処理するようにしました。

    rl.on("SIGINT", () => {
      reject(new Error("SIGINT"));
    });
  try {
    if (process.stdin.isTTY) {
      console.log(
        "メモの内容を入力してください(保存するにはCtrl+Dを押してください):",
      );
    }
    lines = await getInputLines(input);
  } catch (err) {
    if (err.message === "SIGINT") {
      console.log("Ctrl+Cが入力された為メモの作成を中止しました");
      process.exit(1);
    }
    if (err.code == "ENOMEM") {
      throw err;
    }
  }

04.class/memo.js Outdated
try {
await memoDatabase.connect();
} catch (err) {
switch (err.code) {

Choose a reason for hiding this comment

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

err にプロパティアクセスできない値が入っていると err.code でエラーが起きそうです。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer

  • swich文をif文に修正しました。
  • プロパティアクセスできない値が入っていても対応できるようにelse文を用意してどんなケースでも対応できるように修正しました。
  try {
    await memoDatabase.connect();
  } catch (err) {
    if (err.code === "SQLITE_CANTOPEN") {
      console.error(`データベースを開けません: ${err.message}`);
    } else if (err.code === "SQLITE_NOTADB") {
      console.error(`データベースファイルではありません: ${err.message}`);
    } else {
      console.error(`データベースに接続できません: ${err.message}`);
    }
    process.exit(1);
  }

Comment on lines 30 to 33
if (err.code === "SQLITE_ERROR") {
console.error(`テーブルを作成できません: ${err.message}`);
process.exit(1);
}

Choose a reason for hiding this comment

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

err.code === "SQLITE_ERROR" の条件を満たさない例外については異常終了しなさそうです。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
SQLITE_ERROR以外の例外の発生にも対応できるように修正しました。

  try {
    await memoDatabase.createTable();
  } catch (err) {
    if (err.code === "SQLITE_ERROR") {
      console.error(`テーブルを作成できません: ${err.message}`);
    } else {
      console.error(`エラーが発生しました: ${err.message}`);
    }
    process.exit(1);
  }

04.class/memo.js Outdated
break;
} catch (err) {
if (err.code === "ENOMEM") {
console.error(`システムのメモリ制限を超鹿しました: ${err.message}`);

Choose a reason for hiding this comment

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

超過の文字がおかしそうです。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
誤字修正いたしました🙇‍♂️

        if (err.code === "ENOMEM") {
          console.error(`システムのメモリ制限を超過しました: ${err.message}`);
          process.exit(1);
        }

Comment on lines 15 to 17
if (err.code === "SQLITE_CANTOPEN" || err.code === "SQLITE_NOTADB") {
throw err;
}

Choose a reason for hiding this comment

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

if 文の条件を満たさない場合に例外が握り潰されています。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
メソッド使用する側でelse文で期待される例外以外のものも捕捉できるようにしましたので、MemoDatabase#connectでは単にerrをthrowするようにしました。

MemoDatabase#connect

  async connect() {
    try {
      this.#database = await this.#openDatabase(this.#databasePath);
    } catch (err) {
      throw err;
    }
  }

main関数の一部

  try {
    await memoDatabase.connect();
  } catch (err) {
    if (err.code === "SQLITE_CANTOPEN") {
      console.error(`データベースを開けません: ${err.message}`);
    } else if (err.code === "SQLITE_NOTADB") {
      console.error(`データベースファイルではありません: ${err.message}`);
    } else {
      console.error(`データベースに接続できません: ${err.message}`);
    }
    process.exit(1);
  }

import { createInterface } from "readline";

export async function addMemo(database) {
let lines;

Choose a reason for hiding this comment

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

変数は参照の直前で定義してください。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
linesの定義位置を参照の直前の位置に移動しました。

  let lines;
  try {
    lines = await getInputLines(input);

const memos = await database.fetchAll();
if (memos.length === 0) {
console.log("メモが存在しません。");
process.exit(0);

Choose a reason for hiding this comment

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

https://bootcamp.fjord.jp/products/20703#comment_180339 でコメントしている exit の問題が解決していないです。

Copy link
Owner Author

Choose a reason for hiding this comment

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

@cafedomancer
fetchAllで取得したメモの数が0の時は戻り値にnullを返して、それをもとにreadMemodeleteMemo内で対応するように修正しました。

selectMemoの一部

export async function selectMemo(database, message) {
  const memos = await database.fetchAll();
  if (memos.length === 0) {
    return null;
  }

readMemo.js

import { selectMemo } from "../helpers/memoHelpers.js";

export async function readMemo(database) {
  const memo = await selectMemo(database, "表示するメモを選んでください:");
  if (!memo) {
    console.log("メモが存在しません。");
    return;
  }
  console.log(memo.content);
}

deleteMemo.js

import { selectMemo } from "../helpers/memoHelpers.js";

export async function deleteMemo(database) {
  const memo = await selectMemo(database, "削除するメモを選んでください:");
  if (!memo) {
    console.log("メモが存在しません。");
    return;
  }
  await database.delete(memo.id);
  console.log("メモを削除しました。");
}

return new Promise((resolve, reject) => {
const rl = createInterface({
input,
output,

Choose a reason for hiding this comment

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

修正が漏れています。#5 (comment)

Copy link
Owner Author

@taichihub taichihub Jan 6, 2025

Choose a reason for hiding this comment

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

@cafedomancer
修正が漏れているわけではなく意図的に残しています。
というのも、ここのoutputを削除してしまうと「メモの入力中にEnterを押下するとターミナル上から入力した文字が消えてしまう」という現象が起きてしまいます。
それについて調べたところ、createInterfaceの引数にoutputを設定してあげないとechoが無効になってしまうことがわかりました。なのでoutputを残しているのですが、outputの記述をなくした状態でターミナル上で文字の入力中に文字が消えなくなるようにできる方法があるということでしょうか。
https://nodejs.org/api/readline.html?utm_source=chatgpt.com

Choose a reason for hiding this comment

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

なのでoutputを残しているのですが、outputの記述をなくした状態でターミナル上で文字の入力中に文字が消えなくなるようにできる方法があるということでしょうか。

はい。

Copy link
Owner Author

@taichihub taichihub Jan 14, 2025

Choose a reason for hiding this comment

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

@cafedomancer
引数をinputのみにすることで対応しました🙇‍♂️

    const rl = createInterface({
      input,
    });

});

rl.on("close", () => {
lines = lines.join("\n");

Choose a reason for hiding this comment

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

特定の変数に異なるデータ型のデータを再代入するのは分かりづらいのでやめてください。そもそもこの再代入は必要ないような気がします。

Copy link
Owner Author

@taichihub taichihub Jan 6, 2025

Choose a reason for hiding this comment

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

@cafedomancer

  • 再代入が必要なくなったのでlinesconstに修正しました。
  • closeイベントの処理を修正しました。
    rl.on("close", () => {
      resolve(lines.join("\n"));
    });

try {
await memoDatabase.connect();
} catch (err) {
if (err.code === "SQLITE_CANTOPEN") {

Choose a reason for hiding this comment

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

この行で err に対して err.code というプロパティアクセスが行われているので、#5 (comment) にコメントしている問題が解決していないです。

Comment on lines +54 to +57
if (err.code === "ENOMEM") {
console.error(`システムのメモリ制限を超過しました: ${err.message}`);
process.exit(1);
}

Choose a reason for hiding this comment

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

ここで err.code === "ENOMEM" 以外の場合に異常終了していないのは意図的なものですか?#5 (comment)

Comment on lines +14 to +16
} catch (err) {
throw err;
}

Choose a reason for hiding this comment

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

catch したものを throw するだけならそもそも try...catch で処理する意味がないです。#5 (comment)

Comment on lines +23 to +25
if (err.code === "SQLITE_ERROR") {
throw err;
}

Choose a reason for hiding this comment

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

ここも修正が必要です。#5 (comment)

} catch (err) {
if (err.message === "SIGINT") {
console.log("Ctrl+Cが入力された為メモの作成を中止しました");
process.exit(1);

Choose a reason for hiding this comment

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

exit する処理は memo.js に寄せたほうがよいかと。

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.

3 participants