C++ Pig Latin program

Question

I'm just wanting some input on to how others would approach this problem. I am learning and wish to gain insight on others techniques. Be as critical as you need. I want to learn. I feel this code is sloppy and the logic is confusing.

/*
    Pig Latin
    Write a program that reads a sentence as input and converts
    each word    to “Pig Latin.” In one version, to convert a word to
    Pig-Latin you remove the first letter and place that letter at the
    end of the word. Then you append the string “ay” to the word. Here
    is an example:
        English: I SLEPT MOST OF THE NIGHT
        Pig Latin: IAY LEPTSAY OSTMAY FOAY HETAY IGHTNAY
*/
#include <iostream>
#include <string>
using namespace std;

// takes a string argument and returns the pigLatin equivalent
string pigLatin(string);

int main()
{
    string mySentence; 

    getline(cin, mySentence);
    mySentence = pigLatin(mySentence);
    cout << mySentence << endl;

    return 0;
}

string pigLatin(string word){

    //pigLatWord holds word translated in pig latin.
    //pigLatSentence holds entire translated sentence.
    string pigLatWord, pigLatSentence = "";
    int length = 0, index = 0;

    while (word[index] != '\0'){
        // .find returns -1 if no match is found
        if (word.find(' ', index) != -1){
            length = word.find(' ', index);
            length -= index;//length - index = the number of characters in a word
            pigLatWord = word.substr(index, length);
            pigLatWord.insert(length, "ay");
            pigLatWord.insert(length, 1, word[index]);//first letter is inserted at the end of the string
            pigLatWord.erase(0, 1);// erase first letter in string
            index += length + 1;//adding one moves index from 'space' to first letter in the next word
        }
        else{
            pigLatWord = word.substr(index);
            length = pigLatWord.length();
            pigLatWord.insert(length, "ay");
            pigLatWord.insert(length, 1, word[index]);
            pigLatWord.erase(0, 1);
            index = word.length();
        }

        pigLatSentence += (pigLatWord + " ");
    }
    return pigLatSentence;
}

Answer 1

See at @Caridorc first.

Use the full power of the stream.

The standard stream operators will read a word at a time for you.

std::string word;
while(std::cin >> word)
{
     // DO STUFF.
}

The above will read a file. If you just want to read a line. Read a line then use string stream.

// Read a line
std::string line;
std::getline(std::cin, line);

// convert line to stream
std::stringstream linestream(line);

// Loop over it one word at a time.
std::string word;
while(linestream >> word)
{
     // Operate on word.
}

DRY your code

Both sides of your if statement are doing the same thing. The difference is handling the last word (one not terminated by a space). You should move common code outside the if statement just leave the non common code in the if statement.

    if (<test>)
    {
    }
    else
    {
    }

    // This all seems common across the two if statements.
    pigLatWord = word.substr(index);
    length = pigLatWord.length();
    pigLatWord.insert(length, "ay");
    pigLatWord.insert(length, 1, word[index]);
    pigLatWord.erase(0, 1);
    index = word.length();

Declaration when needed.

Put your variable declarations as close to the point of use as you can. This way you don't need to look for their type very far (it is where the code is). They only get constructed when you need them (and destroyed as soon as possible) so preventing excessive use of space and not costing anything if you don't need them.

// These could be moved to the other side of the if.
// As pigWord is only used after the loop (in my new version)
string pigLatWord;

Declare one variable per line.

string pigLatWord, pigLatSentence = "";
int length = 0, index = 0;

Humans need help in reading code. Don't make it hard for them. Declare one variable per line. This also helps when refactoring code later. You will only move the variables you need and source control tools will find it easier to do a diff on the change.

Pass by const reference.

If you do not plan on modifying the input pass by const reference to prevent a copy.

string pigLatin(string const& word){
             //        ^^^^^^

Design.

If I was actually going to write this I would encapsulate the concept of a Pig Latin word in a class. Then you can localize the change in a single well named class. You can then make use of standard routines to help read/print.

class PigLatinWord
{
    // STUFF
};

int main()
{
    // Copies std::cin to std::cout converting to PigLatin
    // on the copy to the output stream.
    std::copy(std::istream_iterator<std::string>(std::cin),
              std::istream_iterator<std::string>(),
              std::ostream_iterator<PigLatinWord>(std::cout, " ")
             );
}

Answer 2

Avoid the following:

using namespace std;

See: Why is “using namespace std;” considered bad practice?

---

Put main at the bottom to avoid function stubs as:

string pigLatin(string);

---

It's 2015 you can omit return 0 to avoid verbosity, the compiler will automatically add it.

---

Reduce verbosity in the main function:

There is not need to reassign mySentence:

int main() {
    string mySentence;
    getline(cin, string mySentence);
    cout << pigLatin(mySentence) << cin;
}

I prefer open braces on the same line of the function definition but using them as you do is more common.

---

Be more user friendly

If the user sees nothing they will think that the program is hanging or number crunching, add a little message or even better let the programme take command-line args.

---

Don't lie in argument names

If I see the function: string pigLatin(string word) { I am going to assume that it translates to pig latin a hem... word and will crash or be wrong when given a sentence. But your function works fine with a sentence ?!

Answer 3

• There's no need to explicitly initialize with empty quotes:

  pigLatSentence = "";
  

  The default std::string constructor will already do this for you:

  pigLatSentence;
  

• Since length stores a value returned from an STL function, make it type std::size_t or std::string::size_type for this use.

• You could use std::string::npos instead of -1 with string objects:

  if (word.find(' ', index) != std::string::npos)