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...