こうすれば可読性が上がったり、短縮してかけるよ、という事があればご教授いただけると、大変助かります。
function cuteInsert() {
$dbh = connectDb();
$dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
// 1回目
$post_id = h($_POST['post_id']);
$sql = "insert into post_ip(post_id, remote_addr, user_agent, created ) values (:post_id, :remote_addr, :user_agent, now())";
$stmt = $dbh->prepare($sql);
$stmt->setFetchMode(PDO::FETCH_ASSOC);
$stmt->bindValue(':remote_addr', $_SERVER['REMOTE_ADDR'], PDO::PARAM_STR);
$stmt->bindValue(':user_agent', $_SERVER['HTTP_USER_AGENT'], PDO::PARAM_STR);
try {
$stmt->execute();
} catch (\PDOException $e) {
throw new \Exception('今日は投稿できません。');
}
// 2回目
$eva = h($_POST['evaluation']);
if ($eva == "0") {
$evaluation = 1;
} else {
$evaluation = $eva + 1;
}
$cute_sql = "update post set evaluation = :evaluation where post_id = :post_id";
$stmt = $dbh->prepare($cute_sql);
$stmt->setFetchMode(PDO::FETCH_ASSOC);
$stmt->bindParam(':post_id', $post_id, PDO::PARAM_INT);
$stmt->bindParam(':evaluation', $evaluation, PDO::PARAM_INT);
$stmt->execute();
return $evaluation;
}
↓Kosuke_Shibuyaさん、KiyoshiMotokiさんにご指摘いただいた内容を加味し、ソースを修正しました。
function cuteInsert() {
$dbh = connectDb();
// $post_id = h($_POST['post_id']); から変更
$post_id = filter_input(INPUT_POST, 'post_id');
$sql = "insert ignore into post_ip(post_id, remote_addr, user_agent, created ) values (:post_id, :remote_addr, :user_agent, now())";
$dbh->beginTransaction();
$stmt = $dbh->prepare($sql);
$stmt->bindValue(':post_id', $post_id, PDO::PARAM_INT);
$stmt->bindValue(':remote_addr', $_SERVER['REMOTE_ADDR'], PDO::PARAM_STR);
$stmt->bindValue(':user_agent', $_SERVER['HTTP_USER_AGENT'], PDO::PARAM_STR);
$stmt->execute();
$count = $stmt->rowCount();
// 登録できた場合
if ($count == "1") {
$eva = filter_input(INPUT_POST, 'evaluation');
if ($eva == "0") {
$evaluation = 1;
} else {
$evaluation = $eva + 1;
}
$cute_sql = "update post set evaluation = :evaluation where post_id = :post_id";
$stmt = $dbh->prepare($cute_sql);
$stmt->bindValue(':post_id', $post_id, PDO::PARAM_INT);
$stmt->bindValue(':evaluation', $evaluation, PDO::PARAM_INT);
$stmt->execute();
$dbh->commit();
// 登録できなかった場合
} else {
$dbh->rollBack();
}
}
-
気になる質問をクリップする
クリップした質問は、後からいつでもマイページで確認できます。
またクリップした質問に回答があった際、通知やメールを受け取ることができます。
クリップを取り消します
-
良い質問の評価を上げる
以下のような質問は評価を上げましょう
- 質問内容が明確
- 自分も答えを知りたい
- 質問者以外のユーザにも役立つ
評価が高い質問は、TOPページの「注目」タブのフィードに表示されやすくなります。
質問の評価を上げたことを取り消します
-
評価を下げられる数の上限に達しました
評価を下げることができません
- 1日5回まで評価を下げられます
- 1日に1ユーザに対して2回まで評価を下げられます
質問の評価を下げる
teratailでは下記のような質問を「具体的に困っていることがない質問」、「サイトポリシーに違反する質問」と定義し、推奨していません。
- プログラミングに関係のない質問
- やってほしいことだけを記載した丸投げの質問
- 問題・課題が含まれていない質問
- 意図的に内容が抹消された質問
- 過去に投稿した質問と同じ内容の質問
- 広告と受け取られるような投稿
評価が下がると、TOPページの「アクティブ」「注目」タブのフィードに表示されにくくなります。
質問の評価を下げたことを取り消します
この機能は開放されていません
評価を下げる条件を満たしてません
質問の評価を下げる機能の利用条件
この機能を利用するためには、以下の事項を行う必要があります。
- 質問回答など一定の行動
-
メールアドレスの認証
メールアドレスの認証
-
質問評価に関するヘルプページの閲覧
質問評価に関するヘルプページの閲覧
+3
(コードに間違いがあるので…)
下記ソースのように2回(実行回数)「execute();」を使用して処理してしまっても、可読性など問題はないでしょうか
「可読性」を考慮するなら、自分だったら処理は分けますし、グローバルな関数は定義せずに、クラスで実装します。
他に気づいたところでは、
この部分は、別の関数に切り分けする。
$dbh = connectDb();
$dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
提示のコードで以下の部分は無意味
$stmt->setFetchMode(PDO::FETCH_ASSOC);
例外ブロックを変更すべき
try ~ catch (\PDOException $e) {
}
以下の部分
$post_id = h($_POST['post_id']);
ここで使われている
h()
が htmlspecialchars
相当の機能であるなら、使用方法を間違っています。
以下の2つについても、
\
の意味を理解せずに使っているのかなと。
$dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
catch (\PDOException $e) {
投稿
-
回答の評価を上げる
以下のような回答は評価を上げましょう
- 正しい回答
- わかりやすい回答
- ためになる回答
評価が高い回答ほどページの上位に表示されます。
-
回答の評価を下げる
下記のような回答は推奨されていません。
- 間違っている回答
- 質問の回答になっていない投稿
- スパムや攻撃的な表現を用いた投稿
評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。
checkベストアンサー
+2
1回目のクエリ実行では
bindValue
を、2回目ではbindParam
を使用しているのが気になります。
"可読性"という点では、特に理由がなければどちらかに統一した方が良いと思います。
以下、ご質問の内容(行数・可読性)とは若干ズレますが、提示いただいたコードで気になった点を上げさせていただきます。
(Kosuke_Shibuya様の回答と重複する内容もあります)
レコードの存在確認について
post_ip
テーブルに既にレコードが登録されているか否かを、
PDOException
が送出されたか否かで判断するのはよろしくありません。
この方法では、DBに障害が発生した場合など、本当に例外的な状況との区別がつかないからです。
MySQL
をお使いであれば、insert into ...
をinsert ignore into ...
に変更し、
PDOStatement::rowCount()
メソッドで登録された行数を確認する方が良いです。
http://php.net/manual/ja/pdostatement.rowcount.php
トランザクションを使用していない
トランザクションを使用していないため、以下のような状況が発生する可能性があります。- 1回目のクエリ実行 -> 成功
- DB障害発生
- 2回目のクエリ実行 -> 失敗
- 結果、post_ipテーブルにレコードは登録されたが、postテーブルは何も変更されない
1回目と2回目のクエリはトランザクションの中で実行し、クエリの実行に失敗したらロールバックしてやる必要があります。
http://php.net/manual/ja/pdo.begintransaction.php
$stmt->setFetchMode(PDO::FETCH_ASSOC); は不要
SQL文の実行後に$stmt->fetch()
を実行しない場合、$stmt->setFetchMode()
を実行する必要はありません。
http://php.net/manual/ja/pdostatement.setfetchmode.php
投稿
-
回答の評価を上げる
以下のような回答は評価を上げましょう
- 正しい回答
- わかりやすい回答
- ためになる回答
評価が高い回答ほどページの上位に表示されます。
-
回答の評価を下げる
下記のような回答は推奨されていません。
- 間違っている回答
- 質問の回答になっていない投稿
- スパムや攻撃的な表現を用いた投稿
評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。
15分調べてもわからないことは、teratailで質問しよう!
- ただいまの回答率 88.35%
- 質問をまとめることで、思考を整理して素早く解決
- テンプレート機能で、簡単に質問をまとめられる
2015/11/24 17:25
頂いた回答を1つ1つ読み込みつつ、ソースを修正していきたいと思います。
取り急ぎ、お礼まで。
2015/11/25 12:24
・この部分は、別の関数に切り分けする。
'$dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);'
DBにアクセスする関数'connectDb();'に組み込みました。
'connectDb();'を呼び出す度に指定しており、モヤモヤしていた箇所だったので、ご指摘いただけた事で確信を持って調べる事ができました。ありがとうございます。
処理を分けてクラスで実装される、という部分について、どういうクラスを作成すればよいのか考え中です。
例えばクラス(.phpファイル)が複数あり、それぞれにDBにアクセスする必要がある場合ですと、'connectDb();'のような関数はfunctions.phpに記述しても問題ないでしょうか。
セキュリティ的にクラス毎に実装したほうが良いなどありましたら、ご指摘いただけると幸いです。
・$stmt->setFetchMode(PDO::FETCH_ASSOC);
他の処理で取得した値を配列に格納した時に使用していたものを、そのままコピペしていましたので、削除しました。
ご指摘ありがとうございます。
・htmlspecialchars
仰るとおり、使用方法を間違っていました。
いろいろと調べたところ、HTMLに出力する際にサニタイズするのが正しいということだったので、修正しました。
・\ の意味
こちらもおっしゃるとおり、意識せずに使用していました。
'$dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);'
を指定しているので、エスケープ処理している上に、’\PDOExceptio’でさらにエスケープ処理をしていた、という認識で良かったでしょうか。
通常'\'は記号をエスケープするときに使用するのだと思いますが、'\PDOExceptio'にはどのような意味があるのでしょうか。
質問を重ねる形で大変恐縮ですが、お時間あるときにでもご教授いただけると幸いです。
2015/11/28 04:35
http://php.net/manual/ja/language.namespaces.rationale.php
2015/11/28 04:38
同種の関数のまとまりを持たせる意味もあるので、自分は、
・DB接続
・SELECTの実行
・UPDATEの実行
・INSERTの実行
・DELETEの実行
というような感じでクラスにまとめています。
2015/11/30 12:10
調べても辿りつけなかったので、大変助かります。
・クラスについて
なるほど、DBで行う処理内容によって、クラスを作っておられるのですね。
大変参考になります。
投稿した内容とは違う質問にも関わらず、ご丁寧に回答いただき、ありがとうございました。