コードレビュー - Githubissues

ghost commented 3 years ago

[GOOD]

URL設計・ルーティングの実装がきれいに出いていて、素晴らしいです👍

[NITS]

https://github.com/prNguyenVietThai/backend-kenshyu/blob/941858c7433a0bf5f4c1b4e401ebe3614a7d8518/src/core/Database.php#L3-L6 パスワード等の情報は基本的にハードコーディングせずに環境変数で設定します。今回Dockerを使って環境構築しているので、Dockerで環境変数を設定するやり方など学べると良いでしょう

[MUST]

記事の投稿・編集画面でCSRF対策をするようにしてください。参考書：体系的に学ぶ安全なWebアプリケーションの作り方：p141

[MUST]

https://github.com/prNguyenVietThai/backend-kenshyu/blob/c28198447c0540afd2e196417838db4877e28f6f/src/controllers/PostController.php#L37-L81 (ここだけではないですが、)複数の処理を同時に行う場合はトランザクションをしましょう。

[GOOD]

return $this->view();

や

return $this->response()

は、MVCの設計が良く出来ていると見受けられます👍

[MUST]

https://github.com/prNguyenVietThai/backend-kenshyu/blob/673e2c6af6864ce4eb02b3a7f73cffabd69c7210/src/services/TagServices.php#L37-L53 （ここだけではないですが、）SQLインジェクション対策はSQLを発行する場所全てで行うようにしましょう getByPostId(int $postId)は実装上$postIdに整数しか入らないようになっていますが、今後このプログラムを書き換える時に意図せず文字列が入っていしまう事もあるかもしれません。そのため、変数の型などの実装に依存せずに、必ずSQLインジェクション対策は行う心構えでいる事が望ましいです

[MUST]

XSSが出来てしまうので、最低限のエスケープ処理などのXSS対策をお願いします参考書：体系的に学ぶ安全なWebアプリケーションの作り方：p88

[IMO]

（修正する必要は無いです） PR TIMESではコードのクオリティを担保するためにcodeclimateを導入しています。実装やフレームワーク的に全て直す事は困難だと思いますので、参考程度に結果を載せておきます。

Starting analysis
Running structure: Done!
Running duplication: Done!

== src/controllers/AuthController.php (3 issues) ==
36-62: Function `login` has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring. [structure]
75-110: Function `signup` has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring. [structure]
75-110: Method `signup` has 31 lines of code (exceeds 25 allowed). Consider refactoring. [structure]

== src/controllers/PostController.php (5 issues) ==
25-94: Function `store` has a Cognitive Complexity of 29 (exceeds 5 allowed). Consider refactoring. [structure]
25-94: Method `store` has 60 lines of code (exceeds 25 allowed). Consider refactoring. [structure]
73-76: Avoid deeply nested control flow statements. [structure]
128-183: Method `update` has 49 lines of code (exceeds 25 allowed). Consider refactoring. [structure]
128-183: Function `update` has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring. [structure]

== src/core/Route.php (1 issue) ==
44-59: Function `request` has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring. [structure]

== src/services/PostServices.php (3 issues) ==
75-104: Method `getById` has 28 lines of code (exceeds 25 allowed). Consider refactoring. [structure]
106-139: Method `update` has 27 lines of code (exceeds 25 allowed). Consider refactoring. [structure]
171: Avoid too many `return` statements within this method. [structure]

Analysis complete! Found 12 issues.

[NITS]

DDLを/src/database以下に入れていますが、例えば localhost:8080/database/db_v2.sql などにアクセスすると外部の人にバレてしまうので、backend-kenshyu/src/database/ではなくbackend-kenshyu/database/にしたほうが良いかと思います

thaicpb commented 3 years ago

@pr-ema コメントして頂きありがとうございました。問題があるところを修正しました。コードレビューお願いします。https://github.com/prNguyenVietThai/backend-kenshyu/commit/bd924f80253558edd61bd736c6afbfce13e1e8a2

ghost commented 3 years ago

@prNguyenVietThai ご対応ありがとうございます！ほとんど問題なく、Webアプリケーションとして完成しています！２点ほど、細かいですが改善できる点がありました。

thaicpb / backend-kenshyu

コードレビュー #1

[GOOD]

[NITS]

[MUST]

[MUST]

[GOOD]

[MUST]

[MUST]

[IMO]

[NITS]