How do you recognize that short blocks of code can be reorganized into something cleaner?

Question

I have some code that I wrote a few weeks ago (the purpose of the code is not as 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 to survive this tonight, and when I cleaned it, I realized that the cleaned version should be 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 cut the code to a logical thread and distorted the source language.

In the end, I took the nested if and turned them over. This has reduced this time. My question is: how can I find out in the future?

What are the telltale signs that I just wrote code that can be reorganized into something shorter?

---

Another example that I had a few weeks ago was something like a permission check: a user can perform an action:

• if they have permission, they can do it
• if they don’t have permission, but override is valid

Which I originally encoded as:

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

The logical conditions of this if clause seemed terse. I tried to return to my course in logical algebra to figure out how to simplify it. In the end, I could do this, so I ended up drawing a truth table:

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

That when I look, this is an OR operation. So my if become:

 if (HasPermission or OverrideEnabled) { ... } 

It is obvious and simple. So, now I wonder how I could not understand this.

---

Which brings me back to my SO question: what control characters can / should I look for to understand that some block of code needs some TLC?

Answer 1

Here are some recommendations from Code Complete, from the top of your head. This is a good book to get such a thing.

• Nested if-else and repeating statements in blocks
• Long loops
• Duplicate lines / operators or frequently used operations can be placed in a function
• If for any reason you copy and paste a line of code over and over

I found that discrete mathematicians influence the way I wrote if statements are now. Usually, I see that I write two identical IF statements in 2 blocks, then I would do some mental “factoring”.

Answer 2

In particular, related to Boolean evaluation, it is 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 equivalent; the interpreter stops evaluating when it sees or after the true variable. This is not very important when a and b are variables, but if they are callable (for example, if(a() or b()) ], b() will not be evaluated if a true.

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

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

becomes

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

Answer 3

Well done. Now that 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 refactoring. Extracting code into methods is not just a great way to eliminate redundant code. This is also a great way to make the document self-sufficient:

I would be inclined to shorten the code to:

 set_final_image_size 

With set_final_image_size and its minions defined like this:

 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 have separated the width and height logic and noticed that it is identical - what if you added, say, getDimension(Direction direction) and setDimension(Direction direction, int length) to your classes? Now you have

 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 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, in my opinion, is starting to look pretty pretty.