PHP

PHP課題「カレンダー・スケジュール」
コードレビューをして見つかった[これはひどい]

先週までの簡単なまとめ

48日目から始めたカレンダー制作を始めました。仕様を決め実装し自らテストして

課題を完成させるものです。

本日までに

月ごとのカレンダーが表示できる(1ヶ月単位)
日ごとの詳細画面がありスケジュールを見ることができる
上記2のスケジュール詳細画面では、一覧表示とスケジュール登録・削除・編集ができる。
ログイン機能を付ける。
複数ユーザーで使えるようにした。
新規登録画面を作った。
テストケースを書きながらテストを実行。
細々とした機能を追加。

を行い、ほぼ完成しました。

スクリーンキャプチャ

本日やったこと

コードレビュー

ということで、コードレビューで指摘された部分を書き出していきたいと思います。

指摘されたところは直していこうと思います。

ということで[これはひどい]と言う形でさらしていこうかなと。

[これがひどい]

変数多すぎ。

$check_hhとかわかりにくいしまとめられるのではないか。

class DayView
{

  public $year;
  public $month;
  public $days;
  public $cal_title;
  public $calendar;
  public $post_year;
  public $post_month;
  public $post_day;
  public $host;
  public $post_title;
  public $create_at;
  public $is_deleted = 0;
  public $keys;
  public $user_id = 1;
  public $link;
  public $sql;
  public $result;
  public $list;
  public $error = '';
  public $check = 0;
  public $check_hh;
  public $check_mm;
  public $flag = 0;

これって必要？

帰ってくる関数の値はtrueなんだからいちいち===trueって書く必要はないのでは？

  // 投稿内容確認
  public function postScheduleDataCheck() {
    if ($this->check_hh->isValid() && $this->check_mm->isValid()) {
        if (!isset($_POST['title']) || $_POST['title']==="") {
         (略)
        } elseif($this->checkTime()==true) {
         (略)
          return true;
        }
      }    
  }

if($this->postScheduleDataCheck()===true) {
(中略)
}

コメントが不十分

コメントを見ても何をやっているかが見えない。

    } else {
      // 新規登録時の場合はチェックする。
      $this->sql .= ';';
      $this->result = $this->lib->readDB($this->sql);
      while($this->list = $this->lib->readDBfetch($this->result)) {
        if ($this->list['time'] == $this->post_time) {
          $this->error = ERR_TIME_SETED;
          $this->check = 1;
        } 
      }
    }