1

I implementet a controller action in the maybe most unelegant way. How could this made better? Table classes are just like after bin/cake bake. I think the part where the Entity is created could be simplyfied very much.

What I'm doing: Books --belongsTo--> Publishers <--habtm--> Publishernumbers When adding a Book to the database, the publishernumber is extracted from the ISBN number. This number is then linked to the publisher in a habtm relation. I need this to suggest the user some publishers when typing an isbn in the form.

The code works for now, but in a year, only god will know what I did here. The first part is straightforward.

public function add()
{
    $book = $this->Books->newEntity();
    $associations = ['associated' =>
            [   
                'Tags',
                'Publishers',
                'Publishers.Publishernumbers'
            ]
        ];
    if ($this->request->is('post')) {
        $data= $this->request->data;


        $publisher = $this->Books->Publishers->get(
            $this->request->data['publisher_id'], 
                ['contain' => ['Publishernumbers']]
        );
        unset($data['publisher_id']);

        $book->publisher = $publisher;

    //extract group- and publishernumber from the ISBN
    $this->loadComponent('Isbn.Isbn');
    $split = $this->Isbn->splitIsbn($this->request->data['isbn']);
    $publishernumber = $split[1].$split[2];

This is the part where the mess begins. I think this could be done way more elegant.

    //check if the publisher contains already the $publishernumber
    //and if not, add it to the entity
    $new = true;
    foreach ($book->publisher->publishernumbers as $n){
        if ($n->number == $publishernumber){
            $new = false;
        }
    }
    if ($new){
        $existingNumber = $this->Books->Publishers->Publishernumbers
            ->findByNumber($publishernumber)
            ->first();
        if (!$existingNumber){

            //publishernumber does not exist in the database
            $pubnumber = $this->Books->Publishers->Publishernumbers
                ->newEntity();
            $pubnumber = $this->Books->Publishers->Publishernumbers
                ->patchEntity($pubnumber, ['number' => $publishernumber]);
            $book->publisher->publishernumbers[] = $pubnumber;

        } else {

            //publishernumber exists in the database 
            //but is not associated with the publisher
            $book->publisher->publishernumbers[] = $existingNumber;
        }
        $book->publisher->dirty('publishernumbers', true);
    }


    $book = $this->Books->patchEntity($book, $data, $associations);

Saving 

    if ($this->Books->save($book, $associations)){
        Cache::delete('exlibrisBooksIndex');
        $this->Flash->success(__('The book has been saved.'));
        return $this->redirect(['action' => 'index']);
    } else {
        $this->Flash->error(__('Error.'));
    }
}
$publishers = $this->Books->Publishers->find('list')
    ->order('name')
    ->toArray();
$this->set(compact('book', 'publishers'));
$this->set('_serialize', ['book']);
}
Matthias Moritz
  • 121
  • 14
  • Step one to cleaning up that controller action is to basically put all of the code in a table class method; Your controller code should look almost the same as baked output irrespective of how complex the act of saving a record is. – AD7six Oct 01 '15 at 08:34
  • Thats what I did. Controller Action looks now like its baked, and all the additional logic is now in the publishersTable. Called in the books afterSave methode. Looks better, and is testable! – Matthias Moritz Oct 16 '15 at 10:03
  • You should add a little more detail of what you did as an answer and accept it – AD7six Oct 17 '15 at 12:47

0 Answers0