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.
Nice post, you take up a lot of interesting topics. It is scary what goes as a Senior developer these days.
November 25, 2009, 2:46 AMHi,
In your last example in the Stupid Loops section, the transformation is not semantically correct, strictly speaking. The first one, tough ugly, returns the last visible element in items. Your clean version returns the first visible one.
Cheers.
November 25, 2009, 3:54 AMInteresting viewpoint on avoiding if/else for initializing variables. For me, golden rule is to avoid multiple initializations of same variable if possible – best if it can be made final. So, in this case
final int number; if (_isVisible) { number = getNumberFromMoon(); } else { number = 1; }
I tend to get a lot more mileage from being certain that variable will never change later in the method rather than from reducing if/else statements.
November 25, 2009, 6:02 AMRe: returning values from within a loop. This used to be discouraged because it makes it harder for the compiler/optimizer (since you have to unwind the stack frame at multiple points, and it may have different sizes depending on where you return from). I’m not sure if Java has this problem, but it doesn’t hurt to make it easy for the compiler to emit efficient code. That’s why I always declare and initialize a return variable and just return that.
November 25, 2009, 6:28 AMtechnically, the last example and fix of section 2 aren’t equivalent. The example ‘bad’ code finds the /last/ isVisible item, and the correction finds the /first/ isVisable item.
November 25, 2009, 7:24 AMI suppose you should return directly with a reversed loop in that case?
November 25, 2009, 7:28 AMseems more like tips for junior devs
November 25, 2009, 8:27 AMGood article although I think you meant to use the word principal and not principle to describe a developer.
November 25, 2009, 8:29 AMgood and informative points
November 25, 2009, 9:01 AMSolid Post, very informational.
November 25, 2009, 9:50 AMAlthough I agree with you in most cases, when it comes to deadlines, and pm’s telling you to just get it done, sometimes.. you’re poorest solutions are the easiest. But good write up, and like the blog.
November 25, 2009, 10:50 AMThanks all for the comments. It was pretty late, so I guess I missed couple points. Thanks for correcting me.
November 25, 2009, 11:04 AMI definitely saw in your post a few offenses I have been guilty of committing. Good stuff.
November 25, 2009, 1:08 PMI completely agree with Isa, I feel the same pain when see such code. Unfortunately, I’ve met a lot of senior developers are too self-confident or too unmotivated to cook better code.
November 25, 2009, 8:31 PMLife’s too short to write bad code! We have to make a decision to stop writing bad code – regardless of tight deadlines. If we call ourselves senior developers, then we should act that way and not compromise on the quality of our code.
November 26, 2009, 12:21 AMBy reading your post, it looks like there is some sort of “code code” that, if carefully followed, would allow one to write the most complex logic without any class of more than 10 methods, without any method of more than 20 lines, or even without any else statement.
While I couldn’t agree more on the general point of your post, some of the guidelines you provide are obviously way too restrictive – too dogmatic if you will. If you ever worked in a big project involving complex business logic – and I take it you did – you know very well that you cannot do it without some larger classes or methods, let alone without any else statement, negative expression or null check…
November 26, 2009, 2:09 AMHi! I am not a senior guy (yet :)), but yesterday I have refactored my own if statement with complicated condition by extracting condition to separate method. Your article gives me confidence of that I am on the right way. Thank you very much
November 26, 2009, 3:08 AM@laurent I see your points. Of course there is no silver bullet for all situations. However as an experience developer, you have to foresee the approaching monster and be cautious while you’re coding. If you have to write more than 20 lines which is still questionable to me, then you go for it. But don’t forget! It’s not just about coding, it’s also understanding the code, take over process when you leave the team etc.. If you can write your code like a poem and make it beautiful, I’m ok with that. I don’t know if I’m making sense?
November 26, 2009, 9:58 AMHi I’m surely not a senior. I agree with many of your statements but they will be readed with care. One solution is not already the better solution. I join Laurent for your 20 lines and 10 methods. I really prefer one method with many lines that do one thing than 4 methods with 5 lines. I don’t like to have to walk trough many method to understand why 1+1=2. Of course if some of the method code can be reused “we” have to write one method with the reusable code.
November 27, 2009, 12:34 AMTo code an interval value like : if (val > 3 && val < 10) I prefer if (3 < val && val < 10) is like mathematic notation 3 < val < 10
For loop, i prefer to use WHILE when the test condition could stop at any value and i use FOR when i loop for every values
November 27, 2009, 2:24 AM@vdaburon, I think that one is a good idea.
@gervais, as I told @laurent; if you can write your code like a poem, then I wouldn’t mind. But honestly, I haven’t seen any guy that can write a poem-like codes in my whole career. Even you code it like that, someone after you will come and change the all the rhyme of your code, and it will be unreadable (mess) later on. Having a huge methods are big signs of incorrect responsibility assignments. I’d say if you have a code like that and you have to do it that way, it’s very likely to move that logic to another class which can take certain arguments while processing. Then it would be more testable, and you would satisfy SRP in your base class and new class.
And one more thing to whole comments: These are not the codes that I invent to satisfy article, these are the codes that I saw written by different senior guys which pisses me off. And as I mentioned in the article, this article is written for my relaxation. Thanks for great interest, everyone is welcome to contribute more techniques or samples. And keep in mind no one has to agree with me ;)
November 27, 2009, 10:20 AMMaking code simpler is relocating the complexity. There is no method to eliminate it. The art is achieving the right balance. And usually art is not achievable by following some simple rules or a check list.
Your general point is really valuable and well explained with real life examples.
November 27, 2009, 3:43 PMIt is not clear what you intend to do with the last “Stupid Loop” still. In the bad example you return items(0) if isVisible is true. The looop is not needed at all. You do the same in the good example, but don’t change the isVisible flag. In the previous example, I would put the isVisible outside the loop.
November 28, 2009, 3:00 PMThank you Isa. We have to refresh our mind from time to time with such nice topics. The problem is that 99% of developers (including myself) swear at bad code but don’t improve it after review :) Appreciate your great work. Keep writing!
November 30, 2009, 12:46 AMquite likely this was your intention:
// 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(); } }
cx
December 1, 2009, 8:15 AMThis is a nice refresher on some best practices in coding standards, thanks!
We use the ReSharper plug-in in Visual Studio 2008 which is very good at highlighting the worst bad practices, and it can often do any necessary recactoring for you with one click.
December 3, 2009, 6:02 AMGood low-level advice (low in the sense that it is closer to the code than to the design :).
December 6, 2009, 8:03 AMI have only one rot. Do not have any rots! All rots are just someone’s problem. This article is problem of guy who has problems with code understanding. He needs more practice and this problem will be solved. One man’s best practice is another worst nightmare. Brake the rules. Do whatever you want. Do wherever you want. Do everything your way. Never follow any loosers rules. No more “this sould be done this way”. Have your own opinion. Be free. ;) I did not tell you to do wrong! Just do it your way. Your way is the best way. Let loosers to make rots of doing your way. Loosers create rots and design patterns because they have no brains to think and understand. They have no fantazy to create. Only thing they can is to copy.
January 6, 2010, 8:11 AM