Method parameter assigned a new value
ID |
objectivec.maintainability.avoid_reassigning_parameters |
Severity |
info |
Remediation Complexity |
trivial |
Remediation Risk |
low |
Remediation Effort |
low |
Resource |
Suspicious Code |
Language |
Objective-C |
Description
Reports a method whose body assigns a new value to one of its own parameters. The body is bounded at the closing brace in the first column, so an assignment in a later method is not attributed to this one, and the method header must itself start in the first column. Compound assignments and increments are not reported, and neither is a write through the parameter - target[key] = value or self.name = name - which changes the object the parameter points at rather than the parameter. An assignment that has been commented out does not report the method, and neither does one written on the same line as the opening brace.
Rationale
A parameter is the record of what the caller passed. Once it has been overwritten, the original value is gone, so a reader partway down the method cannot tell whether name still holds the argument or a substitute installed twenty lines earlier, and a debugger stopped on a later line shows the substitute. It also hides the substitution from the caller’s side of the contract: the method looks like it uses what it was given. In Objective-C the mistake is easy to make in the other direction too, since assigning to a parameter inside a block is a compile error unless the parameter is __block.
The following code illustrates the pattern detected by this rule:
@implementation Formatter
// FLAGGED: Method parameter assigned a new value
- (NSString *)normalize:(NSString *)name {
if (name.length == 0) {
name = @"unnamed";
}
return name;
}
- (NSString *)rebuild {
NSString *name = @"start";
name = @"finished";
return name;
}
@endRemediation
Introduce a local for the value the method actually works with - NSString *trimmed = name ?: @"unnamed"; - and leave the parameter alone, so the argument stays readable for the rest of the method. Where the substitution is the point, a guard clause at the top that computes the local once usually reads better than an assignment buried in a branch.