HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Is there a more elegant solution than this spaghetti code without completely revamping the whole page?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisspaghettiwithoutsolutionthecompletelywholethanmorerevamping

Problem

I use 'GROUP_CONCAT' to get comma separated lists of a person's educational background. Included in those lists are the type of degree, major, issuing establishment, and year received. degreeType is a required field and cannot empty. Prior to looping, I split the lists into arrays, like so:

$degreeType = explode(",", $degreeType);
$major = explode(",", $major);
$issuingEstablishment = explode(",", $issuingEstablishment);
$yearReceived = explode(",", $yearReceived);


I can then do a foreach loop and echo out the information returned. However, since explode() returns an array, even if it's empty, I have to check it first before starting the foreach. As shown below:

$counter = 0;
//echo out education
if($degreeType[0]!=""){
  foreach($degreeType as $dType) {
    if($counter == 0) $info .= "Educational Background";
    $info .= "$dType - ";
    $info .= ($major[$counter]!="") ? "$major[$counter], " : "";
    $info .= $issuingEstablishment[$counter];
    $info .= ($yearReceived[$counter]!="") ? ", $yearReceived[$counter]" : "";
    $info .= "";
    $counter++;
  }
  $info .= "";
}


Alternatively, I could run a check prior to separating the lists. Either solution seems inelegant. Is there a cleaner/faster/better way to split the lists and loop through them, but only if they actually contain information? Initially I was going to dabble with using an object, but mysqli doesn't support that with prepared statements. Any advice is much appreciated!

Solution

Your script is alright, there are just some minor issues:

1.) If string is enclosed inside "" then PHP interpreter checks whether there are some variables or not, if string is enclosed inside '' PHP interpreter does not check it.

2.) You forgot to declare $info variable, it is always good to declare your variables before doing some operations on them like .=.

3.) I don't like using this shorter syntax, because it can be source of bugs which are hard to find, in my opinion it is usually better to use {}:

if($counter == 0) $info .= "Educational Background";

4.) You don't need to repeat $info .= on every line, it can be replaced by ..

I rewrote the script in respect to previously mentioned points:

$degreeType = explode(',', $degreeType);
$major = explode(',', $major);
$issuingEstablishment = explode(',', $issuingEstablishment);
$yearReceived = explode(',', $yearReceived);

$counter = 0;
$info = '';
if($degreeType[0] != ''){
    foreach($degreeType as $dType) {
        if(!$counter) {
            $info .= 'Educational Background';
        }

        $info .= "$dType - "
            . (($major[$counter] != '') ? "$major[$counter], " : '')
            . $issuingEstablishment[$counter]
            . (($yearReceived[$counter] != '') ? ", $yearReceived[$counter]" : '')
            . ''
        ;

        $counter++;
    }
    $info .= "";
}

Code Snippets

$degreeType = explode(',', $degreeType);
$major = explode(',', $major);
$issuingEstablishment = explode(',', $issuingEstablishment);
$yearReceived = explode(',', $yearReceived);

$counter = 0;
$info = '';
if($degreeType[0] != ''){
    foreach($degreeType as $dType) {
        if(!$counter) {
            $info .= '<h2>Educational Background</h2><ul>';
        }

        $info .= "<li>$dType - "
            . (($major[$counter] != '') ? "$major[$counter], " : '')
            . $issuingEstablishment[$counter]
            . (($yearReceived[$counter] != '') ? ", $yearReceived[$counter]" : '')
            . '</li>'
        ;

        $counter++;
    }
    $info .= "</ul>";
}

Context

StackExchange Code Review Q#48003, answer score: 4

Revisions (0)

No revisions yet.