Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have been making a web page in PHP and JavaScript to take employees clock times and do some things with them so I can display them. Everything is currently working quite well, but the code looks rough.

<html>        <meta http-equiv="refresh" content="1; URL=http://localhost/T_A/People.php"></html>

include 'PeopleSelect.php';

class People {

public function __construct($OpNo) {

    $this->Op = $OpNo;
}

Public function GetPersonData() {
    $conn = odbc_connect('easydo', '', '');
    if (!$conn) {
        exit("Connection Failed: " . $conn);
    }

    $sql = "SELECT     ATDSchedulingRule.Name, ATDSchedulingRule.Remark, ATDShiftDetail.WorkHour, RIGHT(ATDShiftDetail.StartTime, 6) AS StartTime, 
                  RIGHT(ATDShiftDetail.EndTime, 6) AS EndTime, RIGHT(CHINA_VISION_PubPersonnel.ID, 4) AS ClockNo, CHINA_VISION_PubCards.CardCode
FROM         ATDShiftDetail INNER JOIN
                  ATDSchedulingRule ON ATDShiftDetail.ATDShift_Ref = ATDSchedulingRule.Shift_Ref2 INNER JOIN
                  CHINA_VISION_PubCards INNER JOIN
                  CHINA_VISION_PubPersonnel ON CHINA_VISION_PubCards.PubPersonnel_Ref = CHINA_VISION_PubPersonnel.Reference ON 
                  ATDSchedulingRule.Name = RIGHT(CHINA_VISION_PubPersonnel.ID, 4)
WHERE     (ATDShiftDetail.WorkHour > 0) AND (CHINA_VISION_PubPersonnel.Telephone > '1') AND ATDSchedulingRule.Name = '$this->Op' 
";

    $rs = odbc_exec($conn, $sql);
    if (!$rs) {
        exit("Error in SQL");
    }
    $Arr = array();
    while (odbc_fetch_row($rs)) {
        $WHR = round((odbc_result($rs, "WorkHour")), 2);

        $Arr = array('Op' => (odbc_result($rs, "Name")), 'Name' => (odbc_result($rs, "Remark")), 'WorkHours' => $WHR, 'Start' => (odbc_result($rs, "StartTime")), 'End' => (odbc_result($rs, "EndTime")), 'CardNo' => (odbc_result($rs, "CardCode")),);
        $CardCode = (odbc_result($rs, "CardCode"));
    }


    return $Arr;
}

Public function GetTimes() {
    $Data = $this->GetPersonData();
    $CardNo = $Data['CardNo'];

    $conn = odbc_connect('easydo', '', '');
    if (!$conn) {
        exit("Connection Failed: " . $conn);
    }

    $sql = "SELECT     TOP (4) DorCtrls_Ref, EventsID, EventTM, CardCode
 FROM         CHINA_VISION_DorEvents
WHERE     (CardCode = '$CardNo') AND (DorCtrls_Ref = '16') AND (EventTM >= DATEADD(hour, 0, DATEADD(day, 0, CAST(CAST(GETDATE() AS date) AS datetime))))
ORDER BY EventTM Asc    ";

    $rs = odbc_exec($conn, $sql);
    if (!$rs) {
        exit("Error in SQL");
    }
        $data = array();
    while (odbc_fetch_row($rs)) {
       $data[] = $this->EventTM = odbc_result($rs, "EventTM");
    }
    //var_dump($this->Swipetimes);
    $Starttime = strtotime($Data['Start']) + 2 * 60 * 60;
    @$Clocktime = strtotime($data[0]) + 2 * 60 * 60;
    $late = $Starttime - $Clocktime;
    $lated = $late / 60;

    $Endtime = strtotime($Data['End']) + 2 * 60 * 60;
   @ $Out = strtotime($data[3]) + 2 * 60 * 60;
    $home = $Endtime - $Out;
    $homed = $home / 60;


    echo '<p>';

    $Arr = @array('Op' => $Data['Op'], 'Name' => $Data['Name'], 'WorkHours' => $Data['WorkHours'], 'Start' => $Data['Start'], 'End' => $Data['End'], 'Clock' => substr($data[0], 11, 8), 'OFF' => substr($data[1], 11, 8), 'ON' => substr($data[2], 11, 8), 'OUT' => substr($data[3], 11, 8), 'Late' => $lated, 'Home' => $homed);


    return $Arr;
}

}

$People = (object) array(
        'People' => array(
            array(
                'Person1' => $person1->GetTimes(),
            ),
            array(
                'Person2' => $person2->GetTimes(),
            ),
            array(
                'Person3' => $person3->GetTimes(),
            ),
            array(
                'Person4' => $person4->GetTimes(),
            ),
            array(
                'Person5' => $person5->GetTimes(),
            ),
            array(
                'Person6' => $person6->GetTimes(),
            ),
            array(
                'Person7' => $person7->GetTimes(),
            ),
            array(
                'Person8' => $person8->GetTimes(),
            ),
            array(
                'Person9' => $person9->GetTimes(),
            ),
            array(
                'Person10' => $person10->GetTimes(),
            ),
            array(
                'Person11' => $person11->GetTimes(),
            ),
            array(
                'Person12' => $person12->GetTimes(),
            ),
            array(
                'Person13' => $person13->GetTimes(),
            ),
            array(
                'Person14' => $person14->GetTimes(),
            ),
            array(
                'Person15' => $person15->GetTimes(),
            ),
            array(
                'Person16' => $person16->GetTimes(),
            ),
            array(
                'Person17' => $person17->GetTimes(),
            ),
            array(
                'Person18' => $person18->GetTimes(),
            ),
            array(
                'Person19' => $person19->GetTimes(),
            ),
            array(
                'Person20' => $person20->GetTimes(),
            ),
            array(
                'Person21' => $person21->GetTimes(),
            ),
            array(
                'Person22' => $person22->GetTimes(),
            ),
            array(
                'Person23' => $person23->GetTimes(),
            ),
            array(
                'Person24' => $person24->GetTimes(),
            ),
            array(
                'Person25' => $person25->GetTimes(),
            ),
            array(
                'Person26' => $person26->GetTimes(),
            ),
            array(
                'Person27' => $person27->GetTimes(),
            ),
            array(
                'Person28' => $person28->GetTimes(),
            ),
            array(
                'Person29' => $person29->GetTimes(),
            ),
            array(
                'Person30' => $person30->GetTimes(),
            ),
            array(
                'Person31' => $person31->GetTimes(),
            ),
            array(
                'Person32' => $person32->GetTimes(),
            ),
            array(
                'Person33' => $person33->GetTimes(),
            ),
            array(
                'Person34' => $person34->GetTimes(),
            ),
            array(
                'Person35' => $person35->GetTimes(),
            ),
            array(
                'Person36' => $person36->GetTimes(),
            ),
            array(
                'Person37' => $person37->GetTimes(),
            ),
            array(
                'Person38' => $person38->GetTimes(),
            ),
            array(
                'Person38' => $person39->GetTimes(),
            ),
            array(
                'Person38' => $person40->GetTimes(),
            ),
            array(
                'Person38' => $person41->GetTimes(),
            ),
        )
);


$time_end = microtime(true);
file_put_contents('People.json', json_encode($People));


 $execution_time = round((($time_end - $time_start) / 60), 3);
echo '<b>Total Execution Time:</b> ' . $execution_time . ' Mins  ';

This code takes people that I have imitated in the included People select file, runs some queries and joins the data, puts it all into an array and then in to a JSON file. However, there must be a better way of doing what I have done here.

Here is how I am picking the people in the PeopleSelect.php file:

 <?php

$dat = date("w");
switch ($dat) {
case 0:
    print("Today is Sunday");
    break;
case 1:
    print("Today is Monday");

    $person1 = new People(5459);
    $person2 = new People(5592);
    $person3 = new People(5630);
    $person4 = new People(5617);
    $person5 = new People(5596);
    $person6 = new People(1004);
    $person7 = new People(1709);
    $person8 = new People(5500);
    $person9 = new People(5542);
    $person10 = new People(1686);
    $person12 = new People(5328);
    $person13 = new People(5430);
    $person14 = new People(1799);
    $person15 = new People(5304);
    $person16 = new People(1769);
    $person17 = new People(1715);
    $person18 = new People(5307);
    $person19 = new People(1877);
    $person21 = new People(1187);
    $person22 = new People(5530);
    $person23 = new People(5580);
    $person24 = new People(5579);
    $person25 = new People(5543);
    $person26 = new People(1920);
    $person27 = new People(5030);
    $person28 = new People(5475);
    $person29 = new People(1377);
    $person30 = new People(5655);
    $person31 = new People(5673);
    $person32 = new People(5674);
    $person33 = new People(5681);
    $person34 = new People(5693);
    $person35 = new People(5709);
    $person36 = new People(5713);
    $person37 = new People(0007);
    $person38 = new People(5714);
    $person39 = new People(5721);
    $person40 = new People(5722);
    break;
case 2:
    print("Today is Tuesday");
    $person1 = new People(5459);
    $person2 = new People(5630);
    $person3 = new People(5617);
    $person4 = new People(5596);
    $person5 = new People(1004);
    $person6 = new People(1709);
    $person7 = new People(5500);
    $person8 = new People(5542);
    $person9 = new People(1686);
    $person10 = new People(5328);
    $person11 = new People(5430);
    $person12 = new People(1365);
    $person13 = new People(1426);
    $person14 = new People(1799);
    $person15 = new People(5304);
    $person16 = new People(1769);
    $person17 = new People(1715);
    $person18 = new People(5307);
    $person19 = new People(1877);
    $person20 = new People(5152);
    $person21 = new People(1187);
    $person22 = new People(5580);
    $person23 = new People(5579);
    $person24 = new People(5543);
    $person25 = new People(5620);
    $person26 = new People(1920);
    $person27 = new People(5030);
    $person28 = new People(5475);
    $person29 = new People(1377);
    $person30 = new People(5655);
    $person31 = new People(5673);
    $person32 = new People(5674);
    $person33 = new People(5681);
    $person34 = new People(5693);
    $person35 = new People(5709);
    $person36 = new People(5713);
    $person37 = new People(0007);
    $person38 = new People(5714);
    $person39 = new People(5721);
    $person40 = new People(5722);


    break;
case 3:
    print("Today is Wednesday");
    $person1 = new People(5459);
    $person2 = new People(5630);
    $person3 = new People(5617);
    $person4 = new People(5596);
    $person5 = new People(1004);
    $person6 = new People(1709);
    $person7 = new People(5500);
    $person8 = new People(5542);
    $person9 = new People(1686);
    $person10 = new People(5328);
    $person11 = new People(5430);
    $person12 = new People(1365);
    $person13 = new People(1426);
    $person14 = new People(1799);
    $person15 = new People(5304);
    $person16 = new People(1769);
    $person17 = new People(1715);
    $person18 = new People(5307);
    $person19 = new People(1877);
    $person20 = new People(5152);
    $person21 = new People(1187);
    $person22 = new People(5580);
    $person23 = new People(5579);
    $person24 = new People(5543);
    $person25 = new People(5620);
    $person26 = new People(1920);
    $person27 = new People(5030);
    $person28 = new People(5475);
    $person29 = new People(1377);
    $person30 = new People(5655);
    $person31 = new People(5667);
    $person32 = new People(5673);
    $person33 = new People(5674);
    $person34 = new People(5681);
    $person35 = new People(5693);
    $person36 = new People(5709);
    $person37 = new People(5713);
    $person38 = new People(0007);
    $person39 = new People(5714);
    $person40 = new People(5721);
    $person41 = new People(5722);

    break;
case 4:
    print("Today is Thursday");


    $person1 = new People(5459);
    $person2 = new People(5630);
    $person3 = new People(5617);
    $person4 = new People(5596);
    $person5 = new People(1004);
    $person6 = new People(1709);
    $person7 = new People(5500);
    $person8 = new People(5542);
    $person9 = new People(1686);
    $person10 = new People(5328);
    $person11 = new People(5430);
    $person12 = new People(1365);
    $person13 = new People(1426);
    $person14 = new People(1799);
    $person15 = new People(5304);
    $person16 = new People(1769);
    $person17 = new People(1715);
    $person18 = new People(5307);
    $person19 = new People(1877);
    $person20 = new People(5152);
    $person21 = new People(1187);
    $person22 = new People(5580);
    $person23 = new People(5579);
    $person24 = new People(5543);
    $person25 = new People(5620);
    $person26 = new People(1920);
    $person27 = new People(5030);
    $person28 = new People(5475);
    $person29 = new People(1377);
    $person30 = new People(5649);
    $person31 = new People(5655);
    $person32 = new People(5667);
    $person33 = new People(5673);
    $person34 = new People(5674);
    $person35 = new People(5681);
    $person36 = new People(5693);
    $person37 = new People(5709);
    $person38 = new People(5713);
    $person39 = new People(0007);
    $person40 = new People(5707);
    $person41 = new People(5714);
    $person42 = new People(5721);
    $person43 = new People(5722);




    break;
case 5:
    $person1 = new People(5630);
    $person2 = new People(5617);
    $person3 = new People(5596);
    $person4 = new People(1004);
    $person5 = new People(1709);
    $person6 = new People(5500);
    $person7 = new People(5542);
    $person8 = new People(1686);
    $person9 = new People(5328);
    $person10 = new People(5430);
    $person11 = new People(1799);
    $person12 = new People(5304);
    $person13 = new People(1769);
    $person14 = new People(1715);
    $person15 = new People(5307);
    $person16 = new People(1877);
    $person17 = new People(5152);
    $person18 = new People(1187);
    $person19 = new People(5530);
    $person20 = new People(5580);
    $person21 = new People(5579);
    $person22 = new People(5543);
    $person23 = new People(5620);
    $person24 = new People(1920);
    $person25 = new People(5030);
    $person26 = new People(5475);
    $person27 = new People(5649);
    $person28 = new People(5655);
    $person29 = new People(5673);
    $person30 = new People(5674);
    $person31 = new People(5681);
    $person32 = new People(5693);
    $person33 = new People(5709);
    $person34 = new People(5713);
    $person35 = new People(0007);
    $person36 = new People(5707);
    $person37 = new People(5714);
    $person38 = new People(5721);
    $person39 = new People(5722);
    print("Today is Friday" . "<p>");
    break;
case 6:
    print("Today is Saturday");
    break;
}
share|improve this question
    
what are those numbers ? new People(number) ... –  JaDogg Aug 13 at 13:36
    
@Bhathiya-JaDogg-Perera they are the persons clock number, it's what i am using in the query to find the person information –  Josh Kirkpatrick Aug 13 at 13:51
    
What is a clock number ? –  JaDogg Aug 13 at 13:52
    
More importantly, how did you obtain that particular list of employees' clock numbers? –  200_success Aug 13 at 16:21

3 Answers 3

There are numerous issues with the code you posted here, and I will discuss them some more in due time. For now, I'll just give you a brief list of issues, with some references to other posts, and pages containing general information that you should look into.

Your code uses classes, but it's far from object-oriented. You may have come across the S.O.L.I.D. principles, but if you haven't: make sure to check them out ASAP.
Your code violates the Single Responsability Principle, for example. This principle states that one class should have one, distinct job. This can be anything from connecting to a remote DB server, to containing simple data. Your class does both of these things, and everything in between. That's bad

You should also get into the habbit of pre-declaring any properties you want your class to have/use. ATM, you are creating a public property Op in the constructor. Not only are pre-declared properties more performant, they also allow you to specify their visibility (protected or private). more details here.

This Op property is blindly being assigned whatever the constructor receives as an argument. But the same value is being concatenated into a query string. That means you are producing code that is vulnerable to injection. Again, this is bad. Using prepared statements is just a whole lot safer, check the manual of odbc_prepare for details.
Also consider adding checks on all of the arguments you are receiving: using type-hints, is_numeric, ctype_* and casts will get you a long way.

I've often stated, and will continue to spread the word, that classes should never echo, or exit, let alone die, see "separation of output" here, and read through the rest of my review, if you like.
A class should be a portable singularity. A piece of code that stands on itself, but can't do anything without external interactions (like a light switch can turn the lights on or off, but only if it's operated). A class, then, should be written in a way that never assumes that its job is critical. If a class can't do its job, it shouldn't kill the entire app with die or exit, it should notify the user (the code that is accessing the instance) by throwing exceptions. Here's another review where I mention/explain this.
Of course, 9/10 times, a DB connection error means the app will crash, but the class that connects to the DB shouldn't make that decision. By throwing an exception, the rest of the application has a chance to catch said exception, write stuff to the logs, and close any streams or other resources that might need closing. It's also not unlikely that you'd want to send a neatly formatted error message to the end user. using exit() or die does not allow you to do this.

Coding style is important. Try to stick to the most commonly accepted standard. Things like Public should be public, and non0-static methods should begin with a lower-case character, and their names are camelCased... Indentation == 4 spaces, no tab chars and all that. The moment you start collaborating on code, and you start using SVC tools like git, you'll see why this matters.

Don't suppress errors: @ $Out = strtotime($data[3]) + 2 * 60 * 60; uses the error suppressing @ of death. Don't. If there is an error, don't ignore it, Fix it! Errors, notices and warnings are there for one reason alone: to let you know that your code might contain bugs. If you write something like

public function foo()
{
    $this->foobr[] = '123';//typo
}

A notice will be raised, because you are using a non-existent property "foobr" as an array. You'll see the notice, and fix the typo. If you ignore such notices, you can waste hours, if not days, trying to figure out why the foobar property isn't being updated when you call the foo method.
When developing, always set your error reporting to the max, and display all errors. Always strive to write notice-free code!

share|improve this answer

Just a couple of points for now:

http-equiv="refresh"

The use of meta refresh is discouraged by the w3c

People Array Creation

Try using a loop to create this array using variable variables.

The way you are currently doing it is hard to maintain (You already have bugs in it: 'Person38' => $person39->GetTimes(), 'Person38' => $person40->GetTimes(), etc.)

Magic Numbers

2 and 60 are magic numbers and should be stored in a properly named field.

CardCode

You set $CardCode = (odbc_result($rs, "CardCode")); but don't seem to use it. In that case, remove it.

PeopleSelect.php

I don't know how you decide what numbers you are using, but check if you can do this differently. The way it currently is, this will be very hard to maintain.

share|improve this answer
    
Hi thank you for your answer, this script running the refresh is only ever used directly on the server so it's not to much of an issue, i guess it would be better to just put it in to a scheduled task or something The PeopleSelect.php uses numbers that are assigned to the employees details like a unique identifier for each person for example i could be 0001 and then the next person could be 0002 and so on and so on so i cant really think of a better way to do this as the people don't go from a set range of like 5000 to 5030 for –  Josh Kirkpatrick Aug 13 at 13:58

This is not the most important aspect of your script, but I wanted to answer about...

SQL formatting

I recommend using line breaks and indentation to make your query easier to read, as inline SQL can be difficult to read, doubly so when using column aliases. Also, optionally, meaningful table aliases would make it less verbose. Here is my formatting suggestion:

$sql = "
  SELECT     
    ATDSchedulingRule.Name, 
    ATDSchedulingRule.Remark, 
    ATDShiftDetail.WorkHour, 
    RIGHT(ATDShiftDetail.StartTime, 6) AS StartTime, 
    RIGHT(ATDShiftDetail.EndTime, 6) AS EndTime, 
    RIGHT(CHINA_VISION_PubPersonnel.ID, 4) AS ClockNo, 
    CHINA_VISION_PubCards.CardCode
  FROM         
    ATDShiftDetail 
    INNER JOIN ATDSchedulingRule 
      ON ATDShiftDetail.ATDShift_Ref = ATDSchedulingRule.Shift_Ref2 
    INNER JOIN CHINA_VISION_PubCards     
      ON CHINA_VISION_PubCards.PubPersonnel_Ref = CHINA_VISION_PubPersonnel.Reference
    INNER JOIN CHINA_VISION_PubPersonnel 
      ON ATDSchedulingRule.Name = RIGHT(CHINA_VISION_PubPersonnel.ID, 4)
  WHERE (ATDShiftDetail.WorkHour > 0) 
    AND (CHINA_VISION_PubPersonnel.Telephone > '1') 
    AND ATDSchedulingRule.Name = '$this->Op' 
";
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.