Refactoring Browser - Smalllint Rules

This file describes the different rules in the Smalllint tool which is part of the Refactoring Browser. Smalllint has over 60 rules that are broken into Lint checks and Transformations. The Transformations section has several predefined transformations that you may want to use on your code. Once a transformation is run, a window will open allowing you to select which transformed methods you wish to accept. The Lint checks section is further divided into five parts: Bugs, Possible Bugs, Unnecessary code, Intention revealing, and Miscellaneous. If you have a rule that you would like to add, please mail us.

Bugs:

Messages sent but not implemented

Checks for messages that are sent by a method, but no class in the system implements such a message. These will certainly cause a doesNotUnderstand: message when they are executed.

Overrides a "special" message

Checks that a class does not override a message that is essential to the base system. For example, if you override the class method from object, you are likely to crash your image. You can modify which methods are checked by editing the methods BasicLintRule>>classShouldNotOverride and BasicLintRule>>metaclassShouldNotOverride.

References an undeclared variable

Checks for references to a variable in the Undeclared dictionary. If you remove a variable from a class that is accessed by a method, you will create an undeclared variable reference for those methods that accessed the variable.

Self/Super sends not implemented

This is similar to the "Message sent but not implemented" check, but only checks messages sent to self or super since these can be statically typed.

Subclass responsibility not defined

Checks that all subclassResponsibility methods are defined in all leaf classes.

Uses A | B = C instead of A | (B = C)

Checks precedence ordering of & and | with equality operators. Since | and & have the same precedence as =, there are common mistakes where parenthesis are missing around the equality operators.

Uses True/False instead of true/false

Checks for uses of the classes True and False instead of the objects true and false.

Variable used, but not defined anywhere

This check is similar to the "References an undeclared variable" check, but it looks for variables that are not defined in the class or in the undeclared dictionary. You probably had to work hard to get your code in this state.

Possible bugs:

Assignment inside unwind blocks should be outside

Checks assignment to a variable that is the first statement inside the value block that is also used in the unwind block. For example, the following code places the assignment of inputStream inside the valueNowOrOnUnwindDo: block when it should be outside:

	methodName
		| inputStream |
		[inputStream := 'test' asFilename readStream.
		self parse: inputStream]
			valueNowOrOnUnwindDo: [inputStream close]

Although the file handling routines should be wrapped with a valueNowOrOnUnwindDo:, wrapping the assignment of inputStream does not do anything. If the code cannot open the file there is no need to try to close nil (which is what inputStream is since it hasn't been assigned). This code should be rewritten as:

	methodName
		| inputStream |
		inputStream := 'test' asFilename readStream.
		[self parse: inputStream]
			valueNowOrOnUnwindDo: [inputStream close]

Not only does this fix a possible bug that occurs when closing nil, it also eliminates the full block that was necessary with the assignment in the previous version.

Defines = but not hash

Checks that all classes that define = also define hash. If hash is not defined then the instances of the class might not be able to be used in sets since equal element must have the same hash.

Has class instance variable but no initialize method

Checks that all classes that have class instance variables also have an initialize method. This makes sure that all class instance variables are initialized properly when the class is filed-into a new image.

Instance variable overridden by temporary variable

Finds methods that have block are temporary variables that override an instance variable. This causes problems if you want to use the instance variable inside the method.

Missing super sends

Checks that some methods contain a super message send. Some methods should always contain a super message send. For example, the postCopy method should always contain a "super postCopy". The list of methods that should contain super message sends is in BasicLintRule>>superMessages.

Modifies collection while iterating over it

Checks for remove:'s of elements inside of collection iteration methods such as do:. These can cause the do: method to break since it will walk of the end of the collection. The common fix for this problem is to copy the collection before iterating over it.

Number addDependent: messages > removeDependent:

Check that the number of addDependent: message sends in a class is less than or equal to the number of removeDependent: messages. If there are more addDependent: messages that may signify that some dependents are not being released, which may lead to memory leaks.

Possible missing "; yourself"

Checks for missing "; yourself" cascaded message send for cascaded messages that are used. This helps locate common coding mistakes such as "anArray := (Array new: 2) at: 1 put: 1; at: 2 put: 2". In this example, anArray would be assigned to 2 not the array object.

Possible three element point (e.g., x @ y + q @ r)

Checks arithmetic statements for possible three element points (i.e., a point that has another point in its x or y part).

References an abstract class

Checks for references to classes that have subclassResponsibility methods. Such references might be creating instances of the abstract class or more commonly being used as the argument to an isKindOf: message which is considered bad style.

Returns a boolean and non boolean

Checks for methods that return a boolean value (true or false) and return some other value such as (nil or self). If the method is suppose to return a boolean, then this signifies that there is one path through the method that might return a non-boolean. If the method doesn't need to return a boolean, you should probably rewrite it to return some non-boolean value since other programmers reading your method might assume that it returns a boolean.

Returns value of ifTrue:/ifFalse: without ifFalse:/ifTrue: block

Check for methods that return the value of an ifTrue: or ifFalse: message. These statements return nil when the block is not executed. For example, the following code will return nil when aBoolean is false.

	methodName
		^aBoolean ifTrue: [0]

If the code should return nil when aBoolean is false, then it should probably be written as:

	methodName
		^aBoolean
			ifTrue: [0]
			ifFalse: [nil]

Sends different super message

Checks for methods whose source sends a different super message. A common example of this is in creation methods. You might define a method such as:

	createInstance
		^super new initialize

If the new method is not defined in the class, you should probably rewrite this to use self instead. Also, if the new method is defined, you might question why you need to use the superclass' new method instead of new method defined in the class.

Subclass of class that has instance variable but doesn't define copyEmpty

Checks that all subclasses of the Collection classes that add an instance variable also redefine the copyEmpty method. This method is used when the collection grows. It copies over the necessary instance variables to the new larger collection.

Temporaries read before written

Checks that all temporaries are assigned before they are used. This can help find possible paths through the code where a variable might be unassigned when it is used.

Uses the result of an add: message

Check for possible uses of the result returned by an add: or addAll: messages. These messages return their arguments not the receiver. As a result, may uses of the results are wrong.

Unnecessary code

Block immediately evaluated

Check for blocks that are immediately evaluated. Since the block is immediately evaluated, there is no need for the statements to be in a block.

Check for same statements at end of ifTrue:ifFalse: blocks

Checks for ifTrue:ifFalse: blocks that have the same code at the beginning or end. While you might not originally write such code, as it is modified, it is easier to create such code. Instead of having the same code in two places, you should move it outside the blocks.

Class not referenced

Check if a class is referenced either directly or indirectly by a symbol. If a class is not referenced, it can be removed.

Instance variables not read AND written

Checks that all instance variables are both read and written. If an instance variable is only read, you can replace all of the reads with nil, since it couldn't have been assigned a value. If the variable is only written, then we don't need to store the result since we never use it. This check does not work for the data model classes since they use the instVarAt:put: messages to set instance variables.

Method just sends super message

Check for methods that just forward the message to its superclass. These methods can be removed.

Methods equivalently defined in superclass

Check for methods that are equivalent to their superclass methods. Such methods don't add anything to the computation and can be removed since the superclass's method will work just fine.

Methods implemented but not sent

Check for methods that are never sent. If a method is not sent, it can be removed.

Unnecessary = true

Check for a =, ==, ~=, or ~~ message being sent to true/false or with true/false as the argument. Many times these can be eliminated since their receivers are already booleans. For example, "anObject isFoo == false" could be replaced with "anObject isFoo not" if isFoo always returns a boolean. Sometimes variables might refer to true, false, and something else, but this is considered bad style since the variable has multiple types.

Variable referenced in only one method and always assigned first

Checks for instance variables that might better be defined as temporary variables. If an instance variable is only used in one method and it is always assigned before it is used, then that method could define that variable as a temporary variable of the method instead (assuming that the method is not recursive).

Variables not referenced

Check for variables not referenced. If a variable is not used in a class, it should be deleted.

Intention revealing:

Assignment to same variable at the end of ifTrue:ifFalse: blocks

Checks for ifTrue:ifFalse: blocks that assign the same variable at the end of the block. Instead of having the assignment being in both blocks, we can instead assign the variable the result of the ifTrue:ifFalse: message. For example, this code:

	aBoolean 
		ifTrue: [foo := true]
		ifFalse: [foo := anotherBoolean]

could be rewritten to:

	foo := aBoolean 
		ifTrue: [true]
		ifFalse: [anotherBoolean]

Once we have simplified the expression by pulling the assignment out of the blocks, then we could see that the code is equivalent to:

	foo := aBoolean or: [anotherBoolean]

Guarding clauses

Checks for ifTrue: or ifFalse: conditions at end of methods that have two or more statements inside their blocks. Such code might better represent the true meaning of the code if they returned self instead. For example, the following code:

	methodName
		a isNil 
			ifFalse:
				[self doSomething.
				self doAnotherThing.
				self doSomethingElse]

might be better represented by:

	methodName
		a isNil ifTrue: [^self].
		self doSomething.
		self doAnotherThing.
		self doSomethingElse

In the first method, a not being nil looks like the exception, but most likely a being nil is the exception which is more obvious in the second method.

ifTrue:/ifFalse: returns instead of and:/or:'s

Checks for common ifTrue: returns that could be simplified. For example,

	foo
		aCondition ifTrue: [^false].
		^true

can be simplified to:

	foo
		^aCondition not

Method defined in all subclasses, but not in superclass

Checks classes for methods that are defined in all subclasses, but not defined in self. Such methods should most likely be defined as subclassResponsibility methods to help document the class. Furthermore, this check helps to find similar code that might be occurring in all the subclasses that should be pulled up into the superclass.

Sends add:/remove: to external collection

Checks for methods that appear to be modifying a collection that is owned by another object. Such modifications can cause problems especially if other variables are modified when the collection is modified. For example, CompositePart must set the container's of all its parts when adding a new component.

Unnecessary size check

Check for code that checks that a collection is non-empty before sending it an iteration message (e.g., do:, collect:, etc.). Since the collection iteration messages work for empty collections, we do not need to clutter up our method with the extra size check.

Uses "size = 0" or "= nil" instead of "isEmpty" or "isNil"

Checks for people using equality tests instead of the message sends. Since the code "aCollection size = 0" works for all objects, it is more difficult for someone reading such code to determine that "aCollection" is a collection. Whereas , if you say "aCollection isEmpty" then aCollection must be a collection since isEmpty is only defined for collections.

Uses at:ifAbsent: instead of at:ifAbsentPut:

Checks for uses of at:ifAbsent: when you could use the shorter at:ifAbsentPut: message. For example, the code "aDictionary at: aKey ifAbsent: [aDictionary at: aKey put: anObject]" should be rewritten as "aDictionary at: aKey ifAbsentPut: [anObject]". The rewrite rule documentation shows how to convert uses of the at:ifAbsent: message to use at:ifAbsentPut:.

Uses detect:ifNone: instead of contains:

Checks for the common code fragment: "(aCollection detect: [:each | 'some condition'] ifNone: [nil]) ~= nil". VisualWorks 2.0 added a contains: method that can simplify this code to "aCollection contains: [:each | 'some condition']". Not only is the contains: variant shorter, it better signifies what the code is doing.

Uses do: instead of collect: or select:'s

Checks for people using the do: method instead of using the collect: or select: methods. This often occurs with new people writing code. They will often write code such as:

	| newCollection |
	newCollection := OrderedCollection new.
	aCollection do: [:each | newCollection add: each doSomething].
	^newCollection

instead of using the collect: method:

	^aCollection collect: [:each | each doSomething]

The collect: and select: variants express the source code's intentions better.

Uses do: instead of contains: or detect:'s

Checks for people using the do: method instead of using the contains: or detect: methods.

Uses ifTrue:/ifFalse: instead of min: or max:

Checks for uses of ifTrue:/ifFalse: when it could be transformed to use min: or max:. For example, this code:

	a < b  ifTrue: [a] ifFalse: [b]

can be rewritten as:

	a min: b

Uses to:do: instead of do:, with:do:, or timesRepeat:

Checks for people using to:do: when a do:, with:do: or timesRepeat: should be used.

Uses whileTrue: instead of to:do:

Checks for people using whileTrue: when the shorter to:do: would work. For example, this common C-like code:

	i := 1.
	[i <= size]
		whileTrue: 
			["self do something with i".
			i := i + 1]

can be written as:

	1 to: size do: [:i | "self do something with i"]

Miscellaneous:

Doesn't use the result of a yourself message

Check for methods sending the yourself message when it is not necessary. For example, the following statement doesn't need the yourself message, since it is not used:

	aCollection addAll: #(a b c); yourself

Now if this statement was assigned to a variable, then you would need to cascade the yourself message in order to get the value of aCollection.

Inspect instances of "A + B * C" might be "A + (B * C)"

Checks for methods that might have precedence problems. People that are used to other languages precedence order often make mistakes when writing Smalltalk code since in Smalltalk all binary operations are performed left-to-right.

Instance variables defined in all subclasses

Checks classes for instance variables that are defined in all subclasses. Many times you might want to pull the instance variable up into the class so that all the subclasses do not have to define it.

Long methods

Returns all methods that have BasicLintRule class>>longMethodSize number of statements. This check counts statements, not lines.

Methods with full blocks

Lists methods that contain full blocks or create a context with the "thisContext" keyword. These methods are a place where inefficiencies can creep in. For example, a common reason why a full block is created is because a block assigns a temporary variable that is not defined inside the block. If the temporary variable is only used inside the block, then the definition of the temporary should be moved inside the block. The "move to inner scope" refactoring will automatically perform this, if you select it from the browser's code tool.

Non-blocks in ifTrue:/ifFalse: messages

Checks for methods that don’t use blocks in the ifTrue:ifFalse messages. People new to Smalltalk might write code such as: "aBoolean ifTrue: (self doSomething)" instead of the correct version: "aBoolean ifTrue: [self doSomething]". Even if these pieces of code are correct, they cannot be optimized by the compiler.

Redundant class name in selector

Checks for the class name in a selector. This is redundant since to call the you must already refer to the class name. For example, openHierarchyBrowserFrom: is a redundant name for HierarchyBrowser.

Refers to class name instead of "self class"

Checks for classes that have their class name directly in the source instead of "self class". The self class variant allows you to create subclasses without needing to redefine that method.

Sends "questionable" message

Check methods that send messages that perform low level things. You might want to limit the number of such messages in your application. For example, using become: throughout your application might not be the best thing. Also, messages such as isKindOf: can signify a lack of polymorphism. You can change which methods are "questionable" by editing the BasicLintRule>>badSelectors method.

String concatenation instead of streams

Check for people using string concatenation inside some iteration message. Since string concatenation is O(n^2), it is better to use streaming since it is O(n) - assuming that n is large enough.

Unnecessary assignment or return in block

Checks valueNowOrOnUnwindDo:, valueOnUnwindDo:, and showWhile: blocks for assignments or returns that are the last statement in the block. These assignments or returns can be moved outside the block since these messages return the value of the block. For example, the code:

	methodName
		| bos |
		bos := BinaryObjectStorage onOld: 'test' asFilename readStream.
		[^bos next]
			valueNowOrOnUnwindDo: [bos close]

can be rewritten as:

	methodName
		| bos |
		bos := BinaryObjectStorage onOld: 'test' asFilename readStream.
		^[bos next]
			valueNowOrOnUnwindDo: [bos close]

Having the assignment or return inside the block will require the VM to create a full block. Full blocks run much slower than copying or optimized blocks.

Utility methods

List methods that have one or more arguments and do no refer to self or an instance variable. These methods might be better defined in some other class or as class methods.

Variable is only assigned a single literal value

If a variable is only assigned a single literal value then that variable is either nil or that literal value. If the variable is always initialized with that literal value, then you could replace each variable reference with a message send to get the value. If the variable can also be nil, then you might want to replace that variable with another that stores true or false depending on whether the old variable had been assigned.

Comments or suggestions can be sent to brant@refactory.com and roberts@refactory.com.
Last updated on 29-Apr-97.