PHP

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

先週までの簡単なまとめ

48日目から始めたカレンダー制作を始めました。仕様を決め実装し自らテストして
課題を完成させるものです。
本日までに

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

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

スクリーンキャプチャ

本日やったこと

• コードレビュー

ということで、コードレビューで指摘された部分を書き出していきたいと思います。
指摘されたところは直していこうと思います。
ということで[これはひどい]と言う形でさらしていこうかなと。

[これがひどい]

変数多すぎ。

• $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;
        } 
      }
    }