2

If there is some code that needs to be implemented across multiple controllers (lets say, 10), e.g.:

// Inside a controller function

var myField = null;
var response = service.callBackend(function(){

    // Other logic

    // starts here
    if (response) {
        myField = { 
             name: response.name,
             value: response.val,
             access: access.READ,
        };
    };
    // ends here

    // Other logic

});

What is a better approach? To create a myField in every controller as an object with the check for response, or extract the logic into separate function?

function helper() {
    return {
        createReadonlyField: function(content) {
            if (!content)
                return null;

            return {
                name: content.name,
                value: content.val,
                access: access.READ
            };
        }
    };    
};

And then in every controller you would call helper function to construct the field:

// Inside a controller function (helper object is passed as Dependency Injection)

var myField = null;
var response = service.callBackend(function(){

    // Other logic

    myField = helper.createReadonlyField(response);

    // Other logic

});

Is the code readability reduced by extracting it to a function?

5
  • It depends on what variables, class and method names you use. In the example you gave, it definitely reduces readability. What the heck is a helper? I guess I'll have to look it up. What type of ReadonlyField is being created? I guess I'll need to look at the code. etc...By using non-descriptive names you made the code convoluted and unreadable. If I have to go elsewhere to figure out what something does then you chose bad names and made the code unreadable. Whereas, a good choice of names could very well have made things much more readable or maybe not.
    – Dunk
    Jul 25 '16 at 21:12
  • Isn't the function name more important? Where it clearly says what it does.
    – mimo
    Jul 25 '16 at 21:19
  • If I have to look elsewhere to see the meaning of something then the names aren't very good. The code isn't as readable as it should be. The goal is to make the code readable like a book. Sometimes it is unavoidable to have some cryptic lines of code but usually putting in a few moments more thought to naming pays off quite a bit. While naming may not appear to be important "at the moment", when you or some other poor developer has to go back to the code 2 years from now then you'll/they'll appreciate the extra few seconds it took you to come up with self-explaining names.
    – Dunk
    Jul 25 '16 at 21:41
  • 1
    I'll also add that if coming up with good names becomes hard then the odds are very good that your design is pretty poor.
    – Dunk
    Jul 25 '16 at 21:43
  • 1
    The call to the backend looks like a get data request. In that case it might even fit better in a model. And as already stated the naming, this thing seems to return a field. In general: Yes move it into a helper when it's really a controller helper but in this case it seems to be data handling, structuring which does look like a job for a model. Jul 26 '16 at 7:40
3

Several comments:

  1. You've created a closure helper to create a relatively simple object, so you could use a simple function instead of a closure. When you use a substantially more complex construct than necessary, it makes the reader wonder why, and this adds to confusion. The following function does the same without the closure.

        function createReadonlyField (content) {
            if (!content)
                return null;
    
            return {
                name: content.name,
                value: content.val,
                access: access.READ
            };
        }
    

However, I appreciate that you may really need a closure in your real code, that the example may just be overly simplified for question asking purposes.

  1. Generally speaking, extracting something to a function is useful because you can give it a good name. However, it is not necessary especially if you only use it once . Definitely extract to function if using more than once (keep it DRY). And sometimes even if only once, when it is relatively isolated and deserves its name.

Most importantly, you need to look at the abstractions you're creating for the client/user/fellow programmers/yourself. A single helper function is likely too small as a unit of abstraction.

A good abstraction necessarily is a collection of capabilities that work together and can hide details of implementation, can be passed around as an entity, and can be swapped out with an alternate implementation.

A single function generally can't do that, because usually you then need to know what the other functions are for the other capabilities, and we can't swap them all out together. So, that leaves the client/user having to know more about the underlying details of the abstraction in order to reach the missing capabilities.

5
  • I am wondering then, does it make more sence to use helper function (appropriately named) to follow DRY principle, or do not use helper function because it does not meet "good abstraction" requirements?
    – mimo
    Jul 25 '16 at 23:01
  • And you are right, overly simplified closure is just for the sake of example, maybe I should have mentioned it in question.
    – mimo
    Jul 25 '16 at 23:03
  • 1
    I would be fine using a helper function, though keeping a lookout for a refactoring that might combine several helpers and maybe some other functionality into something more substantial, with the potential result of turning otherwise public helpers into more private methods. These are just guidelines, so don't sweat it; just look at the big picture (the API you're providing clients/users/yourself) once in a while, in addition to looking at details.
    – Erik Eidt
    Jul 26 '16 at 0:29
  • Can you provide a little more context ? As is, i would encapsulate the backend call in the 'helper' and give it a better name, but maybe there is reasons you didnt do as such ? But mostly, how is used myField ? Can you provide a workaround to setting a global var at 10 different places ?
    – Arthur Hv
    Jul 26 '16 at 3:55
  • @ArthurHavlicek I have updated code example to little more resemble real world usage. Original example was simplified on purpose, so the the answers can focus on the question (is extracting of logic promoting readability or not) rather than discussing other stuff. But maybe there was missing some context.. Hope it is more clear now.
    – mimo
    Jul 26 '16 at 6:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.