Act Like a Senior Developer – About Clean Code

November 25th, 2009

Today it was a hard day for me. As usual, I was looking at a mess around our code base. And the worst thing was that the code that I saw today was not a legacy code :S It has enough test coverage and moreover the system was working well. The problem was the conditional logics used all over the place. It is almost impossible to follow what’s happening and where :S Me and my colleague spent 2 whole days trying to understand what the heck was going on. Finally we figured it out and did what we wanted to do initially.

Here, I wanna just stop and speak loudly to all the developers that call themselves senior or EDIT: principal or if (experience != Experiences.JUNIOR):

People, what’s wrong with us? How come newly implemented code base is turning into a mess this fast? How come we’re allowing such a mess when we see it slowly approaching us like a monster? But of course, when it comes to the interviews, we have to ask candidates questions around design patterns, real-time problem solving skills, test driven design, and refactoring. And seriously is this why we were asking these questions? I’m so emberassed and ashamed of being a senior guy today.

Anyways, long story short, here is a 5 minute crash course about what we forget to foresee:

1. Evil Ifs

As a senior guy, you have to be aware of ifs are evil in most cases. For those who forgot, here is a quick do not forget list:

  • ALLOW THEM WHEN:

Condition can be expressed without AND, OR operators: i.e

if (myContainer.hasItems()) {
    //...
}

Condition is about an encapsulated field: i.e

if (_isVisible) {
    //...
}

Condition is non-float numerical comparison: i.e

if (myCertificates.count() > 3) {
    //...
}

  • BE CAUTIOUS:

When there is one or two AND, OR operators: i.e

if (_isVisible && !isParent) {
    //...
}
// SOLUTION: refactor (extract method, extract local variable, etc)

When there is negative logic: i.e

if (!_iAmSenior) {
    //...
}
// SOLUTION: refactor (make it positive like iAmJunior; 
// positive statements are easy to understand)

When there is a very long condition: i.e

if (myAwesomeServiceImpl.findExtremelyImportantSomething()
                            .equals(myLocalExtremelyImportantThing)) {
    //...
}
// SOLUTION: refactor (make it short or extract local variable, 
// trust me your application is not at the point where you should 
// worry about memory or CPU cycles)

When there is a if/else statement: i.e

if (_isVisible) {
    number = getNumberFromMoon();
} else {
    number = 1;
}
// this creates more than 2 execution paths within one method, 
// make sure you force all the possibilities to block above logic
// SOLUTION: get rid of the second part if possible as in:



number = 1;
if (_isVisible) {
    number = getNumberFromMoon();
}
// if it is not this simple, then make sure you created 
// two different executions paths (with different 
// methods/classes) before coming to this point

  • DON’T ALLOW PLEASE:

When it is a null check:

if (item != null) {
    // do something...
}
// SOLUTION: find the cause of null and eliminate it.

For more information on this topic, please read my other post about null pointer exceptions.

When there is more than two AND, OR, NOT operators: i.e

if ((_isVisible && !isParent) || iAmMaster || youCanPassIsSet) {
    // do something...
}
// SOLUTION: For God's sake! you have to refactor this code. 
// The least you can do is extracting a method out of it 
// if you don't know any design patterns to apply. Check below:



if (isValid()) {
    // do something...
}

When there is nested if/else statement: i.e

if (isThisTrue) {
    if (checkIfHeIsTheMaster() && !hmmmThisSeemsWeird()) {
        // do something...
    } else {
        // do something else...
    }
} else {
    // this is just ugly...
}
// SOLUTION: Use well known OOP technique called POLYMORPHISM, 
// or if the operator is one of (>, <, ==) then various possible 
// design patterns like Strategy (behavior modification), 
// Visitor (context evaluation) or Specification (item selection). 
// If you're not happy, but you don't know what to do then talk 
// to someone you think he/she might know.

Also if you are the maintainer of the code base then you might wanna introduce static code analyzers like PMD. They can easily detect cyclomatic complexities.

Anyways, you guys can easily figure it out. You are welcome to introduce new ones. These are just some that I can think of in the middle of the night.

2. Stupid Loops

Folks, if you’re introducing loops to your logic, please make sure that you really need them. Please check the code snippets and the solutions below; I don’t wanna comment more about this:

for (int i=0; i < 2; i++) {
    if (i == 0) {
        doThis();
    } else if (i == 1) {
        doThat();
    } 
}
// SOLUTION: how about this? You don't need loop there :S
doThis();
doThat();

Another one:

for (int i=0; i < items.size(); i++) {
    if (!isVisible(items.get(i))) {
        continue;
    }
    doThis(); 
}
// SOLUTION: how about this? I stil don't know why you need continue there?
for (int i=0; i < items.size(); i++) {
    if (isVisible(items.get(i))) {
        doThis(); 
    }
}

One more:

item = null;
for (int i=0; i < items.size(); i++) {
    if (isVisible(items.get(i))) {
        item = items.get(i);
    }
}
return item;
// SOLUTION: how about this? returning values directly?
for (int i=0; i < items.size(); i++) {
    if (isVisible(items.get(i))) { // EDIT from user comments
        return items.get(i);
    }
}
return null; // if possible even return new NullItem()

3. Dependency Injections

Woov, this one is an advanced topic ah? Everyone loves dependency injection. Spring sort of frameworks are even forcing you to do it this way right? Let me tell you something; I believe after go-to statements, this one is the most misinterpreted topic in programming history. We senior guys somehow convinced ourselves to something like this: If you can inject it’s cost free! Man… just don’t do this please. Everything has a cost. This is just a way to ease/loose your tight connections with your dependencies. Don’t try to interpret otherwise.

And second thing is the famous setter injection. So if you have a setter injection set up there, it means don’t worry. It’s like we can’t think otherwise :S Opppss. Stop here! It is not like that at all if you still couldn’t figure it out. Setter injection and Constructor injection are 2 well known ways of doing this (there are other ways too). Each of them has its own advantages/disadvantages. However, let’s be honest. Constructor injection is much more declarative than setter injection.

Anyways, it’a very big debate. I’m kinda scared :P People are gonna offend me again. I just wanna say that if possible please try to use constructor injection in your custom code. If you don’t believe me, then refer to Martin Fowler’s IOC Article or Misko Hevery’s Clean Code Talks at #Google, such as: Don’t Look For Things.

4. So Many Dependencies

Just a tiny comment here. Folks, try reading #SOLID principles, ok. DON’T CREATE this much dependency :S The class itself is screaming at you: “I can’t carry this much!”, “You won’t be able to test this”, “I’ve more than 1000 lines”, “You can’t use constructor injection anymore!”

Bottom line, if you have so many dependencies, please try to find a way to separate concerns.

5. So Many Parameters

You have more than 3-4 arguments? Re-consider that code. Probably you don’t need that. I’m sure you can pull up something from that method or at least extract and call separately.

6. Pass What You Need

This bugs me a lot. Look at this:

calculateLoanForSomething(Account, Customer, World);



// here is the method
void calculateLoanForSomething(Account a, Customer c, World w) {
    if (w.date.time.hour == 11) {
        // do something here..
    }
}



// Why r u sending God objects everywhere :S
void calculateLoanForSomething(Account a, Customer c, int hour) { }

7. Long Classes/Methods

Here is my rule of thumb:

  • Is it more than 20 lines? THEN: Refactor
  • Is it more than 10 methods? THEN: Refactor
  • Is it more than 5 package? THEN: Refactor
  • Your colleague is coding? Please pair with him/her as much as you can especially if you think he/she is gonna introduce something bad. At least you can teach him/her or prevent things from happening.

I don’t know if you guys are agree with me or not. I was so ashamed today and wanted to write this. It was more like a self relaxation. Please don’t mind if I used a wrong sentence or anything. It’s almost 2AM and I’m so tired.

For more deeper information about these topics, please refer to Refactoring Book or site, SOLID Principles, Clean Code talks, Clean Code book.

Agile

33 Comments

True Liskov Substitution Principle (LSP)

November 18th, 2009

I know everybody is quoting from #WikiPedia or st, but I guess the true definition of this principle is:

A subtype must require no more and promise no less than its supertype.
 

Bertrand Meyer

Quotes

No Comments

Creating Custom Annotations and Using Them

October 7th, 2009

Okay, here is another topic that I couldn’t find much information in the Internet :) So I guess I’m gonna cover it quickly.

How to Create a Custom Annotations?

There are a lot of documentation about this part in the Internet. All you have to do is basically creating an annotation class like below:

public @interface Copyright {
    String info() default "";
}

And that’s it. Now it’s ready to use! Now you can put copyright information to your classes :) Since we didn’t define any @Target, you can use this annotation anywhere in your classes by default. If you want your annotation to be only available for class-wise or method-wise, you should define @Target annotation. Here is a little table of what options are available:

  • @Target(ElementType.PACKAGE), package header
  • @Target(ElementType.TYPE), class header
  • @Target(ElementType.CONSTRUCTOR), constructor header
  • @Target(ElementType.METHOD), method header
  • @Target(ElementType.FIELD), for class fields only
  • @Target(ElementType.PARAMATER), for method parameters only
  • @Target(ElementType.LOCAL_VARIABLE), for local variables only

If you want your annotation to be available in more than one place, just use array syntax as in:

@Target({ ElementType.PARAMETER, ElementType.LOCAL_VARIABLE })

One thing you may already notice is annotations are interfaces, so you don’t implement anything in them.

How to Make Use of Your Custom Annotations?

Up to here, you can find lots of examples. Okaaay, now let’s do something useful :) For instance, let’s re-implement JUnit’s @Test annotation. As you guys already know, @Test annotation is a marker annotation. Basically it marks the method as test method. If you’re expecting any exceptions, you would set expect attribute in the annotation. You can try anything here, I’m just using this example since everyone knows how @Test annotation works.

First let’s define our annotation:

@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
public @interface Test {
    Class expected();
}

You might notice that I used @Retention. This annotation marks our annotation to be retained by JVM at runtime. This will allow us to use Java reflections later on.

Now we need to write our annotation parser class. This class will parse our annotation and trigger some other invocations related to what we want. Keep in mind that if you have more than one custom annotation, then it’s also wise to have separate parsers for each annotation you define. So I’ll create one for this! The basic idea behind the annotation parser is using Java reflections to access the annotation information/attributes etc. So here is an example parser for our @Test annotation:

public class TestAnnotationParser {
    public void parse(Class<?> clazz) throws Exception {
        Method[] methods = clazz.getMethods();
        int pass = 0;
        int fail = 0;



    for (Method method : methods) {
        if (method.isAnnotationPresent(Test.class)) {
            try {
                method.invoke(null);
                pass++;
            } catch (Exception e) {
                fail++;
            }
        }
    }
}




}

That’s all you need. You parser is ready to use too. But wait a minute, we didn’t implement anything about the annotation attributes. This part is a bit tricky. Because you cannot directly access those attributes from the object graph. Luckily invocation helps us here. You can only access these attributes by invoking them. Sometimes you might need to cast the class to the annotation type too. I’m sure you’ll figure out when you see it:) Anyways here is a bit more logic to take our expected attribute into account:

// ...
// this is how you access to the attributes
Test test = method.getAnnotation(Test.class);
// we use Class type here because our attribute type
// is class. If it would be string, you'd use string
Class expected = test.expected();
try {
    method.invoke(null);
    pass++;
} catch (Exception e) {
    if (Exception.class != expected) {
        fail++;
    } else {
        pass++;
    }
}
// ...

Now everything is ready to use. Below example demonstrates how you use Parser with your test classes:

public class Demo {
    public static void main(String [] args) {
        TestAnnotationParser parser = new TestAnnotationParser();
        parser.parse(MyTest.class);
        // you can use also Class.forName 
        // to load from file system directly!
    }
}

Yeah, I hope you enjoyed. Don’t hesitate to put some comments down if you’ve a better approach? Thanks! Here is the full parser class implementation:

public class TestAnnotationParser {
    public void parse(Class<?> clazz) throws Exception {
        Method[] methods = clazz.getMethods();
        int pass = 0;
        int fail = 0;



    for (Method method : methods) {
        if (method.isAnnotationPresent(Test.class)) {
            // this is how you access to the attributes
            Test test = method.getAnnotation(Test.class);
            Class expected = test.expected();
            try {
                method.invoke(null);
                pass++;
            } catch (Exception e) {
                if (Exception.class != expected) {
                    fail++;
                } else {
                    pass++;
                }
            }
        }
    }
}




}

Edit: Also after receiving some emails, I guess I should add a full working example :) So here is one. Just copy paste and run the show :) 

@Target(ElementType.METHOD)
@Retention(RetentionPolicy.RUNTIME)
@interface Test {
    String info() default "";
}



class Annotated {
    @Test(info = "AWESOME")
    public void foo(String myParam) {
        System.out.println("This is " + myParam);
    }
}



class TestAnnotationParser {
    public void parse(Class clazz) throws Exception {
        Method[] methods = clazz.getMethods();



    for (Method method : methods) {
        if (method.isAnnotationPresent(Test.class)) {
            Test test = method.getAnnotation(Test.class);
            String info = test.info();

            if ("AWESOME".equals(info)) {
                 System.out.println("info is awesome!");
                 // try to invoke the method with param
                 method.invoke(
                    Annotated.class.newInstance(), 
                    info
                 );
            }
        }
    }
}




}



public class Demo {
    public static void main(String[] args) throws Exception {
        TestAnnotationParser parser = new TestAnnotationParser();
        parser.parse(Annotated.class);
    }
}

Java

9 Comments