How to recognize that short code blocks can be refactored into something cleaner?

Question

i have a bit of code that i wrote a few weeks ago (the code's purpose isn't so much important as its structure):

if (_image.Empty)
{
   //Use the true image size if they haven't specified a custom size
   if (_glyphSize.Width > 0)
      imageSize.Width = _glyphSize.Width //override
   else
      imageSize.Width = _image.GetWidth;

   if (_glyphSize.Height > 0) then
      imageSize.Height = _glyphSize.Height
   else
      imageSize.Height = _image.GetHeight
}
else
{
   //No image, but they can still override it with a custom size
   if (_glyphSize.Width > 0) then
      imageSize.Width = _glyphSize.Width
   else
      imageSize.Width = 0;

   if (_glyphSize.Height > 0)
      imageSize.Height = _glyphSize.Height
   else
      imageSize.Height := 0;
}

i was going over it tonight, and as i was cleaning it up, i realized that the cleaned version is must more concise:

//Figure out the final image width
if (_glyphSize.Width > 0)
   imageSize.Width = _glyphSize.Width
else if (not _glyph.Empty)
   imageSize.Width = _glyph.GetWidth
else
   imageSize.Width = 0;

//Figure out the final image height
if (_glyphSize.Height > 0)
   imageSize.Height = _glyphSize.Height
else if (not _glyph.Empty)
   imageSize.Height = _glyph.GetHeight
else
   imageSize.Height = 0;

Note: i've trimmed down the code to bare logical flow, and obfsucated the source language.

In the end i took the nested if's, and inverted them. Doing that allowed this shortening. My question is: how can i recognize this in the future?

What are the tell-tale signs that i've just written some code that can be refactored into something shorter?

---

Another example i had from a few weeks ago was something akin to a permission check: the user can perform an action:

• if they have the permission they can do it
• if they don't have the permission, but the override is in effect

Which i initially coded as:

if ((HasPermission || (!HasPermission and OverrideEnabled))
{
   ...do stuff
}

The logical conditions on that if clause seemed kind of wordy. i tried to reach back to my boolean algebra course to figure out how to simplify it. In the end i could do it, so i ended up drawing a truth table:

Permission  Override   Result
    0           0        0
    0           1        1
    1           0        1
    1           1        1

Which when i look at it is an OR operation. So my if statement became:

if (HasPermission  or OverrideEnabled)
{
   ...
}

Which is obvious and simple. And so now i'm wondering how i couldn't see that to begin with.

---

Which brings me back to my SO question: What tell-tale signs could/should i be looking for in order to recognize that some block of code needs some TLC?

Answer 1

Here are some guidelines from Code Complete, off the top of my head. That is a good book to get for this sort of thing.

1. Nested if-else and repeated statements in blocks
2. Long for-loops
3. Repeated lines/statements or frequently used operations can be placed in a function
4. If for some reasons you are copying and pasting a line of code over and over again

I found discrete maths to have an influence in how I wrote if statements now. Usually, I see I am writing two same IF statements in 2 blocks, then I would do some mental 'factoring'.

Answer 2

Specifically related to boolean evaluation, it's worth noting that most(?) modern languages implement lazy evaluation.

That is, if "a" is true, then if(a) and if(a or b) are logically and functionally equivelant; the interpreter stops evaluating when it sees or after a true variable. This isn't very important when a and b are variables, but if they're callables [e.g. if(a() or b())], b() will not get evaluated when a is true.

You can save a lot of keystrokes (and processor time) by learning this well:

if(!userExists()):
    if(!createUser()):
        errorHandling()
    else:
        doStuff()
else: doStuff()

becomes

if(userExists() or createUser()): doStuff()
else: errorHandling()

Answer 3

Well done. Now, when I see this:

//Figure out the final image width
if (_glyphSize.Width > 0)
...

//Figure out the final image height
if (_glyphSize.Height > 0)
...

I think there is still more refactoring to do. Extracting code into methods isn't just a great way to eliminate redundant code. It's also a great way to make the code self documenting:

I'd be inclined to reduce the code to:

set_final_image_size

With set_final_image_size and its minions defined like so:

def set_final_image_size:
  imageSize.Width = final_image_width;
  imageSize.Height = final_image_height;

def final_image_width:
  if (_glyphSize.Width > 0)
     return _glyphSize.Width;
  else if (not _glyph.Empty)
     return _glyph.GetWidth;
  else
     return 0;

def final_image_height:
  if (_glyphSize.Height > 0)
     return _glyphSize.Height;
  else if (not _glyph.Empty)
     return _glyph.GetHeight;
  else
     return 0;

Answer 4

Now that you've separated the width and height logic, and noticed that it's identical - what if you were to add, say, getDimension(Direction direction) and setDimension(Direction direction, int length) to your classes? Now you've got

if (_glyphSize.getDimension(direction) > 0)
   imageSize.setDimension(direction, _glyphSize.getDimension(direction))
else if (not _glyph.Empty)
   imageSize.setDimension(direction, _glyph.getDimension(direction))
else
   imageSize.setDimension(direction, 0);

Extracting the local brings us:

length = _glyphSize.getDimension(direction);
if (length > 0)
   imageSize.setDimension(direction, length)
else if (not _glyph.Empty)
   imageSize.setDimension(direction, _glyph.getDimension(direction))
else
   imageSize.setDimension(direction, 0);

taking it a little further:

length = _glyphSize.getDimension(direction);
if (length == 0 && !_glyph.Empty)
    length = _glyph.getDimension(direction);
imageSize.setDimension(direction, length);

Which, to my eyes at least, is starting to look pretty nice.