Code Review Checklist

오늘 전자엔지니어 기사 중 Code Review Checklist에 관한 기사가 떠서 읽어보고 많은 부분에서 공감을 할 수 있었다.

S/W를 개발함에 있어 가장 중요한 점들 중 하나는 바로 제대로 된 S/W를 개발하는 것이다. 간단한(사람마다 다르겠지만 대략1,000 lines이 넘어가는 코드를 두서없이 작성하면 나중에 알아보기 너무 힘들다.) S/W일지라도 개발 과정에 있어check 되어야 할 공통적인 부분들이 있을 것이다. 예를 들면, 의도한 대로 S/W가 동작을 하는지, 개발된 S/W에 대한문서는 작성 하였는지, S/W가 exception을 제대로 처리하는지 등과 같은 것들이다.

가만히 생각해 보면 나도 제대로 된 checklist를 만들어 본 적이 없었다. 부끄러운 일이 아닐 수 없다. S/W Engineer의 길을 걷고자 하는 개발자로서 알면서도 제대로 실천하고 있는 일이 과연 얼마나 될까 싶다.

언제나 그렇지만 바로, 당장 행동으로 옮기기는 어렵다. 하지만 이러한 사항들을 유념하고 천천히 하나씩 해볼 수 있도록 노력해야 겠다.

다음은 macadamian사의 홈페이지에 있는 Code Checklist이다.

General Code Smoke Test

Does the code build correctly?
No errors should occur when building the source code. No warnings should be introduced by changes made to the code.

Does the code execute as expected?
When executed, the code does what it is supposed to.

Do you understand the code you are reviewing?
Asa reviewer, you should understand the code. If you don't, the reviewmay not be complete, or the code may not be well commented.

Has the developer tested the code?
Insure the developer has unit tested the code before sending it for review. All the limit cases should have been tested.

Comments and Coding Conventions

Does the code respect the project coding conventions?
Check that the coding conventions have been followed. Variable naming, indentation, and bracket style should be used.

Does the source file start with an appropriate header and copyright information?
Eachsource file should start with an appropriate header and copyrightinformation. All source files should have a comment block describingthe functionality provided by the file.

Are variable declarations properly commented?
Commentsare required for aspects of variables that the name doesn't describe.Each global variable should indicate its purpose and why it needs to beglobal.

Are units of numeric data clearly stated?
Comment the units of numeric data. For example, if a number represents length, indicate if it is in feet or meters.

Are all functions, methods and classes documented?
Describeeach routine, method, and class in one or two sentences at the top ofits definition. If you can't describe it in a short sentence or two,you may need to reassess its purpose. It might be a sign that thedesign needs to be improved.

Are function parameters used for input or output clearly identified as such?
Make it clear which parameters are used for input and output.

Are complex algorithms and code optimizations adequately commented?
Complexareas, algorithms, and code optimizations should be sufficientlycommented, so other developers can understand the code and walk throughit.

Does code that has been commented out have an explanation?
Thereshould be an explanation for any code that is commented out. "DeadCode" should be removed. If it is a temporary hack, it should beidentified as such.

Are comments used to identify missing functionality or unresolved issues in the code?
Acomment is required for all code not completely implemented. Thecomment should describe what's left to do or is missing. You shouldalso use a distinctive marker that you can search for later (Forexample: "TODO:francis").

Error Handling

Are assertions used everywhere data is expected to have a valid value or range?
Assertions make it easier to identify potential problems. For example, test if pointers or references are valid.

Are errors properly handled each time a function returns?
Anerror should be detected and handled if it affects the execution of therest of a routine. For example, if a resource allocation fails, thisaffects the rest of the routine if it uses that resource. This shouldbe detected and proper action taken. In some cases, the "proper action"may simply be to log the error.

Are resources and memory released in all error paths?
Make sure all resources and memory allocated are released in the error paths.

Are all thrown exceptions handled properly?
Ifthe source code uses a routine that throws an exception, there shouldbe a function in the call stack that catches it and handles it properly.

Is the function caller notified when an error is detected?
Considernotifying your caller when an error is detected. If the error mightaffect your caller, the caller should be notified. For example, the"Open" methods of a file class should return error conditions. Even ifthe class stays in a valid state and other calls to the class will behandled properly, the caller might be interested in doing some errorhandling of its own.

Has error handling code been tested?
Don't forget that error handling code that can be defective. It is important to write test cases that exercise it.

Resource Leaks

Is allocated memory (non-garbage collected) freed?
Allallocated memory needs to be freed when no longer needed. Make surememory is released in all code paths, especially in error code paths.

Are all objects (Database connections, Sockets, Files, etc.) freed even when an error occurs?
File,Sockets, Database connections, etc. (basically all objects where acreation and a deletion method exist) should be freed even when anerror occurs. For example, whenever you use "new" in C++, there shouldbe a delete somewhere that disposes of the object. Resources that areopened must be closed. For example, when opening a file in mostdevelopment environments, you need to call a method to close the filewhen you're done.

Is the same object released more than once?
Make sure there's no code path where the same object is released more than once. Check error code paths.

Does the code accurately keep track of reference counting?
Frequentlya reference counter is used to keep the reference count on objects (Forexample, COM objects). The object uses the reference counter todetermine when to destroy itself. In most cases, the developer usesmethods to increment or decrement the reference count. Make sure thereference count reflects the number of times an object is referred.

Thread Safeness

Are all global variables thread-safe?
Ifglobal variables can be accessed by more than one thread, code alteringthe global variable should be enclosed using a synchronizationmechanism such as a mutex. Code accessing the variable should beenclosed with the same mechanism.

Are objects accessed by multiple threads thread-safe?
If some objects can be accessed by more than one thread, make sure member variables are protected by synchronization mechanisms.

Are locks released in the same order they are obtained?
It is important to release the locks in the same order they were acquired to avoid deadlock situations. Check error code paths.

Is there any possible deadlock or lock contention?
Makesure there's no possibility for acquiring a set of locks (mutex,semaphores, etc.) in different orders. For example, if Thread Aacquires Lock #1 and then Lock #2, then Thread B shouldn't acquire Lock#2 and then Lock #1.

Control Structures

Are loop ending conditions accurate?
Checkall loops to make sure they iterate the right number of times. Checkthe condition that ends the loop; insure it will end out doing theexpected number of iterations.

Is the code free of unintended infinite loops?
Check for code paths that can cause infinite loops. Make sure end loop conditions will be met unless otherwise documented.

Performance

Do recursive functions run within a reasonable amount of stack space?
Recursive functions should run with a reasonable amount of stack space. Generally, it is better to code iterative functions.

Are whole objects duplicated when only references are needed?
Thishappens when objects are passed by value when only references arerequired. This also applies to algorithms that copy a lot of memory.Consider using algorithm that minimizes the number of objectduplications, reducing the data that needs to be transferred in memory.

Does the code have an impact on size, speed, or memory use?
Canit be optimized? For instance, if you use data structures with a largenumber of occurrences, you might want to reduce the size of thestructure.

Are you using blocking system calls when performance is involved?
Consider using a different thread for code making a function call that blocks.

Is the code doing busy waits instead of using synchronization mechanisms or timer events?
Doing busy waits takes up CPU time. It is a better practice to use synchronization mechanisms.

Was this optimization really needed?
Optimizationsoften make code harder to read and more likely to contain bugs. Suchoptimizations should be avoided unless a need has been identified. Hasthe code been profiled?

Functions

Are function parameters explicitly verified in the code?
Thischeck is encouraged for functions where you don't control the wholerange of values that are sent to the function. This isn't the case forhelper functions, for instance. Each function should check itsparameter for minimum and maximum possible values. Each pointer orreference should be checked to see if it is null. An error or anexception should occur if a parameter is invalid.

Are arrays explicitly checked for out-of-bound indexes?
Make sure an error message is displayed if an index is out-of-bound.

Are functions returning references to objects declared on the stack?
Don't return references to objects declared on the stack, return references to objects created on the heap.

Are variables initialized before they are used?
Makesure there are no code paths where variables are used prior to beinginitialized. If an object is used by more than one thread, make surethe object is not in use by another thread when you destroy it. If anobject is created by doing a function call, make sure the object wascreated before using it.

Does the code re-write functionality that could be achieved by using an existing API?
Don'treinvent the wheel. New code should use existing functionality as muchas possible. Don't rewrite source code that already exists in theproject. Code that is replicated in more than one function should beput in a helper function for easier maintenance.

Bug Fixes

Does a fix made to a function change the behavior of caller functions?
Sometimescode expects a function to behave incorrectly. Fixing the function can,in some cases, break the caller. If this happens, either fix the codethat depends on the function, or add a comment explaining why the codecan't be changed.

Does the bug fix correct all the occurrences of the bug?
If the code you're reviewing is fixing a bug, make sure it fixes all the occurrences of the bug.

Math

Is the code doing signed/unsigned conversions?
Checkall signed to unsigned conversions: Can sign completion cause problems?Check all unsigned to signed conversions: Can overflow occur? Test withMinimum and Maximum possible values.

지금부터라도 여기서 가장 기본적인 General Code Smoke Test부터 시작해 보는 것은 어떨까 싶다.