Wednesday, October 04, 2006

Short, concise and readable code - invert your logic and stop nesting already!

Have you ever heard this maxim:

"A method should have one and only one exit point"

Well, it couldn't be more wrong. The following are the attributes of a well written method; it should:

  • perform one and only one function,
  • be short and to the point,
  • be easy to read and understand,
  • and it should have a descriptive, accurate and concise name.
Notice that none of these points says anything about how or where a method should exit. To write a clean, easy to read method, follow these guidelines:
  • follow a template. consistent flow is easier to read.
  • check your pre conditions early. If they fail, exit fast
  • nesting is bad. avoid it. invert your logic at every step of the way.
First, the template:
[return value] [method name](parameters)
[throws clause]
{
[check pre conditions]

[core logic]
}
Pretty simple, right? So what's this about invert your logic? Well have a look at the template up there. Do you see any nesting? Right....neither do I. So let me illustrate a common idiom, one that uses in particular the if/else if/else pattern and the single exit strategy:
/**
* Returns the element at index
*/
public Object getElement(int index)
{
Object theItem = null;
boolean listError = false;
boolean indexError = false;

if (list != null) {
if (index > 0 && index < list.size()) {
theItem = list.elementAt(index);
} else {
indexError = true;
}
} else {
listError = true;
}

if (listError) {
throw new Exception("Bad list");
} else if (indexError) {
throw new IndexOutOfBoundsException("Bad index");
} else {
return theItem;
}
}
Wow, what a mouthful. And I didn't even put in a few while loops, case structures and other nonsense I usually see that ends up with code nested 4 and 6 levels deep. Let me rewrite that code up there making it match the pattern I suggested, inverting the logic, and then I will explain what I did.
/**
* Returns the element at index
*/
public Object getElement(int index)
{
if (list == null) {
throw new Exception("Bad list");
}

if (index < 0 || index >= list.size()) {
throw new IndexOutOfBoundsException("Bad index");
}

return list.elementAt(index);
}
Remember when I said check your pre-conditions first, and exit fast. What this allows you to do is evaluate all of the conditions under which your method will fail, and if you detect something amiss, handle it immediately. This strategy is flexible - if the pre-conditions for your class or your method change, you can add and substract the tests for those pre-conditions using this structure without having to modify surrounding code. The worst offender I see is always of the following pattern:

if (condition_to_succeed_is_true) {
do_something();
} else {
do_error();
}
The problem with this is that the reader of your code has to put the conditional test onto their mental stack while they digest what do_something() is doing. If do_something() happens to be long, or complicated, you'll probably blow the mental stack of the reader, forcing them to look at the condition again just to figure out why the do_error() is being done.

On the other hand, when you line up your pre-condition tests linearly, there is no mental stack, the reader simply processes each in turn, and then, when they are all done, they are able to process the real meat - the do_something() - part of your method without all the baggage from your pre-condition tests. So inverting your logic means taking the above test, inverting the condition, and writing it as:
if (!condition_to_succeed_is_true) {
do_error();
return;
}

do_something();

So I hope you remember your CS classes and De Morgan's laws - I find coding like this makes me use them all the time.

There's one other benefit this strategy has, and that's when you are writing synchronized blocks. When you write a synchronized block, you absolutely must strive to perform as little work as is absolutely necessary inside the synchronized block.

Let's look at the problem we have when we use the normal pattern I described above, combined with synchronization -- the code now becomes:
synchronized (lock) {
if (condition_to_succeed_is_true) {
do_something();
} else {
do_error();
}
}
Ugggh! There's no way, in Java, to release the lock before you perform do_something()! (Which we presume takes time and shouldn't be performed under lock). If you invert the logic, however, you can test the condition and release the lock as soon as you've tested it (note that it's often the case that you might need to use some data you acquired during the lock, in that case you should make a copy on the local stack under the lock, and then release it which I have shown below):
synchronized (lock) {
if (!condition_to_succeed_is_true) {
do_error();
return;
}
state = copy_state();
}

do_something(state);
Remember, in these examples I am assuming that do_something(...) is the non trivial part of your method, both in lines of code, complexity, and execution time.

One more thing - I find that using 4 spaces to indent code blocks instead of 2 helps to break me of the habit of nesting my code because it acts like an automatic brake on the indentation level.

15 comments:

Ricky Clarkson said...

The actual case you showed is pretty flawed. A more optimal approach would just be to return list.elementAt(index), because the null check would still be done (but not twice) and the range check would still be done (but not twice).

Even more ideally, elementAt would be a field that references something like a Runnable, e.g.:

interface Function<T,R>
{
R run(T input);
}

Then if List<E> had an elementAt() that returned a Function<Integer,E>, you wouldn't even need to write any code to delegate through, just expose that Function.

If you treat a method as a mathematical function, that takes in values and returns a result, rather than one that modifies things, then single return is probably the most optimal.

You can work with non-results without having chunky if statements, see the Maybe type in Haskell.

Translated to Java:

Maybe<Integer> input=getInputFromUser(); //they may cancel or enter something other than an Integer.

Maybe<Double> squared=input.run(squareRoot);

return squared;

If the original input was Nothing, then squared would be Nothing too.

[squareRoot would be a Function<Integer,Double>]

It is possible, and quite useful, to employ some functional programming idioms in Java, and where I do that, I find that the easiest to read results are often one-liners. When I get to the one-liner, I can then wonder whether the method really needs to exist, especially when it's an instance method.

Taylor said...

You're right, for the *actual* case presented, you wouldn't need the code I wrote. And I agree with your position that a lot of code can be reduced to one-liners...that wasn't my point.

My point was to apply this approach to non-trivial methods just getters, and that cannot be reduced to the mathematical functor style.

Taylor said...

Last paragraph should have read:

My point was to apply this approach to non-trivial methods that cannot be reduced to the mathematical functor style, not just getters.

Taylor said...

Ok, I can understand your resistance to the idea.

I've responded with another blog post that shows how the technique can be applied more broadly.

Part 2

Shaghouri said...

I agree with your points (even the one that allows the use of multiple exit points).

Regarding the benefits of "inverted logic", I would like to add that the use of inverted logic can significantly reduce the nesting level in large methods. This means less indentations (and less run-off text that has to be wrapped). It also happens to makes the code less complex (according to some of the metrics that we use here at work).

The need for multiple exit points is often times a necessity for "inverted logic", especially for methods that return status and error codes.

Stephan.Schmidt said...
This comment has been removed by the author.
Stephan.Schmidt said...

For more than a decade I was a follower of the "one-exit-rule". But as you, I now think this rule is senseless. Keep your methods short enough, let them make only one thing and you're fine. There is no problem with several exit points. Those only become a problem if the method grows beyond 10 lines of code and noone can understand the method from looking at it. Then several exit points add to the problem.

Peace
-stephan

--
Stephan Schmidt :: stephan@reposita.org
Reposita Open Source - Monitor your software development
http://www.reposita.org
Blog at http://stephan.reposita.org - No signal. No noise.

Anonymous said...

搬家 搬家 搬家公司 徵信社 徵信 彩妝造型 新娘秘書 票貼 室內設計 室內設計 徵信 徵信社 外遇 徵信 徵信社 外遇 搬家 搬家 花蓮民宿 花蓮民宿 免費a片 a片 免費av 色情影片 情色 情色網 色情網站 色情 成人網 成人圖片 成人影片 18成人 av av女優 情慾 走光 做愛 sex H漫 免費a片 a片 免費av 色情影片 情色 情色網 色情網站 色情 成人網 成人圖片 成人影片 18成人 av av女優 情慾 走光 做愛 sex H漫 a片 アダルト アダルト アダルトサイト アダルトサイト 離婚 抓姦 外遇蒐證 外遇抓姦 外遇 侵權 仿冒 應收帳款 工商徵信 Shade sail nike shoes 水泵 电动隔膜泵 自吸泵 离心泵 磁力泵 螺杆泵 化工泵 水泵 电动隔膜泵 自吸泵 离心泵 磁力泵 螺杆泵 化工泵 水泵 电动隔膜泵 自吸泵 离心泵 磁力泵 螺杆泵 化工泵 隔膜泵 气动隔膜泵 隔膜泵 气动隔膜泵 隔膜泵 气动隔膜泵 a片 成人網站 成人影片 寵物用品 情趣用品 情趣用品 MBA 在职研究生 在职博士 補習班 花店 花店 補正下着 中古車買賣 貸款 婚紗 婚紗攝影 補習班 留學 情色 情色 百家乐 轮盘 21点 德州扑克 百家乐系统 真人娱乐场 百家乐 足球 德州扑克 电子游戏 英格兰超级联赛 德国甲组联赛 意大利甲组联赛 西班牙甲组联赛 法国甲组联赛欧冠杯 英超 足球比分 足球彩票 体育彩票 即时比分 堆高機 婚禮佈置 宜蘭民宿推薦 寵物用品 情趣用品 情趣用品 坐月子 植牙 牙齒矯正 租屋 催眠 房屋出租 租房子 xo醬 牛軋糖 牛嘎糖 代償 房屋貸款 信用貸款 失眠 減肥 眼鏡 金門高梁酒 變頻洗衣機 票貼 借款 關鍵字廣告 租車

Anonymous said...

免費 a 長片線上看免費 a 長片線上看免費 a 長片線上看免費 a 長片線上看免費 a 長片線上看免費 a 長片線上看免費 a 長片線上看免費 a 長片線上看免費 a 長片線上看免費成人片欣賞免費成人片欣賞免費成人片欣賞免費成人片欣賞免費成人片欣賞免費成人片欣賞免費成人片欣賞免費成人片欣賞免費成人片欣賞免費成人片欣賞免費看成人a片免費看成人a片免費看成人a片免費看成人a片免費看成人a片免費看成人a片免費看成人a片免費看成人a片免費看成人a片免費看成人a片a片線上免費看a片線上免費看a片線上免費看a片線上免費看a片線上免費看a片線上免費看a片線上免費看a片線上免費看a片線上免費看a片線上免費看av女優影片下載av女優影片下載av女優影片下載av女優影片下載av女優影片下載av女優影片下載av女優影片下載av女優影片下載av女優影片下載av女優影片下載成人av影片分享成人av影片分享aa片免費看微風論壇080哈啦聊天室6k聊天室成人聊天室上班族捷克論壇大眾論壇plus論壇080視訊聊天室520視訊聊天室尋夢園上班族聊天室成人聊天室上班族 a片a片影片免費情色影片免費a片觀看小弟第貼影片區免費av影片免費h影片試看 H漫 - 卡通美女短片小魔女貼影片免費影片觀賞無碼a片網美女pc交友相簿美女交友-哈啦聊天室中文a片線上試看免費電影下載區免費試看a短片免費卡通aa片觀看女優影片無碼直播免費性感a片試看日本AV女優影音娛樂網日本av女優無碼dvd辣妹視訊 - 免費聊天室美女交友視訊聊天室080免費視訊聊天室尋夢園聊天室080苗栗人聊天室a片下載日本免費視訊美女免費視訊聊天

Anonymous said...

汽車旅館
消費券優惠
motel
消費券
薇閣
住宿券
廣交會
廣州飯店
廣州
广州
广交会
广州酒店
Canton Fair
Guangzhou Hotel
Guangzhou
広州
広州の交易会
広州のホテル

Anonymous said...

威創牙醫診所除了提供優質的植牙技術外還提供假牙|矯正|牙周病治療,是值得您信賴的牙醫診所

獅王紋身工作室提供專業的無痛刺青技術,獅王紋身在世界TATTOO大賽中,獲獎無數,獅王紋身給您最時尚的作品。

陳駿逸皮膚科診所提供了治療痘痘的服務,皮膚雷射權威,包括雷射脈衝光除斑等,讓您回復青春蘋果臉。

ck皮件處理棧提供專業洗包包|洗鞋子|各式皮件修理保養疑難雜症都有服務,清洗包包專門店讓您的包包、鞋子、永遠保持最新的況態唷。

杏儒中醫診所提供了糖尿病的治療。

seo大師e王國幫您的網站輕鬆在您的行業裡站上第一頁,e王國的關鍵字行銷是您的好幫手,包括關鍵字自然排序、都能讓您獲得完美的效果,以目前的網路行銷不外乎是關鍵字自然排序為主、而關鍵字行銷seo又是e王國的強項讓e王國幫您征服網海。

Anonymous said...

There are ed hardy shirts
,pretty ed hardy shirt for men, ed hardy womens in the ed hardy online store designed by ed hardy ,many cheap ed hardy shirt ,glasses,caps,trouers ed hardy shirts on sale ,
You can go to edhardyshirts.com to have a look ,you may find one of ed hardy clothing fit for you
http://straighthairgirl.blog126.fc2.com
http://www.seriousblogging.com/crazygirlsshirts
http://www.free-blog-site.com/iammyself
http://d.hatena.ne.jp/hotfishing

puma mens shoes
nike air max ltd
NIKE air shoes

nike max ltd
orange converse
adidas shoes
nike shoes
puma shoes
addidas shoes
cheap converse shoes
cheap nike shoes
cheap puma shoes

Anonymous said...

Fantastic!God bless you!Meanwhile,you can visit my China Wholesale,we have the highest quality but the lowest price fashion products wholesale from China.Here are the most popular China Wholesale productsfor all of you.You can visit http://chinaclothes.net.Also the polo clothing is a great choice for you.God bless you!I really agree with your opinions.Also,there are some new fashion things here,gillette razor blades.gillette mach3 razor bladesfor men.As for ladies,gillette venus razor blades must the best gift for you in summer,gillette fusion blades are all the best choice for you.

Anonymous said...

Indonesia
Easy
Learning
Indonesian
lampung
www.lampungservice.com
iPhone

88YYears said...

gread website. please update to articheal and information and join my website Cliphunter and KotakQQ and getting million money every day. thamka for colom comentaring.