Code Review 4) Algorithmic soundness

Welcome to BSP Software’s ‘Code Review’ series

Part 1) Author and Reviewer Preparation
Part 2) Readibility
Part 3) Structure

Peter Gabris, MD of BSP Software Inc

In this section we focus on
Part 4) Algorithmic soundness

1. The execution should be final: a loop exit should be guaranteed, recursion should backtrack. An exception is a message trap in service-like application. That should be guaranteed to yield the control frequently and predictably.
2. The function will always set the return value. The return value should be correct (as defined) for all accepted arguments. What will happen with corner-case input? Unacceptable arguments should be checked early and refused.
3. The variables used should be able to hold any acceptable value without a loss of accuracy. For example monetary values should not be stored as floating point.
4. Exceptions should never be used for expected. For example using an exception to leave a search loop when the item is found is sinful.

Maintainability

1. All errors should be handled. Resources should be released even if an exception or an error condition occurred.
2. Constants should be extracted, consolidated, properly named and placed at a predictable place (usually the top of the program unit, a configuration file or sometimes a specialized singleton).
3. Are all possible null pointers checked before use?
4. Could array indexes get out of bounds? Could a buffer overflow?
5. Could SQL triggers be avoided?
6. Are we using undocumented or deprecated features? Is that use properly commented? Are all workarounds commented?
7. What 3^rd party products we depend on? (How comes this was not handled at the architecture phase?)
8. Do we need so many program units? (How comes this was not handled at the design phase?)

Efficiency

1. Can recursion be avoided?
2. Could SQL cursors be avoided?
3. Is the memory (including heap, stack and disk) used sensibly? Objects should not be duplicated if not necessary. What about non-parallel threads? Are pooled resources retuned as soon as possible?
4. Does the code alternate for an existing system/framework/library function without a proven need or advantage?

Security

1. Check all values coming from the user.
2. Never execute user input. For example, do not use user input to build a sql statement on-the-fly.
3. Encapsulation should be used wherever possible.
4. Where and how are authentication data stored?

Feel free to leave any questions or comments below

Code Review continues in Part 5) Bugs found, now what?