9:19 a.m.
Please take it as feedback and not hassling ;)
INCONSISTENT METHOD NAMES
* ISSUE 1: Many Embedded project classes/intefaces follow BEAN naming convention but
there are some exceptions.
* RULE 1: API should be consistent. Once you're following BEAN naming convention,
whole API must apply to this rule.
Example:
package org.jboss.bootstrap.api.as.config;
|
| public interface JBossASBasedServerConfig<T extends
JBossASBasedServerConfig<T>> extends ServerConfig<T>
| {
| ...
| String getBindAddress();
| T bindAddress(String bindAddress); // this is WRONG, see RULE 1
| ...
| }
|
| package org.jboss.bootstrap.api.server;
|
| public interface Server<K extends Server<K, T>, T extends
ServerConfig<T>>
| {
| ...
| T getConfiguration();
| void setConfiguration(T config); // this is CORRECT
| ...
| }
|
SUGGESTION: The best practice is to follow bean naming convention for setter/getter
methods.
DO NOT DECLARE TO THROW RUNTIME EXCEPTIONS
* ISSUE 2: Many embedded project classes/interfaces declare to throw RuntimeExceptions,
but only for some methods
* RULE 2.1: Declare exception to be thrown if its abstraction is consistent with
class/API (correct: FileOutputStream.write(byte[]) throws IOException)
* RULE 2.2: Declare exception to be thrown if it's expected (wrong:
FileOutputStream.write(byte[]) throws ArrayIndexOutOfBoundsException)
* RULE 2.3: Once API designer decides to declare to throw exception this decision must be
consistent for whole API.
* RULE 2.4: Throwing checked exceptions force users of the API to catch or propagate
them, consider this when designing API.
* RULE 2.5: Runtime exceptions indicate programmer errors. The best practice is to
don't declare them in method signature.
Example:
package org.jboss.bootstrap.api.as.config;
|
| public interface JBossASBasedServerConfig<T extends
JBossASBasedServerConfig<T>> extends ServerConfig<T>
| {
| ...
| URL getServerLibLocation();
| T serverLibLocation(URL serverLibLocation); // this is WRONG: see RULE 2.3
| T serverLibLocation(String serverLibLocation) throws IllegalArgumentException; //
this is WRONG - see RULE 2.1, RULE 2.2, RULE 2.4 and RULE 2.5
| ...
| }
|
|
INCONSISTENT ENUM VALUES
* ISSUE 3: There are inconsistent enum values in embedded API.
* RULE 3: Always define consistent enums/constants to don't confuse users.
Example:
package org.jboss.bootstrap.api.lifecycle;
|
| public enum LifecycleState
| {
| INSTANCIATED,
| PRE_INIT, // this is WRONG, should be INITIALIZING to be consistent with other
constants
| INITIALIZED,
| IDLE,
| STARTING,
| STARTED,
| STOPPING,
| STOPPED
| }
|
|
INCONSISTENT API ABSTRACTION
* ISSUE 4: There are low level exceptions (including runtime exceptions) declared to be
thrown (checked exceptions) in some methods, in others not.
* RULE 4: Instead of throwing low level exception API designer should create exception
that is consistent with API abstraction.
In many cases such main abstraction exception don't need to be specified
in throws clausule (just throw it).
Example:
package org.jboss.bootstrap.api.factory;
|
| public class ServerFactory
| {
| ...
| public static Server<?, ?> createServer(final String implClassName)
| throws IllegalArgumentException, InstantiationException; // this is WRONG, see RULE
4
|
| public static Server<?, ?> createServer(final String implClassName, final
ClassLoader cl)
| throws IllegalArgumentException, InstantiationException // this is WRONG, see RULE
4
| ...
| }
|
| public class ServerFactory
| {
| ...
| public static Server<?, ?> createServer(final String implClassName) // this
is CORRECT
| {
| ...
| catch (InstantiationException ie)
| {
| throws BootstrapException(ie);
| }
| ...
| }
|
| public static Server<?, ?> createServer(final String implClassName, final
ClassLoader cl) // this is CORRECT
| {
| ...
| catch (IllegalArgumentException ie)
| {
| throws BootstrapException(ie);
| }
| ...
| }
| }
|
SUGGESTION: Create abstract exception that best fulfills the API abstraction and consider
if it will be thrown from the API or not.
* should be declared to be thrown from API if it is not assignable from
java.lang.RuntimeException and user have to deal with it
* shouldn't be declared to be thrown from API if it is assignable from
java.lang.RuntimeException and represents programmer error
Example:
* BootstrapException extends RuntimeException // defining API abstraction exception -
it's Throwable getCause() returns the original exception that caused the failure
* LifecycleEventException extends BootstrapException // all exceptions defined in
embedded API should extend main abstraction exception
OTHER CODE INCONSISTENCIES
* ISSUE 5: There are many inconsistencies in embedded API cross different components.
* RULE 5: Avoid API inconsistencies. They usually indicate wrong design or decomposition
of the problem.
Example:
package org.jboss.bootstrap.api.mc.server;
|
| public interface MCBasedServer
| <
| K extends org.jboss.bootstrap.api.server.Server<K, T>,
| T extends org.jboss.bootstrap.api.config.ServerConfig<T>
| >
| extends org.jboss.bootstrap.api.server.Server<K, T>
| {
| org.jboss.kernel.Kernel getKernel();
| }
|
| but
|
| package org.jboss.bootstrap.api.as.server;
|
| public interface JBossASBasedServer
| <
| K extends JBossASBasedServer<K, T>, // this is WRONG: shouldn't it be K
extends org.jboss.bootstrap.api.server.Server<K, T> ?
| T extends org.jboss.bootstrap.api.as.config.JBossASBasedServerConfig<T> //
this is WRONG: should be T extends org.jboss.bootstrap.api.config.ServerConfig<T> ?
| >
| extends org.jboss.bootstrap.api.server.Server<K, T>
| {
| }
View the original post :
http://www.jboss.org/index.html?module=bb&op=viewtopic&p=4268805#...
Reply to the post :
http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&a...