OOP - Class Code Review

[benanamen]

The following is my further attempt to learn OOP. Is there anything technically wrong with it or is there any better practices in the code or its use? Purpose is to display dev/debugging data. Your feedback is appreciated.

 

* Code in forum is on one page so you can easily run it if desired.

<?php
/**
 * Displays data from $_COOKIE, $_SESSION, $_GET, $_POST, $_REQUEST
 * @param string $debug COOKIE, SESSION, GET, POST, REQUEST
 */

class DebugDisplay
    {
    private $title;
    private $debug;

    public function __construct($title, $debug)
        {
        $this->title = $title;
        $this->debug = $debug;
        }

    public function ShowDebug()
        {
        echo '<pre><span style="color:red;font-weight:bold">';
        echo $this->title . '<br>';
        print_r($this->debug);
        echo '</span></pre>';
        }
    }

//----------------------------------------------------------------------------------------
// Test Data
//----------------------------------------------------------------------------------------

$_COOKIE[]  = 'COOKIE Data';
$_SESSION[] = 'SESSION Data';
$_REQUEST[] = 'REQUEST Data';
$_GET[]     = 'GET Data';
$_POST[]    = 'POST Data';

//----------------------------------------------------------------------------------------
// Debugging
//----------------------------------------------------------------------------------------


define("DEBUG", true); // Toggle Debugging

define("SHOW_DEBUG_PARAMS", DEBUG); // Display Sql & Sql Parameters
define("SHOW_SESSION_DATA", DEBUG); // Display Session Data
define("SHOW_POST_DATA", DEBUG); // Display Post Data
define("SHOW_GET_DATA", DEBUG); // Display Get Data
define("SHOW_COOKIE_DATA", DEBUG); // Display Cookie Data
define("SHOW_REQUEST_DATA", DEBUG); // Display Request Data

if (DEBUG === true)
    {
    echo '<div class="error_custom"><H1>DEBUGGING IS ON !!!</H1></div>';
    }

if (SHOW_COOKIE_DATA === true)
    {
    $show = new DebugDisplay('COOKIE', $_COOKIE);
    echo $show->ShowDebug();
    }

if (SHOW_SESSION_DATA === true)
    {
    if (isset($_SESSION))
        {
        $show = new DebugDisplay('SESSION', $_SESSION);
        echo $show->ShowDebug();
        }
    }

if (SHOW_POST_DATA === true)
    {
    $show = new DebugDisplay('POST', $_POST);
    echo $show->ShowDebug();
    }

if (SHOW_GET_DATA === true)
    {
    $show = new DebugDisplay('GET', $_GET);
    echo $show->ShowDebug();
    }

if (SHOW_REQUEST_DATA === true)
    {
    $show = new DebugDisplay('REQUEST', $_REQUEST);
    echo $show->ShowDebug();
    }

Application Output

 

[Jacques1]

• It doesn't really make sense to instantiate a class just for the trivial purpose of making a fancy variable inspection. This should be a function or, if you insist on OOP, a static method.
• The hard-coded HTML output violates the reusability principle. If I want to have debug information in CLI mode, I cannot use the class at all. Even a different formatting isn't possible.
• Method names are supposed to use camelCase according to PSR-1, not PascalCase.
• The class description is confusing. It claims that the class is meant specifically for the PHP superglobals, but the actual implementation has no reference to the superglobals at all. It's a general-purpose debug display which can for example be used for the superglobals.
• What's the @param doing in the class description?

 

A better approach would be to first define a generic interface for debug information and then subclass it for different formattings. This also allows custom formattings:

<?php



interface VarDumper
{
    function dump($title, $data);
}

class HTMLVarDumper implements VarDumper
{
    function dump($title, $data)
    {
        // output for HTML contexts
    }
}

class CLIVarDumper implements VarDumper
{
    function dump($title, $data)
    {
        // output for CLI contexts
    }
}

// the class user is free to define custom classes as well

[benanamen]

It doesn't really make sense to instantiate a class just for the trivial purpose of making a fancy variable inspection. This should be a function or, if you insist on OOP, a static method.

 

I had it as a function. I am just trying to learn OOP. Can you give an example of a static method please.

 

The hard-coded HTML output violates the reusability principle. If I want to have debug information in CLI mode, I cannot use the class at all. Even a different formatting isn't possible.

 

Good to know. I was wondering about the html when I did it. For the purpose of viewing the superglobal arrays, what other format could you possibly want besides print_r?

• Method names are supposed to use camelCase according to PSR-1, not PascalCase.

Yes, I know. I noticed it after I posted. Too late to edit post.

 

The class description is confusing. It claims that the class is meant specifically for the PHP superglobals, but the actual implementation has no reference to the superglobals at all. It's a general-purpose debug display which can for example be used for the superglobals.

 

I realized later that the superglobals are not the only thing that could be passed to the class. Since you pointed it out, I can see how the name could be confusing. In another post I asked how to properly use superglobals with a class. The answer I got was not particularly helpful. What would be a name that makes sense to you?

• What's the @param doing in the class description?

It should actually go right above the constructor right? I noticed later @param $title was missing.

 

A better approach would be to first define a generic interface for debug information and then subclass it for different formattings. This also allows custom formattings:

 

Right now I don't know what that means. Any other info would be helpful. How would I use the code you posted?

Edited January 16, 2017 by benanamen

[Jacques1]

The class name is fine. You just shouldn't claim in the description that the class is written specifically for the PHP superglobals.

 

Yes, the @param tag belongs to method descriptions.

 

An interface can be used as an abstraction layer. When a task may be implemented in different ways, you first define an interface which merely contains the method signatures (i. e. the names and parameters). The implementation is then left to the concrete classes below the interface, which provides a lot of flexibility. This is what my example does: The interface merely says that a VarDumper has a dump() method with two parameters. How exactly the method works is not specified yet. This is up to the classes which implement the interface (in my case HTMLVarDumper and CLIVarDumper).

 

Using the code is simple: You choose the appropriate class for the current context and instantiate it. The application can then use this instance and be sure that it will always get the right formatting.

 

If you always want HTML output:

$varDumper = new HTMLVarDumper();

// ...

$varDumper->dump('POST', $_POST);

If you want HTML or CLI output depending on the server API:

<?php

if (substr(php_sapi_name(), 0, 3) == 'cli')
{
    // CLI output
    $varDumper = new CLIVarDumper();
}
else
{
    // default: HTML output
    $varDumper = new HTMLVarDumper();
}

// ...

$varDumper->dump('POST', $_POST);    // the concrete VarDumper implementation is irrelevant at this point

[benanamen]

Once you showed the last code, yes it is easy. What is the proper way to document the code? Do you do it at ALL the functions or just the interface function? Seems redundant if documentation is exactly the same for each function. I know function alone is the same as public function. Is it best practice to always use public anyways?

 

This is what I have done with your help. Any problems? I have not messed with CLI yet so no clue about that part right now.

<?php
/**
 * Dumps data
 */

interface VarDumper
{
    function dump($title, $data);
}

class HTMLVarDumper implements VarDumper
{
/**
 * @param string $title
 * @param ?? $data
 */
    function dump($title, $data)
    {
    echo '<pre><span style="color:red;font-weight:bold">';
    echo $title . '<br>';
    print_r($data);
    echo '</span></pre>';
    }
}

class CLIVarDumper implements VarDumper
{
    function dump($title, $data)
    {
        // output for CLI contexts
    }
}

// the class user is free to define custom classes as well

$_POST     = ['username' => 'MyUsername', 'email' => 'user@example.com'];
$varDumper = new HTMLVarDumper();
$varDumper->dump('POST', $_POST);

[Jacques1]

You document the interface methods. The methods of the implementing classes automatically inherit this documentation, unless you override it.

 

The explicit public modifier should be used for clarity. Interfaces are an exception, because they can only specify public methods.

<?php

/**
 * A debug tool to print the content of variables (e. g. $_POST)
 */
interface VarDumper
{
    /**
     * Prints a value
     *
     * @param string $title the title to be printed
     * @param mixed  $data  the value
     */
    function dump($title, $data);
}

[benanamen]

Just integrated the code and having a file naming issue. The interface is named VarDumper and there are two class names, HTMLVarDumper, CLIVarDumper.

 

The auto class loader will look for a class file named the same as the "new classname" which for my current use is new HTMLVarDumper(). 

 

If I name the file HTMLVarDumper it negates that it is also a CLIVarDumper. (Currently not implemented though)

 

VarDumper is the more fitting filename but it won't work with the autoloader since that is not one of the class names. Is there some OOP thing I don't know about or do I just name the file HTMLVarDumper.class.php and be done with it?

 

Edit* Wouldnt the class CLIVarDumper best be used as an extended child, thus removing the class from the HTMLVarDumper file? This would also follow the one class per file PSR right? If so how and where would you document that there is an extended CLI class of the same purpose so that it makes sense when you see the interface usage? Would the CLI class then go in a separate folder just for CLI classes?

Edited January 17, 2017 by benanamen

[Jacques1]

Every class and every interface goes into its own file. So there are three separate files in this case.

 

There is no special treatment for the CLI class. It's just one of currently two implementations of the interface. You don't and cannot put it into a separate folder, because autoloaders expect a specific file system structure (folders typically correspond to namespaces).

[benanamen]

I have read up on interfaces and it is not clicking yet. If I have two separate classes why do I need an interface when I can just call one or the other depending on if I want the html class or the CLI class.

 

Additionally, If I am not going to use anything other than the html class why would it need an interface?

[maxxd]

If you're never going to use anything other than the HTML class, there's no need for an interface.

 

Interfaces dictate consistency of functionality across your application. If a method is defined in an interface, the class that implements that interface must implement that method. This means that every class in your *VarDumper family will have a consistent API and you don't have to worry about which specific VarDumper you're using - just call the dump() method because you know for a fact it's there, and you know what parameters need to be passed and in which order. Another great thing about interfaces is that the implementing class inherits the interface type for use in typecasting.

 

So, if you're working a class that makes use of a VarDumper class, you don't need to write a separate method for

callCLIVarDumper(CLVarDumper $dumper){}

and

callHTMLVarDumper(HTMLVarDumper $dumper){}

You'd simply use:

callVarDumper(VarDumper $dumper){}

Again, because of the defined interface, the master class doesn't really care what type of VarDumper it's using, it just knows it can use it and how to do so.

 

However, all three of the above are valid and will work, so if you do need the specificity of calling a method to work with, for example, only an HTMLVarDumper you can do that as well.

 

Hopefully this makes some sense - I'm only about halfway through my second cup of coffee...

[Jacques1]

Just dropping a class into your code may be fine for one-off jobs where you don't give a shit about quality. But if you want a proper OOP infrastructure for a long-term project, you want to program against interfaces rather than concrete classes. This has lots of benefits:

• It separates the API from the implementation. Most of the time, it's completely irrelvant how exactly you've written your code. The HTML elements or echo statements are just trivial details. What matters is the API, and that's what the interface conveys.
• It forces you to actually design your API instead of just writing ad-hoc code.
• It makes your code modular and extensible. I understand that you only need this one implementation right now, but things change. A lot of people thought that they'd only ever need the mysql_* functions to query their database, now they have to rewrite their entire code.
• It's the basis for testing. If you have hard-coded class references everywhere, there's almost no chance for testing an object in isolation. With an interface, you can just plug in a dummy object.

This may seem very theoretical to you right now. But once you start using OOP code from other programmers (libraries, frameworks, ...), you'll quickly see that there's a big difference between good infrastructures and bad infrastructures. A good infrastructure can easily be adjusted and just works. A bad infrastructure gets in your way if you don't do exactly what the author expects you to do.

[benanamen]

If you're never going to use anything other than the HTML class, there's no need for an interface.

 

Interfaces dictate consistency of functionality across your application. If a method is defined in an interface, the class that implements that interface must implement that method. This means that every class in your *VarDumper family will have a consistent API and you don't have to worry about which specific VarDumper you're using - just call the dump() method because you know for a fact it's there, and you know what parameters need to be passed and in which order. Another great thing about interfaces is that the implementing class inherits the interface type for use in typecasting.

 

So, if you're working a class that makes use of a VarDumper class, you don't need to write a separate method for

callCLIVarDumper(CVarDumper $dumper){}

and

callHTMLVarDumper(HTMLVarDumper $dumper){}

You'd simply use:

callVarDumper(VarDumper $dumper){}

Again, because of the defined interface, the master class doesn't really care what type of VarDumper it's using, it just knows it can use it and how to do so.

 

However, all three of the above are valid and will work, so if you do need the specificity of calling a method to work with, for example, only an HTMLVarDumper you can do that as well.

 

Hopefully this makes some sense - I'm only about halfway through my second cup of coffee...

 

Your answer reads to me that I can just call the master class (interface VarDumper) and it "knows"  whether to use interface VarDumper or HTMLVarDumper, but that is not possible. Perhaps you can clarify. I don't get your third example. Reads to me like you are saying $callVarDumper = new VarDumper(VarDumper $dumper); which is not possible.

[ignace]

It doesn't know that's the entire point.

 

someCodeThatCanDumpThings(VarDumper $dumper)

 

The only thing that this function/method knows is that it can call $dumper with a method dump() which has certain arguments. The implementation details however it is unaware of. If this is to HTML or CLI or SOAP. It doesn't actually care.

 

Through dependency injection you'll define what the implementation is going to be for the VarDumper interface.

[benanamen]

After much study I am left with just a few questions.

 

1. Interface file naming- Should an Interface file name be named something.interface.php?

 

2. To use term interface in the interface name or not? When a class implements something, the only thing it could be is an interface so why would you name the interface with the term interface in it? What is best practice and why?

 

Symfony adds "Interface", Laravel is mixed

 

Laravel

use Psr\Log\LoggerInterface as PsrLoggerInterface;
use Illuminate\Contracts\Logging\Log as LogContract;


class Writer implements LogContract, PsrLoggerInterface

Symfony

abstract class AbstractDumper implements DataDumperInterface, DumperInterface
class TestEventSubscriber implements EventSubscriberInterface
class TestEventSubscriberWithPriorities implements EventSubscriberInterface

3. When should I use an interface?

 

4. Should ALL classes be interfaced?

 

*******************************************************************

Here is what I now understand. Anything not correct here?

*******************************************************************

 

Interfaces can only specify public methods therefore no need to do "public function"

You cannot ever call an Interface directly. Wrong/errors e.g $interface = new myInterface();

 

Classes that use an interface MUST have the methods in the Interface or it will error

 

A class can implement more than one interface. Names are separated by commas.

e.g class MyClass implements interface1, interface2, interface3

 

Interface methods have no 'guts' to them, no body.

 

Interfaces are essentially a blueprint for what you can create. They define what methods a class must have, but you can create extra methods outside of those limitations. - stack

 

An interface says, “This is what all classes that implement this particular interface will look like.” Thus, any code that uses a particular interface knows what methods can be called for that interface, and that’s all. - stack

 

if you want shorter names, easier refactoring and semantic meaning you should name your types what they represent and not what they are. https://phpixie.com/blog/naming-interfaces-in-php.html

 

Interface sets the master rules for classes that use it. For example, if I want to output the same data to JSON, XML, CLI, or HTML, all of those classes implementing the Interface MUST have at least the interface method(s). 

 

implements keywords MUST be declared on the same line as the class name like extends. PSR-2

 

Lists of implements MAY be split across multiple lines, where each subsequent line is indented once. When doing so, the first item in the list MUST be on the next line, and there MUST be only one interface per line. PSR-2

 

*****************************************************************

[benanamen]

It doesn't know that's the entire point.

 

someCodeThatCanDumpThings(VarDumper $dumper)

 

The only thing that this function/method knows is that it can call $dumper with a method dump() which has certain arguments. The implementation details however it is unaware of. If this is to HTML or CLI or SOAP. It doesn't actually care.

 

Through dependency injection you'll define what the implementation is going to be for the VarDumper interface.

 

The full working class is in post #5. Can you show me what you just said with the actual class. I can understand code a lot better than explanations of a new concept I am just learning.

 

There are only two working ways I know of so far to use the group of 3 classes (interface, html class, cli class)

 

$varDumper = new HTMLVarDumper();

$varDumper->dump('POST', $_POST);  

 

OR

 

$varDumper = new  CLIVarDumper();

$varDumper->dump('POST', $_POST); 

[Jacques1]

As I already said in #11, it's good practice to always start with an interface and program to that interface rather than any concrete class. The only time you ever reference a class is when you instantiate it to provide an implementation. Or when you use a PHP core class.

 

Personally, I would never mention the term “interface” in the name or filename of an interface, because this is redundant information. It's like starting every function name with “function”.

 

 

 

There are only two working ways I know of so far to use the group of 3 classes (interface, html class, cli class)

 

$varDumper = new HTMLVarDumper();

$varDumper->dump('POST', $_POST);  

 

OR

 

$varDumper = new  CLIVarDumper();

$varDumper->dump('POST', $_POST); 

 

If you instantiate the class at a central location and then pass it to any object which needs a variable dumper, yes, this is a valid approach.

 

There are more advanced concepts like DI containers, but I wouldn't do too much at the same time. If you understand why interfaces are useful, you're definitely ready to write proper OOP code.

[benanamen]

As I already said in #11, it's good practice to always start with an interface and program to that interface rather than any concrete class. 

Got it! So then I should go back to the class LoginAttemptsLog that we worked on and do an Interface for that right? 

 

Personally, I would never mention the term “interface” in the name or filename of an interface, because this is redundant information. It's like starting every function name with “function”.

That makes more sense then all the back and forth I read about it. Do you know of any official documentation why Symfony does it that way or what their thinking was?

 

I will look into DI containers after Interfaces soaks in for awhile. Thanks for all the help. I believe I am getting Interfaces now. It makes sense when I think I may want output in JSON, XML, HTML or something else.

[Jacques1]

Got it! So then I should go back to the class LoginAttemptsLog that we worked on and do an Interface for that right?

 

Yes. This is actually a good example for a highly specialized class which you may very well have to replace at some point. The hard-coded queries won't even work for all SQL database systems, let alone any non-SQL storage.

 

 

 

Do you know of any official documentation why Symfony does it that way or what their thinking was?

 

I don't, and I personally don't really care about project-specific conventions unless I'm actually working on the project.

 

Suffixes can make sense if you don't have an IDE or just like to be super-explicit. Some people also like to prefix all names. I do have an IDE, so this is all just clutter to me.

[benanamen]

This is actually a good example for a highly specialized class which you may very well have to replace at some point. The hard-coded queries won't even work for all SQL database systems, let alone any non-SQL storage.

 

That really turned on the light bulb. Your previous in-code comment "// the class user is free to define custom classes as well" really falls into place now. I could see how a "class user" could want to log the same data to NoSQL or a text file.

 

In the current VarDumper code you have made it clear there are three class files involved.

 

1. For the interface that would give us VarDumper.class.php with the following code. What would be the proper way to document an interface? Do we use the same documentation from the HTMLVarDumper class?

 

VarDumper.class.php

<?php
interface VarDumper
{
    function dump($title, $data);
}

2. My question now goes to the app directory structure best practice for class subdirectories. Would/should all three of these classes go in say a Debugging directory in the main class directory?

 

For example, The Laravel structure is

vendor\laravel\framework\src\Illuminate\    //Class root
vendor\laravel\framework\src\Illuminate\Cookie
vendor\laravel\framework\src\Illuminate\Database
vendor\laravel\framework\src\Illuminate\Session

and so on.

 

So would/should I do similar?

vendor\benanamen\src\MywhateverName\Debugging
vendor\benanamen\src\MywhateverName\Logging

[Jacques1]

I've already answered the documentation question in #6. You document the interface and then let the classes inherit this documentation -- unless one of the classes is so special that you want a class-specific documentation, in which case you can just override the parent documentation.

 

The directory structure should correspond to the namespace structure. Don't create random directories. If you want the classes to be in a Debugging subdirectory, they have to actually be in a Debugging namespace.

[benanamen]

I see how the classes are inheriting the interface documentation when I generate the docs. It seems I have the interface knowledge mastered now. Thanks for helping me understand it! 

[benanamen]

I updated the LoginAttemptsLog.class.php to implement an interface. All works as expected. I would like feedback to make sure it is correct and the interface name LoginLogger.class.php is suitable for this Interface as well as the documentation.

 

Secondly, I purposely added an extra char to an interface function to cause an error. What I noticed is that my set_exemption_handler function is not being called for this fatal error. Is there a different handling for class errors?

Fatal error: Class LoginAttemptsLog contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (LoginLogger::logSuccessfulAttemptX)

The new interface LoginLogger.class.php

<?php
/**
 * A login attempt logger
 */
interface LoginLogger
{
    /**
     * Logs a login attempt
     *
     * @param string $username the attempted username to log
     */
    function logFailedAttempt($username);
    function logSuccessfulAttempt($username);
}

LoginAttemptsLog.class.php

<?php
class LoginAttemptsLog implements LoginLogger
{
    /**
     * @var PDO the connection to the underlying database
     */
    protected $database;

    /**
     * Connection to underlying database
     * @param PDO $database
     */
    public function __construct(PDO $database)
    {
        $this->database = $database;
    }

    /**
     * Log failed login attmpt
     * @param string $username
     */
    public function logFailedAttempt($username)
    {
        $this->logAttempt($username, false);
    }

    /**
     * Log succesful login attempt
     * @param string $username
     */
    public function logSuccessfulAttempt($username)
    {
        $this->logAttempt($username, true);
    }

    /**
     * Insert login attempt status
     * @param string  $username
     * @param boolean $successful
     */
    protected function logAttempt($username, $successful)
    {
        $attemptStmt = $this->database->prepare('
            INSERT INTO
              user_login (login_status, login_ip, login_username, login_datetime)
            VALUES
              (?, INET_ATON(?), ?, NOW())
        ');
        $attemptStmt->execute([($successful ? 1 : 0), $_SERVER['REMOTE_ADDR'], $username]);
    }
}

Edited January 19, 2017 by benanamen

[Jacques1]

An interface shouldn't be stored in a “.class” file. The “.class” suffix should actually go away entirely, because it's redundant. What else would be stored in such a script?

 

The name “LoginAttemptsLog” is unfortunate, because it implies that the class is very generic and pretty much the only valid implementation. But it's the exact opposite: It's highly specialized and just one of many possible implementations. There could also be a file-based logger, a MongoDB logger and whatnot. How are you going to name those? So something like “SQLLoginAttemptsLog” is more appropriate.

 

Methods must be documented individually. You should also describe the parameters (unless it's absolutely clear what they do).

 

An exception handler only catches exceptions, not classical errors. In PHP 5, the only reliable way to handle all errors is to register a shutdown function.

Edited January 19, 2017 by Jacques1

[benanamen]

An interface shouldn't be stored in a “.class” file. The “.class” suffix should actually go away entirely, because it's redundant. What else would be stored in such a script?

 

Just the interface class or the concrete classes that go with it as well. Or do you just mean any class file period?

 

The name “LoginAttemptsLog” is unfortunate, because it implies that the class is very generic and pretty much the only valid implementation. 

 

That's interesting because that's the name you came up with that I adopted. Was it an ok name before the update to an interface or you just get a fresh look at it? (https://forums.phpfreaks.com/topic/302286-oop-code-review/?do=findComment&comment=1538061)

 

Luckily, none of this is really unfortunate. This app is all strictly to learn OOP so anything can be changed at any time.

 

It's highly specialized and just one of many possible implementations. There could also be a file-based logger, a MongoDB logger and whatnot. How are you going to name those? So something like “SQLLoginAttemptsLog” is more appropriate.

 

This is exactly what I was considering in the naming of the Interface itself. I understood from the last few days that an Interface can have many implementations like you just mentioned. In light of the class now being an implementation I agree the name should be more specific as you have suggested.

 

Methods must be documented individually. You should also describe the parameters (unless it's absolutely clear what they do).

 

I took that to mean doing this.

<?php
/**
 * A login attempt logger
 */
interface LoginLogger
{
    /**
     * Logs a failed login attempt
     *
     * @param string $username the attempted username to log
     */
    function logFailedAttempt($username);

    /**
     * Logs a successful login attempt
     *
     * @param string $username the attempted username to log
     */
    function logSuccessfulAttempt($username);
}

[Jacques1]

Just the interface class or the concrete classes that go with it as well. Or do you just mean any class file period?

 

All interfaces, classes, traits, ....

 

Like I said, the suffix is redundant. It's just clutter.

 

 

 

That's interesting because that's the name you came up with that I adopted. Was it an ok name before the update to an interface or you just get a fresh look at it?

 

It's an acceptable name for “naive” OOP where a single implementing class is baked into the application. It doesn't work for more advanced OOP where the number of implementations is potentially infinite.