MyPDOの課題「以下のPHPの改善案をまとめよ」(多くの意見お願いします)

私はプログラマ志望の専門学生なのですが学校のLinuxの課題で「以下のPHPの改善点を挙げ修正、簡潔にまとめなさい」というのが出され、頭の悪い私にはどこをどう改善すればいいのか全くわからず頭を悩ませております。どうかご意見をこと細かく教えてくれますようよろしくお願いします。

<?php class MyPDOClass { private $dbtype; private $dbhost; private $dbuser; private $dbpass; private $dbname; private $dbchar; private $dsn; private $pdo; private $stmh; private $error; private $holderAry; function __construct($type,$host,$user,$pass,$db,$charset = "utf8") { $this->dbtype = $type; $this->dbhost = $host; $this->dbuser = $user; $this->dbpass = $pass; $this->dbname = $db; $this->dbchar = $charset; $this->dbdsn = $type . ":host=" . $host . ";dbname=" . $db . ";charset=" . $charset; $this->pdo = null; if(DEBUG) { echo $this->dsn ."\n"; } } function __destruct() { $this->close(); } public function connect() { try { $this->pdo = new PDO($this->dsn, $this->dbuser, $this->dbpass); $this->pdo->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION); $this->pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false); return true; } catch(PDOException $ex) { $this->error = $ex->getMessage(); return false; } return true; } public function close() { if($this->pdo != null) { $this->pdo = null; } } public function getError() { return $this->error; } public function insert($table,$colStr,$valStr) { $colAry = explode(",", $colStr); $valAry = explode(",", $valStr); if(count($colAry) != count($valAry)) { $this->error = "カラム名の数と値が一致しません"; return false; } if (!$this->prepare($this->getPreSQLInsert($table,$colAry,$colStr))) return false; if (!$this->bindValues($valAry)) return false; if (!$this->execute()) return false; return true; } public function getPreSQLInsert($table,$colAry) { //$this->holderAry = array(); $sql = "INSERT INTO " . "(" . $colStr . " ) VALUES( "; for($i = 0; $i < count($colAry); $i++) { $sql .= ":val" . $i; if($i < count($colAry) - 1) { $sql .= ", "; } array_push($this->holderAry, ":val" . $i); } $sql .= " )"; if(DEBUG) { echo $sql . "\n"; } return $sql; } public function prepare($sql) { try { $this->stmh = $this->pdo->prepare($sql); } catch(PDOException $ex) { $this->error = $ex->getMessaga(); return false; } return true; } public function bindValues($valAry) { try { for($i = 0; $i < count($this->holderAry); $i++) { $this->stmh->bindValue($this->$holderAry[$i],trim($valAry[$i])); } } catch(PDOException $ex) { $this->error = $ex->getMessage(); return false; } return true; } public function execute() { try { $this->stmh->execute(); } catch(PDOException $ex) { $this->error = $ex->getMessaga(); return false; } return true; } public function delete($table, $whereStr = " 1 = 1 " ,$valStr = "") { //$cnt = 0; $valAry = array_filter(explode(",", $valStr),"strlen"); $sql = $this->getPreSQLDelete($table, $whereStr); if (count($this->holderAry)!= count($valAry)) { $this->error = "プレースホルダの数と値の数が一致しません"; return -1; } if(!$this->prepare($sql)) return -1; if(!$this->bindValues($valAry)) return -1; if(!$this->execute()) return -1; return $this->stmh->rowCount(); } public function getPreSQLDelete($table,$whereStr) { $sql = "DELETE FROM " . $table . " WHERE " . $whereStr; $this->getHolders(whereStr); if(DEBUG) { echo $sql . "\n"; } return $sql; } public function getHolders($whereStr) { $this->holderAry = array(); $wordAry = preg_split("/[\s=><\/\*\-\+%]/", $whereStr); $wordAry = preg_grep("/^:.*/", $wordAry); $this->holderAry = array_merge($wordAry); } public function update($table,$colStr,$upValStr,$whereStr = "1=1",$whereValStr = "") { $colAry = explode(",", $colStr); $upValAry = explode(",", $upValStr); $whereValAry = array_filter(explode(",", $whereValStr),"strlen"); $sql = $this->getPreSQLUpdate($table,$colAry,$whereStr); $valAry = array_merge($whereValAry,$upValAry); if(count($this->holderAry) != count($valAry)) { $this->error = "プレースホルダのかずと値の数が一致しません"; return -1; } if(DEBUG) { print_r($valAry); } if(!$this->prepare($sql)) return -1; if(!$this->bindValues($valAry)) return -1; if(!$this->execute()) return -1; return $this->stmh->rowCount(); } public function getPreSQLUpdate($table,$colAry,$whereStr) { $sql = "UPDATE " . $table . " SET "; $this->getHolders($whereStr); for($i = 0; $i < count($colAry); $i++) { $sql .= $colAry[$i] . " = :val" . $i; array_push($this->holderAry,":val" . $i); if ($i < count($colAry)- 1) { $sql .= ", "; } } $sql .= " WHERE " . $whereStr; if(DEBUG) { echo $sql > "\n"; print_r($this->holderAry); } return $sql; } } ?>

ikuwow

2015/11/12 01:10

コードは見やすいようにmarkdown記法でシンタックスハイライトしていただけるとありがたいです。

退会済みユーザー

2015/12/03 06:58

こちらの質問が他のユーザから「質問の範囲が広すぎる」という評価を受けていますわからない点を明確にし、調査したこと・試したことと共に記入していただくと、回答が得られやすくなります。

行動規範の内容に同意します

回答4件

ご自分の回答を書いて、それに付いて意見を求めるようにすると良いと思います。
問題をそのまま書いて、それの回答を求めるというのは安易すぎます。

投稿2015/11/11 12:30

katoy

総合スコア22324

ベストアンサー

どれどれと読み始めたら面白かったので、
面白かった箇所を共有します。

学校のLinuxの課題で「以下のPHPの改善点を挙げ修正、簡潔にまとめなさい」

Linuxの課題でPHPとは見上げたPHPerですね

class MyPDOClass

AnimalClassのように「Class」が名前に含まれている時点でおかしい
せめてPDOを継承して欲しい

コンストラクタの引数とプライベート変数の山

PDOは簡潔にした結果あのDSNなのに、引数だらけにして何がしたいんだろう？
せめてコンフィグファイルからそのまま持ってこれるように配列で受け取れるべき。

try ~ catchの使い方

これぞThe PHPerという感じ。
例外投げて死んでくれないといざエラーが出たときになにもわからん。

簡潔にまとめなさい

最初の数行でこんなに見つかるのに、簡潔にまとめろ…だと…？
「もう突っ込み疲れたよ…こんなオレオレじゃなくて素直にPDO使わせて」←簡潔にまとめた

投稿2015/11/11 15:11

編集2015/11/11 15:13

miyabi-sun

総合スコア21158

miyabi-sun

2015/11/11 15:38

因みに上記上げたのはほとんど設問とは関係ないと思われる突っ込みどころです。英語の先生に「うはｗｗｗ先生の発音マジカタカナｗｗｗ」とイチャモンつけるようなものです。この調子で読み進めていくともっとまずい箇所が見つかるはずなので頑張って！本気で悩んだ回答と、模範解答、楽しみに待ってます。

miyabi-sun

2015/11/12 02:01

今見返すとtry ~ catchは全て戻り値をBooleanで受け取って、 falseだったら$this->errorの文字列を取りに行く設計思想なんですね。それはそれで一貫性が取れてるから「エラーが出たらどうすんねん？」は解消されそうです。しかし、これを使う側は全ての処理を「逐一」ifで評価して、もしfalseだったら…ってやるわけですね。ソースコードがif文だらけになって読めなくなるから自分の中ではNGです。

行動規範の内容に同意します

提示のソースコードではシンタックスエラーがあるので、「以下のPHPの改善点」どころか、論外ですね。せめてシンタックスエラーがない状態から「改善」なら回答しますが…

シンタックスエラーぐらいIDEが指摘してくれます。