Logic flaws

Code review rules

Logic problems concern oddities in the control and data flows of your program. This means questionable coding, erratic behavior, hidden flaws and problems that are waiting to emerge later. These problems can cause your program to fail or to produce bad results, or they can represent optimization opportunities. Spend time examining each problem to determine if and how it should be corrected.

Path analysis

To detect logic problems Project Analyzer runs path analysis. It follows the execution flow through branches, loops and jumps to determine which lines will actually execute at run-time. It checks out when variables are read and when they are written to. As to loops, Project Analyzer evaluates whether a loop executes zero, one or more times and if it's an eternal loop.

Unreachable code found. A block of code is dead because it is not reachable. It will never execute. Unreachable code means impossible branches, loops that don’t run, unused error handlers (On Error style) and labeled blocks that are not being jumped into. Code that immediately follows an unconditional jump (such as a Return or Exit statement) can also be unreachable. Unreachable code cannot run at all. It is a programming error: bad logic or the remains of old code that should have been deleted. Determine whether you should remove the unreachable block or if it should rather be brought back to life. Unreachable code should not be exist. If it is necessary to leave it for later use, consider commenting it out or putting it in an #If..#End If block to exclude it from compilation and to make it explicit it may be required later. — This rule is good for detecting On Error style error handlers that are not in use. It can also detect broken loops that don't loop (last statement never reached). It can even detect code that was removed by writing Exit Sub above it. REACH

Variable read before written. A local variable is being read before a write instruction ever executes. At this point the variable will always contain its default value (zero or empty string). This is most probably a mistake. The preceding write instruction may have been deleted or the execution flow wrongly modified. It could also indicate a superfluous variable or the use of an incorrect variable. This rule ignores error handlers, as the run-time triggering of an error handler is difficult to determine by code analysis. This rule finds variable read before *any* write has occurred, whereas the rule “Variable read before written (along some path)” triggers when that is even a remote possibility. Tip: To improve the accuracy of this rule, enable analysis of library files (Enterprise Edition required). READBW

Variable read before written (along some path). A local variable is being read before a write instruction is guaranteed to have executed. Thus, when the variable is read, its may have a proper value, but it could also contain its default value – potentially due to a mistake. Depending on the case this may be problematic or not. This warning appears when some path up to a read instruction executes a write instruction while some other path does not. Typically a Then branch writes while Else does not (or vice versa). Another case is when a loop writes to a variable that is read after the loop; if the loop executes zero times, the variable is undefined. You must determine case by case if this is an actual mistake or if it is rather by design. In may cases, 1) use of the variable’s default value may be OK or 2) all executing paths will write to the variable. In a summary, this is a “check this out” type of a rule rather than a “must fix” warning. Tip: To improve the accuracy of this rule, enable analysis of library files (Enterprise Edition required). READBW

Object read before written. A local object variable is being accessed, but its value is always Nothing at this point. A run-time error is likely (null pointer reference, object variable not set). This warning appears where an object variable cannot be set by a preceding instruction. This is a severe version of rule “Object read before set (along some path)”, since this rule reports cases where the object is believed to be Nothing at all times regardless of previous execution. See also: “Variable read before written”. Tip: To improve the accuracy of this rule, enable analysis of library files (Enterprise Edition required). READBW

Object read before written (along some path). A local object variable is being accessed, but its value may be Nothing. A run-time error may occur at this point (null pointer reference, object variable not set). This warning appears when some path may have left an object variable unset. To avoid a run-time error verify that the variable will actually contain an object at this point. This rule is related to “Variable read before written (along some path)”, see that for details. Tip: To improve the accuracy of this rule, enable analysis of library files (Enterprise Edition required). READBW

Assigned value (or object) not used. A local variable is assigned a value that is not actually used after the assignment. It is either overwritten or not used at all. Review the code to see if it is a programming error or if you can optimize by deleting the write statement. Make sure you are not removing any required function calls at this point, that is calls that have useful side effects. — Note: This rule only considers reachable code. If a read instruction exists in an unreachable statement, such as an impossible branch, it is not taken as a read. Before deleting any code, look for all use locations of the variable to determine whether the value could actually be required under some other circumstances, such as if you later change some condition. — Details: Arrays are excluded from this rule as well as parameters. VALUE_UNUSED

Parameter value not used. The value passed to a procedure parameter is not actually used. The value is overwritten or execution never flows into a read instruction. Review the code to see if it is an error. — This rule is related to the “Dead parameter” rule. A Dead parameter is one that is not used at all. “Parameter value not used” means the parameter name is indeed in use, but not the value passed to it. VALUE_UNUSED

Passed value not used. A parameter is being passed in a procedure call, but the receiving procedure does not actually use the value. Is the logic still all right? It may be possible to optimize by leaving out the parameter or by passing a dummy constant instead of a calculated value. This rule excludes “out” parameters, which are used to return a value. VALUE_UNUSED

Loop runs forever. An eternal loop was found. Once the loop is entered, there is no exit or all exits are unreachable. The program could jam here. LOOP_FOREVER

Loop runs only once. A loop starts but it doesn’t execute another round. Is it a flaw? Should it run several times or can you just remove this useless loop? — Related rule: A loop that runs zero times is reported as Unreachable code. LOOP_ONCE

Cyclic recursion found. Cyclic recursion happens when A calls B, which calls C, which eventually calls A again. Recursion through two or more procedures is difficult to understand and get to work correctly. There is also a risk of endless recursion. Cyclic recursion is also called indirect recursion, as opposed to direct recursion, where a procedure calls itself repeatedly. Direct recursion is not as problematic as cyclic recursion. Consider replacing cyclic recursion with either a non-recursive technique or direct recursion. Restricting recursion to a single procedure is easier to manage. To prevent endless recursion, you can add a safety counter that breaks the execution if it goes too deep. To fully understand a large recursive cycle, get the Recursion diagram in Enterprise Diagrams. Note: This rule triggers based on static analysis. Due to run-time conditions and checks, the actual execution path might not be recursive. RECURSION

Select Case and conditionals

Case branch(es) missing for Enum. A Select Case statement branches off an Enum value, but it does not contain a Case for every Enum constant. This may indicate missing logic, possibly due to an Enum constant added later. Check to see whether this causes problems when an unhandled Enum value is passed. This rule requires every Enum constant name to be listed in a Case, even if the constants have the same value. Use of the underlying numeric values is not acceptable. Ranges such as Case X To Z are not acceptable either. Listing each individual constant helps prevent breaking existing Select Case blocks if the Enum is later changed or rearranged. To accept Case Else as a default branch, check Ignore when Case Else exists. CASE_MISSING

Case Else missing. A Select Case block is missing its Case Else branch. An unexpected input value can go unhandled. This rule lets you prevent bugs from causing havoc beforehand. You can require each Select Case to have a Case Else just in case an unexpected value comes up. As an exception, Case Else is not required by this rule if existing Case branches cover all theoretically possible input values. Tip: To detect unexpected values while debugging and run the final executable without interruption, add the Debug.Assert False statement in each Case Else that may execute due to an unexpected input value. CASE_ELSE

Case useless. A Case condition will not match any input values. The condition is already covered by an earlier condition or conditions (duplicated Case or totally overlapping range). This warning is also triggered for a never-matching Case x To y, where y is less than x. Note that a Case statement can have multiple conditions separated by commas. This rule reviews each condition separately, so there may be both useful and useless conditions in the same Case statement. See also: Case overlap. CASE_USELESS

Case overlap. A Case condition overlaps an earlier condition. Since previous conditions have priority, this condition will not match all the given values. As an example, Case 1 To 5 and Case 3 To 7 overlap in values 3 to 5. To make the Cases more understandable, rewrite this condition so that overlapping does not occur. Note that a Case statement can have multiple conditions separated by commas. This rule reviews each condition separately, so there may be overlapping and non-overlapping conditions in the same Case statement. See also: Case useless. CASE_OVERLAP

Condition always True or False. A conditional statement always evaluates the same way. This is an unconditional statement, really. It may be a flaw in the code. The rule checks the condition of the following keywords: If, ElseIf, Select, While and Until. Tips: To exclude useless code, use #If False rather than If False. This prevents compilation of dead lines into the executable. To loop eternally, use Do..Loop without either Until or While. Special case: Select Case True is allowed by this rule. In this syntax the real conditions appears after each Case condition as in Select Case True: Case A=B, Case C=D and so on. COND

Condition always True or False at run-time. Due to run-time conditions, such as the actual value of a variable or parameter, a conditional statement always evaluates the same way. This is an unconditional statement, really. It may be a flaw in the code. Be careful in removing or changing the condition as the condition may turn out essential when the code is later changed. COND

Conditional block excluded. A conditional compilation block (#If..#ElseIf..#Else..#End) is excluded due to a False condition. This is OK where conditional compilation is used to produce several versions of the program. In some cases it may indicate remnants of code that should have been removed. This rule is there to help you detect all False branches, should you wish to get rid of them. — A special consideration affects VB6 code that is planned to be migrated to VB.NET. The VB.NET upgrade wizard doesn't migrate code inside a False branch. It copies the code 'as is'. If the False branch is large, it is recommended that you temporarily set the condition it to True before migration. Otherwise you will have to migrate it manually. EXCLUDED

Empty blocks, procedures and modules

Empty block. An empty block was found. Empty blocks may indicate unfinished functionality or functionality that has been deleted or commented out. As an example, an empty If block does not execute anything based on the decision. An empty Else block is totally useless and can be removed. Where an empty If block is followed by an Else block, the logic should be reversed and the Else block removed to make the code more readable. An empty Try block is useless. An empty Catch block does not process the exception it caught. When removing empty blocks be careful that the functionality will not change. Before removing an empty If..Then, make sure that the condition does not execute any important calls. Before removing an empty Case, make sure it does not affect any Case that comes after it. Before removing an empty loop, make sure it indeed does not perform anything useful. — You can optionally allow empty blocks containing a comment. In this programming style, a non-empty comment indicates the block is intentionally empty. — Empty Case Else blocks are not considered to be a problem. Empty Case Else is common practice to indicate that there is no processing for other values in a Select block. EMPTY_BLOCK

Empty procedure/module. This rule detects empty modules, classes, interfaces and the like. In addition, it detects procedures having no executable statements in them. Since declarative statements do not count, a procedure with just a Dim or a Const line can also be reported as an empty procedure. Procedures that need to be empty, such as interface methods, are not reported by this rule. A special case is when an empty Overrides procedure deletes the functionality of an ancestor class method. Overriding should not be used to delete class functionality. The class hierarchy should be redesigned to avoid the need for empty Overrides. Project Analyzer supports an option to ignore the Empty procedure/module warning if there is a comment in the procedure or module. A comment makes it explicit why the procedure or module is empty. The comment also allows you to tell unintentionally blank procedures from intentionally blank ones. EMPTY

Comment directive

The comment directive parameter for the rules on this page is LOGIC.

See also

Problem detection
Code review rules

©Aivosto Oy - Project Analyzer Help Contents