4
\$\begingroup\$

This little project is about working with doctrine Entity object and everything is working fine.

This is my first project in Symfony 3. Please review my service file and give me some advice to improve it. My main concern is lack of error handling.

service.yml

parameters:

services:

      app.ToDoList:
        class: AppBundle\Utils\ToDoList
        arguments: ['@doctrine.orm.entity_manager']

Service file

<?php


namespace AppBundle\Utils;
use AppBundle\Entity\test;

class ToDoList
{
    private $doctrineManager;

    public function __construct($em)
    {
        $this->doctrineManager = $em;
    }

    public function insert($array){
        if(!isset($array['name'])){return false;}

        $task = new test();
        $task->setName($array['name']);
        $task->setDate(new \DateTime());
        if(isset($array['description'])){
            $task->setDescription($array['description']);
        }
        $this->doctrineManager->persist($task);
        $this->doctrineManager->flush();

        return true;
    }

    public function get(){
        $all = $this->doctrineManager->getRepository('AppBundle:test')->findAll();
        if($all==null) {return false;}

        $json = $this->transformIntoJson($all);
        return $json;
    }
    public function getOne($id){
        $one = $this->doctrineManager->getRepository('AppBundle:test')->findOneById($id);
        if($one==null) {return false;}

        $arr[]=$one;
        $json = $this->transformIntoJson($arr);
        return $json;
    }

    public function transformIntoJson($arr){
        $table = [];
        foreach($arr as $value){
            $field = new \stdClass();
            $field->id = $value->getId();
            $field->name = $value->getName();
            $field->description = $value->getDescription();
            array_push($table,$field);
        }
        $table = json_encode($table);
        return $table;
    }

    public function delete($id){
        $one = $this->doctrineManager->getRepository('AppBundle:test')->findOneById($id);
        if($one==null){return false;}

        $this->doctrineManager->remove($one);
        $this->doctrineManager->flush();
        return true;

    }
}

Controller file

<?php

namespace AppBundle\Controller;


use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Symfony\Component\HttpFoundation\Response;

class ToDoController extends Controller
{
    /**
     * @Route("/insert"), name="InsertIntoTable")
     */
    public function insertToList(){
            $test = $this->get("app.todolist")->insert(["name"=>"TaskName",
                                                        "description"=>"TaskDecription"]);
            if($test == true){
                $response = "Task inserted succesfully"; }else{
                $response = "Error";
            }
            return new Response($response);
        }
    /**
     * @Route("/"), name="AllTasks")
     */
    public function getAll(){
       if ($tasks = $this->get("app.todolist")->get()){
           return new Response($tasks);
       }else{
           return new Response("0 tasks");
       }
    }
    /**
     * @Route("/{id}"), name="One task")
     */
    public function getOne($id){
        if($task = $this->get("app.todolist")->getOne($id)){
            return new Response($task);
        }else{
            return new Response("There is no such task");
        }
    }
    /**
     * @Route("/delete/{id}"), name="Dlete task")
     */
    public function delete($id){
        if($task = $this->get("app.todolist")->delete($id)){
            return new Response("Task was deleted succesfully");
        }else{
            return new Response("There are no task with that number");
        }
    }
}

Entity file - created by doctrine

<?php

namespace AppBundle\Entity;

use Doctrine\ORM\Mapping as ORM;

/**
 * test
 *
 * @ORM\Table(name="test")
 * @ORM\Entity(repositoryClass="AppBundle\Repository\testRepository")
 */
class test
{
    /**
     * @var int
     *
     * @ORM\Column(name="id", type="integer")
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private $id;

    /**
     * @var string
     *
     * @ORM\Column(name="name", type="string", length=255)
     */
    private $name;

    /**
     * @var string
     *
     * @ORM\Column(name="description", type="string", length=255)
     */
    private $description;

    /**
     * @var \DateTime
     *
     * @ORM\Column(name="date", type="datetime", nullable=true)
     */
    private $date;


    /**
     * Get id
     *
     * @return int
     */
    public function getId()
    {
        return $this->id;
    }

    /**
     * Set name
     *
     * @param string $name
     *
     * @return test
     */
    public function setName($name)
    {
        $this->name = $name;

        return $this;
    }

    /**
     * Get name
     *
     * @return string
     */
    public function getName()
    {
        return $this->name;
    }

    /**
     * Set description
     *
     * @param string $description
     *
     * @return test
     */
    public function setDescription($description)
    {
        $this->description = $description;

        return $this;
    }

    /**
     * Get description
     *
     * @return string
     */
    public function getDescription()
    {
        return $this->description;
    }

    /**
     * Set date
     *
     * @param \DateTime $date
     *
     * @return test
     */
    public function setDate($date)
    {
        $this->date = $date;

        return $this;
    }

    /**
     * Get date
     *
     * @return \DateTime
     */
    public function getDate()
    {
        return $this->date;
    }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Why are you using a Service for this task? In your example your setting the values hard coded, but I assume that these will submitted by a request later. Why don't you use a FormType, that will also handle Constraints for each form field corresponding to your class members? \$\endgroup\$ Commented Jan 5, 2017 at 20:23

2 Answers 2

3
\$\begingroup\$
  1. It's better to user snake_case for service names. so it's better to be app.to_do_list

  2. I prefer adding now when using new \Datetime(); it's more readable.

  3. Instead of get method you can call it getAll.

  4. You can create a new service and move the transformIntoJson method there and add it as a dependency to your class.

  5. Use snake_case for action names. like all_tasks instead of AllTasks

and please follow PSR-* coding styles.

\$\endgroup\$
2
\$\begingroup\$

You can omit else in most cases:

/**
 * @Route("/"), name="AllTasks")
 */
public function getAll(){
   if ($tasks = $this->get("app.todolist")->get()){
       return new Response($tasks);
   }

   return new Response("0 tasks");
}

Use a FCQN as service id and you can omit the class. And if you use autowire you can omit the arguments too:

services:
  _defaults:
    autowire: true
  AppBundle\Utils\ToDoList: ~

In your controller:

$test = $this->get(ToDoList::class)->insert(...

See

\$\endgroup\$

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.