When I perform these reviews I usually generate pages of notes and constructive criticisms that I share with the project team. I don't pull punches in these reviews. If I spot some code that is smelly I will highlight it and explain why it is wrong. I will call out shoddy code organization and poorly thought out build scripts and stupid package naming and bad error handling and crappy unit tests and useless comments and other details ad nauseum.
But all of these details are the trees. What makes me like or dislike a project is the overview of the forest. To be sure, one element is the sheer number of details that I have to call out. An overgrown forest stands no chance. Here are some other higher level things that I look for:
- Evidence of a professional developer attitude
- Evidence that the end goals are understood
- Evidence of higher order design/architecture
Evidence of a professional developer attitude
Show me that you have a process. That you care about the quality of the project and the code. That you care what other developers think. That you are trying to set a good example. Some indicators:
- Clean, well organized repository
- Repository is up to date and has regular check-ins (with comments!)
- Clear build scripts that serve as documentation for the build process and dependencies
- Unit tests and TDD thinking (even if you don't do TDD)
- Continuous build (or at least nightly)
- A single central build environment (NOT your IDE, doofus!)
Evidence that the end goals are understood
Show me that you understand the user. That you aren't going to show the user error messages like, "Database Error: Code 5387". That the user interface doesn't freeze or time out. That you have processes in place so that you can quickly debug problems in production. If this is an API, show me that it is clear and understandable and that the contract utterly explicit. Some indicators:
- A system for handling errors/exceptions that is logical, that logs details, that presents the user with a message that is clear and simple.
- Asynchronous UI processes
- Sensible timeout logic where appropriate
- Integration/unit tests that monitor task time and alert when changes to the code may have slowed something down
- For APIs do the error messages contain the extra details that a developer would need?
- Do you eat your own dog food?
- Are you logging in such a way and at an appropriate level of verbosity such that you can troubleshoot unexpected problems?
- Documentation for any APIs. Yes, documentation. If you can't clearly explain in a document how to use your API, then what good are you (or it)?
Show me that you have thought about reusability. That you understand the value of layered abstractions. That you understand what an object is and what a process is. Every time you look for a library to do X for you and don't find it, do you then write a decent library to do X? Or do you just hack some code to do X into the middle of some poor unsuspecting class? Some indicators:
- If you are writing a service that does Y, do you have a well orchestrated class/library that does Y, upon which you have layered a service? (as opposed to building the functionality directly into the service classes/methods)
- Are complex tasks broken down into small logical slices? (as opposed to long confusing methods)
- Are things that could/should be reusable actually reusable? (as opposed to having unnecessary interdependencies with the rest of the project)
- Are your objects named like (and function like) nouns?
- Are your method and object names indicative of actual behavior?
- Is the project an assembly of small pieces that fit neatly into a bigger picture? (as opposed to a monolithic beast)