Should i use a more functions to increase readability?

Question

Should I use an easy readable code like this

if (var.isServicePGM() || var.isStandardPGM())
{
    //Much code

    if (var.isServicePGM())
    {
        //Some code
    }
    else if (var.isStandardPGM())
    {
        //Some code
    }
}

or should i use an faster code like this

if (var.isServicePGM() || var.isStandardPGM())
{
    //Much code

    if (var.isServicePGM())
    {
        //Some code
    }
    else
    {
        //Some code
    }
}

You should use whatever is most appropriate to solve your problem. But we don't know what problem above code is supposed to solve so I don't think it is reasonable to give definitive advice here. Readability and performance are both important and you'll often have to make trade-offs. The key to any trade-off is of course knowing what is important in the specific situation. I do question, however, that the “easily readable” code is actually any more readable as its sole difference is that it repeats the obvious. — 5gon12eder, Sep 28 '16 at 08:35
Problem I have here is that //Much code is an indication that your code isn't readable, regardless of the version you use and adding if (var.isStandardPGM()) to the else won't fix that. Yes, make code readable, but first you need to learn to write readable code. — David Arno, Sep 28 '16 at 08:56
Possible duplicate of Should you sacrifice code readability with how efficient code is? — gnat, Sep 28 '16 at 10:06
see also: Why are we supposed to use short functions to sectionalize our code? — gnat, Sep 28 '16 at 10:07

score 5 · Answer 1 · edited Sep 28 '16 at 08:32

5

Not sure how the second code can be faster, only in some cases and only if isStandardPGM() does some hefty calculations. You probably should assign the results to a variable and use those.

bool isService = var.isServicePGM();
bool isStandard = var.isStandardPGM();
if (isService || isStandard)
{
    //Much code
    if (isService)
    {
        //Some code
    }
    else if (isStandard)
    {
        //Some code
    }
}

edited Sep 28 '16 at 08:32

5gon12eder

6,986
2
24
29

answered Sep 28 '16 at 08:25

ElDuderino

160

I don't like this answer, because you're already assuming the comparison between "Standard" and "Service" is the same category. If it really is, why do you have two separate functions to do this? Why not a single function, getServiceType() then compare a String return with "Service", and "Standard"? The above is just a freaking NIGHTMARE to maintain, because it has completely separate functions to return something that really should be done with one function. – Nelson Sep 28 '16 at 08:33
Well obviously I had to assume stuff as I don't know the exact specifications and my answer was rather addressing the difference in speed of those two code pieces and how to make one as fast as the other. I am sure there are many possible ways to design whatever this system will be and you are welcome to write your own answer. But honestly, your approach with String comparison doesn't look so good to me too... – ElDuderino Sep 28 '16 at 09:09
1

Voted up - not because of efficiency, but because we have no idea whether calling isServicePGM or isStandardPGM has any side effects, and whether "//Much code" could change their values. – gnasher729 Sep 28 '16 at 20:33

score 2 · Accepted Answer · edited May 23 '17 at 12:40

2

Premature optimization is evil.

Unless you are absolutely sure that this method is causing a bottleneck and this is already causing issues, you should always focus on readability (and writing bug-free code) first. Highly optimized code tends to be harder to extend and debug in a future.

So, the answer is - yes, you should use more functions to increase readability.

Take a look at this StackOverflow question and answers: https://stackoverflow.com/questions/385506/when-is-optimisation-premature

edited May 23 '17 at 12:40

Community

1

answered Sep 28 '16 at 08:12

Mac70

356

3 months down the line you will have no clue why you called your function names that way. What are you checking for? Neither ServicePGM nor StandardPGM are commonly used nouns, like isInt, isString, isBoolean, etc., so it is extremely vague what its returns are. I am half expecting a boolean, but I really have no idea. Rename them to isType_ServicePGM if you're checking for object type, or use a better class comparison function than a half-baked custom function that has non-obvious returns. – Nelson Sep 28 '16 at 08:27
1

Yes, function naming is an important thing as well. – Mac70 Sep 28 '16 at 08:29
Although in this case, the “faster” version simply avoids repeating the obvious so I doubt that it will be any less readable. – 5gon12eder Sep 28 '16 at 08:30

score 0 · Answer 3 · answered Sep 29 '16 at 14:59

Do not worry about efficiency until you know it's a problem, and you can only tell that when the code is running, not before.
Do worry about maintainability, which is very different from readability. Maintainability is about minimizing the opportunity for people to put in errors when they make changes to the code (for which DRY is a good principle). Readability begs the question - who is reading? Chimpanzees are intelligent, but you'll never make it readable to them. And you can make code appear to be readable just by shoving most of it off the screen with whitespace.

Should i use a more functions to increase readability?

3 Answers3