Skip to main content
Spelling fixes
Source Link
Toby Speight
  • 88.7k
  • 14
  • 104
  • 327

###Pass by const reference to avoid a copy.

Pass by const reference to avoid a copy.

Not sure this is true.

    if (PossiblePalindrome.size() < 3)
    {
        Palidrime = false;
    }

###Not sure this is true. if (PossiblePalindrome.size() < 3) { Palidrime = false; } UnlessUnless they gave you special instructions. A zero and one length string is a palindrome.

###Use auto when you can

Use auto when you can

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

###Prefer to use pre-increment.

Prefer to use pre-increment.

It makes very littellittle difference in this context (so I am not bashing you for this exact usage). But it does make a tiny difference and if used in the wrong place can cause a difference.

###Why not use a break in the loop.

Why not use a break in the loop?

###Be consistent with braces.

Be consistent with braces.

Notice that the operator>> only readreads a single space separated word.

If you ignore space and capatolizationcapitalization the above is an palindrome. You could have used std::getline() to read a line of text from the user.

###Use ternary operator (judicially)

Use ternary operator (judicially)

###Prefer '\n' over std::endl

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++ ioi/o based code. The system libraries are practically optimal in the timing for flush and any attempt by you will just usually screw things up.

###Pass by const reference to avoid a copy.

###Not sure this is true. if (PossiblePalindrome.size() < 3) { Palidrime = false; } Unless they gave you special instructions. A zero and one length string is a palindrome.

###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 to use pre-increment.

It makes very littel difference in this context (so I am not bashing you for this exact usage). But it does make a tiny difference and if used in the wrong place can cause a difference.

###Why not use a break in the loop.

###Be consistent with braces.

Notice that the operator>> only read a single space separated word.

If you ignore space and capatolization the above is an palindrome. You could have used std::getline() to read a line of text from the user.

###Use ternary operator (judicially)

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

Pass by const reference to avoid a copy.

Not sure this is true.

    if (PossiblePalindrome.size() < 3)
    {
        Palidrime = false;
    }

Unless they gave you special instructions. A zero and one length string is a palindrome.

Use auto when you can

Sure you can go full out and specify the types. But in modern C++ we usually 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 to use pre-increment.

It makes very little difference in this context (so I am not bashing you for this exact usage). But it does make a tiny difference and if used in the wrong place can cause a difference.

Why not use a break in the loop?

Be consistent with braces.

Notice that the operator>> only reads a single space separated word.

If you ignore space and capitalization the above is an palindrome. You could have used std::getline() to read a line of text from the user.

Use ternary operator (judicially)

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++ i/o based code. The system libraries are practically optimal in the timing for flush and any attempt by you will just usually screw things up.

added 3712 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

Lets compare this to yours:

###Pass by const reference to avoid a copy.

bool IsPalindrome(std::string PossiblePalindrome)

Here you are passing the string by value. This basically causes the string to be copied (an O(n) operation).

###Not sure this is true. if (PossiblePalindrome.size() < 3) { Palidrime = false; } Unless they gave you special instructions. A zero and one length string is a palindrome.

###Use auto when you can

        std::string::iterator leftSide = PossiblePalindrome.begin();
        std::string::iterator rightSide = PossiblePalindrome.end();

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

        auto leftSide = PossiblePalindrome.begin();
        auto rightSide = PossiblePalindrome.end();

###Prefer to use pre-increment.

It makes very littel difference in this context (so I am not bashing you for this exact usage). But it does make a tiny difference and if used in the wrong place can cause a difference.

So since the pre-increment achieves the same result and is optimal prefer to use this one as in the long run it will pay off.

        rightSide--;

###Why not use a break in the loop.

        while ((leftSide < rightSide) && Palidrime)
        {
            if (*leftSide != *rightSide)
            {
                Palidrime = false;
                // I would just have put a break here.
                // this simplifies the loop test above.
            }
            leftSide++;
            rightSide--;
        }

If you simplify the loop it becomes:

        for (;(leftSide < rightSide); ++leftSide, --rightSide)
        {
            if (*leftSide != *rightSide)
            {
                Palidrime = false;
                break;
            }
        }

###Be consistent with braces.

You normally line your braces up on the same horizontal. But here you place it on the end of the line:

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

Both are fine. BUT for neat code you should be consistent in your usage.

Notice that the operator>> only read a single space separated word.

    std::cin >> PossiblePalindrome;

There are a lot of interesting palindromes that use spaces (that are ignored).

    A man a plan a canal Panama

If you ignore space and capatolization the above is an palindrome. You could have used std::getline() to read a line of text from the user.

###Use ternary operator (judicially)

    if (IsPalindrome(PossiblePalindrome))
    {
        std::cout << " is";
    }
    else
    {
        std::cout << " is not";
    }
    std::cout << " a palindrome" << std::endl;

This case can be simplified using the ternary operator?

    std::cout << "The string "
              << PossiblePalindrome
              << " is"
              << (!IsPalindrome(PossiblePalindrome) ? " not":"")
              << " a palindrome" << "\n";

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

Lets compare this to yours:

###Pass by const reference to avoid a copy.

bool IsPalindrome(std::string PossiblePalindrome)

Here you are passing the string by value. This basically causes the string to be copied (an O(n) operation).

###Not sure this is true. if (PossiblePalindrome.size() < 3) { Palidrime = false; } Unless they gave you special instructions. A zero and one length string is a palindrome.

###Use auto when you can

        std::string::iterator leftSide = PossiblePalindrome.begin();
        std::string::iterator rightSide = PossiblePalindrome.end();

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

        auto leftSide = PossiblePalindrome.begin();
        auto rightSide = PossiblePalindrome.end();

###Prefer to use pre-increment.

It makes very littel difference in this context (so I am not bashing you for this exact usage). But it does make a tiny difference and if used in the wrong place can cause a difference.

So since the pre-increment achieves the same result and is optimal prefer to use this one as in the long run it will pay off.

        rightSide--;

###Why not use a break in the loop.

        while ((leftSide < rightSide) && Palidrime)
        {
            if (*leftSide != *rightSide)
            {
                Palidrime = false;
                // I would just have put a break here.
                // this simplifies the loop test above.
            }
            leftSide++;
            rightSide--;
        }

If you simplify the loop it becomes:

        for (;(leftSide < rightSide); ++leftSide, --rightSide)
        {
            if (*leftSide != *rightSide)
            {
                Palidrime = false;
                break;
            }
        }

###Be consistent with braces.

You normally line your braces up on the same horizontal. But here you place it on the end of the line:

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

Both are fine. BUT for neat code you should be consistent in your usage.

Notice that the operator>> only read a single space separated word.

    std::cin >> PossiblePalindrome;

There are a lot of interesting palindromes that use spaces (that are ignored).

    A man a plan a canal Panama

If you ignore space and capatolization the above is an palindrome. You could have used std::getline() to read a line of text from the user.

###Use ternary operator (judicially)

    if (IsPalindrome(PossiblePalindrome))
    {
        std::cout << " is";
    }
    else
    {
        std::cout << " is not";
    }
    std::cout << " a palindrome" << std::endl;

This case can be simplified using the ternary operator?

    std::cout << "The string "
              << PossiblePalindrome
              << " is"
              << (!IsPalindrome(PossiblePalindrome) ? " not":"")
              << " a palindrome" << "\n";

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

Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

Using the standard functions it becomes relatively simple (a one liner).

#include <iostream>
#include <algorithm>
#include <iterator>

int main()
{
    std::string data = "panap";
    std::cout << std::boolalpha
              << std::equal(std::begin(data),
                            std::begin(data) + (std::size(data) / 2), 
                            std::rbegin(data))
              << "\n";
}