2
\$\begingroup\$

Following scenario: this is the code that resides in Ajax Controller class. It is responsible for handling input in one of the form fields.

The logic is as follows:

If user-inputted data validate as either Tax Number or Statistical Number, it's supposed to: check for presence of such data in database, and, depending on the result, either make a call to external API (in case DB query is empty) or present the results (in case of input being found in database).

If user-inputted data doesn't validate as either Tax or Statistical Number, it is supposed to perform DB query with input treated as the customer name.

Below code works as expected. However, it is long and I was forced to repeat some parts. I'm looking to cut down on lines while preserving exact same logic, but I'm highly unsure as to how to make it even more compressed.

class AjaxController {

  public function customerslistAction() {
    $filterChain = new Zend_Filter();
    $filterChain->addFilter(new Zend_Filter_Digits())
                ->addFilter(new Zend_Filter_Null());

    $input = $filterChain->filter($this->getRequest()->getParam('name'));
    $collection = new Def_Model_Collection_Customer();

    /* check if $input is TaxNumber */
    if (SugApi::isTaxNumber($input)) {
      $data['tax_number'] = $input;
      $allFound = $collection->findByTaxOrStatistical($data);

      // check and return customer with this TAX_NUMBER if it is in database
      if (count(($allFound)) > 0) {
        echo Zend_Json::encode($allFound);
        return;
      }
      // call external api if it isn't in database [catch NotFound Exception]
      else {
        try {
          $sugReport = SugApi::getByTaxNumber($input);
        } catch (\SugApi\Exception\NotFoundException $e) {
          $out['results'][] = array('value' => 'Unknown tax number.');
          echo Zend_Json::encode($out);
          return;
        }
      }
    }

    /* check if $input is StatisticalNumber */
    else if (SugApi::isStatNumber($input)) {
      $data['stat_number'] = $input;
      $allFound = $collection->findByTaxOrStatistical($data);

      // check and return customer with this STATISTICAL_NUMBER if it is in database
      if (count($allFound) > 0) {
        echo Zend_Json::encode($allFound);
        return;
      }
      // call external api if it isn't in database [catch NotFound Exception]
      else {
        try {
          $sugReport = SugApi::getByStatNumber($input);
        } catch (\SugApi\Exception\NotFoundException $e) {
          $out['results'][] = array('value' => 'Unknown statistical number.');
          echo Zend_Json::encode($out);
          return;
        }
      }
    }
    // if $input is neither TAX_NUMBER nor STATISTICAL_NUMBER, 
    // query database for customer with $input as customer_name
    else {
      $collection = new Def_Model_Collection_Customer();
      $data = $collection->fetchAll(array('name' => $this->getRequest()->getParam('name')));

      $out = array();
      foreach ($data as $val) {
        $out['results'][] = array(
          'id' => $val['customer_id'],
          'value' => $val['customer_name'],
          'info' => 'Tax Number: '.$val['customer_tax_number']
        );
      }
      $json = Zend_Json::encode($out);
      echo $json;
      return;
    }

    // parse successful external api response into Customer object and save it
    $customer = new Def_Model_Customer();
    $customer->setOptionsFromSugReport($sugReport);
    $customer->save();

    $out['results'][] = array(
      'id'    => $customer->customer_id,
      'value' => $customer->customer_name,
      'info'  => 'Tax Number: '.$customer->customer_tax_number
    );
    echo Zend_Json::encode($out);
  }
}
\$\endgroup\$
1
\$\begingroup\$

If you don't want to repeat yourself (DRY) then consider using multiple methods, and set properties ($this->collection) and/or pass parameters, as necessary. If you are finding yourself nesting if statements inside a method, it is a good indication that you should be using multiple methods.

For example, for your method, it looks like you could cut the database check into their own method.

If you have a return, then there is no need for an else statement :-)

if($a){
  return;
}
// no need for else

There is no point in setting a variable $json and echo it, just echo it as you did most of the time, but not all.

It looks as though you could break the rest response into it's own method for example with the if count > 0 return true or false to determine the action.

private function method($results){
    if (count(($results)) > 0) {
        echo Zend_Json::encode($result);
        return true;
    }
    return false;
}

Check out this video "Your code sucks, let's fix it - By Rafael Dohms" to help with understanding the above. https://www.youtube.com/watch?v=GtB5DAfOWMQ

I hope this is helpful!

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ Hi and thanks for your time and input. A great remark about the return's and else's - seems I have a bad habit of doing that. I did cut them down now. Seeing your profile picture I feel like I can ask this: the lines with $out['results']: they're both in the code I've put here, but also in findByTaxOrStatistical() collection method, which isn't shown here. Which place would be best to handle such 'representation formatting'? Would it be model class? Mapper class? Or just leave it as it is? Is there a general rule to it ? \$\endgroup\$ – delavey Dec 10 '19 at 12:02
  • \$\begingroup\$ It seems like what your doing is creating a REST response. You should understand that is more than outputting JSON. There is also a status code, like 200, that should be set. There are lots of simple REST response libraries out there, and you may want to look at calling one of them rather than echoing JSON, I would say that is a best practice. (also, so it's been said here, done the exact same things wrong, equal learning partner) \$\endgroup\$ – hxtree Dec 10 '19 at 12:34
  • \$\begingroup\$ The functionality you speak about is handled in SugApi service. I did not put its code here, as it wasn't part of the problem that I wanted to solve. The only exception not covered directly by SugApi is visible in the first post (that is, NotFoundException). \$\endgroup\$ – delavey Dec 10 '19 at 12:59

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.