7
\$\begingroup\$

I have got a technical challenge from a company but they reject my code and I'm not sure how to do this challenge in a more efficient way so I'm here to get some guidance.

The Technical challenge was:

Manage a list of products that have prices.

  • Enable the administrator to set concrete prices (such as 10EUR) and discounts to prices either by a concrete amount (-1 EUR) or by percentage (-10%).
  • Enable the administrator to group products together to form bundles (which is also a product) that have independent prices.
  • Enable customers to get the list of products and respective prices.
  • Enable customers to place an order for one or more products, and provide customers with the list of products and the total price.

I have used Symfony framework to build this API and I'm writing the company response here:

  • SingleProduct and BundleProduct should be polymorphic.
  • ConcretePrice, DiscountedPriceByAmount, DiscountedPriceByPercentage should be polymorphic.
  • The computation of the overall sum of prices for the order should make no assumption about how the individual price was calculated (fix price, or discounted).
  • The response should provide a deep object structure (as opposed to a flat list) that preserves the semantics of the model and is suitable for rendering.

/src/Entity/Product.php

<?php 

// src/Entity/Product.php

namespace App\Entity;

use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Validator\Constraints as Assert;


/**
 * @ORM\Entity(repositoryClass="App\Repository\ProductRepository")
 * @ORM\Table(name="products")
 */
class Product {
    /**
     * @ORM\Column(type="integer")
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private $id;

    /**
    * @Assert\NotBlank()
    * @ORM\Column(type="string", length=150)
    */
    private $title;
    /**
     * @ORM\Column(type="text", length=150, nullable=true)
     */
    private $slug;
    /**
    * @Assert\NotBlank()
    * @ORM\Column(type="float", scale=2)
    */
    private $price;
    /**
    * @ORM\Column(type="string", length=5)
    */
    private $currency = '€';
    /**
    * @ORM\Column(type="string", length=3, options={"comment":"Yes, No"})
    */
    private $isDiscount = 'No';
    /**
    * @ORM\Column(type="string", length=10, options={"comment":"Concrete amount (-1 EUR) or by Percentage (-10%)"}, nullable=true)
    */
    private $discountType;
    /**
    * @ORM\Column(type="integer", length=5, options={"comment":"1 or 10"})
    */
    private $discount = 0;
    /**
    * @ORM\Column(type="string", length=5, options={"comment":"No, Yes, if yes then save product ids in product bundle items"})
    */
    private $isProductBundle = 'No';
    /**
     * @ORM\Column(type="text", length=150, nullable=true)
    */
    private $sku;
    /**
    * @ORM\Column(type="string", length=15, options={"comment":"Active or Pending , only Active products will display to customers"})
    */
    private $status = 'Active';
    /**
    * @ORM\Column(type="string", length=150, options={"comment":"Upload or Link of image"})
    */
    private $imageType = 'Link';
    /**
     * @ORM\Column(type="text")
     */
    private $image = 'https://via.placeholder.com/400x300.png';
    /**
     * @ORM\Column(type="text", nullable=true)
     */
    private $description;
    /**
     * @ORM\Column(type="datetime", nullable=true)
     */
    private $createdAt;
    /**
     * @ORM\Column(type="datetime", nullable=true)
     */
    private $updatedAt;


    //Getters and Setters


}

/src/Entity/ProductBundleItem.php

<?php 

// src/Entity/ProductBundleItem.php

namespace App\Entity;

use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Validator\Constraints as Assert;


/**
 * @ORM\Entity(repositoryClass="App\Repository\ProductBundleItemRepository")
 * @ORM\Table(name="product_bundle_items")
 */
class ProductBundleItem {
    /**
     * @ORM\Column(type="integer")
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private $id;
    /**
     * @ORM\Column(type="integer")
    */
    private $productBundleId;

    /**
    * @ORM\Column(type="integer")
    */
    private $productId;


    //Getters and Setters

/src/Repository/ProductRepository.php

<?php

namespace App\Repository;

use App\Entity\Product;
use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
use Symfony\Bridge\Doctrine\RegistryInterface;

/**
 * @method Product|null find($id, $lockMode = null, $lockVersion = null)
 * @method Product|null findOneBy(array $criteria, array $orderBy = null)
 * @method Product[]    findAll()
 * @method Product[]    findBy(array $criteria, array $orderBy = null, $limit = null, $offset = null)
 */
class ProductRepository extends ServiceEntityRepository
{
    public function __construct(RegistryInterface $registry)
    {
        parent::__construct($registry, Product::class);
    }

    public function findAllQueryBuilder()
    {
        return $this->createQueryBuilder('products');
    }

    // /**
    //  * @return Product[] Returns an array of Product objects
    //  */
    /*
    public function findByExampleField($value)
    {
        return $this->createQueryBuilder('t')
            ->andWhere('t.exampleField = :val')
            ->setParameter('val', $value)
            ->orderBy('t.id', 'ASC')
            ->setMaxResults(10)
            ->getQuery()
            ->getResult()
        ;
    }
    */

    /*
    public function findOneBySomeField($value): ?Product
    {
        return $this->createQueryBuilder('t')
            ->andWhere('t.exampleField = :val')
            ->setParameter('val', $value)
            ->getQuery()
            ->getOneOrNullResult()
        ;
    }
    */
}

/src/Repository/ProductBundleItemRepository.php

<?php

namespace App\Repository;

use App\Entity\ProductBundleItem;
use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
use Symfony\Bridge\Doctrine\RegistryInterface;

/**
 * @method ProductBundleItem|null find($id, $lockMode = null, $lockVersion = null)
 * @method ProductBundleItem|null findOneBy(array $criteria, array $orderBy = null)
 * @method ProductBundleItem[]    findAll()
 * @method ProductBundleItem[]    findBy(array $criteria, array $orderBy = null, $limit = null, $offset = null)
 */
class ProductBundleItemRepository extends ServiceEntityRepository
{
    public function __construct(RegistryInterface $registry)
    {
        parent::__construct($registry, ProductBundleItem::class);
    }

    public function findByProductBundleIdJoinedToProduct($productBundleId)
    {

        return $this->createQueryBuilder('pbi')
                ->select('p.id','p.title', 'p.price', 'p.currency')
                ->leftJoin('App\Entity\Product', 'p', 'WITH', 'p.id = pbi.productId')
                ->where('pbi.productBundleId = :productBundleIdParam')
                ->setParameter('productBundleIdParam', $productBundleId)
                ->getQuery()
                ->getResult();
    }


    // /**
    //  * @return ProductBundleItem[] Returns an array of ProductBundleItem objects
    //  */
    /*
    public function findByExampleField($value)
    {
        return $this->createQueryBuilder('t')
            ->andWhere('t.exampleField = :val')
            ->setParameter('val', $value)
            ->orderBy('t.id', 'ASC')
            ->setMaxResults(10)
            ->getQuery()
            ->getResult()
        ;
    }
    */

    /*
    public function findOneBySomeField($value): ?ProductBundleItem
    {
        return $this->createQueryBuilder('t')
            ->andWhere('t.exampleField = :val')
            ->setParameter('val', $value)
            ->getQuery()
            ->getOneOrNullResult()
        ;
    }
    */
}

/src/Controller/Api/ProductController.php

<?php

// src/Controller/Api/ProductController.php

namespace App\Controller\Api;

use FOS\RestBundle\Controller\Annotations as Rest;
use FOS\RestBundle\Controller\AbstractFOSRestController;
use FOS\RestBundle\View\View;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use App\Service\ProductService;

class ProductController extends AbstractFOSRestController
{

    private $productService;

    public function __construct(ProductService $productService){

        $this->productService = $productService;
    }

    /**
     * Retrieves a collection of Product resource
     * @Rest\Get("/products")
     */

    public function getProducts(Request $request): View
    {

        $params['page'] = $request->query->getInt('page', 1);
        $params['limit'] = $request->query->getInt('limit', 10);

        $products = $this->productService->getProducts($params);

        return View::create($products, Response::HTTP_OK);
    }

    /**
     * Retrieves a Product resource
     * @Rest\Get("/products/{slug}")
     */

    public function getProduct(Request $request, $slug): View
    {
        $product = $this->productService->getProduct($slug);

        return View::create($product, Response::HTTP_OK);
    }

    /**
     * Creates an Product resource
     * @Rest\Post("/products")
     * @param Request $request
     * @return View
    */
    public function addProduct(Request $request): View
    {
        $user = $this->getUser();
        if(!in_array('ROLE_ADMIN',$user->getRoles())){
            return View::create([], Response::HTTP_UNAUTHORIZED);
        }
        $params = json_decode($request->getContent(), true);
        $product = $this->productService->addProduct($params);
        return View::create($product, Response::HTTP_OK);

    }

    /**
     * Creates an Product resource
     * @Rest\Put("/products/{id}")
     * @param Request $request
     * @return View
    */
    public function updateProduct(Request $request, $id): View
    {
        $user = $this->getUser();
        if(!in_array('ROLE_ADMIN',$user->getRoles())){
            return View::create([], Response::HTTP_UNAUTHORIZED);
        }
        $params = json_decode($request->getContent(), true);
        $product = $this->productService->updateProduct($params, $id);
        return View::create($product, Response::HTTP_OK);

    }

    /**
     * Removes the Product resource
     * @Rest\Delete("/products/{id}")
    */
    public function deleteProduct($id): View
    {
        $user = $this->getUser();
        if(!in_array('ROLE_ADMIN',$user->getRoles())){
            return View::create([], Response::HTTP_UNAUTHORIZED);
        }
        $this->productService->deleteProduct($id);
        return View::create([], Response::HTTP_NO_CONTENT);

    }
}

/src/Controller/Api/ProductBundleController.php

<?php

// src/Controller/Api/ProductController.php

namespace App\Controller\Api;

use FOS\RestBundle\Controller\Annotations as Rest;
use FOS\RestBundle\Controller\AbstractFOSRestController;
use FOS\RestBundle\View\View;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use App\Service\ProductBundleService;

class ProductBundleController extends AbstractFOSRestController
{

    private $productBundleService;

    public function __construct(ProductBundleService $productBundleService){

        $this->productBundleService = $productBundleService;
    }

    /**
     * Retrieves a collection of Product bundle resource
     * @Rest\Get("/products-not-bundles")
     */

    public function getProductsIsNotBundles(): View
    {

        $products = $this->productBundleService->getProductsIsNotBundles();

        return View::create($products, Response::HTTP_OK);
    }


    /**
     * Retrieves a collection of Product bundle resource
     * @Rest\Get("/product-bundles")
     */

    public function getProductBundles(): View
    {

        $products = $this->productBundleService->getProducts();

        return View::create($products, Response::HTTP_OK);
    }

    /**
     * Retrieves a Product bundle resource
     * @Rest\Get("/product-bundles/{id}")
     */

    public function getProduct(Request $request, $id): View
    {
        $product = $this->productBundleService->getProduct($id);

        return View::create($product, Response::HTTP_OK);
    }

    /**
     * Creates an Product bundle resource
     * @Rest\Post("/product-bundles")
     * @param Request $request
     * @return View
    */
    public function addProduct(Request $request): View
    {
        $user = $this->getUser();
        if(!in_array('ROLE_ADMIN',$user->getRoles())){
            return View::create([], Response::HTTP_UNAUTHORIZED);
        }
        $params = json_decode($request->getContent(), true);
        $product = $this->productBundleService->addProduct($params);
        return View::create($product, Response::HTTP_OK);

    }

    /**
     * Update an Product bundle resource
     * @Rest\Put("/product-bundles/{id}")
     * @param Request $request
     * @return View
    */
    public function updateProduct(Request $request, $id): View
    {
        $user = $this->getUser();
        if(!in_array('ROLE_ADMIN',$user->getRoles())){
            return View::create([], Response::HTTP_UNAUTHORIZED);
        }
        $params = json_decode($request->getContent(), true);
        $product = $this->productBundleService->updateProduct($params, $id);
        return View::create($product, Response::HTTP_OK);

    }

    /**
     * Removes the Product bundle resource
     * @Rest\Delete("/product-bundles/{id}")
    */
    public function deleteProduct($id): View
    {
        $user = $this->getUser();
        if(!in_array('ROLE_ADMIN',$user->getRoles())){
            return View::create([], Response::HTTP_UNAUTHORIZED);
        }
        $this->productBundleService->deleteProduct($id);
        return View::create([], Response::HTTP_NO_CONTENT);

    }
}

/src/Service/ProductService.php

<?php

namespace App\Service;

use Doctrine\ORM\EntityManagerInterface;
use Pagerfanta\Adapter\DoctrineORMAdapter;
use Pagerfanta\Pagerfanta;
use App\Repository\ProductRepository;
use App\Utils\Slugger;
use App\Entity\Product;

final class ProductService
{
    /**
    * @var ProductRepository
    */
    private $productRepository;
    private $slugger;
    private $em;

    public function __construct(ProductRepository $productRepository, Slugger $slugger, EntityManagerInterface $em){

        $this->productRepository = $productRepository;
        $this->slugger = $slugger;
        $this->em = $em;
    }

    public function getProducts($params): ?array
    {
        $qb = $this->productRepository->findAllQueryBuilder();
        $adapter = new DoctrineORMAdapter($qb);
        $pagerfanta = new Pagerfanta($adapter);
        $pagerfanta->setMaxPerPage($params['limit']);
        $pagerfanta->setCurrentPage($params['page']);

        $products = [];
        foreach ($pagerfanta->getCurrentPageResults() as $result) {
            $products[] = $result;
        }

        $response =[
            'total' => $pagerfanta->getNbResults(),
            'count' => count($products),
            'products' => $products,
        ];
        return $response;


    }

    public function getProduct($slug){
        #Find by id
        //return $this->productRepository->find($id);
        #Or find by slug
        return $this->productRepository->findBy(['slug'=>$slug]);
    }

    public function addProduct($params){

        $product = new Product();
        foreach($params as $key=>$val){
            $property = 'set'.strtoupper($key);
            if(property_exists('App\Entity\Product',$key)){
                $product->$property($val);
            }
        }

        $slug = $this->slugger->slugify($product->getTitle());
        $product->setSlug($slug);
        $product->setCreatedAt(date("Y-m-d H:i:s"));
        $product->setUpdatedAt(date("Y-m-d H:i:s"));

        $this->em->persist($product);
        $this->em->flush();

        return $product;
    }

    public function updateProduct($params, $id){

        if(empty($id))
            return [];
        $product = $this->productRepository->find($id);
        if(!$product){
            return [];
        }
        foreach($params as $key=>$val){
            if($key=='id')
                continue;
            $property = 'set'.ucfirst($key);
            if(property_exists('App\Entity\Product',$key)){
                $product->$property($val);
            }
        }

        $slug = $this->slugger->slugify($product->getTitle());
        $product->setSlug($slug);
        $product->setUpdatedAt(date("Y-m-d H:i:s"));

        $this->em->persist($product);
        $this->em->flush();

        return $product;
    }

    public function deleteProduct($id){
        $product = $this->productRepository->find($id);
        if($product){
            $this->em->remove($product);
            $this->em->flush();
        }
    }
}

/src/Service/ProductBundleService.php

<?php

namespace App\Service;

use Doctrine\ORM\EntityManagerInterface;
use App\Repository\ProductRepository;
use App\Repository\ProductBundleItemRepository;
use App\Utils\Slugger;
use App\Entity\Product;
use App\Entity\ProductBundleItem;

final class ProductBundleService
{
    /**
    * @var ProductRepository
    */
    private $productRepository;
    private $productBundleItemRepository;
    private $slugger;
    private $em;


    public function __construct(ProductRepository $productRepository, Slugger $slugger, EntityManagerInterface $em, ProductBundleItemRepository $productBundleItemRepository){

        $this->productRepository = $productRepository;
        $this->productBundleItemRepository = $productBundleItemRepository;
        $this->slugger = $slugger;
        $this->em = $em;
    }

    public function getProductsIsNotBundles(): ?array
    {
        return $this->productRepository->findBy(['status'=>'Active', 'isProductBundle'=>'No']);

    }

    public function getProducts(): ?array
    {
        return $this->productRepository->findBy(['isProductBundle'=>'Yes'],['id'=>'DESC']);
    }

    public function getProduct($id){
        #Find by id
        //return $this->productRepository->find($id);
        #Or find by slug
        $product = $this->productRepository->findBy(['id'=>$id,'isProductBundle'=>'Yes']);

        $bunleItems = $this->productBundleItemRepository->findByProductBundleIdJoinedToProduct($product[0]->getId());

        $returnData['product'] = $product;
        $returnData['bunleItems'] = $bunleItems;
        return $returnData;
    }

    public function addProduct($params){

        $product = new Product();
        foreach($params as $key=>$val){
            $property = 'set'.strtoupper($key);
            if(property_exists('App\Entity\Product',$key)){
                $product->$property($val);
            }
        }

        $product->setIsProductBundle("Yes");

        $slug = $this->slugger->slugify($product->getTitle());
        $product->setSlug($slug);
        $product->setCreatedAt(date("Y-m-d H:i:s"));
        $product->setUpdatedAt(date("Y-m-d H:i:s"));

        $this->em->persist($product);
        $this->em->flush();

        $productsArr = $params['productsArr'];

        if(count($productsArr)>0){
            foreach($productsArr as $productId){
               $productBundleItem = new ProductBundleItem();
               $productBundleItem->setProductBundleId($product->getId());
                $productBundleItem->setProductId($productId);
                $this->em->persist($productBundleItem);
                $this->em->flush();
            }
        }
        $returnData['product'] = $product;
        $returnData['productsArr'] = $productsArr;
        return $returnData;
    }

    public function updateProduct($params, $id){

        if(empty($id))
            return [];
        $product = $this->productRepository->find($id);
        if(!$product){
            return [];
        }
        foreach($params as $key=>$val){
            if($key=='id')
                continue;
            $property = 'set'.ucfirst($key);
            if(property_exists('App\Entity\Product',$key)){
                $product->$property($val);
            }
        }

        $product->setIsProductBundle("Yes");
        $slug = $this->slugger->slugify($product->getTitle());
        $product->setSlug($slug);
        $product->setUpdatedAt(date("Y-m-d H:i:s"));

        $this->em->persist($product);
        $this->em->flush();

        $productsArr = $params['productsArr'];

        if(count($productsArr)>0){
            foreach($productsArr as $productId){

                $isExist = $this->productBundleItemRepository->findBy(['productId'=>$productId]);
                if(!$isExist){
                    $productBundleItem = new ProductBundleItem();
                    $productBundleItem->setProductBundleId($product->getId());
                    $productBundleItem->setProductId($productId);
                    $this->em->persist($productBundleItem);
                    $this->em->flush();
                }
            }
        }

        $returnData['product'] = $product;
        $returnData['productsArr'] = $productsArr;
        return $returnData;
    }

    public function deleteProduct($id){
        $product = $this->productRepository->find($id);
        if($product){
            $productBundleItems = $this->productBundleItemRepository->findBy(['productBundleId'=>$product->getId()]);
            $this->em->remove($product);

            foreach($productBundleItems as $item){
                $this->em->remove($item);
            }

            $this->em->flush();
        }
    }
}

The whole code can be view here

\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review! You stated: "I'm writing the company response here". Does that mean that is the response from the company about the code you submitted? If so, I would expect to see those class names like BundleProduct in your code and/or repo but I don't... \$\endgroup\$ Commented Jun 12, 2019 at 16:48
  • 1
    \$\begingroup\$ Yes, that is the response from the company where I submitted code, I have used isProductBundle column in Product entity but I have not made a separate class. \$\endgroup\$ Commented Jun 12, 2019 at 16:59

1 Answer 1

Reset to default
7
+250
\$\begingroup\$

I'll try to cover the response part.

SingleProduct and BundleProduct should be polymorphic. ConcretePrice, DiscountedPriceByAmount, DiscountedPriceByPercentage should be polymorphic.

I think they are talking about the following:

  • you need to have a table of products
  • and a table of prices
    • this is not exactly a requirement, but I guess this would be a very logical step because at some point you might need to handle price history, different pricelists due to geography, currencies, payment methods, etc.; but again: this does not seem to be a requirement at this point
  • you need to have a several entities per table, i.e. one "physical" record upon retrieval could be resolved into one of several classes (specifics are here: https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/reference/inheritance-mapping.html)
    • e.g. there is your entity Product, which should be an abstract class with mostly protected members, and some public methods that make sense for any kind of product
    • then there should be SingleProduct inherited from Product, which exposes additional members specific to this kind of product, and maybe override (or provide implementation) to some members of Product
    • and BundleProduct too, inherits from Product
    • as for what exactly must be polymorphic, I guess things like getPrice() -- implementation would be different for SingleProduct and BundleProduct
    • how exactly to do that -- the link above talks about several ways, and their procs and cons; I'd go with "Single Table Inheritance" for the sake of simplicity
    • and of course you need one-to-many/many-to-one relations, e.g. 'BundleProduct has many ProductBundleItem'; currently you're manipulating with IDs directly, but that's not how Doctrine encourages you to do things (see example below, where I talk about REST response)

So, to reiterate:

  • currently you have one Product class with all price/discount/bundling functionality built-in
  • you've been asked to make a nice hierarchy of classes (SingleProduct, BundleProduct, etc.) and make them do one thing, but do it good (think less if statements, more class diversity)
  • this does not imply you should put all functionality there -- a service is a good place to have your business logic; but functionality related to data itself -- how to interpret price, discounts, bundles -- that's a task for some code inside entities

The computation of the overall sum of prices for the order should make no assumption about how the individual price was calculated (fix price, or discounted).

This one is a natural continuation of previous point.

Currently your OrderService has pretty complex code that calculates individual product price. But that's not its concern! If at some place it has a reference to Product -- that is, base product class -- it should not check if this is a concrete product, or bundle, if discount is in effect. It should just be able to do this:

$product->getPrice();

And your class hierarchy should take care about specifics. Re how to achieve that, see above.

The response should provide a deep object structure (as opposed to a flat list) that preserves the semantics of the model and is suitable for rendering.

I believe what they talk about is how your API response looks when is compared to your DB structure: it should be really close, basically 1-to-1.

E.g. your method \App\Service\OrderService::getOrder which prepares data for /customer/orders/single/{id} route:

  • route could be /customer/orders/{id} -- the fact that it is 'single' is already obvious, as API user does GET with URL containing ID.
    • doing PUT to the same URL would do update of order
    • doing POST to /customer/orders (instead of /customers/orders/save) would create a new one
    • etc., this is basically any REST API around -- URL and HTTP method clearly defines what kind of operation that particular API call assumes
  • result of getOrder() is some custom array with items like 'order' and 'orderItems', but it could be literal representation of your object hierarchy (your hierarchy is rather vague at this moment due to lacking relations between entities, so, I'm just guessing here):
GET /customer/orders/id
{
    timestamp: '...',
    address: {
       billing: { city: ..., country: ...},
       shipping: { city: ..., country: ...}
    },
    items: [
        product: {
            name: '...',
            price: ...
        },
        quantity: ...
    ],
    total: ...
}

As you can see, this sample JSON contains implicit (from Doctrine point of view, anyway) relations from your code:

  • order points to several order items
  • order item points to one product
  • order item also has quantity field
  • order has total field
  • order points to one or more addresses of different kind

So, resulting structure should be rich in data, deep (I could get value of order.address.billing.city), and exactly reflect your data structures ("physical" DB records, then your polymorphic entities).

I hope that helps.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ You have put quite some effort in this review. Perhaps adding in code snippets here and there would make it easier to visualise your recommendations. \$\endgroup\$ Commented Jun 12, 2019 at 19:26

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.