7
\$\begingroup\$

The goal of my assignment is simple:

Write a program that converts a given text to "Pig Latin". Pig Latin consists of removing the first letter of each word in a sentence and placing that letter at the end of the word. This is followed by appending the word with letters "ay".

Example

Input: THIS IS A TEST Output: HISTAY SIAY AAY ESTTAY

I want to know if there is any way to write this part of the code in a different/better way:

foreach (string word in engword.Split())

Here is my full code:

string engword = textBox1.Text; //english word
            string pig1 = ""; //pig latin
            string pig2 = ""; //first letter
            string space = " ";
            string extra = ""; //extra letters
            int pos = 0; //position

           foreach (string word in engword.Split())
           {
               if (pos != 0)
               {
                   pig1 = pig1 + space;
               }

               else
               {
                   pos = 1;
               }

               pig2 = word.Substring(0,1);
               extra = word.Substring(1, word.Length - 1);
               pig1 = pig1 + extra + pig2 + "ay";

            }

            MessageBox.Show(pig1.ToString());
        }
    }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ string.Empty is generally preferred to the empty string literal, "". Also if pos only ever equals 1 or 0, why not use a bool? \$\endgroup\$
    – p.s.w.g
    Apr 8, 2013 at 5:40

1 Answer 1

20
\$\begingroup\$

This looks pretty decent for a beginner. Some suggestions:

  • Making a variable space instead of using the literal directly is actually a kind of nice idea. You can improve the code by marking the local const.

  • Why is pos an integer? it only has two values, and you are using it to test a condition. Use a bool instead.

  • For that matter, why have pos in the first place? Pos tells you the same thing as pig1.Length == 0.

  • Any time you write a comment, ask yourself did I add this comment because the code was unclear? and then can I write the code so clearly that I don't need the comment? You say

    string pig2 = ""; // First letter
    

    Dude. If the name of the variable is firstLetter then you don't need the comment.

  • pig2 is just one letter, so it can be a char instead of a string. You can say:

    char firstLetter;
    ...
    firstLetter = word[0];
    
  • You're using pig1 as an accumulator; you're accumulating the final result piece by piece. This is totally fine for small strings. If that string was really big then this is not an efficient technique; you should use StringBuilder instead if you want to make an accumulator for a large string.

  • Finally, like I said, this is fine for a beginner, walk before you run, and so on. To give you a sense of how an expert would write this code, I'd write it like this:

    string pigLatin = string.Join(" ", 
      engword.Split()
             .Select(word => word.Substring(1, word.Length - 1) + word[0] + "ay"));
    

    Which is nicely compact.

\$\endgroup\$
6
  • 2
    \$\begingroup\$ Unless I'm wildly mistaken, there is no overload on String.Split which takes zero arguments, which may be what lead to the question in the first place? Also, you can just say word.SubString(1). There's no need to specify the length. Also, unless I'm wrong, this will fail if the sentence includes a single letter word. There should be a check on the word length before assuming that word.SubString(1) is a valid index. Other than that, great answer, you covered everything I was going to say. \$\endgroup\$ Apr 8, 2013 at 13:29
  • 2
    \$\begingroup\$ +1 simply for mentioning the oft-overlooked value of self-documenting code and forgoing redundant comments. \$\endgroup\$ Apr 8, 2013 at 14:07
  • 4
    \$\begingroup\$ @StevenDoggart, Split() with 0 arguments is basically equivalent to passing new char[0] to fill the params char[] parameter. Pointing it out in case you were asking. If you were not asking, then forget I said anything. \$\endgroup\$ Apr 8, 2013 at 19:23
  • 4
    \$\begingroup\$ @Blorgbeard, as of .NET 4, string.Join is overloaded to support IEnumerable<string> specifically and IEnumerable<T> generically. \$\endgroup\$ Apr 8, 2013 at 21:13
  • 2
    \$\begingroup\$ As @StevenDoggart has pointed out, it is best practise to verify that input you receive is as you expect before operating on it. Given Eric's example, after the call to .Split(), I would add .Where( word => !string.IsNullOrWhiteSpace( word ) ) so that strings made up of spaces, or having many spaces next to one another, do not cause your program to crash. I would probably also check that textBox1.Text was not null before starting, but it maybe that it can be guaranteed to be not-null. \$\endgroup\$ Mar 11, 2014 at 2:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.