3
\$\begingroup\$

Can anyone here please review my PHP code which preserves line break by splitting the line into as many line break as it have?

class PerserveLineBreak
{
    //Preserves line break in HTML element content while stripping by splitting the html element content into two items
    protected $text;
    protected $pattern;
    protected $preservedContent;

    function __construct($text, $pattern="<br />-")
    {
        $this->text = $text;
        $this->pattern = $pattern;
        $this->init();
    }

    function init(){
            $this->preservedContent = $this->perserveLineBreak($this->text,$this->pattern);
    }

    function preparePattern($pattern)
    {
        //adding delimiter and use preg_quote
        $pattern = "#" . $pattern . "#";
        return preg_quote($pattern);
    }

    function getPreservedContent(){
        return $this->preservedContent;
    }

    function perserveLineBreak($content, $searchPattern)
    {
        //looks in for <br /> by default
        return $this->stripListOfString((preg_split($this->preparePattern($searchPattern), $content)));
    }

    function stripListOfString($listString)
    {
        //strips the splitted html element
        $strippedList = array();
        foreach ($listString as $item) {
            array_push($strippedList, trim(strip_tags($item)));
        }
        return $strippedList;
    }

}

I am brand new to PHP programming language and interested in improving my coding standard and OOP design?

Please do review my code and give me constructive feedback?

\$\endgroup\$
5
\$\begingroup\$

After about 10 minutes of reading your code and testing it out, I think I figured out what your class is supposed to, which is not a good sign for such a small class.

To me it seems that the goal of your class is to strip all the tags from a string containing HTML code, without losing the <br> tags, by replacing these tags with newlines. However, when running your code I did not see this behaviour at all.

I ran your code using the default $pattern parameter and the following HTML as input:

"Hello<br />World!<br /><p>How are you doing?<br /><br />Great!<br />"

and the result (using var_dump()) from your getPreservedContent() method was:

array(1) { [0]=> string(35) "HelloWorld!How are you doing?Great!" } 

Which I am not sure what to do with. Why is it in an array? Where are the newlines? They do not seem to be preserved at all!

I am assuming that the desired result for this HTML string would be:

string(67) "Hello
World!
How are you doing?

Great!
"

Based on my observations of the incorrect functionality of your class, design flaws and untidiness I will propose a redesign of your class and explain why I made the changes that I made.

Redesign

First of all, your class seems to be implemented specifically for the use of keeping HTML line breaks. This means that you have two choices here. Either choose to create a class for a more general use case, or make your class very specific to the HTML domain. I would chose for the latter here.

The name of your class does not describe what the intent of your class is. It only states that it preserves line breaks, not that is also strips the HTML tags from the string. I would suggest a name like:

class HtmlTagStripper { ... }

Which arguably might not be a very good name either, but I think it is a step up from where we were, as it clearly states the intent of the class.

This class could then have two public functions, one for regular stripping of tags and one for stripping the tags whilst preserving the newlines.

public function strip($html_text) { ... }

public function stripWithPreservedLineBreaks($html_text) { ... }

As you can see, I have chosen to require the user to pass the HTML text into the function instead of into the constructor of the class. This makes the class stateless and allows for reuse of the class without having to create a new instance.

Furthermore, we do not want the user to be able to pass in a pattern to the constructor either, since this class only works with HTML and therefore only works with <br> tags. Combining these two adjustments, we no longer need to specify a constructor at all!

To match the <br> tags, we need a regular expression, which you already presented in your code via the $pattern parameter. I will implement this regular expression as a constant on the class. The tricky part is that <br> tags can be written in any of the following ways: <br>, <BR>, <br >, <br />, <br class="my-class" /> and many more slight variations. So we need to make the regular expression a bit more robust.

I found such a regex in this StackOverflow question. Implementing this regex as a constant would look like this:

class HtmlTagStripper {
    const BR_TAG_REGEX = '/<br\s*(\s+[^>]+)*[\/]?>/gi';

    ...
}

Now that we have this out of the way, lets start with the implementation of the regular strip() function.

public function strip($html_text) {
    return strip_tags($html_text);
}

Now lets implement the stripWithPreservedLineBreaks() function.

public function stripWithPreservedLineBreaks($html_text) {
    $html_text = $this->convertLineBreakTagsToNewlines($html_text);

    return $this->strip($html_text);
}

private function convertLineBreakTagsToNewlines($html_text) {
    return preg_replace(self::BR_TAG_REGEX, PHP_EOL, $html_text);
}

There are two things to take note of here. First, we have delegated the responsibility of replacing the <br> tags with newlines to a separate function with a very clear name, to improve readability. It is important to specify that this method is private and cannot be used outside of your class, as it encapsulates the internal workings of the class. Second, we delegate the actual stripping of the tags to the basic strip() function that we wrote earlier.

The class as a whole now looks like this:

class HtmlTagStripper {
    const BR_TAG_REGEX = '/<br\s*(\s+[^>]+)*[\/]?>/gi';

    public function stripWithPreservedLineBreaks($html_text) {
        $html_text = $this->convertLineBreakTagsToNewlines($html_text);

        return $this->strip($html_text);
    }

    private function convertLineBreakTagsToNewlines($html_text) {
        return preg_replace(self::BR_TAG_REGEX, PHP_EOL, $html_text);
    }

    public function strip($html_text) {
        return strip_tags($html_text);
    }
}

What to take away from this

  1. You did not specify any visibility modifiers for your functions. Always make clear which functions are meant to be exposed (public) and which are internal (private/protected).
  2. Naming is extremely important in code readability. Name your classes, methods, variables and parameters appropriately.
  3. Always test your code!

Testing

Something I haven't touched on in this review is testing. You should always test your code to ensure/prove that it works the way you intended. Use a unit testing library such as phpunit to test many different inputs to your functions and verify that they are correct.

An example test:

class HtmlTagStripperTest {
    public function testSimpleBrTagIsReplacedWithNewline() {
        $html_tag_stripper = new HtmlTagStripper();
        $stripped_html = $html_tag_stripper->stripWithPreservedLineBreaks("<br>");
        $this->assertEquals(PHP_EOL, $stripped_html);
    }

    public function testComplexBrTagIsReplacedWithNewline() {
        $html_tag_stripper = new HtmlTagStripper();
        $stripped_html = $html_tag_stripper->stripWithPreservedLineBreaks("<br class='my-class' data-x='5'>");
        $this->assertEquals(PHP_EOL, $stripped_html);
    }
}

These two tests verify that different variations of the <br> tag are both replaced with a newline character. Of course you will need to test many more variations as well as test bigger HTML strings and so on.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Agree with you comment about not needing to instantiate a concrete object of this class. You may consider making methods static to further enforce this approach. \$\endgroup\$ – Mike Brant Jul 5 '16 at 21:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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