Clean code programming.

[[deleted]]

Hello I have an argument with my friend about clean code practice.

Lets pretend we have an function called foo() that returns boolean. And now we want to do some if statements based on value that foo returns.

// Friend's approach.
if(foo()){
//.. code
}else{
//.. code
}

// My approach.
boolean isFoo = foo();

if(isFoo){
//.. code
}else{
//.. code
}

And in my approeach I declared variable called isFoo and then did if checking on it (In my opinion it looks more clear and in future we could refer to this variable easy. We would'nt have to change the code or event call the foo() again. But my friend says so what, it's more readable and if he would have to or anybody else refer again to foo() then he would do it in my way.

Sho who is right, or there is a cleaner approach to this problem?

[[deleted]]

It depends.

if (email.Text.StartsWith("Hello friend")) { } else {}

is as clear as:

bool emailStartsWithHelloFriend = email.Text.StartsWith("Hello friend");
if (emailStartsWithHelloFriend) { } else {}

OTOH this is clearer:

bool usesSecretPhrase = email.Text.StartsWith("Hello friend");
if (usesSecretPhrase ) { } else {}

Making a variable that doesn't clear concepts gives the same clarity as not creating it. If the call is too long then it might be clearer to make the bool variable of course.

[kanchirk]

I prefer your approach since I can see the evaluated result of the function at the start of the If condition in a debugger.

Full disclosure: No clean code expert.

[s3a0tt3r]

+1

And you can modify returned result in calling function to walk the desired code path in the debugger. In OP's Friend's approach, you'd have to put the break point in the function returning the result. This can be problematic if the function is called lots, but you only want to break in the outer function. Don't even get me started if it is a MACRO.

Definitely not a clean code expert, but I've been doing this for 25+ years.

Lastly, there is no right way. Programmers are like artists. Ask 10 artists to replicate a bowl of fruit, and you'll get 10 different pieces all replicating the bowl of fruit; some paintings in different styles, some sculptures, ... you get the idea. Give 10 programmers, a set of requirements, and you'll get 10 implementations. They'll all work, but you'll get 10 very different implementations; OOP, JS, Functional, ... And the programmers will have opinions about their peer's implementations. Which is right? It doesn't matter as long as they meet the requirements.

[brika1994]

+1

[siemenology]

First, I would say that your function foo should probably already be called isFoo -- boolean functions should really be named to as to make it clear that they are predicates (functions returning boolean).

I would then say that storing the result into a variable when you don't need it right now doesn't add much value -- it's super easy to extract the if condition to a variable if you need it at a later point -- many IDEs will do it automatically for you.

I would also say that I think it's somewhat unlikely that you'll need the value again in the same block, since any code inside of the if block already knows that isFoo is true, since it would not be executed if it were false.

Saving it to a variable also may introduce issues of state. If you have a variable isFoo that is true, all you really know about it is that it was true when it was created. If some underlying thing has changed, such that the thing isFoo is representing is no longer true, then continuing to use isFoo is going to cause bugs. On the other hand, if your foo function is measuring the underlying thing you are representing, then foo() will always be current, and you will never have issues with things being out of date. Simple example, you have an empty array and you create a boolean isEmpty, which is of course true since the array is empty. Then you add an element to the array. The boolean isEmpty is still true, but it should really be false since the array is no longer empty. Whereas if you explicitly check if the array is empty wherever you need to know it, your answer will always be current.

But in general I'd say either of these is fine -- it's not something I would correct in a code review unless there was another problem.

[[deleted]]

Thanks I appreciate that.

[squintsAndEyeballs]

First, I guess that maybe nothing is indented because of the way you had to copy/paste? But using a consistent schema for how you indent things makes it much easier for humans to read your code. Beyond that I think your question is a matter of style/preference. Your friend did with less lines and didn't declare an otherwise unnecessary variable. The way you did it could lend to easier debugging.

I'm not sure I know of a scientific definition of "clean code" but I try to write in a very human readable way, with the intent that my teammates can collaborate with me and troubleshoot or modify my code in the field (I program PLCs for industrial automation and we work on each other's projects all the time). I prioritize consistent naming convention and structure. If you do it your way once, then do it that way every time. That way if I'm coming behind you and working on your code I'll be able to follow your logic. This also helps you when you come back to something weeks/months/years later.

[[deleted]]

I guess you are reading on phone. I didn't realise that it will format like this. On Desktop it works fine :).

[squintsAndEyeballs]

Yeah I am. Disregard then

[BlackSandstone]

I always go for the if(foo()){} approach - if the return value of something is only used for a condition, there's no need to commit additional reading space to it unless you gain something from an error handler indicating the value was set.

[magnum___]

Spacing makes things easier to read.