0
\$\begingroup\$

I have a class which parses a .csv file and converts it into successful rows and errors. Currently I have 12 different kind of errors (mail is not valid, missing column x, row is duplicate etc.).

Each error has a custom message. I also need to know in other classes what kind of error a row was, so a simple string is not enough. So far I am using this error class:

<?php

namespace App\Registration\GroupUploads;

/**
 * Represents all possible errors that may occur during a group upload.
 */
class Error
{

  const USEREXISTS  = 7;
  const MAILINVALID = 1;
  const SEALUSED    = 2;
  const MIXEDUP     = 3;
  // ..

  protected $id;
  protected $options;

  /**
   * Each error has a unique ID
   * @param int $id
   * @param array $options optional
   */
  protected function __construct($id, $options = null)
  {
    $this->id      = $id;
    $this->options = $options;
  }

  /**
   * Get Message of current object
   * @return string
   */
  public function getMessage()
  {
    switch ($this->id) {
      case static::USEREXISTS:
        return 'User already exists';
        break;

      case static::MAILINVALID:
        return 'Mail is not valid';
        break;

      case static::SEALUSED:
        return 'Sealnr is already used by other user.';
        break;

      case static::SEALUSED:
        return 'They messed up and mixed the seals inbetween orgas.';
        break;

      // ...

      default:
        return 'No message provided for error code';
        break;
    }
  }

  /**
   * Create error for an existing user
   * @return bool
   */
  public static function getDuplicateUserError()
  {
    return new static(static::USEREXISTS);
  }

  /**
   * Check if class is duplicate user error
   * @return bool
   */
  public function isDuplicateUserError()
  {
    return $this->id == static::USEREXISTS;
  }

  // ...

}

There are 12 constants, each representing the id of a specific error. There is one method getMessage() which uses a switch with 12 cases. Then I have 12 messages of the kind getErrorMessageX and another 12 methods isThisErrorNameX.

I feel that this is kind of messy and redundant. Also their might be me more error cases in the future, which will bloat up the class even more.

I thought about creating 12 separate classes, each class named by the error name. Something like that:

<?php 

namespace App\Registration\GroupUploads;

/**
 * Error for existing user
 */
class UserExistsAlreadyError
{

  public function getMessage()
  {
    return 'User already exists';
  }

}

So instead of if($error->isDuplicateUserError()) I would write if($error instanceof UserExistsAlreadyError) .

However, I think creating 12 different classes, where each consists only of 12 lines, is a bit too much. Is there a better solution then those two extremes?

\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

This is how I ended up doing it now:

<?php

namespace App\Registration\GroupUploads\Errors;

/**
 * Error that appear during parsing csv file
 * @var [type]
 */
abstract class Error
{
  const USEREXISTS  = 7;
  const MAILINVALID = 1;
  const SEALUSED    = 2;
  const MIXEDUP     = 3;
  const QUALMISSING = 20;

  public $id;
  protected $msg;

  /**
   * Get error message
   * @return [type] [description]
   */
  public function getMsg()
  {
    return $this->msg;
  }

  /**
   * Error if user existed before registration
   * @param  [type] $name [description]
   * @return [type]       [description]
   */
  public static function userAlreadyExists($name)
  {
    $error = new Error();
    $error->id   = static::USEREXISTS;
    $error->msg  = "User " . $name .  " already exists";
    return $error;
  }

  /**
   * Mail is not valid
   * @param  [type] $mail [description]
   * @return [type]       [description]
   */
  public static function mailInvalid($mail)
  {
    $error = new Error();
    $error->id   = static::MAILINVALID;
    $error->msg  = "The mail " . $mail .  " is not valid";
    return $error;
  }

  //..
}

This way the getMsg method is short. Also adding a new error only requires to add one method instead of two (and no modification of the getMsg function).

To identify an error I call if($error->id == Error::USEREXISTS){...}.

Having the id was quite important, because I wanted to store the errors in DB.

\$\endgroup\$
-1
\$\begingroup\$

Its better to use factory pattern for creating custom errors. I'm working on Java and following Java code might be helpful:

class CsvException extends Exception {
    public static final int USEREXISTS = 7;
    public static final int MAILINVALID = 1;
    public static final int SEALUSED = 2;
    public static final int MIXEDUP = 3;

    public static final CsvException createException(int type) {
        CsvException exc = null;
        switch (type) {
        case CsvException.USEREXISTS:
            exc = new UserExistException();
            break;

        case CsvException.MAILINVALID:
            exc = new MailInvalidException();
            break;

        case CsvException.SEALUSED:
            exc = new SealUsedException();
            break;

        case CsvException.MIXEDUP:
            exc = new MixedUpException();
            break;

        default:
            exc = new CsvException();
            break;
        }
        return exc;
    }

    @Override
    public String getMessage() {
        return "A CSV Exception occurred!";
    }
}

class UserExistException extends CsvException {

    @Override
    public String getMessage() {
        return "User already exists!";
    }

}

class MailInvalidException extends CsvException {
    @Override
    public String getMessage() {
        return "Invalid email ID!";
    }
}

class SealUsedException extends CsvException {
    @Override
    public String getMessage() {
        return "Seal is already used!"; //I'm assuming message and this class by exception type
    }
}

class MixedUpException extends CsvException {
    @Override
    public String getMessage() {
        return "Mixed up!";
    }
}

And you can use above exception factory like below:

    ...

    public void checkMail() throws MailInvalidException {
        //If invalid email ID
        throw (MailInvalidException)CsvException.createException(CsvException.MAILINVALID);
    }

    public void checkUser() throws UserExistException {
        //If user already exists
        throw (UserExistException)CsvException.createException(CsvException.MAILINVALID);
    }

    ...
\$\endgroup\$
3
  • \$\begingroup\$ I believe these objects are not intended to be thrown \$\endgroup\$ Commented Apr 29, 2019 at 11:35
  • 1
    \$\begingroup\$ You should add more details about your code because it's not clear what this code is about. \$\endgroup\$
    – t3chb0t
    Commented Apr 29, 2019 at 12:26
  • \$\begingroup\$ @t3chb0t, The first code section contains required classes for exception, the 2nd code section shows a small example of use of Exception factory. You can read more about factory pattern here : geeksforgeeks.org/design-patterns-set-2-factory-method \$\endgroup\$
    – Girish
    Commented Apr 30, 2019 at 3:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.