Bug 185745 - [introduce parameter object] introduces object to method which is not selected
Summary: [introduce parameter object] introduces object to method which is not selected
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.3 RC2   Edit
Assignee: Karsten Becker CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2007-05-07 08:38 EDT by Benno Baumgartner CLA
Modified: 2007-06-06 12:27 EDT (History)
1 user (show)

See Also:
martinae: review+
benno.baumgartner: review+
martinae: review+


Attachments
Better check of selection (3.80 KB, patch)
2007-05-22 06:23 EDT, Karsten Becker CLA
no flags Details | Diff
Updated version (8.48 KB, patch)
2007-05-23 11:11 EDT, Karsten Becker CLA
no flags Details | Diff
fix (8.60 KB, patch)
2007-05-24 08:11 EDT, Benno Baumgartner CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benno Baumgartner CLA 2007-05-07 08:38:49 EDT
I20070503-1400

Given:
package test;
public class E1 {
	public void foo(int x, int y) {}
	
	public static void main(String[] args) {
		new E1().foo(1, 2);
	}
}
1. Select 'foo' in body of 'main'
2. Introduce Parameter Object...
Is:
 A parameter object is introduced to 'main'
Should:
 Introduce it to the selected method, like change method signature does
Comment 1 Karsten Becker CLA 2007-05-22 06:23:18 EDT
Created attachment 68093 [details]
Better check of selection
Comment 2 Benno Baumgartner CLA 2007-05-23 08:23:02 EDT
This leads to a NPE easily=>Whenever getSingleSelectedMethod(*) returns null...
Also code style:
1. Do not comment in method body what method does, if you really feel the need to do that then refactor the method such that it is clear what it does. The code is the doc.
2. Use the pattern
if (expr)
    return null;

if (expr)
    return null;

return validElement;
3. There is commented code, why do you not set the tooltip/help/description?
Comment 3 Karsten Becker CLA 2007-05-23 11:11:54 EDT
Created attachment 68357 [details]
Updated version

2.1 Comments describing the general problem are not too bad. This code section is "inherited" from Markus. Sometimes it simple is hard to write meaningful code.
2.2 I clearly prefer
if (elementAt instanceof IMethod)
  return (IMethod) elementAt;
return null;
over
if (!(elementAt instanceof IMethod))
  return null;
return (IMethod) elementAt;
because it shows that when elementAt is an instance of IMethod I simply return it. Otherwise the assumptions about the result are deferred until the last line.
2.3 I'm not sure where this would be shown, but I updated it.

NPEs resulting from getSingleMethod are now handled correctly
Comment 4 Martin Aeschlimann CLA 2007-05-24 08:04:46 EDT
patch is good
I would change the following:
- remove the added 'tab' in 'ExceptionHandler.handle(e....'
- move the null check out of run(IMethod)
Comment 5 Benno Baumgartner CLA 2007-05-24 08:11:28 EDT
Created attachment 68553 [details]
fix

- Don't even THINK about catching java.lang.Exception!
- I can't parse this, a project tries to keep a certain code style, it is not that important what a single comitter thinks is better, consistency is more important.
- 'private run' can throw CoreException
Comment 6 Martin Aeschlimann CLA 2007-05-24 08:40:29 EDT
please release.
Comment 7 Benno Baumgartner CLA 2007-05-24 09:03:02 EDT
fixed > I20070524-0010
Comment 8 Benno Baumgartner CLA 2007-05-29 06:33:35 EDT
verified in I20070527-0010