24

Interfaces to string classes typically have of method named IsEmpty (VCL) or empty (STL). That's absolutely reasonable because it's a special case, but the code that uses these methods often has to negate this predicate, which leads to a "optical (and even psychological) overhead" (the exclamation mark is not very obvious, especially after an opening parenthesis). See for instance this (simplified) code:

/// format an optional time specification for output
std::string fmtTime(const std::string& start, const std::string& end)
{
    std::string time;
    if (!start.empty() || !end.empty()) {
        if (!start.empty() && !end.empty()) {
            time = "from "+start+" to "+end;
        } else {
            if (end.empty()) {
                time = "since "+start;
            } else {
                time = "until "+end;
            }
        }
    }
    return time;
}

It has four negations, because the empty cases are those to be skipped. I often observe this kind of negation, also when designing interfaces, and it's not a big problem but it's annoying. I only wish to support writing understandable and easy-to-read code. I hope you'll understand my point.

Maybe I'm only struck with blindness: How would you solve the above problem?


Edit: After reading some comments, I think it's nessessary to say that the original code uses the class System::AnsiString of the VCL. This class provides an IsEmpty method, which is very readable:

 if (text.IsEmpty()) { /* ... */ } // read: if text is empty ...

if not negated:

 if (!text.IsEmpty()) { /* ... */} // read: if not text is empty ... 

...instead of if text is not empty. I think the literal is was better left to the reader's fantasy to let also the negation work well. Ok, maybe not a widespread problem...

6
  • 18
    How more concise than a single extra ! character do you want it? I would suggest the use of local variables to simplify reading the negated expressions. bool hasStart = !start.empty(); then the logic becomes easier to read: if (hasStart || hasEnd) { ... Commented Feb 19, 2014 at 14:00
  • … or you could refactor all the if's to be on positive empty tests, and still eliminate the outermost one. Commented Feb 19, 2014 at 14:03
  • 4
    "I only wish to support writing understandable and easy-to-read code." If people reading your code are having problems understanding !foo.empty() then you have a much bigger problem. Look around, its used everywhere and everyone understands it well.
    – PlasmaHH
    Commented Feb 19, 2014 at 14:10
  • 2
    @PlasmaHH That's true, but a not is clearly harder to overlook...
    – Wolf
    Commented Feb 19, 2014 at 14:18
  • 5
    @Wolf: No, it isn't. I hate when ppl litter code with the additional and, not and or operators provided by C++, instead of just using the more outstanding and equally understandable operators &&, || and !. See it like this: When ppl use the lingual forms, the whole boolean code gets less structured, because there are only words and no more punctuaction; and then, a not is less outstanding. Just like long phrases without any punctuation are hard to read to many people out there in the world and probably space creatures and also it probably has historical reasons that punctuati... Commented Feb 19, 2014 at 14:22

14 Answers 14

30

In most cases you can reverse the order of the ifand the else to clean up the code:

const std::string fmtTime(const std::string& start, const std::string& end)
{
    std::string time;
    if (start.empty() && end.empty()) {
        return time;
    }

    if (start.empty() || end.empty()) {
        if (end.empty()) {
            time = "since "+start;
        } else {
            time = "until "+end;
        }
    } else {
        time = "from "+start+" to "+end;
    }
    return time;
}

Or even cleaner after some more refactoring:

std::string fmtTime(const std::string& start, const std::string& end)
{
    if (start.empty() && end.empty()) {
        return std::string();
    }

    if (start.empty()) {
        return "until "+end;
    }    

    if (end.empty()) {
        return "since "+start;
    }

    return "from "+start+" to "+end;
}

And for the ultimate compactness (although I prefer the previous version, for its readability):

std::string fmtTime(const std::string& start, const std::string& end)
{
    return start.empty() && end.empty() ? std::string()
         : start.empty()                ? "until "+end
         :                  end.empty() ? "since "+start
                                        : "from "+start+" to "+end;
}

Another possibility is to create a helper function:

inline bool non_empty(const std::string &str) {
  return !str.empty();
}

if (non_empty(start) || non_empty(end)) {
...
}
6
  • The helper function approach seems rather interesting - right now I read Scott Meyers Item #23, Third Edition
    – Wolf
    Commented Feb 19, 2014 at 14:15
  • 6
    @JamesKanze Readability is a personal matter. I prefer to read code with less scope levels. The early return also helps to clarify special cases and conditions. Commented Feb 19, 2014 at 14:53
  • Hmm, I'd even almost considering turning the 4 ifs/returns in the last example into a single return with a conditional operator. If formatted correctly it could even be readable and concise.
    – Cruncher
    Commented Feb 19, 2014 at 21:46
  • @Cruncher nesting conditional operators may cause parenthesisitis ;)
    – Wolf
    Commented Feb 20, 2014 at 13:20
  • This sort of logical refactoring, by the way, is very easy in a modern IDE such as NetBeans. Commented Feb 20, 2014 at 20:37
12

I think I'd eliminate the conditions in favor of a little math:

const std::string fmtTime(const std::string& start, const std::string& end) {

    typedef std::string const &s;

    static const std::function<std::string(s, s)> f[] = {
       [](s a, s b) { return "from " + a + " to " + b; }           
       [](s a, s b) { return "since " + a; },
       [](s a, s b) { return "until " + b; },
       [](s a, s b) { return ""; },
    };

   return f[start.empty() * 2 + end.empty()](start, end);
}

Edit: if you prefer, you can express the math as start.empty() * 2 + end.empty(). To understand what's going on, perhaps it's best if I expound on how I thought of things to start with. I thought of things as a 2D array:

enter image description here

(Feel free to swap the "start empty" and "end empty", depending on whether you prefer to think in row-major or column-major order).

The start.empty() and end.empty() (or the logical not of them, if you prefer) each act as as an index along one dimension of this 2D matrix. The math involved simply "linearizes" that addressing, so instead of two rows and two columns, we get one long row, something like this:

enter image description here

In mathematical terms, that's a simple matter of "row * columns + column" (or, again, vice versa, depending on whether you prefer row-major or column-major ordering). I originally expressed the * 2 part as a bit-shift and the addition as a bit-wise or (knowing the least significant bit is empty, because of the previous left-shift). I find that easy to deal with, but I guess I can understand where others might not.

I should probably add: although I've already mentioned row-major vs. column-major, it should be fairly obvious that the mapping from the two "x.empty" values to positions in the array is basically arbitrary. The value we get from .empty() means that we get a 0 when the value is not present, and a 1 when it is. As such, a direct mapping from the original values to the array positions is probably like this:

enter image description here

Since we're linearizing the value we have a few choices for how we do the mapping:

  1. simply arrange the array to suit the values as we get them.
  2. invert the value for each dimension individually (this is basically what led to the original question--the constant use of !x.empty())
  3. Combine the two inputs into a single linear address, then "invert" by subtracting from 3.

For those who doubt the efficiency of this, it actually compiles down to this (with VC++):

mov eax, ebx
cmp QWORD PTR [rsi+16], rax
sete    al
cmp QWORD PTR [rdi+16], 0
sete    bl
lea eax, DWORD PTR [rbx+rax*2]
movsxd  rcx, eax
shl rcx, 5
add rcx, r14
mov r9, rdi
mov r8, rsi
mov rdx, rbp
call    <ridiculously long name>::operator()

Even the one-time construction for f isn't nearly as bad as some might think. It doesn't involve dynamic allocation, or anything on that order. The names are long enough that it looks a little scary initially, but in the end, it's mostly four repetitions of:

lea rax, OFFSET FLAT:??_7?$_Func_impl@U?$_Callable_obj@V<lambda_f466b26476f0b59760fb8bb0cc43dfaf>@@$0A@@std@@V?$allocator@V?$_Func_class@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AEBV12@AEBV12@@std@@@2@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@2@AEBV42@AEBV42@@std@@6B@
mov QWORD PTR f$[rsp], rax

Leaving out the static const doesn't really seem to affect execution speed much. Since the table is static, I think it should be there, but as far as execution speed goes, it's not the kind of massive win we might expect if the table initialization involved four separate dynamic allocations, or anything like that.

7
  • 1
    @Wolf I don't think it's "too clever" - the approach is not too hard to read, and can be easily ported to pre-C++11 compilers with a switch statement. Commented Feb 19, 2014 at 15:34
  • @dasblinkenlight switch isn't in the collection yet... ;)
    – Wolf
    Commented Feb 19, 2014 at 15:40
  • 9
    @dasblinkenlight Not too hard to read? I know a number of C++ programmers who would have to ask for help before they could understand it. I had to look twice myself, and I've done a lot of low level C++, so I am familiar with bit manipulations and the implicit conversion of bool to int (which in itself is something I'd avoid). Commented Feb 19, 2014 at 16:30
  • This is overcomplicated and inefficient. If you really really want to use bitwise logic go for a switch: switch (start.empty() <<1 | end.empty()) { case 0: return ""; case 1: return "until " + b; case 2: return "since " + a; case 3: return "from " + a + " to " + b; } Yet I would prefer the original code, as it is easier to understand, and after compiler optimization it could look like this. Commented Feb 20, 2014 at 10:34
  • 1
    @C.E.Gesser: Upon what basis do you conclude that this is inefficient? Have you actually timed/profiled it? I have to admit I'm a bit puzzled by the claims that this is hard to understand. Which part do you find difficult, the multiplication by 2 or the bitwise or? Commented Feb 20, 2014 at 11:08
7

You could say

if (theString.size()) { .... }

Whether that is more readable is a different matter. Here you are calling a method whose primary purpose is not to tell you if the thing is empty, and relying on an implicit conversion to bool. I would prefer the !s.empty() version. I might use not instead for fun:

if (not theString.empty()) { .... }

It might be interesting to see the correlation between people who find the ! and not versions confusing.

7
  • That's not applicable generally - size() (or its equivalent) can have different complexity than empty(). AFAIK, std::shared_ptr::unique() is more efficient than std::shared_ptr::use_count(). Commented Feb 19, 2014 at 14:02
  • 2
    @Wolf: I find the not-operator more obvious. And size() has O(n) complexity for some containers, while empty() is O(1). Commented Feb 19, 2014 at 14:19
  • 1
    @phresnel Agreed, plus, size() is for getting a thing's size! But are you aware of any container whose size() method is O(n) in C++11? I know about C++03's std::list. Commented Feb 19, 2014 at 14:25
  • @juanchopanza: Yikes. It looks like the standard has changed wording that now all container's size() should be O(const). The semantic issue of course is untouched by this. I am looking whether the container empty() or ! empty(), not for its size. Commented Feb 19, 2014 at 14:34
  • 4
    @phresnel: and that was a very unfortunate change, since now list::splice is O(N) just so it can count the elements and update the lists' sizes accordingly :( Commented Feb 19, 2014 at 14:38
5

I have to refactor this, purely out of anal retentive disorder…

std::string fmtTime( const std::string & start, const std::string & end ) {
    if ( start.empty() ) {
        if ( end.empty() ) return ""; // should diagnose an error here?

        return "until " + end;
    }

    if ( end.empty() ) return "since " + start;

    return "from " + start + " to " + end;
}

There… clean clean clean. If something here is difficult to read, add a comment, not another if clause.

4
  • @MatthieuM. You can't reason logically about the code if there are returns all over the place. There are cases where multiple returns are reasonable: a switch or an if/else if chain in which every case ends with are return, for example. But something like the above is simply unacceptable, and would get a programmer fired in any place I've worked. Commented Feb 19, 2014 at 16:27
  • 1
    @JamesKanze: well, it's amusing, because at the place I work this code would be an example of good practice (if it had sysmetic {} even for one-liners if) and the nested if/else presented by the OP would never pass code-reviews. Commented Feb 19, 2014 at 17:24
  • @MatthieuM. But the code above also has nested if/else (except they've hidden the else). If you think of the return as a go to the end of the function (which is what it actually is), then the above is real spaghetti. Commented Feb 19, 2014 at 17:38
  • 6
    @JamesKanze Except it's not. Think of return as eliminating the given condition from the remainder of the function. An early return traps errors or otherwise narrows the semantic scope of what's happening. Special cases like "if both are empty" shouldn't add nesting to the entire body, with everything else! Commented Feb 19, 2014 at 23:30
4

Usually it's just better to not use such complicated conditional code. Why not keep it simple?


const std::string fmtTime(const std::string& start, const std::string& end)
{
    if (start.empty() && end.empty())
    {
        return "";
    }

    // either start or end or both are not empty here.

    std::string time;

    if (start.empty())
    {
        time = "until "+end;
    }
    else if (end.empty())
    {
        time = "since "+start;
    }
    else // both are not empty
    {
        time = "from "+start+" to "+end;
    }

    return time;
}

3
  • What about return std::string(); ? but the rest ready good.
    – Wolf
    Commented Feb 19, 2014 at 15:08
  • I don't think it's really important here whether it should return std::string() or just "". Commented Feb 19, 2014 at 15:18
  • return std::string(); may be faster, as return "" will call std::string(const char*), witch will need to check the length of its argument. Commented Feb 19, 2014 at 15:24
3

Globally, I have no problem with the way you've written it; it's certainly cleaner that the alternatives that others are proposing. If you're worried about the ! disappearing (which is a legitimate worry), use more white space.

if ( ! start.empty() || ! end.empty() ) ...

Or try using the keyword not instead:

if ( not start.empty() || not end.empty() ) ...

(With most editors, the not will be highlighted as a keyword, which will draw even more attention to it.)

Otherwise, two helper functions:

template <typename Container>
bool
isEmpty( Container const& container )
{
    return container.empty();
}

template <typename Container>
bool
isNotEmpty( Container const& container )
{
    return !container.empty();
}

This has the added advantage of giving the functionality a better name. (Function names are verbs, so c.empty() logically means "empty the container", and not "is the container empty". But if you start wrapping all of the functions in the standard library that have poor names, you've got your work cut out for you.)

4
  • Interesting point. Sadly, the white spaces are problematic with my formatter, the not isn't supported in my development environment.
    – Wolf
    Commented Feb 19, 2014 at 14:31
  • Sorry, I overlooked your helper functions, really nice ones - but not applicable in my company I fear ;)
    – Wolf
    Commented Feb 19, 2014 at 14:40
  • @C.E.Gesser Maybe, but that's not the full truth: Is there only one right way to read the if (container) condition? These templates seem to be intended to work globally.
    – Wolf
    Commented Feb 19, 2014 at 14:56
  • Your approach reduces the tokens, the negation (and therefore the psychological load) keeps intact.
    – Wolf
    Commented Feb 21, 2014 at 10:31
3

Without using negation.. ;)

const std::string fmtTime(const std::string& start, const std::string& end)
{
   std::string ret;
   if (start.empty() == end.empty())
   {
     ret = (start.empty()) ? "" : "from "+start+" to "+end;
   }
   else
   {
     ret = (start.empty()) ? "until "+end : "since "+start;
   }
   return ret;
}

EDIT: okay cleaned up a little more...

1
  • That's true. But it's also harder to get. (I said not hard, but harder, and it has also 4 returns)
    – Wolf
    Commented Feb 19, 2014 at 14:22
3

Since no one cared to type the complete answer with my comment, here it goes:

Create local variables that simplify the reading of expressions:

std::string fmtTime(const std::string& start, const std::string& end)
{
    std::string time;
    const bool hasStart = !start.empty();
    const bool hasEnd   = !end.empty();
    if (hasStart || hasEnd) {
        if (hasStart && hasEnd) {
            time = "from "+start+" to "+end;
        } else {
            if (hasStart) {
                time = "since "+start;
            } else {
                time = "until "+end;
            }
        }
    }
    return time;
}

The compiler is smart enough to elide those variables, and even if it did not, it won't be less efficient than the original (I expect both to be a single test of a variable). The code now is a bit more readable for a human that can just read the conditions:

if has start or end then

Of course you might also do different refactors to further simplify the number of nested operations, like singling out when there is no start or end and bailing out early...

7
  • Not a bad idea. But if the nesting is a problem, if ( hasStart && hasEnd ) ... else if ( hasStart ) ... else if ( hasEnd ).... Nice and clear, with no nested conditions (for a human reader, at least). Commented Feb 19, 2014 at 14:32
  • @JamesKanze the nesting was already there ;) ...but if this could be improved... let's see...
    – Wolf
    Commented Feb 19, 2014 at 14:37
  • @Wolf I know. But of course, the less you nest, the clearer the code (provided you don't throw in conditional returns and such, which are even worse than excessive nesting). Commented Feb 19, 2014 at 16:25
  • @JamesKanze: Agree, while in the past I have heard that single return makes code more readable, if that is going to require having multiple levels of nested branches, it soon becomes harder to reason about. Simple code, low nesting, proper names for variables... all helps readability Commented Feb 19, 2014 at 17:18
  • 1
    @DavidRodríguez-dribeas: IMHO, if the purpose of a function is to return something without side-effects, then returning computations directly is clearer than storing them to a temporary except in cases where one benefits from being able to change the temporary after it's written (if code writes to the temporary and then later returns it, one must inspect the code between the write and the return to see if the returned value will match what was initially written). If a function will have side-effects, all returns should be before the first one or after the last.
    – supercat
    Commented Feb 19, 2014 at 21:48
2

I struggle with the psychological overhead of negative logic as well.

One solution to this (when it cannot be avoided) is to check for the explicit condition, consider:

if (!container.empty())

vs

if (container.empty() == false)

The second version is easier to read because it flows as you would read it out loud. It also makes it clear that you're checking a false condition.

Now if that is still not good enough for you, my advice would be to create a thin wrapper class that inherits from whatever container you're using and then create your own method for that particular check.

For example with strings:

class MyString : public std::string
{
   public:
     bool NotEmpty(void)
     { 
       return (empty() == false); 
     }
};

Now it becomes just:

if (container.NotEmpty())...
12
  • 1
    In my experience there are few software folks who can really appreciate clearly written source code. The problem also is that this is sort of a subjective choice. I try to make an argument that code should be written the way you would read it as an english sentence. Sometimes that helps but there are many who are just so comfortable with dealing in negative logic they can't be convinced.
    – Lother
    Commented Feb 19, 2014 at 14:14
  • @Lother: But empty() == false is not a very english phrase. You don't say "The bucket's emptiness is false", you say "The bucket is empty" or "The bucket is not empty", i.e. bucket.empty() or ! bucket.empty(). Commented Feb 19, 2014 at 14:51
  • @Wolf: Oh noes, this one, too? I thought you were more on the lingual side, whereas the ==false form is neither very lingual, nor natural to the programming language? Commented Feb 19, 2014 at 14:52
  • @phresnel It's the lesser evil compared to if(!predicate) because it's fast to scan (who reads this time?), but you are absolutely right: it reads ugly.
    – Wolf
    Commented Feb 19, 2014 at 15:03
  • 1
    @Lother: Must say I still don't follow. I follow the argument that negative logic is bad (I always strive to keep it to a minimum, but not less). There should always only be one indirection, if you have more, you are breaking the Law of Demeter. And instead of a single boolean expression, you now reduce two boolean expressions to a single one, via an indirection or set combinator that is operator==. Commented Feb 19, 2014 at 16:41
2

If all you're concerned about is the ease with which ! can be overlooked, you can use the standard C++ alternative token not instead:

const std::string fmtTime(const std::string& start, const std::string& end)
{
    std::string time;
    if (not start.empty() or not end.empty()) {
        if (not start.empty() and not end.empty()) {
            time = "from "+start+" to "+end;
        } else {
            if (end.empty()) {
                time = "since "+start;
            } else {
                time = "until "+end;
            }
        }
    }
    return time;
}

(Refer to [lex.digraph] in the standard for alternative tokens)

0
2

Would you consider assigned a good opposite?

#include <string>

template <typename CharType>
bool assigned(const std::basic_string<CharType>& s)
{
    return !s.empty();
}

std::string fmtTimeSpec(const std::string& from, const std::string& to)
{
    if (assigned(from)) {
        if (assigned(to)) {
            return "from "+from+" to "+to;
        }
        return "since "+from;
    }
    if (assigned(to)) {
        return "until "+to;
    }
    return std::string();
}

Structural improvements of the "test function" came from numerous useful answers. Special thanks to:

0
1

Coming back to the API design aspect

(it may not be applicable to strings, but on container classes in general)

By pure chance I found an excellent answer to this old question (emphasizes mine)

What about using any()? [...]

in a completely unrelated post being the answer to the question

How do I know if a generator is empty from the start?

To contrast empty and any might be poor in English but it absolutely makes sense in API design.

0

To express the opposite form of ".isEmpty()" usage, I prefer this way:

 if (textView.getText().toString().isEmpty()){

            //do the thing if textView has nothing inside as typed.

 }else if (textView.getText().toString() != ""){

            // do the thing if textView has something inside as typed.
        }

Also, you may use ".equals("")" instead of "!=" typography as recommended by Android Studio.

textView.getText().toString().equals("")
1
  • You seem to answer a C++ question in Java.
    – Wolf
    Commented Oct 21, 2022 at 13:33
0

A better way to express options

To be quite honest: Until now, I didn't even realize that I misused string type to negatively(!) express the presence of boundaries of a range. And that was obviously the real cause of my headache.

C++17 introduced optional. So there is little reason left to complain about the shortcomings (in terms of expressiveness) of empty() and negation.

Let's have a look at a working example,[1] [2] that uses the original string type and – as a proof of concept – another type (int for simplicity, this should better be some date type):

#include <iostream>
#include <optional>
#include <string>
#include <sstream>

template <typename T>
std::string format_range(const std::optional<T>& start,
                         const std::optional<T>& end)
{
    std::stringstream range;
    if (start) {
        if (end) {
            range << "from " << *start << " to " << *end;
        } else {
            range << "since " << *start;
        }
    } else if (end) {
        range << "until " << *end;
    }
    return range.str();
}

template <typename T>
void invoke_format_range(const T& start, const T& end)
{
    using namespace std;
    optional<T> NONE;

    cout << format_range<T>(NONE, NONE) << endl;
    cout << format_range<T>(start, NONE) << endl;
    cout << format_range<T>(NONE, end) << endl;
    cout << format_range<T>(start, end) << endl;
}

int main()
{
    invoke_format_range(std::string("START"), std::string("END"));
    invoke_format_range(1, 12);

    return 0;
}

[1] If you cannot use a C++17 compatible compiler, it is relatively easy to adapt optional using your own rudimentary implementation (or try boost::optional of course).

[2] See online demo at https://onlinegdb.com/OCw2c5mkO

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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