A question I've been asked on interactive phone screens is to code a test to determine whether a string is
a palindrome or not. I am providing two examples here: one in C and one in C++. They both work and they both compile
without errors or warnings. I believe my C example is good, with the possible exception of variable names
(some of you may not like the register char* declarations, but they are valid in C).

My goal was an \$O(n)\$ solution with out reversing the string.

My question is about the C++ version, and I miss using iterators. Is there a way to make this more C++ like?

\$\begingroup\$Since you’ve posted the C code: register might be valid C but that’s pretty irrelevant on a Code Review site. Is it good C? No: it’s pretty much obsolete. In fact, if the compiler honours it at all it’s more likely than not to screw with the compiler’s own, much superior optimisation logic. Don’t use register. Definitely not here.\$\endgroup\$
– Konrad RudolphNov 21 '17 at 22:36

\$\begingroup\$@KonradRudolph It does tell the compiler that you're never going to dereference the variable... not that it doesn't know that in 80% of cases.\$\endgroup\$
– wizzwizz4Nov 22 '17 at 18:14

\$\begingroup\$@wizzwizz4 My point exactly. The compile can figure this out on its own just fine.\$\endgroup\$
– Konrad RudolphNov 22 '17 at 18:15

\$\begingroup\$@pacmaninbw In the C++ code you have a variable called Palidrime. Is that a typo or am I just unaware of the word?\$\endgroup\$
– Maybe_FactorNov 23 '17 at 5:54

\$\begingroup\$@Maybe_Factor: And in the C code, a variable named Palendrome. I think typos...\$\endgroup\$
– Matthieu M.Nov 23 '17 at 8:14

10 Answers
10

There are some things to be said about your C version as well, but since you explicitly asked about the C++ version (and also because my C-knowledge is not that great), I will leave those for somebody else to comment on.

General Hints and Tips

You are passing possiblePalindrome by value where a pass by const& would be much more appropriate since you don't modify the string.

This helps eliminate redundant layers of indentation and makes the code more readable.

Since you don't write anything to the argument string, you should use std::string::const_iterator to iterate it (in addition to making the parameter const, see point 1).

Please, don't use std::endl. It's horrible. Why? Because it does not do what its name advises and is pretty much redundant. std::endldoes write an end-of-line character, but it also flushes the underlying buffer, which is seldom what you want (and if you really need that functionality, you should use std::flush).

Making your Code more C++-y

As you remarked yourself, your current code is not very much C++-like, but we're going to change that by making good use of the standard library. As you know, checking whether a string is a palindrome is equal to checking the to halves of the string for equality, only that the latter half is traversed in reverse order. Luckily for us, checking ranges of elements for equality is one of the things that are needed quite frequently by a lot of people, so C++ offers the very useful std::equal.

std::equal, in its most basic form, takes three iterators: one to the beginning of a range of elements, one to its end, and another one that points to the beginning of the range the elements should be compared with. The only problem here is that we actually need our second range to be iterated in reverse. Again, the STL comes to our rescue. There is std::reverse_iterator, which, who would have guessed, allows iterating in reverse order, and also rbegin, which is a member function of std::string that returns a reverse iterator from the end of the string.

However, as you might have already noticed, we're wasting something here. In particular, we iterate through the whole string although we actually only need to check up to the middle. Thus, we adapt our code accordingly:

\$\begingroup\$I assume you meant auto end_it = std::next(s.begin(), std::distance(s.begin(), s.end()) / 2);, @RiaD? And while we're at it, I'd prefer to rename it to, say, middle_it. But I do agree that the line auto end_it = s.begin(); just looks wrong, and forces one to look twice to figure out that it's not a bug in the context where it's used. Better IMO to just immediately set the iterator to the value it's actually going to have.\$\endgroup\$
– Ilmari KaronenNov 22 '17 at 4:56

\$\begingroup\$So @BenSteffan's answer? The over-generalizing critique of other answers probably could have just been a comment.\$\endgroup\$
– SnowhawkNov 21 '17 at 23:01

2

\$\begingroup\$@Snowhawk At least 3 answers here got that midpoint using std::next, std::distance, function templates, or all of the above. Could've just added. Nothing wrong with those approaches for generic code, but prefer the simplest method.\$\endgroup\$
– DavidNov 21 '17 at 23:06

2

\$\begingroup\$@LokiAstari there's some contention over that advice. Personally, I'm with the camp that says use the free functions when you need to use them generically (such as when the container is a template type), otherwise use the member functions. I don't believe in over-generalizing in C++ as it can often be more complicated more verbose code.\$\endgroup\$
– DavidNov 22 '17 at 21:13

1

\$\begingroup\$@LokiAstari, I believe Herb Sutter is on the other camp. There was a proposal authored by him, but I believe it was not accepted.\$\endgroup\$
– IncomputableNov 23 '17 at 5:18

3

\$\begingroup\$@LokiAstari: Actually, if you go the free function route, it should really be using std::begin; begin(c), so that if a type defines a free-standing begin rather than a member function begin it's also picked up. And that really verbose, compared to c.begin(). I don't see the point of using free-standing begin/end when unnecessary.\$\endgroup\$
– Matthieu M.Nov 23 '17 at 8:20

\$\begingroup\$Prefer using std::begin(str) and similar functions over calling str.cbegin() directly (after all, you already do it in middle). Also, that way, is_palindrome could easily be generalized to check any container whether it is equal to its reversal. template<typename Container> bool is_palindrome(const Container& cont) { return std::equal(std::begin(cont), middle(cont), std::rbegin(cont)); }\$\endgroup\$
– hoffmaleNov 21 '17 at 19:01

\$\begingroup\$@Snowhawk, thanks. This might as well be canonical now.\$\endgroup\$
– IncomputableNov 21 '17 at 19:20

1

\$\begingroup\$This is a great solution (+1) but it’s not a code review.\$\endgroup\$
– Konrad RudolphNov 21 '17 at 22:29

Use auto when you can

Sure you can go full out and specify the types. But modern C++ we usally like the compiler to deduce the types (especially if the exact type is not important. For iterators the exact type is not important just the fact that it is in the group of types that implement an iterator).

Prefer '\n' over std::endl

The only difference here is a flush of the buffer.
Manually flushing the buffer is actually the major cause of slow down in C++ io based code. The system libraries are practically optimal in the timing for flush and any attempt by you will just usually screw things up.

So the general case start with '\n' then test to see if std::endl improves (it generally will not).

\$\begingroup\$Palidrime? In the future it might be worth copy/pasting from the question directly...\$\endgroup\$
– Nic HartleyNov 22 '17 at 16:13

\$\begingroup\$@Toby I'm not sure how that matters? It's spelled isPalindrome in the question, not Palidrime. (And if the question has been edited, which I can't easily tell on mobile, then it should be rolled back and the asker informed that code in questions shouldn't be changed after you get an answer)\$\endgroup\$
– Nic HartleyNov 22 '17 at 16:30

Before coding, did you go over the requirements with the interviewer? Are strings limited to just words? What about sentences/phrases? Any adjustments for capital letters, punctuation, word dividers? Consider that the phrases *A man, a plan, a canal, Panama!", "Was it a car or a cat I saw?", or "No 'x' in Nixon" are considered palindromic phrases.

Also, why do you ignore strings of length 1 and 2? Document the reasoning.

int main(int argc, const char * argv[]) {

Omit the parameters of main if you do not intend on utilising command-line arguments.

std::cin >> PossiblePalindrome;

When reading from a stream, check to make sure the read was successful.

\$\begingroup\$A lot of good points. I don't remember the exact requirement, I only thought of single words when I was asked the question. I should have asked that specific question.\$\endgroup\$
– pacmaninbwNov 22 '17 at 16:53

Take care with inputs and outputs

Spot the problem here:

char testString[BUFSIZ];
scanf("%s", testString);

Although testString only has enough space to hold BUFSIZ characters (including the string terminator), we read an unbounded string from standard input. And never test the result of scanf, so we don't know whether any characters were even read.

There's also no checking of the result of printf() - that's arguably not a problem here, but in an interview question, I'd expect to see a comment explaining that you have consciously chosen to skip the check that you would normally put in your production code.

The buffer overrun isn't a problem in the C++ version (as a std::string will expand as necessary), but there is a missing check of cin.good() after reading from it. The idiomatic form is something like

Write for localisation

Not all languages can negate a sentence with a single change to one part like this, so if your program were to be internationalized and ship with message catalogs for each language, this statement would probably need to be re-written. Perhaps something like the following would be better:

printf(iSPalindrome
? "The test string %s is a palindrome\n"
: "The test string %s is not a palindrome\n",
testString);

But I'd lean towards an if/else so that the compiler can still check that the arguments agree with the format string.

The C++ exhibits a similar problem. Additionally, the C version will output a doubled space ("is a") for the negative case. That could have been avoided by moving one of the spaces into the "Not" - but don't compose strings like that anyway.

\$\begingroup\$Yes the C version added the space, I changed it in the C++ version. It's funny, after all the times I beat on people to check input for errors, I forgot it myself. I chose BUFSIZ because it's a common header macro, and it generally evaluates to 1024.\$\endgroup\$
– pacmaninbwNov 22 '17 at 16:47

This is code in the spirit of the standard template library, and it would work for any data structure equipped with bidirectional iterators. It also can use a custom comparision function if you wanted to say, for example that all vowels are equivalent and all constanants are equivalent.