Displaying a timetable

Question 1

The following code displays a Timetable, but the code needs too long finish the whole process.

<?php
class showTimeTableModel {
 static $home = false;
 static $type = NULL;
 static $c_id = NULL;
 static $class = NULL;
 static $subjects = NULL;
 static $kn;
 static $year;
 static $kw;
 static $TimeTableArray = array();
 static $SubejctArray = array();
 static $EventArray = array();
 static $TypeArray = array();
 public static function show($type, $home = false) {
 self::$type = $type;
 self::$home = $home;
 self::$c_id = ClassModel::user()->u_c_id;
 self::$class = ClassModel::infos(self::$c_id);
 foreach(SubjectModel::getSubjects() as $value) {
 self::$SubejctArray[$value->s_id] = $value; //Get all Subjects
 }
 foreach(ClassModel::getEventTypes() as $key => $value) {
 self::$TypeArray[$key] = $value; //Get all 'Events' in this week
 }
 self::getWeekNumber();
 self::getYearNumber();
 $start = new DateTime();
 $end = new DateTime();
 $start->setISODate(self::$year, self::$kn, 1);
 $end->setISODate(self::$year, self::$kn, 7);
 foreach(EventModel::getEventSpan($start->format('Y-m-d'),$end->format('Y-m-d')) as $value) {
 self::$EventArray[$value->e_s_id] = array($value->e_day => array("e_id" => $value->e_id,"e_title" => $value->e_title,"e_type" => $value->e_type));
 }
 if(!self::getTimeTableJSON()) {
 echo '
 <div class="row">
 <div class="col s12 center">
 <h4>No Timetable</h4>
 </div>
 </div>
 ';
 return;
 }
 self::showHeader();
 self::generateTableHead();
 self::generateTableRows();
 echo '</tbody></table>';
 }
 private static function getWeekNumber() {
 if(Request::get("kw") == NULL) {
 $options = self::$class->c_options;
 $json = json_decode($options);
 if($options != NULL && !empty($json) && json_decode($options)->nextweek <= date("N")) {
 self::$kn = (date("W")+1);
 } else {
 self::$kn = date("W");
 }
 } else {
 self::$kn = $_GET["kw"];
 }
 }
 private static function getYearNumber() {
 if(Request::get("year") == NULL) {
 self::$year = date("Y");
 } else {
 self::$year = $_GET["year"];
 }
 }
 private static function getTimeTableJSON() {
 $tt = TimetableModel::getTimeTables(self::$c_id);
 if(!$tt)
 return false;
 $weeks = "";
 $i = 0;
 foreach($tt as $value) {
 $weeks .= $value->tt_week;
 $i++;
 }
 self::$kw = TimetableModel::getWeek(self::$kn, $i, $weeks);
 $row = TimetableModel::getTimeTableWeek(self::$c_id, self::$kw);
 $json = $row->tt_json;
 $array = json_decode($json);
 self::$TimeTableArray = $array;
 return true;
 }
 private static function showHeader() {
 if(self::$home)
 return;
 else if(self::$type == "personal")
 $link = "/personal";
 else if(self::$type == "class")
 $link = "/class";
 $date_show_lastweek = new DateTime();
 $date_show_lastweek->setISODate(self::$year, (self::$kn-1));
 $date_show_nextweek = new DateTime();
 $date_show_nextweek->setISODate(self::$year, (self::$kn+1));
 echo '
 <div class="row">
 <div class="col s12 center">
 <a class="btn primary" href="'.strtok($_SERVER["REQUEST_URI"], "?").'?kw='.$date_show_lastweek->format('W').'&year='.$date_show_lastweek->format('Y').'"><i class="fa fa-chevron-left" aria-hidden="true"></i> Letze Woche</a>
 <a class="btn primary" href="'.strtok($_SERVER["REQUEST_URI"], "?").'?kw='.$date_show_nextweek->format('W').'&year='.$date_show_nextweek->format('Y').'">Nächste Woche <i class="fa fa-chevron-right" aria-hidden="true"></i></a>
 </div>
 </div>
 ';
 }
 private static function generateTableHead() {
 $THeaderString = self::THeaderString();
 if(!self::$home)
 $time = '<th>Zeit</th>';
 else
 $time = "";
 echo '
 <table width="100%" id="TimeTable" class="bordered">
 <thead>
 <th>Stunde</th>
 '.$time.'
 '.$THeaderString.'
 </thead>
 <tbody>
 ';
 }
 private static function THeaderString() {
 $weekday = Data::getGermanDays();
 $THeaderString = "";
 $date_show = new DateTime();
 for ($wd = 0; $wd < 7; $wd++) {
 if(in_array($wd,json_decode(self::$class->c_options)->daysnotshown)) {
 continue;
 }
 $date_show->setISODate(self::$year, self::$kn, ($wd+1));
 $iscurrent = ($date_show->format('Y-m-d') == date("Y-m-d")) ? 'class="info lighten-5"' : "";
 $THeaderString .= '<th '.$iscurrent.'>'.$weekday[$wd].' '.$date_show->format('d.m').'</th>';
 }
 return $THeaderString;
 }
 private static function generateTableRows() {
 $next_hour = true;
 $s = 1;
 $max = count((array) self::$TimeTableArray);
 while($s <= $max) {
 if($next_hour || self::$TimeTableArray->{$s-1}->to == self::$TimeTableArray->{$s}->from) {
 echo '<tr>';
 self::TableRowsLession($s);
 $s++;
 $next_hour = false;
 } else {
 echo '<tr id="pause">';
 self::TableRowsPause($s);
 $next_hour = true;
 }
 echo '</tr>';
 }
 }
 private static function TableRowsPause($s) {
 echo '<td></td>';
 if(!self::$home)
 echo '<td></td>';
 for($i = 0; $i < 7; $i++) {
 if(in_array($i,json_decode(self::$class->c_options)->daysnotshown)) {
 continue;
 }
 echo '<td></td>';
 }
 }
 private static function TableRowsLession($s) {
 $from = self::$TimeTableArray->{$s}->from;
 $to = self::$TimeTableArray->{$s}->to;
 $curr = date("H:i",time());
 if($curr >= $from && $curr < $to && self::$kn == date("W"))
 $iscurrent = 'class="info lighten-5"';
 else
 $iscurrent = "";
 echo '<th '.$iscurrent.' style="padding-top:0px;padding-bottom:0px;">'.$s.'</th>';
 if(!self::$home)
 echo '<td '.$iscurrent.' style="padding-top:0px;padding-bottom:0px;">'.$from.' - '.$to.'</td>';
 for($i = 0; $i < 7; $i++) {
 if(in_array($i,json_decode(self::$class->c_options)->daysnotshown)) {
 continue;
 }
 self::TableCellLession($s,$i);
 }
 }
 private static function TableCellLession($s,$i) {
 $day = self::$TimeTableArray->{$s}->{$i};
 $rowspan = self::getRowspan($s,$i);
 $show = self::isShown($s,$i);
 if(empty($day)) {
 echo "<td></td>";
 } else {
 if($show) {
 self::getSubjectInfos($day,$s,$rowspan,$i);
 }
 }
 }
 private static function getRowspan($s, $i) {
 $result = true;
 $span = 0;
 $rowspan = 1;
 $day = self::$TimeTableArray->{$s}->{$i};
 do {
 if(self::$TimeTableArray->{$s+$span}->{"to"} != self::$TimeTableArray->{$s+($span+1)}->{"from"}) {
 $result = false;
 }
 if(!empty($day) && $day == self::$TimeTableArray->{$s+$span}->{$i} && $result) {
 $rowspan = $rowspan+$span+1;
 }
 $span++;
 } while ($span < (count((array) $arr)-1) && $result);
 return $rowspan;
 }
 private static function isShown($stunde_key, $day_key) {
 $day = self::$TimeTableArray->{$stunde_key}->{$day_key};
 $show = true;
 if(!empty($day) && $day == self::$TimeTableArray->{$stunde_key-1}->{$day_key}) {
 $show = false;
 }
 if(self::$TimeTableArray->{$stunde_key-1}->{"to"} != self::$TimeTableArray->{$stunde_key}->{"from"}) {
 $show = true;
 }
 return $show;
 }
 private static function getSubjectInfos($subjects,$stunde_key,$rowspan,$day_key) {
 $array = self::$TimeTableArray;
 if(self::$type == "personal") {
 self::privateTimeTable($subjects,$rowspan,$stunde_key,$day_key);
 } else if(self::$type == "class") { 
 self::classTimeTable($subjects,$rowspan,$stunde_key,$day_key);
 }
 }
 private static function privateTimeTable($subjects,$rowspan,$s,$day_key) {
 $date_show = new DateTime();
 $date_show->setISODate(self::$year, self::$kn, ($day_key+1));
 $from = self::$TimeTableArray->{$s}->from;
 $to = self::$TimeTableArray->{$s+($rowspan-1)}->to;
 $curr = date("H:i",time());
 if($curr >= $from && $curr < $to && $date_show->format('Y-m-d') == date("Y-m-d"))
 $iscurrent = 'info lighten-5';
 else
 $iscurrent = "";
 if(count($subjects) != 1) {
 echo "<td class='".$iscurrent."' rowspan='".$rowspan."' style='padding:0px;'><table width='100%'><tr id='Group' style='border-style:none;'>";
 foreach($subjects as $group_key => $group) {
 $subject = "";
 $show_td = false;
 foreach(UserModel::getGroups() as $groupl => $grouvpv) {
 if(!in_array(Session::get("u_id"),json_decode($grouvpv->g_users,true)))
 continue;
 $json = $grouvpv->g_timetable;
 $arr = json_decode($json, true);
 $week = $arr[$s][$day_key][self::$kw];
 $dayname = Data::getGermanDays();
 foreach($arr as $key => $week_value) {
 if($key == $s && key($arr[$s]) == $day_key && in_array($group,$arr[$s][key($arr[$s])][self::$kw])) {
 $subject = $subject;
 $show_td = true;
 }
 }
 }
 if($group == $subject && !empty($subject) || $show_td) {
 self::displayCell($date_show, $group, $iscurrent, $rowspan, false);
 }
 }
 echo "</tr></table></td>";
 } else {
 self::displayCell($date_show, $subjects[0], $iscurrent, $rowspan);
 }
 }
 private static function classTimeTable($subjects,$rowspan,$s,$day_key) {
 $date_show = new DateTime();
 $date_show->setISODate(self::$year, self::$kn, ($day_key+1));
 $curr = date("H:i",time());
 $from = self::$TimeTableArray->{$s}->from;
 $to = self::$TimeTableArray->{$s+($rowspan-1)}->to;
 if($curr >= $from && $curr < $to && $date_show->format('Y-m-d') == date("Y-m-d"))
 $iscurrent = 'info lighten-5';
 else
 $iscurrent = "";
 if(count($subjects) != 1) {
 echo "<td ".$iscurrent." rowspan='".$rowspan."' style='padding:0px;'><table width='100%'><tr style='border-style:none;'>";
 foreach($subjects as $group_key => $group) {
 self::displayCell($date_show, $group, $iscurrent, $rowspan, false);
 }
 echo "</tr></table></td>";
 } else {
 self::displayCell($date_show, $subjects[0], $iscurrent, $rowspan);
 }
 }
 private static function displayCell($date_show, $subject, $iscurrent, $rowspan, $border = true) {
 $isEvent = self::$EventArray[$subject][$date_show->format('Y-m-d')];
 $event_exist = ($isEvent != NULL) ? true : false;
 $color = ($event_exist ? 'color:'.self::$TypeArray[$isEvent["e_type"]]["color"].';' : '');
 $class = ($event_exist ? 'tooltipped' : '');
 $html = '<td ';
 $html .= 'class="'.$iscurrent.' '.$class.'" ';
 $html .= 'data-tooltip="'.$isEvent["e_title"].'" ';
 $html .= 'day="'.$date_show->format('Y-m-d').'" ';
 $html .= 'rowspan="'.$rowspan.'" ';
 $html .= 'style="text-align:center;font-size:2rem;'.($border ? '' : 'border-style:none;').''.($event_exist ? 'padding:0px;' : 'padding:15px 5px;').'">';
 if($event_exist)
 $html .= '<a href="/event/infos/'.$isEvent["e_id"].'" style="display:block;height:100%;width:100%;padding:15px 5px;'.$color.'">';
 $html .= self::$SubejctArray[$subject]->s_short;
 $html .= '<p style="font-size:1rem;">';
 $html .= self::$SubejctArray[$subject]->s_room;
 $html .= "</p>";
 if($event_exist)
 $html .= "</a>";
 $html .= "</td>";
 echo $html;
 }
}

Do you have suggestions?

Question 2

Without understanding what is happening in all the deep coed to this code, it would be impossible to understand where potential bottlenecks are. Where do you think the bottlenecks are based on your efforts in testing this code? Also you might have problem getting good reviews as the code is hard to read without any comments and with depended seemingly gathered from static method calls that hide any understanding from one reading this code.

Question 3

As mentioned in comment, there is really not enough context to be able to speak to performance issues (or areas where you may want to investigate performance). Utimately, you should familiarize yourself with PHP debugging tools such that you can learn to pinpoint performance bottlenecks in an effective manner.

I will give some high-level thoughts on the code. I haven't really gone into detail in thinking about the underlying logic, because frankly, at this point, this code needs some refactoring to get to the point where you can expect a more fully thought-out review.

Some thoughts:

This code is hard to read. You should seek out examples of good professional code as well as PHP code standard resources like the PHP-FIG coding standards.

Consider using a style checking tool to enforce these standards.

Styling problems include:

no comments.
extremely long lines of code. Strive to keep under 80 characters.
an overabundance of loose comparisons. This is actually more than style consideration. People who use loose comparisons by default in PHP are probably going to be writing buggy code. You should consider always using strict comparisons except for those relatively rare cases where a loose comparison makes sense.
inconsistent casing around method names. Typical standards would have lowercase first character. Also, consider upper case for first character in class name.
intermingling of snake_case and camelCase. Be consistent in your code (though I know PHP as a language is not consistent).
variable names are often not meaningful. What is the purpose of $c_id, $kn, $kw for example? Without comments and without meaningful variable names (which can sometimes eliminate need for comments), the reader would have to dive into the code itself to understand what these class properties might mean. That is really poor for someone writing code that is using this class.
you should explicitly specify the visibility of your class properties.

Your class does way too much. Right now this class:

directly reads user input. (Your code even does this in private methods which should ideally not be operating on data that is not already properly validated elsewhere);
instantiates numerous objects of different classes;
generates content for display in view;
hold logic for assembling time tables;
holds HTML view template;
contains CSS styling logic;
orchestrates output of the view based on content.

You REALLY need to think about decoupling these types of concerns when you are designing your classes.

Just to think about this. If someone wanted to make a simple change for a CSS style for this element, they have to go into a PHP class (not even a template) to make this change. That is a questionable design.

I am not sure that you are using static properties/methods appropriately, though I am not 100% certain without better understanding how this class fits in the overall application.

It seems to me that an object in your system that is tasked with rendering a TimeTable might have separate instances. You spend a lot of time "constructing" the various data you store on static properties. This seems more like instance behavior rather than behavior that should be setting globally-accessible static property values. Is there more than one time table on a page? If so, why would you be resetting these static values for another "instance".

If this class is truly intended to have a common set up across the whole application (i.e. really be used statically), then I would think you would best be served by getting the vast majority of logic out of this class, either into configuration which is read into static properties, or into separate object dependencies that could be passed to this class. This class should not need to know how to set up the the time table to be rendered. Tt should be given a properly set up time table (and other dependencies) and do one thing, render it.

Also, if this class is truly expected to be set up only once, I would expect to not have to recalculate/reset the values in show() every time a time table needs to be rendered based on this configuration. You could perhaps have an initialize() "constructor"-like method to set the class up and then calls to show() could just focus on rendering. Again, though, this is really almost instance behavior as this would have the same effect as instantiating a single object of this class and operating against it across the application.

Looking at the ways you are accessing your other classes, I wonder if this is a wider consideration, as you seem to be taking an almost exclusively static approach to interacting with other objects in the application.

Think about dependency injection over static methods for instantiation, as you can really bloat your classes if they all need to hold understanding for how to instantiate all their dependencies (and perhaps handle errors/ exceptions that could potentially arise during dependency set up).

In your one public method, you do nothing to validate that the parameters passed are reasonable for use with this function. You should validate and fail out (ideally with some sort of exception) if proper dependencies are not provided.

You should definitely not be setting up dependencies in your private methods as you are often doing (reading user input, instantiating related objects, etc.) Your public methods are your contract with the caller. Your caller needs to give you all the dependencies you need. You need to validate those dependencies within the public methods such that the private methods that might be called from there know with 100% certainty that their dependencies are set up exactly the way they need to be. Otherwise, you get into having to have validation code sprinkled all across your class methods.

I don't agree with your approach of setting/getting JSON-serialized data to class properties. JSON is a serialization format, suitable only to represent a datagram for interchange between application components, services, etc. There is no reason that in a class property (static or concrete instance) you should be storing JSON for items you are going to be working with in your code. Why introduce the need to serialize/deserialize just to work with your properties? Store the actual data structure, not the JSON. If the class gets JSON handed to it from dependency, that is fine, but it should deserialize it before setting on the property, such that a) you can validate the JSON is valid and can be deserialized and b) other methods in your class that need to work with this data do not have to deserialize it first.

Mike Brant Mike Brant 9,85814 silver badges24 bronze badges · Accepted Answer · 2016-10-21 15:24:46Z