JBoss Design Flaws

[mazeiar]

Hi,

I?m a PhD student and recently I?m working on detecting design flaws from the source code. I want to propose a framework, containing a design knowledge-base and a classifier, to automatically detect design flaws in the source code of object-oriented systems. I performed two sets of measurement on JBoss Application Server v. 4.0.0 and now I want to evaluate my work. I really need feedback from designers, developers and experts who know the JBoss design. I?ve selected two design flaws at the class level, namely ?God Class? and ?Shotgun Surgery (see the end of this post). I want to know are these classes really problematic? I mean, it was a design decision to have these god classes and shotgun surgery or it?s a flaw which should be refactored in the future? For instance, if a class has shotgun surgery, for the next releases it may make problems for maintenance and change. So, have these classes made problems in the releases after 4.0.0?

For your information, I list definitions of these design flaws:

Design flaw (also known as ?code smell? or ?rotting design?) is any violation of design principles and heuristics. (You can find more info in Martin Fowler?s book on Refactoring)

?God class?: A god class is a class that does most of the functionalities of a system in the comparison with other classes in the system. In a nutshell, god Class refers to those classes which tend to centralize the intelligence of the system. An instance of a god class performs most of the operations, delegates only minor details to a set of trivial classes, and uses the data from other classes. This design flaw is partially analogous to Fowler?s Large Class smell. Hence, god classes deviate from the heuristic of manageable complexity, low coupling as well as tend to be also non-cohesive.


?Shotgun Surgery?: A class that is coupled to a large number of other classes, and would produce a large number of changes throughout the system in the event of an internal change, contributes to the Shotgun Surgery smell. Shotgun Surgery means that a change in a given class implies many (small) changes to a lot of different classes. By the definition, a class which presents the shotgun surgery flaw tends to be coupled to a large number of other classes. In a nutshell, shotgun surgery deviates from the heuristic of minimization the number of messages in the protocol of the class.

References:

M. Fowler, K. Beck, J. Brant, W. Opdyke, and D. Roberts. Refactoring : Improving the Design of Existing Code. Addison-Wesley, 1999.

A. J. Riel. Object-Oriented Design Heuristics. Addison-Wesley, 1996.

Measurement #1

God Class Design Flaw
Class names
iiop.CDRStream
iiop.InterfaceRepository
jaxrpc.Call
jaxrpc.SOAPMonitorApplet.SOAPMonitorPage
jaxrpc.SerializationContextImpl
server.BasicVisitor
server.EJBQLToSQL92Compiler
server.JDBCEJBQLCompiler
server.JDBCUtil
server.MarshalledInvocation
server.ProxyAssembler
webservice.SOAPMonitorApplet.SOAPMonitorPage
jmx.org.jboss.mx.capability.OptimizedMBeanDispatcher



Shotgun Surgery Design Flaw
Class Names
cache.Fqn
cache.TreeCache
cluster.HAPartition
connector.JmsMessage
connector.WrappedStatement
jaxrpc.AxisFault
jaxrpc.BasicHandler
jaxrpc.Constants
jaxrpc.DeserializationContext
jaxrpc.EngineConfiguration
jaxrpc.JavaUtils
jaxrpc.MessageContext
jaxrpc.Messages
jaxrpc.OperationDesc
jaxrpc.SOAPConstants
jaxrpc.SOAPElementAxisImpl
jaxrpc.SerializationContext
jaxrpc.SymTabEntry
management.J2EEManagedObject
management.J2EETypeConstants
server.AbstractInterceptor
server.AllowedOperationsAssociation
server.AllowedOperationsFlags
server.BeanMetaData
server.Container
server.EnterpriseContext
server.EntityContainer
server.EntityEnterpriseContext
server.Interceptor
server.Invocation
server.JDBCCMRFieldBridge
server.JDBCEntityBridge
server.JDBCEntityMetaData
server.JDBCFieldBridge
server.JDBCUtil
server.MetaData
server.SQLUtil
aop.org.jboss.aop.Advisor
aop.org.jboss.aop.AspectManager
aop.org.jboss.aop.joinpoint.Invocation
aop.org.jboss.aop.joinpoint.InvocationBase
aop.org.jboss.aop.pointcut.MatcherHelper
aop.org.jboss.aop.pointcut.ast.Node
aop.org.jboss.aop.pointcut.ast.PointcutExpressionParserVisitor
aop.org.jboss.aop.pointcut.ast.SimpleNode
aspects.org.jboss.aspects.versioned.StateManager
jms.org.jboss.jms.serverless.NotImplementedException
jmx.javax.management.Descriptor
jmx.javax.management.MBeanFeatureInfo
jmx.javax.management.MBeanServer
jmx.javax.management.MBeanServerConnection
jmx.javax.management.ObjectName
jmx.javax.management.RuntimeOperationsException
jmx.org.jboss.mx.modelmbean.ModelMBeanConstants
messaging.org.jboss.mq.Connection
messaging.org.jboss.mq.SpyJMSException
messaging.org.jboss.mq.SpyMessage
messaging.org.jboss.mq.SpyMessage.Header
messaging.org.jboss.mq.il.Invoker
messaging.org.jboss.mq.il.ServerIL
messaging.org.jboss.mq.il.uil2.msgs.BaseMsg
messaging.org.jboss.mq.server.BasicQueue
messaging.org.jboss.mq.server.JMSServerInterceptor
messaging.org.jboss.mq.server.JMSServerInterceptorSupport
system.org.jboss.system.ServiceMBeanSupport

Measurement #2

God Class Design Flaw
Class names
iiop.CDRStream
iiop.InterfaceRepository
jaxrpc.Call
jaxrpc.SOAPMonitorApplet.SOAPMonitorPage
jaxrpc.SerializationContextImpl
server.BasicVisitor
server.EJBQLToSQL92Compiler
server.JDBCEJBQLCompiler
server.JDBCUtil
server.MarshalledInvocation
server.ProxyAssembler
webservice.SOAPMonitorApplet.SOAPMonitorPage
jmx.org.jboss.mx.capability.OptimizedMBeanDispatcher



Shotgun Surgery Design Flaw
Class Names
AnnotationParserVisitor.java /aop/src/main/org/jboss/aop/annotation/ast
AspectDefinition.java /aop/src/main/org/jboss/aop/advice
AspectFactory.java /aop/src/main/org/jboss/aop/advice
AspectManager.java /aop/src/main/org/jboss/aop
BaseConnectionManager2.java /connector/src/main/org/jboss/resource/connectionmanager
ConnectionListener.java /connector/src/main/org/jboss/resource/connectionmanager
ConstructorMatcher.java /aop/src/main/org/jboss/aop/pointcut
DistributedState.java /aspects/src/main/org/jboss/aspects/versioned
DistributedVersionManager.java /aspects/src/main/org/jboss/aspects/versioned
FamilyClusterInfo.java /cluster/src/main/org/jboss/ha/framework/interfaces
Instrumentor.java /aop/src/main/org/jboss/aop/instrument
Interceptor.java /aop/src/main/org/jboss/aop/advice
Interceptor.java /cache/src/main/org/jboss/cache/interceptors
Invocation.java /aop/src/main/org/jboss/aop/joinpoint
InvocationBase.java /aop/src/main/org/jboss/aop/joinpoint
JMSException.java /j2ee/src/main/javax/jms
JmsMessage.java /connector/src/main/org/jboss/resource/adapter/jms
LockStrategy.java /cache/src/main/org/jboss/cache/lock
MatcherHelper.java /aop/src/main/org/jboss/aop/pointcut
MetaDataResolver.java /aop/src/main/org/jboss/aop/metadata
MethodMatcher.java /aop/src/main/org/jboss/aop/pointcut
MuNumber.java /common/src/main/org/jboss/util
Mutable.java /common/src/main/org/jboss/util
NestedThrowable.java /common/src/main/org/jboss/util
NestedThrowable.java /common/src/main/org/jboss/util
Node.java /aop/src/main/org/jboss/aop/annotation/ast
Node.java /aop/src/main/org/jboss/aop/pointcut/ast
Pointcut.java /aop/src/main/org/jboss/aop/pointcut
PointcutExpressionParserVisitor.java /aop/src/main/org/jboss/aop/pointcut/ast
Region.java /cache/src/main/org/jboss/cache/eviction
SimpleMetaData.java /aop/src/main/org/jboss/aop/metadata
SimpleNode.java /aop/src/main/org/jboss/aop/pointcut/ast
SimpleNode.java /aop/src/main/org/jboss/aop/annotation/ast
StateManager.java /aspects/src/main/org/jboss/aspects/versioned
SynchronizationManager.java /aspects/src/main/org/jboss/aspects/versioned
TreeCache.java /cache/src/main/org/jboss/cache
TreeCacheAop.java /cache/src/main/org/jboss/cache/aop
TreeCacheAopView.java /cache/src/main/org/jboss/cache/aop
TreeCacheView.java /cache/src/main/org/jboss/cache
TreeCacheView2.java /cache/src/main/org/jboss/cache
TxSupport.java /aspects/src/main/org/jboss/aspects/tx
Util.java /aop/src/main/org/jboss/aop/pointcut
VersionManager.java /aspects/src/main/org/jboss/aspects/versioned
VersionReference.java /aspects/src/main/org/jboss/aspects/versioned
AbstractInterceptor.java /jmx/src/main/org/jboss/mx/interceptor
BaseDeserializerFactory.java /jaxrpc/src/main/org/apache/axis/encoding/ser
BaseSerializerFactory.java /jaxrpc/src/main/org/apache/axis/encoding/ser
BasicHandler.java /jaxrpc/src/main/org/apache/axis/handlers
Emitter.java /jaxrpc/src/main/org/apache/axis/wsdl/toJava
Enum.java /jaxrpc/src/main/org/apache/axis/enum
Interceptor.java /jmx/src/main/org/jboss/mx/interceptor
J2EEDeployedObject.java /management/src/main/org/jboss/management/j2ee
J2EEDomain.java /management/src/main/org/jboss/management/j2ee
J2EEManagedObject.java /management/src/main/org/jboss/management/j2ee
J2EETypeConstants.java /management/src/main/org/jboss/management/j2ee
JavaClassWriter.java /jaxrpc/src/main/org/apache/axis/wsdl/toJava
JavaWriter.java /jaxrpc/src/main/org/apache/axis/wsdl/toJava
LoaderRepository.java /jmx/src/main/org/jboss/mx/loading
ManagedObjectFactory.java /management/src/main/org/jboss/management/j2ee/factory
MBeanFeatureInfo.java /jmx/src/main/javax/management
MBeanServerConnection.java /jmx/src/main/javax/management
Messages.java /jaxrpc/src/main/org/apache/axis/utils
Monitor.java /jmx/src/main/javax/management/monitor
NodeImpl.java /jaxrpc/src/main/org/apache/axis/message
NormalizedString.java /jaxrpc/src/main/org/apache/axis/types
NotificationConstants.java /management/src/main/org/jboss/management/j2ee
NotImplementedException.java /jms/src/main/org/jboss/jms/serverless
OpenType.java /jmx/src/main/javax/management/openmbean
OperationsException.java /jmx/src/main/javax/management
RelationException.java /jmx/src/main/javax/management/relation
RepositoryClassLoader.java /jmx/src/main/org/jboss/mx/loading
SimpleDeserializer.java /jaxrpc/src/main/org/apache/axis/encoding/ser
SOAPElementAxisImpl.java /jaxrpc/src/main/org/apache/axis/message
SOAPElementImpl.java /jaxrpc/src/main/org/apache/axis/message
SOAPEnvelopeAxisImpl.java /jaxrpc/src/main/org/apache/axis/message
SOAPFaultImpl.java /jaxrpc/src/main/org/apache/axis/message
SOAPHandler.java /jaxrpc/src/main/org/apache/axis/message
StateManagement.java /management/src/main/org/jboss/management/j2ee
StatisticImpl.java /management/src/main/org/jboss/management/j2ee/statistics
StatsBase.java /management/src/main/org/jboss/management/j2ee/statistics
SymTabEntry.java /jaxrpc/src/main/org/apache/axis/wsdl/symbolTable
tcpmon.java /jaxrpc/src/main/org/apache/axis/utils
TypeEntry.java /jaxrpc/src/main/org/apache/axis/wsdl/symbolTable
Utils.java /jaxrpc/src/main/org/apache/axis/wsdl/toJava
ValueExp.java /jmx/src/main/javax/management
WSDDConstants.java /jaxrpc/src/main/org/apache/axis/deployment/wsdd
WSDDDeployableItem.java /jaxrpc/src/main/org/apache/axis/deployment/wsdd
WSDDDeployment.java /jaxrpc/src/main/org/apache/axis/deployment/wsdd
WSDDElement.java /jaxrpc/src/main/org/apache/axis/deployment/wsdd
WSDDHandler.java /jaxrpc/src/main/org/apache/axis/deployment/wsdd
AbstractDeploymentScanner.java /system/src/main/org/jboss/deployment/scanner
AbstractInvoker.java /remoting/src/main/org/jboss/remoting
AbstractServerLoginModule.java /security/src/main/org/jboss/security/auth/spi
BaseMsg.java /messaging/src/main/org/jboss/mq/il/uil2/msgs
BasicQueue.java /messaging/src/main/org/jboss/mq/server
Connection.java /messaging/src/main/org/jboss/mq
DeploymentException.java /system/src/main/org/jboss/deployment
DeploymentInfo.java /system/src/main/org/jboss/deployment
InvocationRequest.java /remoting/src/main/org/jboss/remoting
JMSDestinationManager.java /messaging/src/main/org/jboss/mq/server
JMSServerInterceptor.java /messaging/src/main/org/jboss/mq/server
MessagePool.java /messaging/src/main/org/jboss/mq
MessageReference.java /messaging/src/main/org/jboss/mq/server
MsgTypes.java /messaging/src/main/org/jboss/mq/il/uil2/msgs
OIL2Request.java /messaging/src/main/org/jboss/mq/il/oil2
OIL2SocketHandler.java /messaging/src/main/org/jboss/mq/il/oil2
SocketManager.java /messaging/src/main/org/jboss/mq/il/uil2
SpyJMSException.java /messaging/src/main/org/jboss/mq
SpyMessage.java /messaging/src/main/org/jboss/mq
SpyMessage.java /messaging/src/main/org/jboss/mq
SpySession.java /messaging/src/main/org/jboss/mq
SubDeployer.java /system/src/main/org/jboss/deployment
Subscription.java /messaging/src/main/org/jboss/mq
UsernamePasswordLoginModule.java /security/src/main/org/jboss/security/auth/spi

[acoliver]

Some of these I laugh and say "haha...you got it". Others its like "the spec this came from makes it that way or its a simplification" and some are like "who cares?" ... Particularly funny is so much of JBossMQ (which we're rewriting) and TreeCache (you should open the file).

However, I might suggest a different approach than listing these and just asking the developers in a forum. One no one is probably as interested in your research as you are (objective mesurement for a subjective issue) and there are social aspects that make honest replies likely to be spartan and controversial. Moreover, what is in it for us really? Consider offering to fix one or two or getting involved such that you say "I think XYZ should be redesigned" and why. Then based on the discussion and reaction you your patches you'll get concrete information.. However you just asked busy people interested in other things to do your work for you... Good book from harvard uni press: http://www.amazon.com/gp/product/0674012925/002-6943602-5605610?v=glance&n=283155

[adrian.brock]

"mazeiar" wrote:

?Shotgun Surgery?: A class that is coupled to a large number of other classes

I think there is something wrong with your analysis when you cannot differentiate
a contract (spec or otherwise) from this "design flaw".

Contracts don't change.

I do agree on some of the JBossMQ implementation, e.g. BasicQueue.
In fact, you can find my complaints about this class all over the forums. ;-)

Strangely, you didn't pick on a worse class
org.jboss.ejb.plugins.jms.JMSContainerInvoker
I don't know how many design flaws are involved and its related classes, but it is lots. ;-)
That doesn't mean it doesn't work, it just means it is hard to change.

[mazeiar]

The analysis is only based on the source code, so I agree with you that it cannot discriminate design decisions/contracts from design flaws. Actually, this is why I post this message to get feedback from developers. I'm thinking about using other design artifacts to help classifiers but as you know these information are still rarely structured and machine-readable. For the case of model-driven approaches, UML-based design documents and system architecture can be useful, but for JBoss I don't have access to these documents.

In fact, I started my work by architectural recovery to extract the concrete architecture, as implemented, and find the differences (deviations) with abstract architecture, as designed. But I was looking for an objective measurement for finding design flaws and this was the main casue of trying to analyze the source code by well-known OO metrics.

I use a two-level filter (classifier) to detect design flaws, in this case only two flaws. For org.jboss.ejb.plugins.jms.JMSContainerInvoker my measurements show it has been detected by the first level filter as a hotspot and potential flaw (due to highly coupled with other classes) but it's been rejected by the second filter (for shotgun surgery). The reason is that for this flaw only coupling with other classes is not enough, but the fan-in is very important. It means how much the other classes depend on this class which may lead to maintenance/evolution problems in the future in case of change in this class.

Thank you for your reply.

"mazeiar" wrote:

?Shotgun Surgery?: A class that is coupled to a large number of other classes

I think there is something wrong with your analysis when you cannot differentiate
a contract (spec or otherwise) from this "design flaw".

Contracts don't change.

I do agree on some of the JBossMQ implementation, e.g. BasicQueue.
In fact, you can find my complaints about this class all over the forums. ;-)

Strangely, you didn't pick on a worse class
org.jboss.ejb.plugins.jms.JMSContainerInvoker
I don't know how many design flaws are involved and its related classes, but it is lots. ;-)
That doesn't mean it doesn't work, it just means it is hard to change.