Community
Participate
Working Groups
The code of the discovery plug-ins needs to be reviewed to ensure Galileo requirements etc. are met before plug-ins are moved out of the sandbox directory. The o.e.m.discovery.test plug-in should be renamed to o.e.m.discovery.tests as part of the move.
Created attachment 135504 [details] patch
The patch addresses some of the points that Shawn and I noticed in a code review. Most changes are trivial (e.g. renaming BUNDLE_ID to ID_PLUGIN to follow Mylyn conventions). David, it would be good if you could review changes to these files and the FIXME comments: - Regexp in RemoteBundleDiscoveryStrategy - ConnectorDiscoveryWizardMainPage Other suggestions that we noted in the review: - Replace usages of throw new IllegalArgumentException() with Assert.isNotNull(). - Propagate cancellation by throwing OperationCancelledException instead of InterruptedException or returning. Policy.checkCancelled() is a useful helper method. Generally methods that take a progress monitor may throw OperationCancelledException so the invoking code should handle that gracefully. - Replace instances of IRunnableWithProgress by ICoreRunnable to avoid exception conversion boiler plate code. - Provide the context in the message which operation was executed if logging to the error log. Using e.getMessage() is usually not very helpful, particularly when the message is null. - Consider replacing usages of SubProgressMonitor by SubMonitor which is more robust to clients not invoking beginTask()/done() properly. - Monitors should generally be wrapped in a try/finally block: try { monitor.beginTask(); ... } finally { monitor.done(); } - ConnectorCategory, Icon: Why are the fields protected instead of private? - Merge oe.m.internal.discovery.ui.Messages and WorkbenchMessages On a general note: The result of getActiveWorkbenchWindow() can be null at any time and needs be checked, e.g. when invoking PlatformUI.getWorkbench().getActiveWorkbenchWindow().getShell(). I fixed two instances but may have misses some.
I have added a default proxy provider to WebLocation so proxy support should work now.
Thanks for the review. My comments follow: (In reply to comment #2) > The patch addresses some of the points that Shawn and I noticed in a code > review. Most changes are trivial (e.g. renaming BUNDLE_ID to ID_PLUGIN to follow > Mylyn conventions). David, it would be good if you could review changes to these > files and the FIXME comments: > > - Regexp in RemoteBundleDiscoveryStrategy @.*@ is greedy whereas @.*?@ is not. This can make a difference in some cases. > - ConnectorDiscoveryWizardMainPage Why put child controls in a list instead of using the array directly? > > Other suggestions that we noted in the review: > > - Replace usages of throw new IllegalArgumentException() with > Assert.isNotNull(). Why? IllegalArgumentException is specifically designed for this purpose. > - Propagate cancellation by throwing OperationCancelledException instead of > InterruptedException or returning. Policy.checkCancelled() is a useful helper > method. Generally methods that take a progress monitor may throw > OperationCancelledException so the invoking code should handle that gracefully. IRunnableWithProgress requires that InterruptedException be thrown when cancelled > - Replace instances of IRunnableWithProgress by ICoreRunnable to avoid exception > conversion boiler plate code. Originally the code was using ICoreRunnable but I switched it to use IRunnableWithProgress instead. Error handling with ICoreRunnable and CommonUiUtil isn't what I would like (some errors are logged and not propagated, so it's impossible to notify the user that an error occurred). > - Provide the context in the message which operation was executed if logging to > the error log. Using e.getMessage() is usually not very helpful, particularly > when the message is null. > - Consider replacing usages of SubProgressMonitor by SubMonitor which is more > robust to clients not invoking beginTask()/done() properly. > - Monitors should generally be wrapped in a try/finally block: > try { > monitor.beginTask(); > ... > } finally { > monitor.done(); > } agreed, though in all cases here the progress monitor is disposed after an exception > - ConnectorCategory, Icon: Why are the fields protected instead of private? to facilitate the generation gap pattern > - Merge oe.m.internal.discovery.ui.Messages and WorkbenchMessages > > On a general note: The result of getActiveWorkbenchWindow() can be null at any > time and needs be checked, e.g. when invoking > PlatformUI.getWorkbench().getActiveWorkbenchWindow().getShell(). I fixed two > instances but may have misses some. Lots of good points, will incorporate your suggestions into the code. Thanks for taking the time to do a thorough code review.
> @.*@ is greedy whereas @.*?@ is not. This can make a difference in some cases. Thanks. That's great to know. > > - ConnectorDiscoveryWizardMainPage > > Why put child controls in a list instead of using the array directly? That is what the patch does? - for (Control child : new ArrayList<Control>(Arrays.asList(body.getChildren()))) { + for (Control child : body.getChildren()) { > > Other suggestions that we noted in the review: > > > > - Replace usages of throw new IllegalArgumentException() with > > Assert.isNotNull(). > > Why? IllegalArgumentException is specifically designed for this purpose. I find Assert.isNotNull() very concise. It's a mostly a convention that we have started to replace IllegalArgumentException by Assert statements. Feel free to leave it, it's not worth changing existing code though. > > - Propagate cancellation by throwing OperationCancelledException instead of > > InterruptedException or returning. Policy.checkCancelled() is a useful helper > > method. Generally methods that take a progress monitor may throw > > OperationCancelledException so the invoking code should handle that > gracefully. > > IRunnableWithProgress requires that InterruptedException be thrown when > cancelled Yes, that was the reason for introducing ICoreRunnable. I noticed that it's safest to guard against OperationCancelledException when passing a progress monitor to a method. The purpose of CoreRunnable is to take care of converting the exceptions. > > - Replace instances of IRunnableWithProgress by ICoreRunnable to avoid > exception > > conversion boiler plate code. > > Originally the code was using ICoreRunnable but I switched it to use > IRunnableWithProgress instead. Error handling with ICoreRunnable and > CommonUiUtil isn't what I would like (some errors are logged and not propagated, > so it's impossible to notify the user that an error occurred). Good point. Do you have suggestions how to improve the API of CommonUiUtil to address that concern? > agreed, though in all cases here the progress monitor is disposed after an > exception Okay. To ease future maintenance and potential reuse of the code I think it makes sense to always follow the try/finally pattern for monitors. > > - ConnectorCategory, Icon: Why are the fields protected instead of private? > > to facilitate the generation gap pattern Can you point me to documentation where the usage of protected fields is described in more detail? We have discussed this in the past and from my experience it limits evolution of classes since it exposes a lot of implementation detail. > Lots of good points, will incorporate your suggestions into the code. Thanks > for taking the time to do a thorough code review. I should have pointed out that we were impressed with the quality of the code overall. The points above are merely suggestions for improvement that we noted during the review but are not blockers for moving the plug-ins out of the sandbox.
(In reply to comment #5) > > Why put child controls in a list instead of using the array directly? > > That is what the patch does? My mistake. I'll change it. > > > Other suggestions that we noted in the review: > > > > > > - Replace usages of throw new IllegalArgumentException() with > > > Assert.isNotNull(). > > > > Why? IllegalArgumentException is specifically designed for this purpose. > > I find Assert.isNotNull() very concise. It's a mostly a convention that we have > started to replace IllegalArgumentException by Assert statements. Feel free to > leave it, it's not worth changing existing code though. I prefer IllegalArgumentException -- however I'm used to writing code sans-OSGi/Eclipse dependencies. In general I prefer to use facilities provided by the Java platform unless there's a compelling reason to do otherwise. In this case, to me it looks like Assert provides very little benefit while introducing an API dependency. There are other issues. Consider the following case: bc. public Object doSomething(Object arg) { if (arg == null) { throw new IllegalArgumentException(); } else { return arg; } } In this case if Assert was introduced you'd get a compile error. In other words, using a throw statement lets the compiler know that there is no way for that code path to proceed. Using @throw@ has other benefits too: * it makes it easier to set a breakpoint. Setting a breakpoint in Assert will catch _all_ codepaths where there's a problem, whereas using @throw@ gives the developer the flexibility of only stopping for a specific code path _or_ using an exception breakpoint to catch all problems * it makes the stack trace smaller, thus easier to understand. > > IRunnableWithProgress requires that InterruptedException be thrown when > > cancelled > > Yes, that was the reason for introducing ICoreRunnable. I must be missing something... what's wrong with using InterruptedException? > Good point. Do you have suggestions how to improve the API of CommonUiUtil to > address that concern? How about having CommonUiUtil always propagate all exceptions? Alternatively you could wrap non-CoreException classes in a new CoreException. > Okay. To ease future maintenance and potential reuse of the code I think it > makes sense to always follow the try/finally pattern for monitors. Fair enough. > > > - ConnectorCategory, Icon: Why are the fields protected instead of private? > > > > to facilitate the generation gap pattern > > Can you point me to documentation where the usage of protected fields is > described in more detail? We have discussed this in the past and from my > experience it limits evolution of classes since it exposes a lot of > implementation detail. In general I agree with you wrt protected fields limiting the evolution of a class... however in this case ConnectorCategory is designed to be as close to a simple POJO as possible. A transfer object, if you will. Thus if the fields go away or change, then subclasses that use those fields will also need changing. BTW, this code is also internal -- so if we need to evolve the class then we can easily do so. > > Lots of good points, will incorporate your suggestions into the code. Thanks > > for taking the time to do a thorough code review. > > I should have pointed out that we were impressed with the quality of the code > overall. The points above are merely suggestions for improvement that we noted > during the review but are not blockers for moving the plug-ins out of the > sandbox. Thanks, I appreciate the comment.
> I prefer IllegalArgumentException -- however I'm used to writing code > sans-OSGi/Eclipse dependencies. In general I prefer to use facilities provided > by the Java platform unless there's a compelling reason to do otherwise. In > this case, to me it looks like Assert provides very little benefit while > introducing an API dependency. Fair enough. I consider org.eclipse.core.runtime a useful utility library that runs outside of OSGi and a dependency that most plug-ins have anyways. [...] > Using @throw@ has other benefits too: > > * it makes it easier to set a breakpoint. Setting a breakpoint in Assert will > catch _all_ codepaths where there's a problem, whereas using @throw@ gives the > developer the flexibility of only stopping for a specific code path _or_ using > an exception breakpoint to catch all problems > * it makes the stack trace smaller, thus easier to understand. All good arguments. For me the better readability of the source code and the consistency with Eclipse platform code weights stronger. I don't think we ever formalized the convention of using Assert instead of IllegalArgumentException so we can just go with either. > > > IRunnableWithProgress requires that InterruptedException be thrown when > > > cancelled > > > > Yes, that was the reason for introducing ICoreRunnable. > > I must be missing something... what's wrong with using InterruptedException? I simply don't like the fact that platform uses two different exception types to indicate the same condition. It complicates the exception handling code and there is a risk that callers don't handle OperationCanceledException in places where InterruptedException is declared although I have seen code that can throw either. I prefer to have a single code path that deals with cancellation. > How about having CommonUiUtil always propagate all exceptions? Alternatively > you could wrap non-CoreException classes in a new CoreException. I think I would prefer to propagate RuntimeExceptions in that case since it really is an unrecoverable error if the ICoreRunnable throws anything other than a CoreException of OperationCancelledException. I'll take a look at the places where CommonsUiUtil is used to check if that change makes sense. > In general I agree with you wrt protected fields limiting the evolution of a > class... however in this case ConnectorCategory is designed to be as close to a > simple POJO as possible. A transfer object, if you will. Thus if the fields go > away or change, then subclasses that use those fields will also need changing. > BTW, this code is also internal -- so if we need to evolve the class then we can > easily do so. Good. I'll try to remember that in case we move the class to an API package :). David, let me know when you are done with your changes and it's safe to move the plug-ins in cvs.
(In reply to comment #7) > David, let me know when you are done with your changes and it's safe to move the plug-ins in cvs. I've applied most of the review-based changes. Please move these at your convenience.
Thanks! Done. I'll send out a notification to mylyn-dev.
