Skip to content

全体レビュー #10

@ghost

Description

[GOOD] PHPとSQLの文法を理解して実装できています

[GOOD] オブジェクト指向プログラミングについて理解し、拡張性と保守性の高いコードを書くことができています

[MUST] URLの設計・実装に関して

index?controller=(CONTROLLER_NAME)&action=(ACTION_NAME)&id=(ITEM_ID)

?以降はあくまでクエリパラメータであって、RESTfulなURLとは言えません。しかし、今回はフレームワークを使わない為、ルーティングの実装は難しいかったかと思います。修正はMUSTではないので、Laravel研修の時にRESTfulなURL設計ができると良いでしょう。

もし修正する場合は.htaccessで全てのアクセスをindex.phpに飛ばして、 $_SERVER['REQUEST_URI']からURLをパースしてcontrollerとactionを取得するなどができるかなーと思います。

[MUST] アクセスさせたくないファイル・ディレクトリへのアクセスについて

現状ではアクセス制御を行っていないので、http://localhost:9999/controllers/ とかにアクセス出来てしまいます。セキュリティ上よろしくないので、アクセスを制限しましょう

(やり方)
/home/sites下は.htaccessで設定を書き換えられるように、NoneからAllに変更
https://github.com/prcaoduc/backendkenshu2/blob/820ff5f7e065ad5eaee55692bd27fb5f8947b026/httpd.conf#L286

/home/sites/下に.htaccessファイルを追加

Options -Indexes

http://astro.jp/pn/linux/3262
https://qiita.com/sunnyG/items/7e5bd6e8dc9b04c9978e

.htaccess自体セキュリティ上良くないとありますが、まずは使ってみて何故良くないのか理解すると良いかと思います。

[MUST] require_onceのパスについて

https://github.com/prcaoduc/backendkenshu2/blob/820ff5f7e065ad5eaee55692bd27fb5f8947b026/sites/models/article.php#L2
相対パスになっています。request_onceは実行時のファイルを元に相対パスをたどるので、思わぬバグを生んでしまう可能性があります。
また、IDEで参照先を見つけられず、うまく補完が効かない可能性もあります
スクリーンショット 2020-11-13 11 11 01

__DIR__dirname(__DIR__)などを使って、フルパスでアクセスするようにしましょう(Laravelだとオートローダーがあるのであまり考慮する必要は無いかもしれません)

https://www.php.net/manual/ja/language.constants.predefined.php
https://www.php.net/manual/ja/function.dirname.php

[MUST] ファイル間をまたがって使う変数について

route.php$controllerindex.phpで定義されたものが使われています。

こうなるとroute.phpだけを読んだ人は$controllerが何かわからないので、設計を見直す必要があります。

(Viewで使われている変数は、テンプレートエンジンを使ってないので仕方ないです。)

[MUST] クッキーに保存されているデータについて

https://github.com/prcaoduc/backendkenshu2/blob/820ff5f7e065ad5eaee55692bd27fb5f8947b026/sites/controllers/authentications_controller.php#L30-L34
自動ログインをクッキーにログイン情報を保存することで実装しているようですが、基本的にクッキーは漏洩することを考慮して、重要な情報を保存しない用にしましょう。
「体系的に学ぶ 安全なwebアプリケーションの作り方」の4.8 クッキー出力にまつわる脆弱性5.1.4 自動ログインを参考にすると良いでしょう。

[IMO] ファイル名について

snake_caseで書かれていますが、IDEにクラスファイルと認識させる為にCamelCaseで書くと良いかなーと思います
スクリーンショット 2020-11-13 11 21 17
スクリーンショット 2020-11-13 11 21 41

[NITS] 複数行にまたがる配列の宣言について

https://github.com/prcaoduc/backendkenshu2/blob/820ff5f7e065ad5eaee55692bd27fb5f8947b026/sites/controllers/authentications_controller.php#L93-L98
とても細かいことですが、PHPの配列の宣言は最後の要素の行末に,をつけても問題ないので、積極的につけていきましょう。
理由は、配列の要素が各行~,と書かれて一貫性がある事、要素を最後に追加したい時に現在の最後の行に,を新たに加える手間とGitの無駄な差分がなくなるためです。

[GOOD] Modelの実装がよく考えられていました

LaravelのEloquentに似た作りで、よく出来ていました。(もしかしたらEloquentを参考にしていたのかもしれませんが)

ModelからDBを操作する時にstaticメソッドを用意していますが、Eloquentではファザードを用いることでstaticメソッドっぽい使い勝手の良さとオブジェクト指向を両立させていると思われます(Laravelのコードを完全に理解しているわけではないので間違っていたらごめんなさい)
また、PRTIMESのLaravelで書かれたマイクロサービスのコードにはドメイン駆動設計の観点からEloquentを採用していません。

ちょっと踏み入った難しい話になりましたが、今後勉強する上で参考にすると良いかもしれません。
laravelのコア: https://github.com/laravel/framework

[NITS] var_dumpについて

デバッグでvar_dumpは使いやすく便利なのですが、サービスをリリースするとユーザーにサーバーの情報が見えてしまうのでよろしくないです。
https://github.com/prcaoduc/backendkenshu2/blob/820ff5f7e065ad5eaee55692bd27fb5f8947b026/sites/controllers/articles_controller.php#L108
基本的にエラーが起きた場合はtry~catchをしてエラーメッセージをログファイルに残します

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions