DEV Community

Most effective and simplest way to write readable code.

Girish Budhwani on November 15, 2017

We already know the way but probably out of laziness we don’t follow it. It’s the most underestimated way of writing clean code and there is a misc...
Collapse
 
skuzzle profile image
Simon Taddiken

Personally, I do not like the kind of method in your improved version. They neither take arguments nor do they produce values. This means they rely on state set by someone else and they produce side effects that others (might) rely on. In the initial version it is easier to reason why things are ordered as they are, because I can see which usage of a variable depends on which write to this variable. However, I do support the extraction of methods to give them a proper name, but with two additions:

  • Make (all of!) the method's dependencies explicit (parameters)
  • Produce as less as possible side effects (produce a single value and return it)

Let's not argue about procedural programming style, where avoiding side effects is not always easy or even possible. But there are a lot of occasions where following these principles leads to code which is easier to understand and to reason about. That is because from a method's call site, I can not only see what it is intended to do (=method name) but also what it needs to do this (=parameters) and what is the result of doing it (=return value).

Collapse
 
girish3 profile image
Girish Budhwani • Edited

I totally agree that side effects cannot be avoided as far as procedural programming is concerned. But I would still prefer the improved version (with or without parameters) because either way there are side effects. Certainly, the code example I used is somewhat trivial (no inter-dependent methods, no return values) and I am sure we can break down our code even in that case. My only concern is that the high level code should be made declarative.

Collapse
 
scotthannen profile image
Scott Hannen

I like it. It indicates that six different steps take place and what they are. Compared to the original version, now you can actually tell what the code does. When you have to modify it, it's much easier to find the part you're looking for. It's also easy to tell if the code in any of those steps affects any of the other steps.
In effect it's like a table of contents for the code. We don't pick up a product manual and read the whole thing to find one detail. We read the table of contents.

Collapse
 
johannesvollmer profile image
Johannes Vollmer • Edited

I think extracting to methods is good, but I think it's better to not hide initialization. For example, I would have preferred it like
mView.setTabLayout(createLayout()) because more clearly conveys what's being done (and has less side effects and so on).

Also, having methods indicates that you plan to reuse the code inside it. I think immediately invoked functions is a great way to handle that (although not all languages support that):


function Init(){
    (function restoreGroupSelection(){
        // preferences = ... 
        // manager.restore(preferences)
    })()
} 

You might also use lambdas or similar code grouping elements.

Collapse
 
nikolamalesevic profile image
Nikola Malesevic • Edited

"having methods indicates that you plan to reuse the code inside it"

That's what really bothers me about having all those small functions. I don't need them anywhere else and my IntelliSense is soon full of garbage.

Collapse
 
girish3 profile image
Girish Budhwani • Edited

Separation of concern is another reason why we define methods and if we define proper method names then we need not worry about IntelliSense drop down list size.

Collapse
 
lluismf profile image
Lluís Josep Martínez

No shit Sherlock 😂😂

Collapse
 
andy profile image
Andy Zhao (he/him)

I don't think everyone knows how to refactor or write readable code, especially if they're a beginner. Let's not assume, yeah?

Collapse
 
lluismf profile image
Lluís Josep Martínez

Fair enough. But you should take into account that this kind of click bait titles do not help your web. I expect a bit more.

Thread Thread
 
girish3 profile image
Girish Budhwani

I do wrote in the title that its the simplest way. I guess you are the only prey ;)

Collapse
 
girish3 profile image
Girish Budhwani • Edited

I know 😃 but mostly its not followed through.

Collapse
 
lluismf profile image
Lluís Josep Martínez

In my experience it's followed most of the times, when it's necessary of course not blindly. In your example the original code is simple and very well structured. Putting a couple of lines of code in a method that is not reused outside this class is a bit extreme. But that's just me :-)

Thread Thread
 
girish3 profile image
Girish Budhwani

Example I used is just a snippet. The init() method earlier actually spanned 1000s of lines. And the extra line I have put between each set of logic is so that its easy to follow.

Thread Thread
 
lluismf profile image
Lluís Josep Martínez

If a method has 1000s of lines then it's definitely a nightmare. But moving every 2/3 lines to a method will only make it worse. You'll end up with hundreds of methods. You need another kind of refactoring, perhaps creating multiple classes with different responsibilities.

Thread Thread
 
girish3 profile image
Girish Budhwani

Yeah we actually did the refactoring and created multiple classes and methods.

Collapse
 
alphawong profile image
Alpha • Edited

Disagree, code is for human to read. A good code comment will enable following developer easy to pickup. For instance

// php5
function SetUserBalance($balance){
  return [
    'balance' => $balance
  ]
}

// php7
// Should I use float or string?
function SetUserBalance(float $balance):array

function SetUserBalance(string $balance):array

A meaning comment always help developer to maintain the code without misunderstanding.

If we can make a comment as follow

// php5
/**
SetUserBalance will set the balance for the user
@param float $balance must be string in order to avoid possible lost
@return array about the current user information.
**/
function SetUserBalance($balance){
  return [
    'balance' => $balance
  ]
}

See also github.com/Microsoft/vscode/blob/m...

Collapse
 
joshcheek profile image
Josh Cheek

They both have tradeoffs, so my default position is to be cool with either, until I have some reason to prefer one or the other. One tip for reading code that looks like the former is to read the LHS of assignments and ignore the RHS, unless you are specifically interested in it (eg in the same way a method name abstracts its body, a variable name abstracts its assignment).

Collapse
 
girish3 profile image
Girish Budhwani

I wouldn't say by defining methods we are making any trade-off and at some abstraction level we need to assign some values, just that assignment shouldn't be done at high level code. I like your LHS/RHS tip :)

Collapse
 
joshcheek profile image
Josh Cheek • Edited

There are tradeoffs for every decision :) Eg while the initialization method was ugly, all the code within it was scoped to it. Now the code is in methods, even though they are only used by a single method. So while the constructor gets more declarative, the class gets more cluttered.

I often write objects that could basically be functions, the constructor receives their arguments, and then they have a call method to do whatever it is that they do, and all other methods are private. Within that frequent pattern, I often find it useful to restrict variable setting and getting to initialize and call. This forces the integration to happen at the top, which reduces coupling. It means I only have to look in those two places to see everything that can access that variable I'm trying to debug. By pushing the assignment into other methods, I can't glance at one spot and know all the candidates. Do note that you can mitigate this by having the method initialize the object and return it, and then performing the assignment in the constructor.

Also note that the methods here are nice declarative names, but they are implicitly depending on something. I can't tell from the code sample, but it's common for one step to need the other to have been performed already, so methods allow an implicit dependency on order of invocation. Note that you can also fix this while still using methods: pass the dependencies as arguments. This also allows you, again, to see everything that accesses some variable of interest.

Basically, instance variables are like global variables, but for a much smaller globe. So, if you're not careful, your instance variables can have the same pains as global variables, and methods operate on implicit state are the primary way to do that. So, I try to keep ivar access at the top... but, that's just a heuristic, I also write code like your pre and post refactored examples :)