3
\$\begingroup\$

I have class with some basic string operations. Any suggestion to improve the class

    class StringHelpers
    {
       function replace_between($str, $needle_start, $needle_end, $replacement) {
          $pos = strpos($str, $needle_start);
          $start = $pos === false ? 0 : $pos + strlen($needle_start);

          $pos = strpos($str, $needle_end, $start);
          $end = $start === false ? strlen($str) : $pos;

          return substr_replace($str,$replacement,  $start, $end - $start);
       }

       function getTextBetweenTags($string, $tagname) {
          $pattern = "/<$tagname>([\w\W]*?)<\/$tagname>/";
          preg_match($pattern, $string, $matches);
          return $matches[1];
       }

       function checkIfValueContainLetters($value){
          return  preg_match('/[A-Z]+[a-z]+[0-9]+/', $value);
       }

public function createTableRowElementsByValue($bgColor, $tdName, $tdValue)
    {
        $tableRow = '<tr bgcolor="' . $bgColor . '"><td>' . $tdName . '</td><td>' . $tdValue . '</td>  </tr>';
        return $tableRow;
    }

    public function changeBgColorWhiteToBlue()
    {
        if ($this->color == 'D4E4F3') {
            $this->color = '';
            return '';
        }
        $this->color = 'D4E4F3';
        return 'D4E4F3';
    }

    public function createTableRow($tdName)
    {
        return '<td>'. $tdName.'</td>';
    }

    }
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Could you please provide some example data, and describe what your code is supposed to do? \$\endgroup\$
    – holroy
    Commented Jan 13, 2016 at 20:55

2 Answers 2

3
\$\begingroup\$

These functions do widely different things (replace stuff in a generic string, handle HTML, handle colors, ...), so there are probably better places for them then a generic StringHelpers class. A class like this would grow quite large if you handle all functions like this.

createTableRowElementsByValue for example doesn't really have anything to do with strings, and changeBgColorWhiteToBlue is something else entirely.

  • Naming: checkIfValueContainLetters is very misleading. I'm not sure if the regex is even correct, since it checks for such an odd thing (definitely a thing too specific for a generic StringHelpers class). Did you maybe mean ^[A-Za-z0-9]*$? Because that would somewhat match your name (except for the numbers). If that's what you want, isAlphaNumeric would be a better name.
  • Naming: changeBgColorWhiteToBlue also doesn't do what it's name suggest. It sets the color to D4E4F3, except if it is already D4E4F3, in which case it sets it to nothing; there is no white involved.
  • Naming: createTableRow doesn't create a row (that would be tr).
  • getTextBetweenTags isn't very stable. Input such as <foo>ba/&!r</foo>, foo, ... would break it.
\$\endgroup\$
3
  • \$\begingroup\$ Good points. Any suggestions on the gettextbewTweentags \$\endgroup\$ Commented Jan 14, 2016 at 4:20
  • \$\begingroup\$ @DacorieS parsing HTML is hard, and depending on what you need the function for, a regex will not be the right solution (use a HTML parser instead). If it's just for throw-away code or some specific corner-case, try ^<$tagname>(.*)<\/$tagname>$, and before returning the result, check if there actually was one (but it really depends on your use-case; with this regex for example, there could be tags in your return if they where inside the tagname tag). \$\endgroup\$
    – tim
    Commented Jan 14, 2016 at 9:49
  • \$\begingroup\$ Very true. I can do some more validations since I am looking for a particular value \$\endgroup\$ Commented Jan 14, 2016 at 13:25
1
\$\begingroup\$

I haven't code PHP in a while, but some thing can be applied to multiple languages:

1) parameters checking - replace_between function does not check input parameters. While providing wrong parameter (negative indexes, stop after start etc.) is generating proper exceptions in languages like C# and Java, in PHP may lead to confusing results, as explained here when using negative indexes.

2) (somewhat) ambiguous function name - getTextBetweenTags can be named getFirstTagText, as it seems to get only text found on first provided $tagname. Also, changeBgColorWhiteToBlue is doing something else (already specified in previous answer)

3) string formatting - PHP allows interpolated variables (or interpolated strings as C# calls it), but it seems to be a little slower. So,

'<tr bgcolor="' . $bgColor . '"><td>' . $tdName . '</td><td>' . $tdValue . '</td>  </tr>';

becomes something like

'<tr bgcolor='$bgColor'><td>'$tdName'</td><td>'$tdValue'</td>  </tr>';

Personally, I prefer a (very) little speed downgrade over (much) improved readability.

4) avoid magic numbers/strings - D4E4F3 appears three times in your code. I would define a constant for it. Or even better, define css styles and set/unset them.

5) class name - StringHelper suggests generic string manipulations (like replacing, padding etc.), so only replace_between function fits here. A better container name for the rest of the functions would be HtmlHelpers perhaps.

Slightly off-topic, but might be useful: if you need to parse Html content, I advice to take a look into PHP Simple HTML DOM Parser.

\$\endgroup\$
1
  • \$\begingroup\$ Lovely tips. I was unaware of the interpolated variables and php simple html Dom parser. I will look into them \$\endgroup\$ Commented Jan 14, 2016 at 14:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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