Testing - Codereview Checklist

	Gieno  Testing : Codereview Checklist
	Overall/General                 Does the code live up to requirements?                 Is the code well-designed, well thought-out?                 Is the code legible and understandable?                 Is the code maintainable?                 Is the code completed? Are incomplete sections accompanied by bugs in the product studio database?                 Is the code versioned? How would you know that you are looking at the most current version of the code?   Conformance to Standards                 Do all modules have proper module headers that describe the purpose of the module and provide an overview of any important information about the module?  Is the information in the header correct and up-to-date?  Is there a section in the module header for standard revision history information?                 Do all procedures have proper procedure headers that describe the purpose of the procedure and provide an overview of any important information about the procedure?  Is the information in the header correct and up-to-date?  Does the header discuss potential for ErrJumps?  Does it discuss expected entry or enforced exit states of sbCur?                 Is proper Hungarian used in all cases?  Is the naming appropriate?  Are the naming conventions followed? Are the class/object/method/constant/interface names clear and sensible?                 Are proper indentation and other coding conventions used, with tab stops set every 4 spaces (or as your team's coding standard requires)?                 Are return statements coded as return(TRUE) rather than return TRUE?   Code design issues                 Has correct use of shared libraries been made? Could code have been reused from elsewhere, or should the code be repackaged for use elsewhere?                 Will the code work correctly on 64 bit?                 Does your code have multiple exit points. If so, can they be combined.                 Will the code work correctly on Win9x? (DO WE CARE?)   Code-Specific Details                 Are all input values and return values well-defined?                 Are all return states well-defined and correct?  In particular, if sbCur is used/modified, is it reset on return as expected by all callers and/or as documented in the procedure header?                 Is all error handling correct?  In particular, do all routines which allocate resources contain a DoJmp-invoked error-handler for releasing those resources, and are all such resources handled correctly?  Are errors being checked?                 Are all boundary cases handled correctly?                 Have boundary conditions been checked?                 Is the code exception safe?                 Is the code free of leaks?                 Switch statements: Do they deal correctly with unexpected values (default should assert(FALSE) if it is not otherwise valid)?  Are there any missing cases?  Are all cases handled?  Are any break statements missing where expected?                 Loops: Do they always terminate?                 Are potential side effects from called routines guarded against or dealt with properly?                 Are all SB dependencies correctly maintained?                 Are pointers which are derived from handles used within their limited lifetimes?                 Are all pointers de-referenced properly for the type of data that they point to?                  Are all variables initialized before they are used?                 Are there any variables (local or global) that are not used?                 Are underlying data structures used properly?                 Are there any unused data structure elements?                 Are there any "magic numbers" that should be made into manifest constants?                 Where global general-purpose buffers are utilized, are test-code possession semaphores used correctly?                 Be wary of all blt-type operations.  Can they overrun the destination buffer?  Can they read past the end of the source buffer?  Are assertions used to make sure this does not happen?                 Are there any useful assertions (or other debugging code) missing?                  Is the code well validated?                 Could this code ever crash in retail?                 Are comments correct and concise?  Are there enough comments?  Could I understand this code without any help from the author?                  Did you check all return codes for warning or errors, including memory allocations (ASSERT’s aren’t good enough)? Be sure and take into account out of memory, missing registry keys, network problems, etc.                 Have you anticipated any possible Exceptions that could get thrown?                 Are all uses of memory, either stack or allocated, protected from running off the end of the memory?                 Are all of your buffers and allocated memory big enough to handle localized content?                 Are you freeing up all the memory you allocated and releasing all objects where necessary?                 Can any of the data be accessed by two threads at the same time? If so, is this data protected?                 Have you thought through any possibilities for a deadlock?                 Are all boundary cases dealt with correctly (either handled or returning an error)?                 Do you know all the callers of functions you changed and what the impact on them is? (If not, consider not making this change!!!)   Performance Issues                 Is the code efficient? Have the right algorithms and data structures been chosen?                 Are any header files included that are unnecessary?  Be particularly aware of any that include unused csconst data.                 Switch statements: Is the default case the first item?  Is the mostly-used multi-case directly below it?  In cases where the default just asserts, is the body of the default and the most-used multi-case placed within #ifdef TEST?                 Loops: Are unnecessary calculations performed as part of the termination clause?  Is there code that can be pulled out of the body of the loop?                 Are multiple flag variables implemented efficiently?  Consider space versus time requirements: frequently-used flags should be byte-sized variables, infrequently-used flags can be packed into bitfields (byte or word-sized as appropriate).  Similarly, these considerations should be applied to any variables stored in bitfields.                 Does cleanup code take advantage of the fact that many cleanup routines will not ErrJump?                 Is there code here which duplicates code found elsewhere in the project?                 Are there any local variables which are used only once, where a constant value might be better?                 Are there any unnecessary globals?                 Check for divides by powers of two (should right shift instead, compiler does not optimize div 2).                 Are there any other performance problems?                 Is code using any pointer arithmetic that would not work on 64-bit?   Testability Issues                 Is the code structured so that a small number of test cases will give good code coverage?                 Does the code create or present obscure situations that (black-box) testing is not likely to detect?                 Does code contain hooks for QA to access for testing?                 Is testability designed into the code?                 Have test cases been covered?  I.e., have you reviewed the cases that QA has identified as being important to test.   Security Checklist                            Code compiled with -GS                 Debug builds compiled with -RTC1                 Check all untrusted input is verified prior to being used or stored                 All buffer management functions are safe from buffer overruns                 Strsafe.h is used for string functions                 All DACLs are well formed and "good" - not NULL or Everyone (Full Control)                 Temporary file names are unpredictable                 Check that file requests are not for devices (i.e. COM1, PRN, etc.)                 No user data written to HKLM in the registry                 No resources opened for GENRIC_ALL when lesser permissions wil suffice (least privileges are used)                 Exported APIs with byte count vs. word count are documented (we should use one consistent approach - either char count or byte count)                 Impersonation function return values are checked                 For every impersonation there is a revert                 Service code does not create windows and is not marked interactive   RPC                 IDL files compiled with /robust                 [range] used if appropriate                 Use of packet privacy and integrity investigated                 Correct IDL attributes are used for string/data lengths and sizes   COM/ATL                 Are interface pointer out parameters correctly ref-counted                 Are pointer out-parameters allocated with CoTaskMemAlloc                 Are the references released correctly in failure cases?                 Is an interface pointer released before it is overwritten (esp. in a loop)                 Do objects store any interface pointers which can get used within different apartments - and in that case are they correctly marshalled                 COM Singleton is NOT used in case of in-proc COM objects                 If any in-parametes are stored away, are they ref-counted                 CComObject's critical section is re-used using Lock/Unlock functions whenever possible                 Have you checked the code for any reference leaks (possibly with bounds checker)   Checklist before review                 Have you run prefast and made sure there are no warnings                 Have you run the code with AppVerifier                 Have you used checked for handle and memory leaks with some leak detection mechanism                 Have you used bounds-checker for interface leaks                 Have you run buffy and analyzed all the dangerous functions used? (in case they are safe, please put comments to ignore buffy and explain the safety)