Someone paid someone for this



  • Found this form validation code in a legacy PHP project:

        if( $this->GetPreference('selfreg_force_email_twice') )
          {
        if( !isset($params[$email_field.'_again'] ) )
          {
    //        $params['error'] = 1;
    //        $params['message'] = $this->Lang('error_nosecondemailaddress');
    //        $this->myRedirect( $id, 'default', $returnid, $params );
    //        return;
        if (strlen($errors)>0)
              $errors .= '~';
            $errors .= $this->Lang('error_nosecondemailaddress');
          }
    
    if( $params[$email_field] != $params[$email_field.'_again'] )
      {
    

    // $params['error'] = 1;
    // $params['message'] = $this->Lang('error_emaildoesnotmatch');
    // $this->myRedirect( $id, 'default', $returnid, $params );
    // return;
    if (strlen($errors)>0)
    $errors .= '~';
    $errors .= $this->Lang('error_maildoesnotmatch');
    }
    }

    This same thing happens another 35 times in the script. If the error string isn't empty, append a tilde character, then append the error message.

    And yes, the formatting is verbitem, here's a more interesting example:

        // check for required parameters
        if( !isset( $params['group_id'] ) )
          {
    	$this->_DisplayErrorPage( $id, $params, $returnid,
    				  $this->Lang('error_insufficientparams'));
    	return;
          }
    
    // Check to ensure all required fields have some content
    // and validate email fields
    
    //go through each paramter
    foreach( $params as $key => $val ) {
    
    //make sure the param is a hidden field (cause it has all the details in it in a ';' seperated list)
      if( preg_match( '/^hidden_/', $key )) {
      $propname = substr($key,strlen('hidden_'));
      $arr = split(";",$params[$key]);
      $required = $arr[3];
      //if the field is required and is empty, throw an error
      if( $required == 2 && (!isset($params['input_'.$propname]) || $params['input_'.$propname] == '' )) {
    

    // $params['error'] = 1;
    // $params['message'] = $this->Lang('error_requiredfield',$propname);
    // $this->myRedirect( $id, 'default', $returnid, $params );
    if (strlen($errors)>0)
    $errors .= '~';
    $errors .= $this->Lang('error_requiredfield',$propname);
    }

    //this is an email address and needs to be validated as such
      if( $propname == 'email_address' ) {
          $result = $feusers->IsValidEmailAddress( $params['input_'.$propname],
    					       $feusers->GetPreference('do_smtp_validate_email') );
          if( $result[0] == false )
    	{
    

    // $params['error'] = 1;
    // $params['message'] = $result[1];
    // $this->myRedirect( $id, 'default', $returnid, $params );
    if (strlen($errors)>0)
    $errors .= '~';
    $errors .= $result[1];
    } //end validate email check
    } //end validate email
    } //end if hidden
    } //end foreach



  • Those last dozen lines of mis-dented (and dys-dented) code piss me off.  And that third if is totally inexcusable.



  • @hoodaticus said:

    Those last dozen lines of mis-dented (and dys-dented) code piss me off. 

    One time I had to take over a project where the codebase was right-aligned at the 80th column. And of course the naming convention for the variables and methods was soundex-friendly (all uppercase, no middle vowels).

    That codebase was awful. One of the major contributors was a "clever" guy who had the nasty habit of copy-pasting the entire code from one application to the other, then use gotos to skip over the parts he did not need, but sometime a little overlap was needed so the code flow would come back before the previous goto, etc.

    What is painful is that the applications themselves were completely awesome (from the client perspective) because over the years the in-house dev bent backward to add any feature the client requested. This prevented a business case from being made for a total rewrite or even a change in naming convention. And to this day, this massive pile of crazyness is still maintained (not by me!).



  • @thistooshallpass said:

    That codebase was awful. One of the major contributors was a "clever" guy who had the nasty habit of copy-pasting the entire code from one application to the other, then use gotos to skip over the parts he did not need, but sometime a little overlap was needed so the code flow would come back before the previous goto, etc.

    What is painful is that the applications themselves were completely awesome (from the client perspective) because over the years the in-house dev bent backward to add any feature the client requested. This prevented a business case from being made for a total rewrite or even a change in naming convention. And to this day, this massive pile of crazyness is still maintained (not by me!).

    I think SpectateSwamp is maintaining it now...


  • @thistooshallpass said:

    One time I had to take over a project where the codebase was right-aligned at the 80th column.
    The code wasn't in Arabic or Hebrew, was it?



  • I don't see what the big deal is with this is .. sure it's not the greatest solution but not too WTFy. The tilde could just be a delimiter for the error parsing functions and the formatting wonkiness could be due to just using both tab and space idents so it looks fine in one editor but not in another with different (or no) tabstops. You should see some of the legacy code i've had to clean up with complete mix of tabs vs spaces, aligned to 80 cols with long function calls spread across 10 lines. Although the lack of curly braces on some of the ifs is a bit sketchy.



  • @hoodaticus said:

    @thistooshallpass said:
    One time I had to take over a project where the codebase was right-aligned at the 80th column.
    The code wasn't in Arabic or Hebrew, was it?

    At the time I asked "wtf?" and I was told that this requirement was coming from the Enterprise Architect, an old guy. I went to ask him and he explained that it's easier to read code using the type command in a dos prompt if it's right-aligned at the 80th column, and that anyways the "important stuff" is always on the right side. I was skeptical about someone being able to review code using "type" but he actually had a batch file on his desktop so he could d&d source code files he wanted to review. Hearing him talk about the benefits of HP3000 over Wintel also explained the unpleasant naming convention.



  •  @thistooshallpass said:

    Hearing him talk about the benefits of HP3000 over Wintel also explained the unpleasant naming convention.
    I bet the code was COBOL, too.



  • @hoodaticus said:

    @thistooshallpass said:
    Hearing him talk about the benefits of HP3000 over Wintel also explained the unpleasant naming convention.
    I bet the code was COBOL, too.

    With that column alignment, let's hope it wasn't Fortran.

     


  • ♿ (Parody)

    @cconroy said:

    @hoodaticus said:
    @thistooshallpass said:
    Hearing him talk about the benefits of HP3000 over Wintel also explained the unpleasant naming convention.
    I bet the code was COBOL, too.

    With that column alignment, let's hope it wasn't Fortran.

    Or python.



  • @boomzilla said:

    @cconroy said:
    @hoodaticus said:
    @thistooshallpass said:
    Hearing him talk about the benefits of HP3000 over Wintel also explained the unpleasant naming convention.
    I bet the code was COBOL, too.

    With that column alignment, let's hope it wasn't Fortran.

    Or python.

    Or [url=http://en.wikipedia.org/wiki/Whitespace_%28programming_language%29]Whitespace[/url].


Log in to reply