occurred Mar 5, 2021 at 12:58

Typo fix

edited Mar 3, 2021 at 4:05

Anit Shrestha

• 489
• 4
• 11

Inline Side-by-side Side-by-side Markdown

Add one more point to improve.

edited Mar 1, 2021 at 18:07

Anit Shrestha

• 489
• 4
• 11

Inline Side-by-side Side-by-side Markdown

answered Mar 1, 2021 at 17:57

Anit Shrestha

• 489
• 4
• 11

FYI - I'm a bit rusty with PHP and not a PHP expert in anyways.

TL;DR

• Be strict on coding style and standard.
• Clean the class responsibility, implement (at least) Single Responsibility Principle.
• Make smaller functions.
• Write Unit test.

---

Retrospect

Use Strict!

This generated a lot of errors that was not there before.

<?php declare(strict_types=1);

Access Specifiers

Private, Protected and Public are used, which is great to see but I think there has been put a lot of thought for doing it.

Class

In general, class OutputHandler is too large, more than 400 lines.The standard rule would be maximum lines of a Class is around 100.

It has too many functionalities which could be separated out in another class of just utility functions which can be called directly.

Like IsCommandLineInterface() we can move other common usage functions to a separate file so that our class could be cleaner. Or if are moving with a pure Object Oriented approach then probably make another class.

I think the best way would be to make separate classes for CLI and Web version. We can have a OutputHandler Abstract base class with some similary functionalities which can be extended by class CliOutputHandler and WebOutputHandler child classes to implement needed features dependently.

Furthermore,I can see the validation process could be move out to a separate class just with the validation functionality.

And some more classes are possible for other small utility purposes which could be used just as a function but as I have mentioned let go complete OO.

Constructor

We might want to do pass example array kind of input variable here and do the validation in the constructor. It would be a good practice.

public function __construct($theme = 'default')
{
 $this->getTheme($theme);
 // This is variable is not used so remove. 
 // $this->defenv = ENVIRONMENT_OUTPUT_HANDLER;
}

Function getTheme()

public function getTheme(string $theme = 'default'): void
{
 if (isset(self::THEMESLIST[$theme])) {
 $color = self::THEMESLIST[$theme];
 $this->themeused = $theme;
 
 } else {
 $color = self::THEMESLIST['default'];
 $this->themeused = 'default';
 }
 IsCommandLineInterface() ? 
 $this->setHighlightThemeCli($color)
 : $this->setHighlightTheme($color);
 
} 

Function output()

I can see the goodness of strongly type being implemented in few functions. What I would love to see is that, that standard being maintained in this function signature as well.

Minor modification is done in the implementation.

public function output($var, $env = null, $retrieve = false)
{
 
 $indents = $this->getIndent($var);
 $string = $this->analyzeVariable($var, $indents);
 if (IsCommandLineInterface() ) {
 $string = $this->highlightCodeCli($string);
 } else {
 $string = $this->highlightCode($string);
 }
 $string = $this->applyCss($string);
 $this->resetHighlight();
 return ($retrieve ? $string : $this->outView($string));
}

Function getIndent()

One of function I am not happy seeing as I am not able to understand what's the purpose of the function and the way it has been implemented.

We are returning indents array with [key => value] but we are doing trying to find the length of the array and objects, this is not done using mb_strlen.

private function getIndent($data): array
{
 $indents = ['key' => 0, 'val' => 0];
 if ( is_array($data) || is_object($data) ) {
 
 $data = is_object($data) ? (array) $data : $data;
 
 // This literally does not make any sense.
 // Why are we looping and assigning value to 
 // the same variable $value? And the $value is 
 // used in the local scope.
 // array_walk_recursive($data, function (&$value) {
 // $value = is_object($value) ? (array) $value : $value;
 // });
 
 // THIS WILL GIVE ERROR!
 // var_dump($value);
 $deep = ($this->calcDeepArray($data) + 1) * 4;
 
 array_walk_recursive($data, function ($value, $key) use (&$indents) {
 if (mb_strpos(strval($key), chr(0)) !== false) {
 $key = str_replace(chr(0), "'::'", $key);
 $key = substr($key, 4);
 }
 
 // It does not make sense to use mb_strlen for
 // array and objects.
 if (!is_array($value) 
 && !is_object($value) 
 && !is_resource($value) 
 && !is_null($value)) {
 $indents['val'] = ($indents['val'] >= mb_strlen(strval($value))) 
 ? $indents['val'] : mb_strlen(strval($value));
 $indents['key'] = ( $indents['key'] >= mb_strlen(strval($key)) ) 
 ? $indents['key'] : mb_strlen(strval($key));
 }
 
 }, $indents);
 $indents['key'] += $deep;
 $indents['val'] += $deep / 2;
 } else {
 $indents = ['key' => mb_strlen('variable'), 'val' => mb_strlen($data)];
 }
 return $indents;
}

function analyzeVariable

This is a bit messy. Let try to remove the recursion and check it other simple way. I believe the purpose of the function is to validate the variables in the input. A good way to design a API is to stick to some form of input standards. I think that usage of JSON format would be good as it helps to validate the schema easily. Here is an example of JSON Schema validator:

https://github.com/opis/json-schema

But it is not mandatory to do it.

protected function analyzeVariable($data, array $indents): string { $varname = 'variable';

// We are using anonymous function to do some 
// analysis but I believe there is a better 
// way to do it, even without the use of recursion!
// I am not sure what are the validation criteria
// as the function is too hard to understand what's
// going on.
$pretty = function ($indents, $varlentitle, $v = '', $c = " ", $in = 0, $k = null) use (&$pretty) {
 $r = '';
 
 // Previously, checking of object and array was done
 // like this:
 // if ( is_array($data) || is_object($data) ) {
 // Why are we chaning the way we check?
 if (in_array(gettype($v), ['object', 'array'])) {
 
 if (!is_null($k)) {
 if (mb_strpos($k, chr(0)) !== false) {
 $k = str_replace(chr(0), "'::'", $k);
 $k = substr($k, 4);
 }
 }
 $lenname = mb_strlen("'$k'");
 $lenkeys = $indents['key'] - $in - $lenname;
 if ($lenkeys < 0) {
 $lenkeys = 0;
 }
 $eval = $this->evaluateVariable($v);
 $v = (array) $v;
 $lenkey = $indents['val'] - mb_strlen($eval['val']) + 1;
 
 if (empty($v)) {
 $r .= ($in != 0 ? str_repeat($c, $in) : '') . (is_null($k) ? '' : "'$k'"
 . str_repeat($c, $lenkeys) . "=> " . $eval['val'] . "[],"
 . str_repeat(" ", $lenkey - 6) . "// "
 . $eval['desc']) . (empty($v) ? '' : PHP_EOL);
 } else {
 $r .= ($in != 0 ? str_repeat($c, $in) : '') . (is_null($k) ? '' : "'$k'"
 . str_repeat($c, $lenkeys) . "=> " . $eval['val'] . "["
 . str_repeat(" ", $lenkey - 4) . "// "
 . $eval['desc']) . (empty($v) ? '' : PHP_EOL);
 
 foreach ($v as $sk => $vl) {
 $r .= $pretty($indents, $varlentitle, $vl, $c, $in + 4, $sk) . PHP_EOL;
 }
 $r .= (empty($v) ? '],' : ($in != 0 ? str_repeat($c, $in / 2) : '') .
 (is_null($v) ? '' : str_repeat($c, $in / 2) . "],"));
 }
 } else {
 if (mb_strpos($k, chr(0)) !== false) {
 $k = str_replace(chr(0), "", $k);
 }
 $lenkey = $indents['key'] - mb_strlen("'$k'") - $in;
 if ($lenkey < 0) {
 $lenkey = 0;
 }
 $eval = $this->evaluateVariable($v);
 $lenval = $indents['val'] - (mb_strlen("'" . $eval['val'] . "'"));
 if ($lenval < 0) {
 $lenval = 0;
 }
 $r .= ($in != -1 ? str_repeat($c, $in) : '') . (is_null($k) ? '' : "'$k'"
 . str_repeat($c, $lenkey) . '=> ') . $eval['val']
 . str_repeat(" ", $lenval) . '// ' . $eval['desc'];
 }
 return str_replace("0円", "", $r);
};
// DO WE EVEN RECACH HERE?
// There is a return in the recursive function 
// and I'm not sure if we even reach down low.
$varlentitle = mb_strlen('$' . $varname);
if (in_array(gettype($data), ['object', 'array'])) {
 $string = '$' . $varname . str_repeat(" ", (($indents['key'] - $varlentitle) >= 0 ? $indents['key'] - $varlentitle : 1)) . '= ['
 . str_repeat(" ", $indents['val'] - 2) . '// main array node.'
 . rtrim($pretty($indents, $varlentitle, $data), ',') . ';';
} else {
 $eval = $this->evaluateVariable($data);
 $string = '$' . $varname . str_repeat(" ", $indents['key']) . '=' . $eval['val'] . ';'
 . str_repeat(" ", $indents['val'] - 1) . '// ' . $eval['desc'];
}
return $string;

}

Function evaluateVariable()

Well improvements can be here as well

if (is_object($var)) {
 // Is there any specific reason for it?
 // I am not able to figure out!
 ob_start();
 $string = explode('{', ob_get_clean());
 return ['val' => '(object) ', 'desc' => rtrim(reset($string)) . '.'];
}
// Im not sure why we need to do this?
// What are we storing in buffer and 
// reusing? And we are returning so what
// will be the purpose of doing this?
ob_start();
$string = ob_get_clean();
if (mb_strpos($string, 'resource') !== false) {
 return ['val' => 'resource', 'desc' => rtrim($string) . '.'];
} elseif (mb_strpos($string, 'of type ') !== false) {
 return ['val' => 'resource', 'desc' => rtrim($string) . '.'];
}
unset($string);
 

---

Conclusion

Well those were some things I could see but that is just my perception. There is a lot of room for improvement and I can see there are good signs that your are trying to do that, which is a promising sign :).

The first thing I would recommend I go through Clean Code, SOLID Principles, and Write Unit Test. PSR has already been mentioned so it would be good to follow them as well.

As I read from one of your replies that is in your future plan, I would say amend that plan immediately as writing test will help you become a good software engineer and a better programmer! And writing is not that hard, at all!

Here is a brief article on Unit Test : https://codeanit.medium.com/developers-guide-write-good-test-5e3e3cdec78e

To further improve the quality of your code you can use Static Analysis Tools, an example: https://github.com/squizlabs/PHP_CodeSniffer

I wish you all the very best!