Is there a concise opposite of "empty"?
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.
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.
In most cases you can reverse the order of the if
and 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)) {
...
}
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:
(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:
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:
Since we're linearizing the value we have a few choices for how we do the mapping:
- simply arrange the array to suit the values as we get them.
- invert the value for each dimension individually (this is basically what led to the original question--the constant use of
!x.empty()
) - 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.