4
\$\begingroup\$

Is there a better way to write this code.
Suppose I have 10 files to delete and I have to repeat code (public function destroy() ) 10 times.

  1. First remove the database entry.
  2. List item, then remove the file.

OOP - "don't repeat yourself", it works but I repeat the code.

public function image_path() {
        return $this->upload_dir .DS.  $this->filename; 
    }

    public function image_path2() {
        return $this->upload_dir .DS.  $this->filename2;    
    }

    public function destroy() {
        if($this->delete()) {
            $target_path = PUBLIC_PATH.DS.'public'.DS.$this->image_path();
            return unlink($target_path) ? true : false;
        } else {
            return false;
        }
    }

    public function destroy2() {
        if($this->delete()) {
            $target_path2 = PUBLIC_PATH.DS.'public'.DS.$this->image_path2();
            return unlink($target_path2) ? true : false;
        } else {
            return false;
        }
    }

delete_photo.php

  // Delete photo
  $result = $photo->destroy();
  $result = $photo->destroy2();

photo class

public function delete() {
        global $database;

        $sql = "DELETE FROM " . self::$table_name . " ";
        $sql .= "WHERE id='" . $database->escape_value($this->id) . "' ";
        $sql .= "LIMIT 1";
        $result = $database->query($sql);
        return $result;
    }



protected static $db_fields=array('id', 'filename', 'filename2', 'type', 'type2','size', 'size2', 'caption');
    public $id;
    public $filename;
    public $filename2;
    public $type;
    public $type2;
    public $size;
    public $size2;
    public $caption;
\$\endgroup\$
4
  • \$\begingroup\$ why do you have $filename1 and $filename2 \$\endgroup\$
    – dustytrash
    Nov 29 '19 at 20:46
  • \$\begingroup\$ Could you post the schema as well \$\endgroup\$
    – dustytrash
    Nov 29 '19 at 20:46
  • \$\begingroup\$ $filename1 and $filename2 to upload 2 or more files. \$\endgroup\$ Nov 29 '19 at 21:40
  • \$\begingroup\$ sql schema? -> id, caption, filename, filname2, size, size2, type, type2 \$\endgroup\$ Nov 29 '19 at 21:42
3
\$\begingroup\$

Yes, clearly you should write D.R.Y. code by not writing multiple methods that perform the same task.

You only need one image_path() and one destroy() method, it is the data delivery to those methods that needs to change and then of course the methods need to be altered to iterate the data.

If you expect to be handing multiple files the same way, I recommend a class variable (array of filenames) which can be filled as necessary, then iterated by your methods.

This mean doing away with:

public $filename;
public $filename2;

and instead use:

public $filenames = [];

A final note about reducing syntax:

if($this->delete()) {
    $target_path = PUBLIC_PATH.DS.'public'.DS.$this->image_path();
    return unlink($target_path) ? true : false;
} else {
    return false;
}

Can be rewritten as:

if (!$this->delete()) {
    return false;
}
return unlink(PUBLIC_PATH . DS . 'public' . DS . $this->image_path());

or even shorter if you prefer inline conditions:

return !$this->delete() ? false : unlink(PUBLIC_PATH . DS . 'public' . DS . $this->image_path());

That said, there is nothing wrong with if blocks, but your ? true : false is unnecessary, I try to avoid declaring single-use variables, and I prefer to write failure / false / early returns before positive outcomes.

\$\endgroup\$

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.