salesforce code review best practices for controllers

As you are asking about classes used as controllers, in addition to other code review items the idea of "thin controllers" should be considered. This isn't an idea that is unique to the Salesforce world, and material like Thin controllers fat models provide an overview of the concept. It only matters when a controller starts to get large and complicated. A large or poor unit test for the controller is an indicator that there is a problem.

This is a special case of the idea that no class should be "fat" where "fat" means that a lot of unrelated functionality or functionality at different levels of abstraction is kept in one place. In a "thin" controller, any complex business logic will be delegated to other classes and those other classes will have their own separate unit tests. That keeps the controller focussed on providing data for the page and actions for the page.

Another matter unique to controller classes is the view state that gets transferred to the browser and back again and so impacts page performance. This helps simplify the code when editing is done. But because controller fields are included in the view state by default, it is common for data that isn't needed to be included. Not important for say a boolean flag field, but fields holding things like large lists of large SObjects should be reviewed.

A controller pattern that I have under-used is the ability to compose multiple classes via the Controller Extension mechanism. This works for custom controllers as well as standard controllers. So where multiple controllers are being reviewed that contain repeated code or are using inheritance to achieve code re-use, using controller extensions instead could be considered.

One must follow the coding guidelines and best practices as mentioned in this great resource. https://developer.salesforce.com/page/Apex_Code_Best_Practices

To facilitate Code Review options, You can try the Apex PMD plugin for eclipse. Available at http://codescan.villagechief.com/eclipse/ Github, Jenkins facilitate the code review at peer level but does not provide any analysis on its own like PMD, FindBugs

In addition to the other excellent answers you've received, it's my opinion that Salesforce's primary concern when it comes to Controllers is Security. For that reason, I'd recommend the following Technical Article by Dan Appleman: Without Sharing: Best practices when bypassing Apex sharing rules and object security.

I'd add that the following blog post Apex Best Practices: The 15 Apex Commandments would also seem to apply to your question.

Beyond that, there are many different Best Practices covered in the Visualforce Developer’s Guide. Visualforce Controllers are no longer involved with a growing list of technologies they need to support that encompass many things including JS Remoting, web services call-outs, JS, jQuery (and JS variants of all kinds), HTML5, CSS2, CSS3, xml, JSON, Salesforce1 Mobile, Lightning (aura enabled controllers), SOCL, SOSL, SAQL and more. It's an increasingly complicated environment that's not going to be getting simpler.