Should clean code always obey these rules?

Question

Regarding the book "Clean Code - A Handbook of Agile Software Craftsmanship":

• it says that functions should do one thing. Should else be avoided? If so, should return true and return false that use to come together really be avoided too?

• it says that functions should have one level of abstraction. Is high-level like ruby or like less indenation level and is low-level like assembly or more indentation level?

Answer 1

Should else be avoided? If so, should return true and return false that use to come together really be avoided too?

They're basically suggesting that you avoid functions like this:

public bool IsEdible() {
  if (this.ExpirationDate > Date.Now &&
       this.ApprovedForConsumption == true &&
       this.InspectorId != null) {
    return true;
  } else {
    return false;
  }
}

Instead, Clean Code suggests that you do something like:

public bool IsEdible() {
  return IsFresh() && IsApproved() && IsInspected();
}

That's not to say that conditionals are evil; everything in moderation is fine. Know when to use the tools in your toolbox, but don't bring out the hammer just because something looks like it might be a nail.

Likewise, if your function tries to bite off more than it can chew, you should break it up into multiple functions:

public void EatAndDiscard(Fruit f) {
  try {
    f.Peel();
  } catch (NoPeelerAvailableException e) {
    basket.Notify("fruit " + f.Name + " could not be peeled");
    this.WaitFor(this.Peeler);
  }

  this.Consume(f);
  Scraps<Fruit> s = f.GetScraps();
  s.Dispose();
}

Instead, try something like:

public void Consume(Fruit f) {
  Peel(f);
  Eat(f);
  Discard(f);
}

It says that functions should have one level of abstraction. Is high-level like ruby or like less indenation level and is low-level like assembly or more indentation level?

"Level of abstraction" is a somewhat nebulous concept, but essentially it means that a method talks about everything at the same plane of understanding. For example, here are some statements about the same problem that are at different levels of abstraction, ordered roughly from lowest to highest:

• A salesman wants to visit N different cities around the country without visiting any city twice, winding up at the same place he started, and in as short a distance as possible.
• A salesman wants to make a closed tour of N cities that requires him to travel the least distance possible.
• Find a closed tour on a graph of N vertices that minimizes the cost.
• Find the shortest Hamiltonian tour on N vertices.

If you were writing a method and you used a HamiltonianTour object in the same breath as a Salesman object, that would be a good clue that you were talking about the problem at two different levels of abstraction. This is bad, because it suggests you're not thinking about the problem in a consistent way, which in turn means you may be overcomplicating things.

Having multiple levels of abstraction means that the method is trying to think about things or anticipate situations it probably shouldn't be responsible for, or at least could delegate to somebody else. If your function looks like it has a lot of special conditions or talks about things at too many different levels, it could probably be simplified.

Answer 2

For the first point, I think you misunderstood what the author meant by "one thing".

If/Else statements are fine, the method is allowed to execute more than one branch of logic, as long as the function only has a single responsibility.

Using StackOverflow as an example, they might have one function to look up user information, another to look up the users answers, another to lookup their reputation history, etc. and these would be broken up into methods like LookupUserById, LookupUserCommentHistory, etc. It wouldn't just be one method called GetUser, because that would be a giant, unmaintainable mess.

I'm not quite sure what you're asking for the second part though.

Answer 3

Your question is on the borderline of being incomprehensible, but I'll give it a go anyway.

it says that functions should do one thing. Should else be avoided? If so, should return true and return false that use to come together really be avoided too?

What they are saying is that your functions should be like this:

public void takeOutTheTrash() { /* move that rubbish */ }

public void walkTheDog() { /* exercise that beast */ }

not

public void doTheDomesticThings() {
    /* take out trash, walk the dog */
}

But this is good, because

public void doTheDomesticThings() {
    takeOutTheTrash();
    walkTheDog();
}

it says that functions should have one level of abstraction. Is high-level like ruby or like less indentation level and is low-level like assembly or more indentation level?

This is particularly hard to understand, but I'm pretty sure that you don't mean "indentation". (That's literally about the amount of spaces you put on the left side of source code lines to make it readable.)

What the book is probably trying to say is that this is wrong:

public void doTheDomesticThings() {
    takeOutTheTrash();
    walkTheDog();
    for (Dish dish : dirtyDishStack) {
       sink.washDish(dish);
       teaTowel.dryDish(dish);
    }
}

because the doTheDomesticThings method both operating at the higher level of abstraction of doing coarse-grained tasks, and the lower level of micro-tasks; e.g drying each individual dish. A better solution is this:

public void doTheDomesticThings() {
    takeOutTheTrash();
    walkTheDog();
    doTheDishes();
}

Answer 4

It's a good book. Wish more read it.

Regarding doing one thing --- what do you mean with "else be avoided"? It's really as simple as answering the question "What is this function doing?". If it's "it verifies that the input is okay and loads a user from the database and checks that his permissions are okay, ... and so on", it's clearly doing several things. Functions should be short and sweet, and have a succinct name that describes exactly what it does. It's okay to have a function that does a lot of things by invoking many other functions.

Answer 5

To your first point, doing one thing means it should return a value and not modify other values as side effects.

The second point is not talking about language level abstraction nor code formatting. It is talking about abstraction levels within your problem domain. Actually, I think John Feminella's example is a great example of keeping things at a single abstraction level. RAther than the IsEdible() method having to know about things like expiration dates and such, it can just use the abstraction of IsFresh().

Answer 6

• it says that functions should do one thing. Should else be avoided? If so, should return true and return false that use to come together really be avoided too?

That is an extreme interpretation of the "functions should do one thing" rule.

It is normal for a function to have multiple conditional branches. Use common sense.

• it says that functions should have one level of abstraction. Is high-level like ruby or like less indenation level and is low-level like assembly or more indentation level?

Probably none. I have not read the book (although I am adept at software craftsmanship), but I guess the "functions should have one level of abstraction" rule refers to the stack of abstractions you have in any non-trivial program.

You may have functions dealing with wire format of files or protocols, and functions dealing with business logic. A single function should fall neatly in one category.

Answer 7

Functions should do one thing in the sense that you should be able to describe what they do in a single phrase. That one thing could be simple or complicated. "Compare to zero" and "display complicated shape" are single phrases, but obviously one function will be more code than the other. You seem to be confusing that with doing one thing in the language, which is entirely different.

I'm not sure what "one level of abstraction" means here, perhaps that each function should tend to be one step less abstract than its callers (which is a vague statement in itself), or that functions should not try to mix levels of abstraction. For example, fiddling with the lighting in detail while generally displaying a complicated shape is probably mixing things up too much.

Answer 8

He's talking about coherence more than anything: a function should have one clear purpose. That might be to decide between two alternatives, so it's obviously acceptable to use "else" or to return different values.

The levels of abstraction are sort of like collections of functions that view the problem at about the same degree of focus. Low-level functions are ones that are focused on one very specific part of the problem and high-level functions deal with the problem in more general terms. Typically functions at a higher level of abstraction make use of those at a lower level of abstraction. This is a little like the difference between high-level and low-level languages but at a more conceptual level, and the functions are usually all implemented in the same language. It doesn't really have anything to do with levels of nested indentation, but it's usually a good idea not to write functions that are too deeply indented: the parts at deeper levels of indentation should be separated out as functions in their own right.

Answer 9

In part, these both really come down to...

• Make your code succinct (see @John Feminella)
• Make your code readable (@John Feminella's example also is good here)
• Make your code have a purpose, a reason, a whatever you want to call it. I like @David Thornley's "single phrase" approach.
• Avoid "DoAnything" methods where the purpose of the code varies with the inputs.

Really bad code...

public object DoAnything (object input, string operation)
{ 
  object returnValue= null;
  if (input is int)
    returnValue = ((int)input) * 2;
  else if (input is double)
    returnValue = ((double)input)++;
  else if (input is string)
  {
    string inputString= input.ToString();
    if (operation == "replace_at")
      returnValue = inputString.Replace('@','2');
    else
      returnValue = inputString.Length
  }

 return returnValue;
}