4
\$\begingroup\$

Recently me and colleague had a discussion about the following piece of code (simple bool function that checks if string is a number, +1000 not allowed, -1000, 1234 ... allowed).

He felt that it was hackish, while I thought it was nice, clean and elegant (since it uses STL, not hand made loops).
So is this a judgement call or one of us is wrong? Is this code elegant or hack?

 bool validate(const std::string& m)  
 {
   if (m.empty()) 
     return false;
   return m.end() == find_if(*m.begin() == '-'? ++m.begin() : m.begin(),
                                    m.end(), not1(ptr_fun(isdigit)));
 }
\$\endgroup\$
9
  • 2
    \$\begingroup\$ It's not elegant if it has code for special cases (that's part of my definition of elegant). \$\endgroup\$ Jun 22 '12 at 20:30
  • 2
    \$\begingroup\$ For somebody unfamiliar with stl, this is unreadable. Simple for loop is easy, and understood by anybody \$\endgroup\$
    – elmes
    Jun 22 '12 at 20:31
  • 3
    \$\begingroup\$ It seems pretty darn hard to read to me... I would never do so many things in one statement. \$\endgroup\$
    – jahroy
    Jun 22 '12 at 20:31
  • 3
    \$\begingroup\$ @elmes: Even for somebody extremely familiar with STL, this is damned hard to read. \$\endgroup\$ Jun 22 '12 at 20:35
  • 2
    \$\begingroup\$ I have nothing against the algorithms. But would be better if you did not try and do it in one line. One list does not equal elegant. Easy to read (at a glance) equal elegant. \$\endgroup\$ Jun 23 '12 at 0:41
4
\$\begingroup\$

I had to really look at your code and think about what it was doing before I figured it out. That tells me that this code is not elegant by my definition of "elegant". My definition of elegant is something like,

Easy to understand, easy to maintain, efficient in execution, robust.

I would suggest something like this as a start:

if( m.empty() )
  return false;

string::iterator start = m.begin();
if( *start == '-' )
  ++start;

if( m.end() == find_if( start, m_end(), not1(ptr_fun(isdigit)) ))
  return true;
else
  return false;

Not much caring for the use of find_if, I'd refine it to:

if( m.empty() )
  return false;

string::size_type pos = 0;
if( m[0] == '-' )
  pos = 1;

if( m.find_first_not_of("0123456789", pos) == m.end() )
  return true;
else
  return false;

I find this to be much more elegant than the 1-liner you wrote. It's more code, sure. But it is exceedingly simple to understand and maintain. You found a bug yourself just by brain-compiling it. That's what code should look like.

\$\endgroup\$
8
  • 2
    \$\begingroup\$ if i compiled and ran in my head correctly second transformation returns true for "42-1729" \$\endgroup\$ Jun 22 '12 at 22:04
  • \$\begingroup\$ @NoSenseEtAl: Edited, please take a look. \$\endgroup\$ Jun 22 '12 at 22:45
  • 2
    \$\begingroup\$ i guess you also need to remove minus from find_first_not_of, but it is just a minor thing \$\endgroup\$ Jun 22 '12 at 22:58
  • 2
    \$\begingroup\$ if (…) return true; else return false; is a huge antipattern in my book. \$\endgroup\$ Jun 23 '12 at 10:17
  • 1
    \$\begingroup\$ return ( m.find_first_not_of("0123456789", pos) == m.end() ); would be preferable, imho. \$\endgroup\$
    – yannis
    Jun 23 '12 at 14:24
2
\$\begingroup\$

I think in this case, pretty much pure C does the job as well as anything:

bool check_num(std::string const &in) {
    char *end;

    strtol(in.c_str(), &end, 10);
    return !in.empty() && *end == '\0';
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Corner case is that std::string allows '\0' within the string. Unlikely to fail but not foolproof. \$\endgroup\$ Jun 24 '12 at 8:32
2
\$\begingroup\$

If you insist on using find_if (fine by me), and not mutating variables (fine by me as well!), your code still suffers from three deficiencies:

  1. Too much happening in a single statement. Separate.
  2. Why *m.begin() instead of m.front() or even m[0]?
  3. ++ first and foremost mutates a value. ++m.begin(), even though it compiles, smells, since we discard the mutated value and just use the value returned by the expression. This definitely qualifies as a hack.

But since string::iterator is a random-access iterator, you can just write + 1. In cases where that doesn’t work, there’s now std::next which calls std::advance internally and returns the modified result.

bool validate(const std::string& m)  
{
    if (m.empty()) 
        return false;

    auto begin = m[0] == '-' ? std::next(m.begin()) : m.begin();
    return find_if(begin, m.end(), not1(ptr_fun(is_digit))) == m.end();
}
\$\endgroup\$
9
  • \$\begingroup\$ @3. what variable, string or it returned by m.begin()? Afaik it modifies iterator, so it is legit. std::string s="C++11"; ++s.begin(); ++s.begin(); ++s.begin(); char c=*s.begin(); assert (c='S'); \$\endgroup\$ Jun 23 '12 at 13:58
  • \$\begingroup\$ @NoSenseEtAl I take that objection back partially. However, the operation is still not really meaningful, since ++ modifies its argument. But we discard that modified value, since it’s a temporary. This still works since prefix ++ also returns the modified value but it’s still not an entirely meaningful operation – the side-effect was just ignored. \$\endgroup\$ Jun 23 '12 at 14:08
  • \$\begingroup\$ still I dont get your objection, that temporary lives long enough to be used as input param (begin iterator for find_if). So to make it clear is it a stye or correctness objection? \$\endgroup\$ Jun 23 '12 at 14:10
  • \$\begingroup\$ @NoSenseEtAl It’s an objection of style, not of correctness (it was initially, but that was my mistake, because the style confused me). \$\endgroup\$ Jun 23 '12 at 14:15
  • \$\begingroup\$ Ah, just noticed you posted an answer, going for the sane return nonetheless. if (…) return true; else return false; made me chuckle. \$\endgroup\$
    – yannis
    Jun 23 '12 at 14:29

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.