[Refactoring] Driver improvement for temp to instance variable

src/Refactoring-Core/RBCondition.class.st

@@ -56,25 +56,6 @@ RBCondition class >> checkInstanceVariableName: aName in: aClass [
 	^ OCScanner isVariable: string
 ]
 
-{ #category : 'instance creation' }
-RBCondition class >> checkNotMultipleTemporaryDefinitionsOf: aString in: aClass [
-
-	| condition |
-	condition := self new.
-	condition
-		block: [
-				| methods |
-				methods := self multipleMethodsDefiningTemporary: aString in: aClass ignore: [ :class :selector | false ].
-				methods size > 1
-					ifTrue: [
-							condition errorMacro:
-								'More than one method defines temporary variable ' , aString , ': ' , (methods collect: [ :m | m selector ]) asString.
-							false ]
-					ifFalse: [ true ] ]
-		errorString: aClass printString , ' <1?:does not >define<1?s:> temporary variable ' , aString.
-	^ condition
-]
-
 { #category : 'instance creation' }
 RBCondition class >> definesClassVariable: aString in: aClass [
 	^self new
@@ -374,25 +355,6 @@ RBCondition class >> methodDefiningTemporary: aString in: aClass ignore: aBlock
 	^ nil
 ]
 
-{ #category : 'utilities' }
-RBCondition class >> multipleMethodsDefiningTemporary: aString in: aClass ignore: aBlock [
-
-	| searcher methods method |
-	searcher := OCParseTreeSearcher new.
-	methods := Set new.
-	method := nil.
-
-	searcher matches: aString do: [ :aNode :answer | methods add: method ].
-	aClass withAllSubclasses do: [ :class |
-			class selectors do: [ :each |
-					(aBlock value: class value: each) ifFalse: [
-							| parseTree  |
-							method := class methodFor: each.
-							parseTree := class parseTreeForSelector: each.
-							parseTree ifNotNil: [ searcher executeTree: parseTree ] ] ] ].
-	^ methods
-]
-
 { #category : 'instance creation' }
 RBCondition class >> referencesClassVariable: aString in: aClass [
 

src/Refactoring-Core/RBTemporaryToInstanceVariableRefactoring.class.st

@@ -114,17 +114,9 @@ RBTemporaryToInstanceVariableRefactoring >> applicabilityPreconditions [
 RBTemporaryToInstanceVariableRefactoring >> breakingChangePreconditions [
 
 	^ {
-		  (RBCondition withBlock: [
-				   (class allSubclasses anySatisfy: [ :cls | cls definesInstanceVariable: temporaryVariableName asString ]) ifTrue: [
-						   self refactoringWarning:
-							   ('One or more subclasses of <1p> already defines an<n>instance variable with the same name. Proceed anyway?'
-								    expandMacrosWith: class name) ].
-				   true ]).
-		  (RBCondition checkNotMultipleTemporaryDefinitionsOf: temporaryVariableName in: class).
-		  (ReVariablesNotReadBeforeWrittenCondition new
-			   subtree: parseTree;
-			   variables: temporaryVariableName;
-			   checkForTemporaryVariables: true) }
+		  self preconditionNoSubclassDefinesVar.
+		  self preconditionNoMultipleTempOccurences.
+		  self preconditionNoReadBeforeWritten }
 ]
 
 { #category : 'initialization' }
@@ -143,6 +135,31 @@ RBTemporaryToInstanceVariableRefactoring >> isTemporaryVariableNameValid [
 		self refactoringError: temporaryVariableName , ' is a block parameter' ]
 ]
 
+{ #category : 'preconditions' }
+RBTemporaryToInstanceVariableRefactoring >> preconditionNoMultipleTempOccurences [
+
+	^ ReMultipleMethodsDontReferToTempVarCondition name: temporaryVariableName class: class selector: selector 
+]
+
+{ #category : 'preconditions' }
+RBTemporaryToInstanceVariableRefactoring >> preconditionNoReadBeforeWritten [
+
+	^ ReVariablesNotReadBeforeWrittenCondition new
+		  subtree: parseTree;
+		  variables: { temporaryVariableName };
+		  checkForTemporaryVariables: true
+]
+
+{ #category : 'preconditions' }
+RBTemporaryToInstanceVariableRefactoring >> preconditionNoSubclassDefinesVar [
+
+	^ RBCondition
+		  withBlock: [ (class allSubclasses anySatisfy: [ :cls | cls definesInstanceVariable: temporaryVariableName asString ]) not ]
+		  errorString:
+			  ('One or more subclasses of <1p> already defines an<n>instance variable with the same name.'
+				   expandMacrosWith: class name)
+]
+
 { #category : 'preconditions' }
 RBTemporaryToInstanceVariableRefactoring >> preconditions [
 
@@ -159,10 +176,6 @@ RBTemporaryToInstanceVariableRefactoring >> prepareForExecution [
 RBTemporaryToInstanceVariableRefactoring >> privateTransform [
 
 	self removeTemporaryOfClass: class.
-	class allSubclasses do: [ :cls |
-		(cls definesInstanceVariable: temporaryVariableName)
-			ifTrue: [ cls removeInstanceVariable: temporaryVariableName ]
-			ifFalse: [ self removeTemporaryOfClass: cls ] ].
 	class addInstanceVariable: temporaryVariableName
 ]
 
@@ -178,8 +191,8 @@ RBTemporaryToInstanceVariableRefactoring >> removeTemporaryOfMethod: aSelector i
 	method := aClass methodFor: aSelector.
 	methodParseTree := method parseTree.
 	methodParseTree ifNil: [ self refactoringError: 'Could not parse method' ].
-	( matcher := self parseTreeRewriterClass removeTemporaryNamed: temporaryVariableName )
-		executeTree: methodParseTree.
+	(methodParseTree temporaryNames includes: temporaryVariableName) ifFalse: [ ^ self ].
+	(matcher := self parseTreeRewriterClass removeTemporaryNamed: temporaryVariableName) executeTree: methodParseTree.
 	method compileTree: matcher tree
 ]
 

src/Refactoring-Core/ReMultipleMethodsDontReferToTempVarCondition.class.st

@@ -0,0 +1,79 @@
+"
+I check if temporary variable name is used in other methods of the class.
+"
+Class {
+	#name : 'ReMultipleMethodsDontReferToTempVarCondition',
+	#superclass : 'ReVariableNameCondition',
+	#instVars : [
+		'class',
+		'selector'
+	],
+	#category : 'Refactoring-Core-Conditions',
+	#package : 'Refactoring-Core',
+	#tag : 'Conditions'
+}
+
+{ #category : 'instance creation' }
+ReMultipleMethodsDontReferToTempVarCondition class >> name: aString class: aClass selector: aSelector [
+
+	^ (self name: aString)
+		  class: aClass;
+		  selector: aSelector yourself
+]
+
+{ #category : 'checking' }
+ReMultipleMethodsDontReferToTempVarCondition >> check [
+
+	^ self violators isEmpty
+]
+
+{ #category : 'setter' }
+ReMultipleMethodsDontReferToTempVarCondition >> class: aRBClass [ 
+
+	class := aRBClass.
+
+]
+
+{ #category : 'utilities' }
+ReMultipleMethodsDontReferToTempVarCondition >> multipleMethodsDefiningTemporary: aString in: aClass ignore: aBlock [
+
+	| searcher methods method |
+	searcher := OCParseTreeSearcher new.
+	methods := Set new.
+	method := nil.
+
+	searcher matches: aString do: [ :aNode :answer | methods add: method ].
+	aClass withAllSubclasses do: [ :cls |
+			cls selectors do: [ :each |
+					(aBlock value: cls value: each) ifFalse: [
+							| parseTree |
+							method := cls methodFor: each.
+							parseTree := cls parseTreeForSelector: each.
+							parseTree ifNotNil: [ searcher executeTree: parseTree ] ] ] ].
+	^ methods
+]
+
+{ #category : 'accessing' }
+ReMultipleMethodsDontReferToTempVarCondition >> selector: aSelector [
+
+	selector := aSelector
+]
+
+{ #category : 'displaying' }
+ReMultipleMethodsDontReferToTempVarCondition >> violationMessageOn: aStream [
+
+	self violators ifEmpty: [ ^ self ].
+	^ aStream
+		  nextPutAll: 'More than one method defines temporary variable ';
+		  nextPutAll: name;
+		  nextPutAll: ': ';
+		  nextPutAll: (self violators collect: [ :m | m selector ]) asArray asString
+]
+
+{ #category : 'checking' }
+ReMultipleMethodsDontReferToTempVarCondition >> violators [
+
+	| methods |
+	methods := self multipleMethodsDefiningTemporary: name in: class ignore: [ :cls :selectors | false ].
+	^ methods reject: [ :m | m selector = selector ]
+]

src/Refactoring-Core/ReRefactoring.class.st

@@ -59,10 +59,11 @@ ReRefactoring >> canReferenceVariable: aString in: aClass [
 { #category : 'scripting api - conditions' }
 ReRefactoring >> checkBreakingChangePreconditions [
 	"Check a preconditions and raise an error on violations. This method is part of the scripting API since it raises an error."
-	
+
 	| failedPreconditions |
 	failedPreconditions := self failedBreakingChangePreconditions.
 	failedPreconditions ifEmpty: [ ^ self ].
+
 	RBRefactoringWarning signalFor: failedPreconditions
 ]
 

src/Refactoring-Core/ReVariablesNotReadBeforeWrittenCondition.class.st

@@ -48,6 +48,7 @@ ReVariablesNotReadBeforeWrittenCondition >> variables: aCollection [
 { #category : 'displaying' }
 ReVariablesNotReadBeforeWrittenCondition >> violationMessageOn: aStream [
 
+	self check ifTrue: [ ^ self ].
 	aStream
 		nextPutAll: 'Cannot extract selected code because variables: ';
 		nextPutAll: variables asString;

src/Refactoring-DataForTesting/ReClassToConvertTemporaryToInstanceVariable.class.st

@@ -0,0 +1,34 @@
+Class {
+	#name : 'ReClassToConvertTemporaryToInstanceVariable',
+	#superclass : 'Object',
+	#instVars : [
+		'instVar'
+	],
+	#category : 'Refactoring-DataForTesting-ForConvertingVariables',
+	#package : 'Refactoring-DataForTesting',
+	#tag : 'ForConvertingVariables'
+}
+
+{ #category : 'action' }
+ReClassToConvertTemporaryToInstanceVariable >> doAction1 [
+
+	| temp |
+	temp := 35.
+
+	^ temp
+]
+
+{ #category : 'action' }
+ReClassToConvertTemporaryToInstanceVariable >> doAction2 [
+
+	| instVar |
+	instVar := 3.
+	^ instVar
+]
+
+{ #category : 'initialization' }
+ReClassToConvertTemporaryToInstanceVariable >> initialize [
+
+	<ignoreUnusedVariables: #( #instVar )>
+	super initialize
+]

src/Refactoring-UI-Tests/ReConvertTemporaryToInstanceVariableDriverTest.class.st

@@ -0,0 +1,50 @@
+Class {
+	#name : 'ReConvertTemporaryToInstanceVariableDriverTest',
+	#superclass : 'ReDriverTest',
+	#instVars : [
+		'testingEnvironment'
+	],
+	#category : 'Refactoring-UI-Tests-Driver',
+	#package : 'Refactoring-UI-Tests',
+	#tag : 'Driver'
+}
+
+{ #category : 'getter' }
+ReConvertTemporaryToInstanceVariableDriverTest >> classToConvertTemporaryToInstanceVariable [
+
+	^ ReClassToConvertTemporaryToInstanceVariable
+]
+
+{ #category : 'running' }
+ReConvertTemporaryToInstanceVariableDriverTest >> setUp [ 
+
+	super setUp.
+	testingEnvironment := RBClassEnvironment classes: self classToConvertTemporaryToInstanceVariable withAllSubclasses 
+]
+
+{ #category : 'tests' }
+ReConvertTemporaryToInstanceVariableDriverTest >> testConvertTempFailure [
+
+	| driver |
+	driver := ReConvertTemporaryToInstanceVariableDriver new.
+	self setUpDriver: driver.
+	driver class: self classToConvertTemporaryToInstanceVariable selector: #doAction2 variable: 'instVar'.
+
+	driver runRefactoring.
+
+	self assert: driver refactoring changes changes size equals: 0
+]
+
+{ #category : 'tests' }
+ReConvertTemporaryToInstanceVariableDriverTest >> testConvertTempSuccessfully [
+
+	| driver |
+	driver := ReConvertTemporaryToInstanceVariableDriver new.
+	self setUpDriver: driver.
+	driver class: self classToConvertTemporaryToInstanceVariable selector: #doAction1 variable: 'temp'.
+
+	driver runRefactoring.
+
+	self assert: driver refactoring changes changes size equals: 2.
+
+]

src/Refactoring-UI/ReBrowseMethodChoice.class.st

@@ -17,5 +17,5 @@ ReBrowseMethodChoice >> action [
 { #category : 'accessing' }
 ReBrowseMethodChoice >> description [ 
 
-	^ 'Browse the method(s)'
+	^ 'Browse the method'
 ]

src/Refactoring-UI/ReBrowseMethodsChoice.class.st

@@ -0,0 +1,19 @@
+Class {
+	#name : 'ReBrowseMethodsChoice',
+	#superclass : 'ReMethodChoice',
+	#category : 'Refactoring-UI-Choices',
+	#package : 'Refactoring-UI',
+	#tag : 'Choices'
+}
+
+{ #category : 'accessing' }
+ReBrowseMethodsChoice >> action [
+
+	driver browseMethods 
+]
+
+{ #category : 'accessing' }
+ReBrowseMethodsChoice >> description [
+
+	^ description ifNil: [ description := 'Browse the methods' ]
+]