-1
\$\begingroup\$

Functionality:

This method removes a list of specific terms (e.g., LLC, INC, Inc, Company, etc.) from a list of public companies names. It is part of a class that can be viewed in this link.

I'd appreciate it if you would possibly review it for best coding practices and efficiency.

Method:

/**
* 
* @return a string of company name without common words to be embedded in the URL
*/
public static function slugCompany($c){
    $c=strtolower($c);
    $c=preg_replace('/[^\da-z\s]/i', '', $c);
    $words=self::COMPANY_STOPWORDS;
    $c=preg_replace('/\b('.$words.')\b/i', '', $c);
    $c=preg_replace('/(\s+)/i', '-', trim($c));
    return $c; 
}

Constant:

const COMPANY_STOPWORDS = '11000th|american|and|a|beneficial|bond|b|class|common|company|corporation|corp|commodity|cumulative|co|c|daily|dep|depositary|depository|debentures|diversified|due|d|each|etf|equal|equity|exchange|e|financial|fund|fixedtofloating|fixed|floating|f|group|g|healthcare|holdings|holding|h|inc|incorporated|interests|interest|in|index|income|i|junior|j|k|liability|limited|lp|llc|ltd|long|l|markets|maturity|municipal|muni|monthly|m|noncumulative|notes|no|n|of|one|or|o|portfolio|pay|partnership|partner|par|perpetual|per|perp|pfd|preference|preferred|p|q|redeemable|repstg|representing|represents|rate|r|sa|smallcap|series|shs|shares|share|short|stock|subordinated|ser|senior|s|the|three|term|to|traded|trust|two|t|ultrashort|ultra|u|value|v|warrant|weight|w|x|y|z';

Function Call:

$cn=strtolower(UpdateStocks::slugCompany($s["quote"]["companyName"]));

Example Input:

AGILENT TECHNOLOGIES INC
ALCOA CORP
PERTH MINT PHYSICAL GOLD ETF

Example Output:

agilent-technologies
alcoa
perth-mint-physical-gold

PHP7.1

\$\endgroup\$
4
\$\begingroup\$

I would clean it up a bit:

public static function slugCompany($company){
    $replace = [
        '/[^\da-z\s]/i'                        => '', //remove punctuation I guess
        '/\b('.self::COMPANY_STOPWORDS.')\b/i' => '', //remove these companies
         '/^\s+|\s+$/'                         => '', //trim
         '/\s+/'                               => '-' //replace space with -
    ];

    return preg_replace(array_keys($replace), $replace, strtolower($company));
}

I cleaned up all these local variables, got rid of a few repetitive calls. The array_keys and array structure are mainly for readability reasons and ease of use. For example now you can easly add replacements if you need to because they use a common array for the replacements.

Sandbox

Its more of a readability fix as it's hard to know all the input you may have so I have to trust that you covered all the edge cases.

Your code

Hope it helps.

\$\endgroup\$
  • 1
    \$\begingroup\$ Sure all I did is re-arange some stuff. I usually write my own code 3 or 4 times before I am happy with it. Once to get it working, once to make it readable and once to improve the performance. Don't be afraid to say this is too much I need to simplify it. Its a process like writing an English paper. \$\endgroup\$ – ArtisticPhoenix Mar 15 at 15:23
5
\$\begingroup\$

There are several things to point out here.

For the record, I rewrote and tested the process without a class on my localhost -- the adjustment for you to convert my snippet to suit your class should be simple.

New Snippet:

$slugs = <<<SLUGS
AMERIS BANCORP
ALCENTRA CAPITAL CORP
ABEONA THERAPEUTICS INC
SLUGS;  // yes, I tested the entire battery

const COMPANY_STOPWORDS = '11000th|american|and|beneficial|bond|class|common|company|corporation|corp|commodity|cumulative|co|daily|dep|depositary|depository|debentures|diversified|due|each|etf|equal|equity|exchange|financial|fund|fixedtofloating|fixed|floating|group|healthcare|holdings|holding|inc|incorporated|interests|interest|in|index|income|junior|liability|limited|lp|llc|ltd|long|markets|maturity|municipal|muni|monthly|noncumulative|notes|no|of|one|or|portfolio|pay|partnership|partner|par|perpetual|per|perp|pfd|preference|preferred|redeemable|repstg|representing|represents|rate|sa|smallcap|series|shs|shares|share|short|stock|subordinated|ser|senior|the|three|term|to|traded|trust|two|ultrashort|ultra|value|warrant|weight|[a-z]';

function slugCompany($slug){
    return trim(preg_replace(['/[^\da-z\s]+|\b(?:' . COMPANY_STOPWORDS . ')\b/', '/\s+/'], ['', '-'], strtolower($slug)), '-');
}

foreach (explode(PHP_EOL, $slugs) as $slug) {
    echo "<div>$slug => " , slugCompany($slug) , "</div>";
}

Output:

AMERIS BANCORP => ameris-bancorp
ALCENTRA CAPITAL CORP => alcentra-capital
ABEONA THERAPEUTICS INC => abeona-therapeutics

First about your code...

public static function slugCompany($c){
    $c=strtolower($c);
    $c=preg_replace('/[^\da-z\s]/i', '', $c);
    $words=self::COMPANY_STOPWORDS;
    $c=preg_replace('/\b('.$words.')\b/i', '', $c);
    $c=preg_replace('/(\s+)/i', '-', trim($c));
    return $c; 
}
  • You are making three separate passes through the string(s).
  • You are inefficiently finding-replacing each individual "non-digit/non-alpha/non-space" character.
  • You are making case-insensitive matching, but you know everything is already lowercase.
  • The capture group on the third preg_replace() call is unnecessary.
  • You should use one space on either side of the = assignment operator.

Regarding ArtisticPhoenix's post, it makes a single preg_replace() call and places some importance on readability, but...

public static function slugCompany($company){
    $replace = [
        '/[^\da-z\s]/i'                        => '', //remove punctuation I guess
        '/\b('.self::COMPANY_STOPWORDS.')\b/i' => '', //remove these companies
         '/^\s+|\s+$/'                         => '', //trim
         '/\s+/'                               => '-' //replace space with -
    ];

    return preg_replace(array_keys($replace), $replace, strtolower($company));
}
  • preg_replace() is making too many separate passes through the string(s).
  • Case-insensitive matching is used, but everything is already lowercase.

Some basic principles:

  1. Avoid using regular expressions if there is a single non-regex function that can do the same job in a sensible/readable manner. In my snippet, I am using trim(..., '-') to trim the leading/trailing hyphens rather than another replacement pass of ~^-+|-+$~.
  2. Endeavor to reduce total function calls where it doesn't negatively impact your script logic. In this case, there is no use in calling preg_replace() multiple times because the function is happy to receive an array of patterns and an array of replacements.
  3. When multiple patterns share a duplicate replacement string, try to merge the two patterns -- this means the task is completed in one pass.
  4. When the intention is to matching multiple characters that may possibly be side-by-side, match as many as possible. Consider this scenario, you are standing over an open carton of eggs. I ask you to pick up all the eggs. Doing 12 squats to extract each egg individually (while good for personal health) is an obviously inefficient choice. Just reach down one time and pick up the whole carton -- done in one pass.
  5. Don't use capture groups unless you actually need to capture substrings. While this extends the pattern length by 2 characters, it is a clear indication of pattern intention and removes the possibility of numbered backreferences associated with capture groups. In other words, use (?:...) versus (...).
  6. Avoid declaring single-use variables. There are a few exceptions to this rule (like adding clarity to a line of code via a descriptive variable name AND to reduce horizontal scrolling in a script for devs), but in this case I felt returning a one-liner was tolerable.
  7. You can condense 26 of the alternatives in your const by writing [a-z] versus individually listing letters. This will improve pattern efficiency and pattern length.
  8. If pattern efficient is the highest priority AND the piped const values are very rarely modified, you could go the extra mile and try to reduce the number of "alternatives". For instance, you can combine ...|of|or|o|... to be ...|o[fr]?|... and so on. Some would say that this comes at a cost of readability/maintainability.
\$\endgroup\$

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.