scary code to debug

[garrensilverwing]

i wrote this code with some of your help (thanks very, very much!) and i need a bit of help with it, when i enter in any amount of PGN's it works perfectly with one exception, it duplicates the extra game at the end every time which gives me one extra MySQL query which can screw up the entire /games portion of my webiste (offsetting the links provided by the search, etc) so i was wondering if there is a way to either prevent the final array value from being processed or to just delete it entirely before the entire array is processed here is the code if you need it (ignore the notes i wrote them so i would be lessed confused when i looked at the code, i deleted most of them because of length issues though):

        echo "<ol>"; 
        $string = $_POST['pgn'];
        $string = strtolower($string);
        $string = stripslashes($string);     //removes any slashes from $string to protect against malicious users
        $string = str_replace("[","<br>[",$string); more easily read by humans
        $string = str_replace("\"1-0\"","!",$string);
        $string = str_replace("\"0-1\"","@",$string);
        $string = str_replace("\"1/2-1/2\"","#",$string);
        $string = str_replace("1-0","~",$string);
        $string = str_replace("0-1","~",$string);
        $string = str_replace("1/2-1/2","~",$string);
        $string = str_replace("!","\"1-0\"",$string);
        $string = str_replace("@","\"0-1\"",$string);
        $string = str_replace("#","\"1/2-1/2\"",$string);
        $pgn=explode("~",$string);
        foreach($pgn as $number => $game)
            {
            preg_match_all('/\[(\w+) "([^"]+)"\]/', $game, $matches, PREG_SET_ORDER); 
            foreach($matches as $key => $value)
                {
                    $tag=$value[1];
                    $data[$tag]=$value[2];
                }
            $wName=$data['white'];
            $bName=$data['black'];
            $wRating=$data['whiteelo'];
            $bRating=$data['blackelo'];
            $eco=$data['eco'];
            $date=$data['date'];
            $outcome=$data['result'];
            $error=0;
            if(preg_match('/,/', $wName)==1 OR preg_match('/ /', $wName)==1)
                {
                    $wName=str_replace("'","",$wName);
                    if(preg_match('/,/',$wName)==1)
                        {
                            $wName=str_replace(" ","",$wName);
                            $white_name=explode(",",$wName);
                            if(count($white_name)< 3)
                                {
                                    $wLast=$white_name[0];
                                    $wFirst=$white_name[1];
                                }
                            else
                                {
                                    $error=1;
                                    echo "Error the format of black's name is incorrect. Please have black's name in either \"last, first\" or \"first last\" formats.";                                }                    
                        }
                    else
                        {
                            $white_name=explode(" ",$wName);
                            if(count($white_name) < 3)
                                {
                                    $wFirst=$white_name[0];
                                    $wLast=$white_name[1];
                                }
                            else
                                {
                                    $error=1;
                                    echo "Error the format of white's name is incorrect. Please have white's name in either \"last, first\" or \"first last\" formats.";
                                }
                            
                        }
    
                }
            else
                {
                    $wFirst=$wName;
                    $wLast=$wName;
                }
            if(preg_match('/,/', $bName)==1 OR preg_match('/ /', $bName)==1)
                {
                    $bName=str_replace("'","",$bName);
                    if(preg_match('/,/',$bName)==1)
                        {
                            $bName=str_replace(" ","",$bName);
                            $black_name=explode(",",$bName);
                            if(count($black_name)< 3)
                                {
                                    $bLast=$black_name[0];
                                    $bFirst=$black_name[1];
                                }
                            else
                                {
                                    $error=1;
                                    echo "Error the format of black's name is incorrect. Please have black's name in either \"last, first\" or \"first last\" formats.";     //warns the user that the name is incompatable    
                                }                
                        }
                    else     //if it is not in "last, first" format it is assumed it is in "first last" format or another format with spaces
                        {
                            $black_name=explode(" ",$bName); //seperates black's first and last name
                            if(count($black_name) < 3)     //checks if there is more than 2 names found
                                {
                                    $bFirst=$black_name[0];     //seperates black's first and 
                                    $bLast=$black_name[1];     //last name from the array
                                }
                            else
                                {
                                    $error = 1;
                                    echo "Error the format of black's name is incorrect. Please have black's name in either \"last, first\" or \"first last\" formats.";     //warns the user that the name is incompatable
                                }
                            
                        }
    
                }
            else     //if black's name is not in last, first or first last, that is there are no spaces or commas
                {
                    $bFirst=$bName;     //sets both white's first and 
                    $bLast=$bName;      //last name to the pgn's original value (usually an online handle)
                }
            if($date!=="????.??.??" AND $date!=="*" AND $date!=="" AND $date!=="??")     //verifies that there is usable data in the pgn's date tag
                {
                    $day=explode(".",$date);     //seperates the date's year month and day in to an array
                    $year=$day['0'];     //since the standard pgn date format is "yyyy.mm.dd" the first array value is used for year
                    if(strlen($year)!==4)     //if the number of numbers in year is not four then the format of date must have been different
                        {
                            $year = $day['1'];     //this sets the year to the second value of array $day, that is if the date is in "dd.yyyy.mm" or "mm.yyyy.dd" format
                        if(strlen($year)!==4)     //if the number of numbers in $year is not for then the format must be "dd.mm.yyyy" or "mm.dd.yyyy"
                            {
                                $year = $day['2'];     //sets the year to third value in array $day
                            }
                        }
                }
            else
                {
                    $year=0000;     //uses whatever the pgn's date tag says either
                }
            if($outcome !== "*" AND $outcome !== "??" AND $outcome!=="")     //verifies there is usable data in the pgn's result tag
                {
                    if($outcome == "0-1")     //if black wins
                        {
                            $result = 0;     //used to signify black wins
                        }
                    if($outcome == "1-0")     //if white wins
                        {
                            $result = 1;     //used to signify white wins
                        }
                    if($outcome == "1/2-1/2")     //if the game is a draw
                        {
                            $result = 2;     //used to signify a draw
                        }            
                }
            else
                {
                    $result = 3;     //this will be used to signify that the result is still pending whether the game was adjourned or never completed
                }
            if($error!==1)
                {
                    $sql = "INSERT into `games`(`wFirst`,`wLast`,`wRating`,`bFirst`,`bLast`,`bRating`,`eco`,`result`,`year`)
                    VALUES ('$wFirst','$wLast','$wRating','$bFirst','$bLast','$bRating','$eco','$result','$year');";
                    mysql_query($sql) or die(mysql_error());
                    echo "<li>$wFirst $wLast ($wRating) vs $bFirst $bLast ($bRating) $eco $result $year, has been entered into that database.</li>";     //places each game's info in an ordered list and states that it has been submitted properly
                }
            else
                {
                    echo "your pgn ($game) was not submitted to the database, please check the formatting of the names";
                }
            }
        echo "</ol>";
?>

 

[garrettroyce]

Is it possible that the script is execution correctly and the data you're feeding it is incorrect?

 

[lothop]

So if error==1 put the data into the database?
Is this correct?

foreach($pgn as $number => $game){
            if($error!==1){
                    $sql = "INSERT into `games`(`wFirst`,`wLast`,`wRating`,`bFirst`,`bLast`,`bRating`,`eco`,`result`,`year`)
                    VALUES ('$wFirst','$wLast','$wRating','$bFirst','$bLast','$bRating','$eco','$result','$year');";
                    mysql_query($sql) or die(mysql_error());
                    echo "<li>$wFirst $wLast ($wRating) vs $bFirst $bLast ($bRating) $eco $result $year, has been entered into that database.</li>";     //places each game's info in an ordered list and states that it has been submitted properly
                }else{
                    echo "your pgn ($game) was not submitted to the database, please check the formatting of the names";
                }
            }

Your sql query is still within your foreach. So it will run the query as many times as it meets the foreach requirements.

 

[garrettroyce]

So if error==1 put the data into the database?
Is this correct?

foreach($pgn as $number => $game){
            if($error!==1){
                    $sql = "INSERT into `games`(`wFirst`,`wLast`,`wRating`,`bFirst`,`bLast`,`bRating`,`eco`,`result`,`year`)
                    VALUES ('$wFirst','$wLast','$wRating','$bFirst','$bLast','$bRating','$eco','$result','$year');";
                    mysql_query($sql) or die(mysql_error());
                    echo "<li>$wFirst $wLast ($wRating) vs $bFirst $bLast ($bRating) $eco $result $year, has been entered into that database.</li>";     //places each game's info in an ordered list and states that it has been submitted properly
                }else{
                    echo "your pgn ($game) was not submitted to the database, please check the formatting of the names";
                }
            }

Your sql query is still within your foreach. So it will run the query as many times as it meets the foreach requirements.

It says

if (error !== 1)

 

[garrensilverwing]

the problem is it always puts the last game i load into it twice, not every game just the last game i think when i explode the pgn's it does the last game twice for some reason and then when i run the array it puts it in so im thinking something along the lines of:

$last = count($pgn) - 1;
foreach(pgn as $key => $value)
     {
     if($key!==$last)
          {
          dataextract($value);
          }
     }

what do you think?

 

[misson]

the problem is it always puts the last game i load into it twice, not every game just the last game i think when i explode the pgn's it does the last game twice for some reason

The result and the end of moves for the last game gets replaced with '~', like all the others. This means after the explode, there's an empty string after all the game data (try explode('~', 'a~b~')). When you call preg_match_all('/\[(\w+) "([^"]+)"\]/', $game, $matches, PREG_SET_ORDER); on an empty string, nothing matches so $matches is set to an empty array. The

foreach($matches as $key => $value) {
                    $tag=$value[1];
                    $data[$tag]=$value[2];
                }

loop thus skips over the body and the contents of $data from the previous iteration are carried over. One lesson to learn from this is than if you don't want values to carry over iterations, make sure you reset the variables at the start of the iteration. A better solution (part of what you post) is to factor out the loop body into a function to parse the game data. This puts the parsed data in a local variable which will be discarded when the call returns, thus ensuring the game data doesn't persist over loop iterations.

Your impulse to refactor the loop body into a function is a good one because programming is all about breaking a process down into tasks, which should be turned into functions or method calls. Your code could do with some more such refactoring, such as a function to parse player names. Spend more time designing will help to produce code nicely compartmentalized into functions (and modules and classes ...). You can also try designing top-down and writing pseudocode to start. Learning to refactor early is very helpful.

and then when i run the array it puts it in so im thinking something along the lines of:

$last = count($pgn) - 1;
foreach(pgn as $key => $value)
     {
     if($key!==$last)
          {
          dataextract($value);
          }
     }

Executing tests in a loop that are true only once (or twice) is horribly inefficient. Better to find a way to move it outside the loop, which is extremely easy when the exceptional case/s is/are at one/both end/s of the string. Try:

$pgn=explode("~",$string);
if (! $pgn[count($pgn)-1]) {
  array_pop($pgn);
}
foreach($pgn as $number => $game) {
    //...

Voila: no empty string at the end of the array. A more robust alternative would be to test that the call to preg_match_all finds at least one match:

function parseGame($gameStr) {
  if (preg_match_all('/\[(\w+) "([^"]+)"\]/', $game, $matches, PREG_SET_ORDER)) {
    // game parse succeeded. Now parse fields.
    // ...
  } else {
    // No PGN tags, so parse failed.
    // ...
  }
}

Some other improvements: if you find yourself using multiple calls to str_replace, you can probably replace them with a single call to preg_replace for more readable, more efficient code. In this case, replace the nine calls with:

$string = stripslashes($string);     //removes any slashes from $string to protect against malicious users
$string=preg_replace('/([^"])(1-0|0-1|1\/2-1\/2)([^"])/', '\1\2~\3', $string);
$pgn=explode("~",$string);

Other lines are included for context.

Similarly, you shouldn't need to use as many calls to preg_match as you do when parsing names. Try something like:

function parseName($name) {
    if (preg_match('/^\s*([^, ]+)(,?)\s+(\S+)\s*$/', $name, $matches)) {
        if ($matches[2]) { // comma found, so "last, first" format
            return array($matches[3], $matches[1]);
        } else { // "first last"
            return array($matches[1], $matches[3]);
        }
    } else {
        // handle parse error
        //...
    }
}

Adding break elements ("<br/>") to every tag all at once is inefficient given than errors will (hopefully) be few and thus most games will never be displayed to the user. Better to do this just when there's an error. This means move the line

$string = str_replace("[","<br>[",$string); //more easily read by humans

to the point that you print out a game string.

Making a single insert is more efficient than multiple SQL inserts. Use the format: INSERT INTO <table_name> (<col_name>, ...)
VALUES (<value>, ...)
(<value>, ...)


I'll post an outline utilizing the above suggestions later.

If you want a book to study REs, the PHP RE docs recommend Jeffrey Friedl's "Mastering Regular Expressions". Not having read it, I can't proclaim its qualities, but it is published by O'Reilly.

 

[garrensilverwing]

thanks a lot for pointing out the array_pop function to me, the loop code was just what i thought of while i was at my day job with no computer to check it i added the array_pop() and now it works perfectly so i will just have to work on its effeciency

i think i will leave the replacement strings the way they are because its not simply replacing everything, each game has two results and i only want to replace the second one, which is why i replace the ones in quotation marks and then replace them back so unless there is a better way to do that I will leave it alone

it is working fine now and since i spent almost an entire day working on this one piece of code im kind of sick of it and will probably try some more of your suggestions later

the reason i added the <br> was so i could read it if it failed to enter it into the database for whatever reason but i can see it would be easier to add the code into the errors because it would have less memory usage overall

 

[misson]

i think i will leave the replacement strings the way they are because its not simply replacing everything, each game has two results and i only want to replace the second one, which is why i replace the ones in quotation marks and then replace them back so unless there is a better way to do that I will leave it alone

The statement preg_replace('/([^"])(1-0|0-1|1\/2-1\/2)([^"])/', '\1\2~\3', $string); will only replace results that do not begin or end with double quotes, which should skip all [result "..."] tags. You could also try a negative lookbehind:

preg_replace('/(?<!result ")(1-0|0-1|1\/2-1\/2)/', '\1~', $string);