Is this Code-ReUse abuse?

PHP
1

Mainly the if getErrors() == true…
For me it’s easy to read but it’s sometimes irritating to type. Is this considered code re-use and abuse?

Any suggestions on how to trim it down if I should?
(I’m going to initialize the File inside the form object at some point, but that’s not what I’m asking yet) :slight_smile:

	function createSave()
	{
		$file = new File_Handler('file', false);
		$file->setType = 'zip|gz|tar|rar';
		$file->setLocation('../../../');
		$file->save();

		if ($file->getErrors() == false)
		{
			$this->form->post('assign_package');
			$this->form->post('title|length=1,100');
			$this->form->post('decription');
			$this->form->setData('created', DATE_TIME);
			$this->form->setData('physical_file', 'UNSURE YET');

			if ($this->form->getErrors == false)
			{
				$this->db->insert('files', $this->form->getData());

				if ($this->db->getErrors() == false)
				$this->status('Results saved!');

				else
				$this->status($this->db->getErrors());

			}
			else
			$this->status($this->form->getErrors());
		}
		else
		$this->status($file->getErrors());
	
		refresh('files');
	}
2

Yes, it begins to look pretty messy as the conditional statements nesting goes many levels deep. I know what you mean, this happened to me many times, especially when validating a lot of data, and all I remember is that the code was becoming hard to read, maintain and modify later on. Then I began using exceptions for this and things got much better. Try this:

function createSave()
{
    try {
        $file = new File_Handler('file', false);
        $file->setType = 'zip|gz|tar|rar';
        $file->setLocation('../../../');
        $file->save();
    
        $this->form->post('assign_package');
        $this->form->post('title|length=1,100');
        $this->form->post('decription');
        $this->form->setData('created', DATE_TIME);
        $this->form->setData('physical_file', 'UNSURE YET');

        $this->db->insert('files', $this->form->getData());

        $this->status('Results saved!');

    } catch (Exception $ex) {
        $this->status($ex->getMessage());
    }

    refresh('files');
}

Then you can add as many points to catch errors as you wish without risking to make your code messy. Simply replace saving errors in your object property with throw Exception syntax and that’s it, the getErrors() method will not be necessary. You may want to use your own exception class if the built-in one is not enough.

3

Update: I realize that you may want to catch more than one error at a time and my previous example will not allow this. In such cases I collect errors somewhere in an object property (similar to what you were doing) and then use validate() method to throw the exception:


function createSave()
{
    try {
        $file = new File_Handler('file', false);
        $file->setType = 'zip|gz|tar|rar';
        $file->setLocation('../../../');
        $file->save();
        $file->validate();
    
        $this->form->post('assign_package');
        $this->form->post('title|length=1,100');
        $this->form->post('decription');
        $this->form->setData('created', DATE_TIME);
        $this->form->setData('physical_file', 'UNSURE YET');
        $this->form->validate();

        $this->db->insert('files', $this->form->getData());
        $this->db->validate();

        $this->status('Results saved!');

    } catch (MyException $ex) {
        $this->status($ex->getErrors());
    }

    refresh('files');
}

You would need to use your own exception class if you wanted to pass error messages as arrays because the standard one will not accept array parameters. Or use a workaround with delimiting error messages with some character. Personally, I would not do $db->validate(); because on db errors I almost always want to throw a very critical error and handle it in a special way (halt the application, show some generic alert to the user, log the exact error to a file, etc.).

4

Remember that you can catch multiple types of exception…

If this is the route that OP is going to take, then I would suggest that on failure of the $db->validate, a ‘dbValidationException’ is thrown.

This can then be caught:


try {
   .........
} catch (dbValidationException e) {
   .........
   echo 'Post data not valid';
} catch (formException e) {
   .......
   echo 'Some sort of form error';
} catch (dbException e) {
   .....
   echo 'Epic failure..... db.... display generic error msg and log';
} catch (Exception e) {
   echo 'Some sort of other exception occurred....';
}

Personally I find this much neater as all the error handling is done in the same place.

The down side… you need to write a few custom exceptions… easy to do but can clutter up the file system a bit.

G

5

Also if you don’t want to go with Exception and keep checking for errors, why don’t
replace the:
if ($this->db->getErrors() == false)
with
if (!$this->db->getErrors() )

Just a bit shorter.

6

Wow I never considered a try block for all those things, this could be extremely awesome!! Im going to monkey with this soon! Thanks :slight_smile:

7

And the reason I did:

if ($this->db->getErrors() == false)

Rather than
if (!$this->db->getErrors())

is because I found it easier to skim and read what im doing with the syntax highlighting.

8

At least you didn’t do:
if ($this->db->getErrors() == true)

That just drives me crazy, when I see it.

9

How about an hasErrors() method? That would be even easier to read. :slight_smile:

10

I did that to keep with the idea of “getter/setter” logic, because it returns an array of errors or false. If it was boolean I’d probably do has :stuck_out_tongue: