Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

For a project I need to give a load of diverseinformation in JSON format. All the information will be used on the same page so a single call would result in the least overhead. The information all concerns the same database object and is all nescessary on the page. It basically is a collection of counts of the amount of objects that are of one or more of a certain type (the types are all booleans) and we require to know a lot of different variations of this. I used the code below but my co-worker believes the way I put it in the JSON list is a bit clunky and the code could be far more performant. How could I improve this code?

public function getContactsStatisticsAction()
{
    $response = new Response();
    $json = array();
    $em = $this->getDoctrine()->getEntityManager();
    $cr = $em->getRepository('BlaCoreBundle:Company');

    $json['numberOfCompanies'] = $cr->numberOfCompanies();
    $json['numberOfAccounts'] = $cr->numberOfCompanies(array("typeAccount" => true));
    $json['numberOfCompetitors'] = $cr->numberOfCompanies(array("typeCompetitor" => true));
    $json['numberOfSuppliers'] = $cr->numberOfCompanies(array("typeSupplier" => true));
    $json['numberOfOthers'] = $cr->numberOfCompanies(array("typeOther" => true));
    $json['numberOfUnassigned'] = $cr->numberOfCompanies(array("typeAccount" => false, "typeCompetitor" => false,"typeSupplier" => false,"typeOther" => false));

    $json['numberOfJustAccounts'] = $cr->numberOfCompanies(array("typeAccount" => true, "typeCompetitor" => false, "typeSupplier" => false));
    $json['numberOfJustCompetitors'] = $cr->numberOfCompanies(array("typeAccount" => false, "typeCompetitor" => false, "typeSupplier" => false));
    $json['numberOfJustSuppliers'] = $cr->numberOfCompanies(array("typeAccount" => false, "typeCompetitor" => false, "typeSupplier" => false));

    $json['numberOfCompetitorAndAccounts'] = $cr->numberOfCompanies(array("typeAccount" => true, "typeCompetitor" => true, "typeSupplier" => false));
    $json['numberOfCompetitorAndSuppliers'] = $cr->numberOfCompanies(array("typeAccount" => false, "typeCompetitor" => true, "typeSupplier" => true));
    $json['numberOfSupplierAndAccounts'] = $cr->numberOfCompanies(array("typeAccount" => true, "typeCompetitor" => false, "typeSupplier" => true));
    $json['numberOfCompaniesAndAccountsAndSuppliers'] = $cr->numberOfCompanies(array("typeAccount" => true, "typeCompetitor" => true, "typeSupplier" => true));

    $response->setContent(json_encode($json));
    return $response;
}


public function numberOfCompanies($filters = array())
{
    $qb = $this->getEntityManager()->createQueryBuilder();
    $qb->select('count(c.id)');
    $qb->from('BlaCoreBundle:Company', 'c');
    $sizeFilters = count ($filters);
    $keys = array_keys($filters);
    if($sizeFilters >= 1){
        $qb->where('c.' . $keys[0] . ' = ' . (int) $filters[$keys[0]]);
    }
    for($i = 1; $i < $sizeFilters; $i++){
        $qb->andWhere('c.' . $keys[$i] . ' = ' . (int) $filters[$keys[$i]]);
    }
    return $qb->getQuery()->getSingleScalarResult();
}
share|improve this question
1  
what specific parts are you (or your co-worker) concerned about? The response data structure? The number of queries being issued? other? –  Rob Apodaca Dec 29 '12 at 1:33
1  
Those two, really. The fact that the response data structure is quite large and the amount of queries that has to be run. –  Kristof Dec 29 '12 at 22:51
add comment

1 Answer 1

up vote 1 down vote accepted

I think that what you have is probably fine. Personally, I'd move the construction of the result array to a view but this is not that big of a deal. That might help with the "clunkiness".

With regards to your concern about the size of the json response: You say that all of the information in it is needed by the app. It doesn't look all that big to me. Unless you are actually experiencing problems as a result of the size, I'd leave it alone. If it were too big, you could consider implementing appropriate caching (allow the browser to cache the results and/or use etags) or pagination.

Regarding the number of individual sql calls: I'm sure you could put it all into one query if need be.

Both of these potential issues, response size and number of queries, seem to me to be pre-optimization concerns. In the past, I've spent lots of time pre-optimizing stuff and never realized any real gain. Now, I implement what is needed and only optimize when the real need arises. The bottom line: I don't see any problems with what you've got and unless the result is unacceptably slow or resource intensive, don't worry about it.

Hope it helps.

share|improve this answer
1  
Great, thanks for the answer! It definitely helps. Any idea how a single query would work for something like this though? Since it are different counts... –  Kristof Dec 30 '12 at 15:26
1  
You might be able to use a series of sum() functions. Or simply use sub-selects –  Rob Apodaca Dec 30 '12 at 15:41
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.