Uploading file for related entity

Question

I see that my code smells. The first file is a controller, and createAction. But I don't know where to delegate this.

BookController.php - Whole controller class

So this is most 'smelly' code to me:

public function createAction(Request $request) {
 $user = $this->get('security.context')->getToken()->getUser();
 $book = new Book();
 $form = $this->createForm(new BookType($user->getId()), $book);
 $form->handleRequest($request);
 if($form->isValid()) {
 $book->upload();
 $em = $this->getDoctrine()->getManager();
 $em->persist($book);
 $em->flush();
 $covers = $request->files->get('covers');
 foreach ($covers as $cover) {
 $coverObj = new BookCover();
 $coverObj->setFile($cover);
 $coverObj->setBook($book);
 $coverObj->upload();
 $em->persist($coverObj);
 }
 $em->flush();
 return $this->redirect($this->generateUrl('matiit_book_homepage'));
 }
 return $this->render('MatiitBookBundle:Book:new.html.twig', ['form' => $form->createView()]);
}

Rest of files:

• new.html.twig
• Book.php
• BookType.php
• BookCover.php

Uploading in entities is the second thing which bothers me.

It would be nice to know what is worth testing here - I have no experience with tests so it would be very useful for me.

• qooplmao: How come you are not using a postPersist for the upload in your book, also why are you using $coverObj->upload(); when it should already be covered by the postPersist? Also I don't think the first $em->flush() is needed. How come you have a field file in your form but are getting the files from an upload covers?

Answer 1

BookController.php

I don't see anything to change, the only thing which i could do different is the return line from

 return $this->render('MatiitBookBundle:Book:new.html.twig', ['form' => $form->createView()]);

i would write something like this to make it more clear

return $this->render(
 'MatiitBookBundle:Book:new.html.twig', 
 [
 'form' => $form -> createView()
 ]
);

So if one day you need to add another line you will just add a , and the code will still look good.

new.html.twig

Why var $file? Why this $? It remember me something which is related to jQuery... Do you really need this? I would just do var file or choose a more specific name. (no idea right now, file could be ok but what is book_file? a input type="file"?)

Book.php

You use documentation only to specify the type of the fields? You could start with add the documentation of the fields and document all methods (or at least public/protected methods)

I'm wrong or here the indentation is a bit messed up? Could be just a copy/paste bug, anyway from protected $category; it wil look like you start write an inner class. (same for the end of the file)

Fix it if it's in your code.

What is length? I'm not English so maybe i'm wrong but i don't think length is a great name for a Book class (without documentation it's more hard to understand what is it).

It's the book pages? What about totalPages? It's the length (size) of the file in bytes? What about fileSize?

BookType.php

class BookType extends AbstractType

Why not call your class AbstractType as AbstractBookType?

AbstractType could sound as something which is related to variables type, not your class.

I know you use namespaces but i think it's important to make the name of something very clear.

Here you could add documentation too.

BookCover.php

It looks so similar to Book.php

Some methods are similar, maybe Book should have an instance of BookCover inside instead of recreate the methods from zero?

I need more info about it.

protected function getUploadRootDir()
{
 // the absolute directory path where uploaded
 // documents should be saved
 return __DIR__.'/../../../../../web/'.$this->getUploadDir();
}

I'm not a PHP expert, but what if one day you change the location? You should remember to edit this method inside BookCover and inside Book too! Can't you make it in a static field for the two classes?

And you should maintain the /../../../../../ to point in the new location!

Anyway my review is more about style than other.

• matiit: var named $file is because I like to name all cached jQuery variables with prefix $. It makes it easy to know that this is jQuery object with all its methods.
• matiit: AbstractType is just a class from Symfony. Namespaces makes it self-explanatory - i can't agree.
• matiit: To the rest I would agree. Thank You very much.
• Marco Acierno: Suppose in another file (maybe the main file) you use it AbstractType abc; (example) what is AbstractType? One should go up and see the namespace?
• matiit: it's the built in symfony class