Bug 158624 - API Review for Native Execution Support Module
API Review for Native Execution Support Module
Status: NEW
Product: ide
Classification: Unclassified
Component: Code
6.x
All All
: P3 (vote)
: TBD
Assigned To: issues@ide
issues@ide
issue_reviewed
: API_REVIEW_FAST
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-16 16:46 UTC by Andrew Krasny
Modified: 2014-04-14 14:35 UTC (History)
4 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
Architecture Questionary Answers (73.47 KB, text/xml)
2009-02-16 18:12 UTC, Andrew Krasny
Details
Java Doc (25.18 KB, application/x-gzip)
2009-02-16 18:12 UTC, Andrew Krasny
Details
RicExecution module javadoc (17.07 KB, application/x-gzip)
2009-02-17 06:53 UTC, ivan
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Krasny 2009-02-16 16:46:30 UTC
I would like to ask for an API review of the module that extends ExtExecution functionality.
WikiPage was created for this API review: http://wiki.netbeans.org/NativeExecutionSupportModule.
It contains links to module's documentation.

Thanks,
=Andrew
Comment 1 Andrew Krasny 2009-02-16 16:48:53 UTC
Comments from Petr Hejl:

PH01: There seems to be methods with duplicated functionality in NativeProcess. What is the semantical difference
between pairs like cancel() - destroy() and waitFor() - waitResult()?
PH02: NativeProcess.Listener is not used anywhere in NativeProcess so I think it should be defined in
NativeProcessBuilder where it is actually used.
PH03: The old and new state has to be delivered to NativeProcess.Listener. This can complicate things terribly
especially in multithreaded environments. If you don't have strong justification to do so, please remove states from the
interface (implementation can get new state via direct call to NativeProcess). In such case you could even use the
ChangeListener defined in JDK and delete your own interface completely - PH02 is not valid in such case ;) .
PH04: I think ExecutionEnvironment should be merged to process builder (perhaps a method setHost(user, host, port) in
builder would do the same as the whole ExecutionEnvironment from the API point of view).
PH05: I'm not sure about the util package. Are all the classes API? What is the suggested use case?
PH06: HostNotConnectedException - do not inherit from Throwable, use Exception or better IOException. Anyway I think
java.net.ConnectException or java.net.UnknowHostException can do the same job for you.
PH07: Implementation note - I would use package org.netbeans.modules.nativeexecution instead of
org.netbeans.modules.nativeexecution.api.impl and package org.netbeans.modules.nativeexecution.api.util instead of
org.netbeans.modules.nativeexecution.util. 
Comment 2 Andrew Krasny 2009-02-16 16:52:20 UTC
> PH01: There seems to be methods with duplicated functionality in NativeProcess. What is the semantical difference 
> between pairs like cancel() - destroy() and waitFor() - waitResult()?

This has been redesigned. So no valid anymore.

> PH02: NativeProcess.Listener is not used anywhere in NativeProcess so I think it should be defined in 
> NativeProcessBuilder where it is actually used.

This has been redesigned. So no valid anymore.

> PH03: The old and new state has to be delivered to NativeProcess.Listener. This can complicate things terribly 
> especially in multithreaded environments. If you don't have strong justification to do so, please remove states from 
> the interface (implementation can get new state via direct call to NativeProcess). In such case you could even use 
> the ChangeListener defined in JDK and delete your own interface completely - PH02 is not valid in such case ;) .

Now ChangeListener is used.

> PH04: I think ExecutionEnvironment should be merged to process builder (perhaps a method setHost(user, host, port) in 
> builder would do the same as the whole ExecutionEnvironment from the API point of view).

It is still a separate class as it is a convenient way to pass information about *where* to start a process between parties.

> PH05: I'm not sure about the util package. Are all the classes API? What is the suggested use case?

There are some usecases described in documentation.

> PH06: HostNotConnectedException - do not inherit from Throwable, use Exception or better IOException. Anyway I think 
> java.net.ConnectException or java.net.UnknowHostException can do the same job for you.

ConnectException is used now.

> PH07: Implementation note - I would use package org.netbeans.modules.nativeexecution instead of 
> org.netbeans.modules.nativeexecution.api.impl and package org.netbeans.modules.nativeexecution.api.util instead of 
> org.netbeans.modules.nativeexecution.util. 

Packages renamed.
Comment 3 Andrew Krasny 2009-02-16 16:57:37 UTC
In case we all agree on friend module, API_REVIEW_FAST can be done... So, adding this keyword.
If not, we may go through the full review. In this case I need a list of reviewers.
Comment 4 _ ludo 2009-02-16 17:04:14 UTC
Internal SWAN links for an API review of an open source project?

Weird...
Comment 5 Andrew Krasny 2009-02-16 18:12:02 UTC
Created attachment 77037 [details]
Architecture Questionary Answers
Comment 6 Andrew Krasny 2009-02-16 18:12:54 UTC
Created attachment 77038 [details]
Java Doc
Comment 7 ivan 2009-02-17 06:51:58 UTC
Owwww.
And I was just about to start the API review for my RichExecution.
I'm leaving for skiing tomorrow til the end of the week so I'll just
quickly attach the latest javadoc for it so you can get a flavor for it.
Comment 8 ivan 2009-02-17 06:53:32 UTC
Created attachment 77056 [details]
RicExecution module javadoc


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo