Commit fe0398d
committed
Fix logical operators task.
This was asking for a goal that conflicted with moreWaysToReturn.go -- logicalOperators.go considered paths that may circumvent a return statement as acceptable, while moreWaysToReturn.go wanted these maybe cases flagged as problems. Here we take moreWaysToReturn.go as correct and amend the good/bad labels in logicalOperators.go accordingly, also adding a few more test cases.1 parent 1655c03 commit fe0398d
1 file changed
Lines changed: 76 additions & 18 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -50,76 +50,134 @@ func someOtherCondition() bool { | |||
| 50 | 50 | return true | |
| 51 | 51 | } | |
| 52 | 52 | ||
| 53 | - func logicalAndThenBranchGood() { | ||
| 53 | + func logicalAndThenBranchSometimesBad() { | ||
| 54 | 54 | ||
| 55 | 55 | if errorSource() != ErrNone && someOtherCondition() { | |
| 56 | - // Good: in the then-block, where we're certain there has been an error, | ||
| 57 | - // we return early. In the other branch an error is possible but not certain. | ||
| 56 | + // Bad: there is a route from a positive error test around the 'return' statement. | ||
| 58 | 57 | return | |
| 59 | 58 | } | |
| 60 | 59 | doSomething() | |
| 61 | 60 | ||
| 62 | 61 | } | |
| 63 | 62 | ||
| 64 | - func logicalAndThenBranchBad() { | ||
| 63 | + func logicalAndThenBranchAlwaysBad() { | ||
| 65 | 64 | ||
| 66 | 65 | if errorSource() != ErrNone && someOtherCondition() { | |
| 67 | - // Bad: in the then-block, where we're certain there has been an error, | ||
| 68 | - // we do not return early. | ||
| 66 | + // Bad: there is no return statement at all. | ||
| 69 | 67 | insteadOfReturn() | |
| 70 | 68 | } | |
| 71 | 69 | doSomething() | |
| 72 | 70 | ||
| 73 | 71 | } | |
| 74 | 72 | ||
| 75 | - func logicalAndElseBranchUncertain() { | ||
| 73 | + func logicalAndElseBranchAlwaysBad2() { | ||
| 76 | 74 | ||
| 77 | 75 | if errorSource() == ErrNone && someOtherCondition() { | |
| 78 | 76 | doSomething() | |
| 79 | 77 | } else { | |
| 80 | - // Uncertain: an error is not a precondition for entering either branch. | ||
| 81 | - // We should be conservative and NOT flag this function as a problem. | ||
| 78 | + // Bad: On error we may enter either branch, but neither one returns. | ||
| 82 | 79 | insteadOfReturn() | |
| 83 | 80 | } | |
| 84 | 81 | doSomething() | |
| 85 | 82 | ||
| 86 | 83 | } | |
| 87 | 84 | ||
| 88 | - func logicalOrElseBranchGood() { | ||
| 85 | + func logicalAndThenBranchGood() { | ||
| 86 | + | ||
| 87 | + if someOtherCondition() && errorSource() != ErrNone { | ||
| 88 | + // Good: whenever an error is indicated we return. | ||
| 89 | + return | ||
| 90 | + } | ||
| 91 | + doSomething() | ||
| 92 | + | ||
| 93 | + } | ||
| 94 | + | ||
| 95 | + func logicalAndElseBranchGood() { | ||
| 96 | + | ||
| 97 | + if someOtherCondition() && errorSource() == ErrNone { | ||
| 98 | + // Good: whenever an error is indicated we return. | ||
| 99 | + doSomething() | ||
| 100 | + } else { | ||
| 101 | + return | ||
| 102 | + } | ||
| 103 | + | ||
| 104 | + } | ||
| 105 | + | ||
| 106 | + func logicalAndElseBranchGood2() { | ||
| 107 | + | ||
| 108 | + if errorSource() == ErrNone && someOtherCondition() { | ||
| 109 | + // Good: whenever an error is indicated we return. | ||
| 110 | + doSomething() | ||
| 111 | + } else { | ||
| 112 | + return | ||
| 113 | + } | ||
| 114 | + doSomething() | ||
| 115 | + | ||
| 116 | + } | ||
| 117 | + | ||
| 118 | + func logicalOrElseBranchSometimesBad() { | ||
| 89 | 119 | ||
| 90 | 120 | if errorSource() == ErrNone || someOtherCondition() { | |
| 91 | 121 | doSomething() | |
| 92 | 122 | } else { | |
| 93 | - // Good: in the else-block, where we're certain there has been an error, | ||
| 94 | - // we return early. In the other branch an error is possible but not certain. | ||
| 123 | + // Bad: there is a route from a failing error test that bypasses the return statement. | ||
| 95 | 124 | return | |
| 96 | 125 | } | |
| 97 | 126 | doSomething() | |
| 98 | 127 | ||
| 99 | 128 | } | |
| 100 | 129 | ||
| 101 | - func logicalOrElseBranchBad() { | ||
| 130 | + func logicalOrElseBranchAlwaysBad() { | ||
| 102 | 131 | ||
| 103 | 132 | if errorSource() == ErrNone || someOtherCondition() { | |
| 104 | 133 | doSomething() | |
| 105 | 134 | } else { | |
| 106 | - // Bad: in the else-block, where we're certain there has been an error, | ||
| 107 | - // we do not return early. | ||
| 135 | + // Bad: regardless of error status, we do not return. | ||
| 108 | 136 | insteadOfReturn() | |
| 109 | 137 | } | |
| 110 | 138 | doSomething() | |
| 111 | 139 | ||
| 112 | 140 | } | |
| 113 | 141 | ||
| 114 | - func logicalOrThenBranchUncertain() { | ||
| 142 | + func logicalOrThenBranchAlwaysBad() { | ||
| 115 | 143 | ||
| 116 | 144 | if errorSource() != ErrNone || someOtherCondition() { | |
| 117 | - // Uncertain: an error is not a precondition for entering either branch. | ||
| 118 | - // We should be conservative and NOT flag this function as a problem. | ||
| 145 | + // Bad: regardless of error status, we do not return. | ||
| 119 | 146 | insteadOfReturn() | |
| 120 | 147 | } else { | |
| 121 | 148 | doSomething() | |
| 122 | 149 | } | |
| 123 | 150 | doSomething() | |
| 124 | 151 | ||
| 125 | 152 | } | |
| 153 | + | ||
| 154 | + func logicalOrThenBranchGood() { | ||
| 155 | + | ||
| 156 | + if someOtherCondition() || errorSource() != ErrNone { | ||
| 157 | + // Good: whenever an error is indicated we return. | ||
| 158 | + return | ||
| 159 | + } | ||
| 160 | + doSomething() | ||
| 161 | + | ||
| 162 | + } | ||
| 163 | + | ||
| 164 | + func logicalOrElseBranchGood() { | ||
| 165 | + | ||
| 166 | + if someOtherCondition() || errorSource() == ErrNone { | ||
| 167 | + // Good: whenever an error is indicated we return. | ||
| 168 | + doSomething() | ||
| 169 | + } else { | ||
| 170 | + return | ||
| 171 | + } | ||
| 172 | + | ||
| 173 | + } | ||
| 174 | + | ||
| 175 | + func logicalOrThenBranchGood2() { | ||
| 176 | + | ||
| 177 | + if errorSource() != ErrNone || someOtherCondition() { | ||
| 178 | + // Good: whenever an error is indicated we return. | ||
| 179 | + return | ||
| 180 | + } | ||
| 181 | + doSomething() | ||
| 182 | + | ||
| 183 | + } | ||
0 commit comments