addressalign-toparrow-leftarrow-leftarrow-right-10x10arrow-rightbackbellblockcalendarcameraccwcheckchevron-downchevron-leftchevron-rightchevron-small-downchevron-small-leftchevron-small-rightchevron-small-upchevron-upcircle-with-checkcircle-with-crosscircle-with-pluscontroller-playcredit-cardcrossdots-three-verticaleditemptyheartexporteye-with-lineeyefacebookfolderfullheartglobe--smallglobegmailgooglegroupshelp-with-circleimageimagesinstagramFill 1languagelaunch-new-window--smalllight-bulblightning-boltlinklocation-pinlockm-swarmSearchmailmediummessagesminusmobilemoremuplabelShape 3 + Rectangle 1ShapeoutlookpersonJoin Group on CardStartprice-ribbonprintShapeShapeShapeShapeImported LayersImported LayersImported Layersshieldstar-shapestartickettrashtriangle-downtriangle-uptwitteruserwarningyahooyoutube

Too many conditions

From: Saiprasad K.
Sent on: Tuesday, March 19, 2013, 12:57 AM
Hello Everyone,

                         I came across some piece of code at my work today and just thought I could share with you and also I'm keen to know your thoughts.

Unfortunately, I'm not authorized to put the real code here. However, it is easier to explain.

There is an Utility class which has one public method of this signature.

public static AccessControl doCheck(final UserDetail user)

AccessControl is a simple POJO that has about 10 Boolean properties to indicate whether something is enabled or not. Based on this, the clients will take the necessary action.

UserDetail is another simple POJO that contains basic information about the user [email, location, departments etc].

The logic of the doCheck method is to check a variety of conditions and return a populated AccessControl object.

These checks are mainly based on about 20 system properties and some of the fields in the UserDetail object.

There are quite a lot of 'and' , 'or' , 'not' with a lot of nesting.

This code has become overly complicated to read, digest and modify. Although there are a lot of unit tests that guard the code, the readability is somewhat lost and importantly, most of the team are a bit hesitant to touch that piece of code.

Also, we're envisaging that these conditions would increase in the coming days based on a new additional set of system properties. 

We have been thinking how to simplify this. 

Couple of  obvious approaches:  
Firstly,  to rename the conditions and flags to reflect to more "human friendly" names.  
Secondly, to extract the conditions as logical groups so that it can be re-used [pretty much like higher order conditions].

Well, that could help us to improve the code readability to some extent. But I still feel there could be other ways to improve it. 

One thought that came up during a discussion was to  use a simple rule engine. 

I'm not 100% sure if we should go for a rule engine for this.

Just wanted your thoughts/suggestions on improving this code.  

Many thanks in advance.

Ta,
Sai.

People in this
group are also in: