25

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...

Improve this question
7
  • 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. Commented Feb 19, 2014 at 14:10
  • 3
    @PlasmaHH That's true, but a not is clearly harder to overlook... 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

Reset to default
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)) { ... }
Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

The helper function approach seems rather interesting - right now I read Scott Meyers Item #23, Third Edition
@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.
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 nesting conditional operators may cause parenthesisitis ;)
This sort of logical refactoring, by the way, is very easy in a modern IDE such as NetBeans.
|
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.

Improve this answer

7 Comments

@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.
@dasblinkenlight switch isn't in the collection yet... ;)
@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).
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.
@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?
|
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.

Improve this answer

7 Comments

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().
@Wolf: I find the not-operator more obvious. And size() has O(n) complexity for some containers, while empty() is O(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.
@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.
@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 :(
|
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.

Improve this answer

4 Comments

@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.
@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.
@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.
@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!
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; }
Improve this answer

3 Comments

What about return std::string(); ? but the rest ready good.
I don't think it's really important here whether it should return std::string() or just "".
return std::string(); may be faster, as return "" will call std::string(const char*), witch will need to check the length of its argument.
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.)

Improve this answer

4 Comments

Interesting point. Sadly, the white spaces are problematic with my formatter, the not isn't supported in my development environment.
Sorry, I overlooked your helper functions, really nice ones - but not applicable in my company I fear ;)
@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.
Your approach reduces the tokens, the negation (and therefore the psychological load) keeps intact.
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...

Improve this answer

1 Comment

That's true. But it's also harder to get. (I said not hard, but harder, and it has also 4 returns)
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...

Improve this answer

7 Comments

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).
@JamesKanze the nesting was already there ;) ...but if this could be improved... let's see...
@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).
@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
@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.
|
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())...
Improve this answer

12 Comments

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: 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().
@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?
@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.
@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==.
|
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)

Improve this answer

Comments

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:

Improve this answer

Comments

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.

Improve this answer

Comments

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("")
Improve this answer

1 Comment

You seem to answer a C++ question in Java.
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

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.