Get and display employee time clock data

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.

```
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 (!$con

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!

StackExchange Code Review Q#59902, answer score: 6