<< May 2009 | Home | July 2009 >>

The Key to Maintenance is Compartmentalisation

Over the last couple of large projects I have noticed a trend in the way software developers/engineers/architects like to come out with statements like "that code is shit". I'm not just talking about me saying it, or how they react to the code that I write (come on, I write great code!). But general sweeping statements like this crop up all the time on these projects, related to everyones code. And managers then perpetuate these statements.

I think the reason is easy to understand. When a developer takes over responsibility for some code they want to make it clear that any problems with that code are unrelated to them. They don't want to take responsibility for the code which someone else has written. Because writing code is inherently creative and the same functionality can be written many different ways, you are almost guaranteed that a developer taking over some code "would have written it differently".

What I have also noticed is that the developers coming out with statements like this do not know or do not want to understand the conditions under which the code was originally written. As an architect, I realise there are many many factors which influence the creation of code, budget being a major one. What some developers also do not fully appreciate is that the perfect implementation not only doesn't exist (because every developer would do it differently) but that it often isn't required. The perfect solution considers many cases which don't need to be programmed, as they "may" be required in the future, but currently are not. From a financial point of view it is possible to determine whether or not to implement functionality in software (see my previous posting). So from that basis, a developer should work to a simple 80/20 rule and implement something that will suffice, as opposed to something that will exist for the next quarter of a century, unchanged because of its greatness.

Often developers which make statements that the code they are inheriting is poor, want to refactor it. They give reasons that the enlightened manager/team-leader/developer understands that refactoring is part and parcel of modern coding. Or perhaps they give reasons like without refactoring or rewriting, the code is unmaintainable. But is that really true? Does experience show that once a project has gone into production that there are many bugs or many change requests? During maintenance what proportion of code will ever actually be changed because of bugs or change requests? The project I am working on now has some code it in which we have not touched in 18 months (since going into production). The code works, although today we do not fully understand that code, but importantly we have its source code! This code is without a doubt poor. It has relatively little documentation. We have a list of all changes that are likely to come in the next years (perhaps 5 years ahead), none of which a related to this code. The intended lifespan of the code is at least another 20 years. After 18 months in production there have been zero reported bugs related to this code. So should we refactor in order to be able to optimise the code and make it a nicer design so that we understand it today? I am of the opinion that we should not. Doing so would not add any value.

Indeed does a developer need to fully understand the code in order to be capable of maintaining it? I am of the opinion (and please feel free to disagree) that a good developer doing maintenance should be capable of maintaining code that they currently do not fully understand and which has been implemented using different styles/patterns/techniques than they would. In fact I would go as far as saying it is imperative for a maintenance developer to be able to do this. The reason is simple: to keep maintenance budget to a minimum. At the time of going to production, you almost certainly do not know the areas of code which will need maintaining due to bugs. You may be aware of future change requests or further project phases where changes will be made. The result is that a maintenance developer should not start refactoring code just to make it "more correct", unless that developer intends to support the software for the rest of its life. If that is not the case (and in my experience most developers move on to different projects within at least three years), then changing the code is useless as the next maintenance developer to come along will simply claim that the code is unmaintainable and poorly implemented and want to start the refactoring all over again.

The skill of being able to inherit code and appreciating that at that time you do not need to fully understand the code is in my opinion extremely important for reducing maintenance costs. The key is "compartmentalisation". A typical example in maintenance is that you will often come across code which is duplicated. The gut reaction is to refactor it because a "law" of OO is to have code reuse. How often though does it happen that during the refactoring you realise that getting the common code out is painful and not optimal? Often. You might even end up having to build in a work around (guaranteed to cause the next maintenance developer to claim you wrote crap code). And certainly, such refactoring may introduce new bugs, requiring it to be fully regression tested.

The solution then, is to "compartmentalise" the code. The maintenance developer should become at ease with the situation and make a mental note (or even in a erm... document) that this situation exists. Without knowing whether this code has a problem with bugs and without being able to anticipate any changes which may come, you cannot guarantee that the changes you make during refactoring will be optimal. However, by understanding such code and having several related "compartments" you can plan correct refactoring, should a bug or change come your way which affects that code.

What I am about to write now is controversial, but please read it with an open mind. If you make the assumption that your maintenance developers can compartmentalise and live with code duplication, then during the project phase where you implement your code, you can use compartmentalisation to your advantage: parallel development. If you have two developers implementing similar things and you get them to work together to create a single design, it will cost you more money. Firstly, because of the communication required (the positive communication as well as the negative whereby they disagree and spend time comprimising). Secondly because while one does the implementation of the common code, the other will be potentially idle. Thirdly, a common design may not be an optimum because of comprimise in order to make it fit both/all situations.

Hey, if worst comes to worst, at least if a bug turns up in on half of some duplicated code, the users will only see it once and not in every instance of some commonly used core code. There is a rumour in our office that aeronautical systems are developed parallel in duplicate by two teams who do not communicate. This ensures that critical systems can continue to run if a bug crops up in one of them... Of course you would actually need three such systems in order to be able to determine which one is actually displaying the bug.

Anyway, if you have got this far without claiming I am a nut case then congratulations for having an open mind. However please realise I am not advocating code duplication for the sake of it. As a designer or architect you should be in a position to determine where code duplication might occur and to plan it sufficiently correctly so that your team can work efficiently and use common components as required. But if your plan cannot efficiently include such a common component, or the complexity of it means that at the planning stage you are not able to determine the usefulness of common code, then don't get caught up in the "code reuse is imperative" mantra that OO brought us. When OO was invented, the idea of code reuse was first at the method level - OO empowered programmers to reuse functions very easily. Making a law that code duplication is illegal simpy constrains management/architects/designers from using their brain to consider the options (and be creative, which after all puts the fun back into software development).

Copyright (c) 2009 Ant Kutschera

Social Bookmarks :  Add this post to Slashdot    Add this post to Digg    Add this post to Reddit    Add this post to Delicious    Add this post to Stumble it    Add this post to Google    Add this post to Technorati    Add this post to Bloglines    Add this post to Facebook    Add this post to Furl    Add this post to Windows Live    Add this post to Yahoo!

Java EE Security and Spring

Spring Security (originally ACEGI) does not seem to work out of the box with Java EE security (see here). I guess the reason is clear, namely that the people from Spring don't want you to be tied into Java EE and that makes sense because Spring something that you can use without Java EE. But instead of having to learn something new and instead of having to configure my security in a non-standard way, I wanted a way to ensure my services are only called by someone with the right authorization, exactly when they are deployed in a Java EE environment.

Since a standard web application running in a Java EE container can be configured to have security, I expected to be able to configure Spring to use the web request to check the Principal for its security roles before calling a service, but that isn't possible. So, I set to work to quickly create a little library to help me out.

The first step was to implement a Filter in the web application so that it could intercept every call and insert the web request (containing the Principal) into a ThreadLocal variable. The idea was that the ThreadLocal variable could then be used further up the call stack to query the security roles of the authenticated user. The filter implementation looks like this:

    public void doFilter(
            ServletRequest request,
            ServletResponse response,
            FilterChain chain)

            throws IOException, ServletException { 

        //set the request in the security thread local
        SecurityHelper.set((HttpServletRequest)request);

        chain.doFilter(request, response);

        //all done, so remove references to unwanted objects

        SecurityHelper.finished();
    }


The filter is configured in the Java EE web application's web.xml like this:

    <filter>
        <filter-name>maxantSpringSecurity</filter-name>
        <filter-class>
            uk.co.maxant.spring.security.SecurityFilter
        </filter-class>
    </filter>
   
    <filter-mapping>
        <filter-name>maxantSpringSecurity</filter-name>
        <url-pattern>/*</url-pattern>
    </filter-mapping>

The filter calls the SecurityHelper which uses a ThreadLocal variable to store the web request as follows. The web request can be used to gain access to the Principal, as well as ask if the Principal has a given role.

    private static ThreadLocal tls = new ThreadLocal();

    public static void set(HttpServletRequest request){
        tls.set(request);
    }

    public static boolean isInRole(String role){
        HttpServletRequest req = tls.get();
        if(req == null){
            return false;
        }else{
            return req.isUserInRole(role);
        }
    }


The Security Helper also contains an additional method which can be called to release the reference to the request. This is important because the web request contains a reference to the session and that might be large. We don't want to be holding on to such things for a long time as they may use memory unncessarily. This method can is called as soon as all security queries are finished, and is implemented as follows. For more information on thread locals and threads that are stored in pools, see here.

    public static void finished(){
        tls.remove();
    }


The final step was to configure my Spring configuration to setup an aspect to intercept all service calls. The advice which this aspect gives is whether the current user has the role required to call the method. The required role comes from an annotation added to the service methods and the current user comes from the Principal stored in the ThreadLocal from step one, above. If the user has rights, the service method is called. If not, a simple SecurityException is thrown, providing details about the missing roles. In cases where the user is not logged in, the Security Exception is also thrown. In cases where a Service method has no annotation, then no security is checked. The aspect is configured in the Spring configuration like this:  

    <!-- security aspect -->
    <bean id="security"
        class="uk.co.maxant.spring.security.SecurityAspect">
        <!-- execute before the transactional advice (hence the lower order number)
            but after profiling -->
        <property name="order" value="2" />
    </bean>

    <aop:config>
        <!-- this advice will execute around the transactional advice -->
        <aop:aspect id="securityAspect" ref="security">
            <aop:pointcut id="serviceMethod"
                expression="execution(* uk.co.maxant.gwtdemo16.server.*Service.*(..))" />
            <aop:around method="checkSecurity"
                pointcut-ref="serviceMethod" />
        </aop:aspect>
    </aop:config>

The expression in the above pointcut needs to be configured to match your package and Service naming conventions. 

The aspect itself is implemented like this:

    public Object checkSecurity(ProceedingJoinPoint call)
            throws Throwable {

        String[] requiredRoles = null;
        MethodSignature ms =
                       (MethodSignature)call.getSignature();
        Method m = ms.getMethod(); //lets check it for the
                                   // RolesAllowed annotation
        RolesAllowed roles = m.getAnnotation(RolesAllowed.class);
        if(roles != null){
            requiredRoles = roles.value();
            for(String role : requiredRoles){
                if(SecurityHelper.isInRole(role)){
                    //ok, no problem, lets continue
                    //with the call
                    requiredRoles = null; //since we can now
                                          // carry out the call
                    break;
                }
            }
        }
   
        if(requiredRoles == null){
            Object returnValue = call.proceed();
            return returnValue;
        }else{
            String rrs = "[";
            for(String s : requiredRoles){
                rrs += s + ", ";
            }
            rrs = rrs.substring(0, rrs.length() - 2) + "]";
           
            throw new SecurityException("To call [" +
                call.getSignature().toLongString() +
                "] the user must be authenticated and " +
                "authorized in these roles: " + rrs);
        }
    }
 

The annotation is defined as such:

    @Retention(value=RetentionPolicy.RUNTIME)
    @Target(value={ElementType.METHOD})
    public @interface RolesAllowed {
        String[] value();
    }


The annotation is used like this, on all service methods requiring security:

    @RolesAllowed ("registered") 

 
What I now have is a simple library which I can configure using a Filter and an Aspect. Additionally to allow the user to log in in the first place, I can use a standard Java EE security constraint in my web application. The standard steps to use this security extension of Spring would be:

  • Add a security constraint to the web application
  • Add the annotation to the service methods requiring security
  • Add the filter to the web application
  • Add the aspect to the Spring configuration
  • Add the library to the classpath

I understand that Spring does not want you to need a dependency on Java EE, but on the other hand if I have that dependency due to other requirements, I do not want to have to configure my application using a third party security concept (Spring) as opposed to a standard one (Java EE). Spring Security is not even part of Standard Spring - it is downloaded seperately. The more extensions you use, the more skills your developers need. The maxant Spring Security extension provides a very simple way to secure your service methods.

Download the library including source code here.

Copyright (c) 2009 Ant Kutschera

 

Social Bookmarks :  Add this post to Slashdot    Add this post to Digg    Add this post to Reddit    Add this post to Delicious    Add this post to Stumble it    Add this post to Google    Add this post to Technorati    Add this post to Bloglines    Add this post to Facebook    Add this post to Furl    Add this post to Windows Live    Add this post to Yahoo!