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

exceptionのクラス図を作成 #76

Merged
merged 2 commits into from
Jul 20, 2022
Merged

exceptionのクラス図を作成 #76

merged 2 commits into from
Jul 20, 2022

Conversation

katataku
Copy link
Owner

例外クラスのクラス図を作りました。
Transactionの擬似コードと変更してます。

変更前:throw IExecutor::MethodNotAllowed();という投げ方
今回 :throw HTTPException(403);という投げ方

この方がわかりやすいと思いましたが、いかがでしょ〜

@katataku katataku linked an issue Jul 19, 2022 that may be closed by this pull request
@katataku
Copy link
Owner Author

exceptionを単独のmdにするのではなく、モジュール.mdに含める

@katataku
Copy link
Owner Author

exceptionを単独のmdにするのではなく、モジュール.mdに含める

修正done

@hayashi-ay
Copy link
Collaborator

変更前:throw IExecutor::MethodNotAllowed();という投げ方
今回 :throw HTTPException(403);という投げ方
この方がわかりやすいと思いましたが、いかがでしょ〜

どういう例外が発生しているかは変更前の方が分かりやすいと思います。
HTTPException(413)が投げられていてもどういうエラー内容なのかはスタータスコードを覚えていないと分からないですし。

@katataku
Copy link
Owner Author

変更前:throw IExecutor::MethodNotAllowed();という投げ方
今回 :throw HTTPException(403);という投げ方
この方がわかりやすいと思いましたが、いかがでしょ〜

どういう例外が発生しているかは変更前の方が分かりやすいと思います。 HTTPException(413)が投げられていてもどういうエラー内容なのかはスタータスコードを覚えていないと分からないですし。

コメントあざすです!

うーむ。
じゃあマクロ定数使って、

#DEFINE HTTP_STATUS_CODE_METHOD_NOT_ALLOWED 403

...

throw HTTPException(HTTP_STATUS_CODE_METHOD_NOT_ALLOWED)

とかどう思いますか〜?

感覚ベースなんですが、メソッド名が事由フレーズというのがどうも引っ掛かってます。。

@hayashi-ay
Copy link
Collaborator

メソッド名が事由フレーズというのがどうも引っ掛かってます。。

"method not allowd"というメッセージがあってそれ由来でクラス名を決めているという順番ではないと思っています。
許可されていないメソッドを使用しているという事象に対する例外で、そのメッセージは今回は"method not allowed"ですが必ずしもそうである必要はないと思っています。もっと丁寧に"GET method is not allowed"とかでもいいですし。

@katataku
Copy link
Owner Author

@hayashi-ay 口頭で会話した件備忘です。

  • はやしさんの指摘は、「わかりやすさ」的な意味でのコメントだった。
  • 僕も、はやしさんの案の方がわかりやすいなという気がしてきました。
  • 今回は実装のシンプルさをとって、throw HTTPException(403);として実装する。

Copy link
Collaborator

@Yamada-Ika Yamada-Ika left a comment

Choose a reason for hiding this comment

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

LGTM !

@Yamada-Ika Yamada-Ika merged commit dba83dc into main Jul 20, 2022
@Yamada-Ika Yamada-Ika deleted the feat/75_error_class branch July 20, 2022 08:05
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