Can somebody look at my C# code?

[Raziaar]

This isn't for any game... i'm starting to learn C#(C sharp) for eventual migration into game development, especially with muds. I figure this is the best place to go to ask for critique on my code, for those of you who are familiar with C#.

Anyways... I didn't add really any comments, since I didn't intend to for this test program. This program is something I wrote after reading a chapter in a book called C# programming for the absolute beginner. I was trying to let the stuff in the chapter sink in, and so I tried writing this program without constant looking back and forth. Basically from memory. Can you tell me where I could improve it? I know I could/should get rid of the goto, but I wanted to include it just to say I know how to use them <grins>

What the program is supposed to do is ask the person for their name, then ask them to provide a sentence. It will then take that sentence, break it down into individual words, showing the user, then it will proceed to break each of those individual words down into the first letter of the words, then after that... the remaining letters of the word. If there are no remaining letters(a one letter word), it will notify the user of this fact.

using System;
using System.Collections.Generic;
using System.Text;

namespace ConsoleApplication1
{
    class Program
    {
        static void Main(string[] args)
        {
            string firstLetter;
            string restOfWord;
            string sentence;
            string userName;


            Console.Write("What is your name?  ");
            userName = Console.ReadLine();
            Console.WriteLine();
            Console.WriteLine("Hello there {0}. Please type a sentence and press \"enter\".", userName);
            Console.WriteLine();
            question:
            sentence = Console.ReadLine();
            if (sentence == "") 
            {                   
                Console.WriteLine("You didn't type a sentence! Please type another and press \"enter\".");
                Console.WriteLine();
                goto question;
            }
            Console.WriteLine();
            Console.WriteLine();
            Console.WriteLine("Excellent! Now... we're going to take your sentence and divide it into words.");
            Console.ReadLine();
            Console.WriteLine();
            foreach (string word in sentence.Split())
            {
                Console.WriteLine("{0}", word);
            }
            Console.WriteLine();
            Console.WriteLine();
            Console.WriteLine("Next, we're going to show you the first letter of each word.");
            Console.ReadLine();
            Console.WriteLine();
            foreach (string word in sentence.Split())
            {
                firstLetter = word.Substring(0, 1);
                Console.WriteLine("{0}  --  is the first letter of the word: {1}", firstLetter, word);
            }
            Console.WriteLine();
            Console.WriteLine();
            Console.WriteLine("Finally, we'll show you all the rest of the letters in the word, excluding the first.");
            Console.ReadLine();
            Console.WriteLine();
            foreach (string word in sentence.Split())
            {
                restOfWord = word.Substring(1, word.Length - 1);
                if (restOfWord == "")
                {
                    Console.WriteLine("\tThere are no remaining letters in the word: {0}", word);
                }

                else
                {
                    Console.WriteLine();
                    Console.WriteLine("{0}  --  are the remaining letters of the word: {1}", restOfWord, word);
                }
            }
            Console.WriteLine();
            Console.WriteLine();
            Console.WriteLine("Press \"enter\" to exit the program.");
            Console.ReadLine();
        }//end main
    }//end class
}//end namespace

NOTE: if this is in the wrong area(probably is), please move it to the right spot?

 

[Cole]

I never touched Console C# but im sure I could help.

Also when dealing with strings lets say minipulating them....like lowercase, uppercase, and things like that USE STRINGBUILDER. You see lets say I did....

string hey = "DagDS";
if(hey.toupper() == "DAGDS")
{
}
in memory it actually creates a new string. This is a waste of space! Then you have to wait for the garabage collector to delete it. Stringbuilder however gets past this and is much more memory efficent when dealing with strings.

Also
I simplified the code to this and it worked the same(Getting rid of all the useless Writelines();
"
using System;
using System.Collections.Generic;
using System.Text;

namespace ConsoleApplication1
{
class Program
{
static void Main(string[] args)
{
string firstLetter;
string restOfWord;
string sentence;
string userName;


Console.Write("What is your name? ");
userName = Console.ReadLine();
Console.WriteLine("Hello there {0}. Please type a sentence and press \"enter\".", userName);
question:
sentence = Console.ReadLine();
if (sentence == "")
{
Console.WriteLine("You didn't type a sentence! Please type another and press \"enter\".");
goto question;
}
Console.WriteLine("Excellent! Now... we're going to take your sentence and divide it into words.");
Console.ReadLine();
foreach (string word in sentence.Split())
{
Console.WriteLine("{0}", word);
}
Console.WriteLine("Next, we're going to show you the first letter of each word.");
Console.ReadLine();
foreach (string word in sentence.Split())
{
firstLetter = word.Substring(0, 1);
Console.WriteLine("{0} -- is the first letter of the word: {1}", firstLetter, word);
}
Console.WriteLine("Finally, we'll show you all the rest of the letters in the word, excluding the first.");
Console.ReadLine();
foreach (string word in sentence.Split())
{
restOfWord = word.Substring(1, word.Length - 1);
if (restOfWord == "")
{
Console.WriteLine("\tThere are no remaining letters in the word: {0}", word);
}

else
{
Console.WriteLine("{0} -- are the remaining letters of the word: {1}", restOfWord, word);
}
}
Console.WriteLine("Press \"enter\" to exit the program.");
Console.ReadLine();
}//end main
}//end class
}//end namespace"

 

[Raziaar]

I never touched Console C# but im sure I could help.

Also when dealing with strings lets say minipulating them....like lowercase, uppercase, and things like that USE STRINGBUILDER. You see lets say I did....

string hey = "DagDS";
if(hey.toupper() == "DAGDS")
{
}
in memory it actually creates a new string. This is a waste of space! Then you have to wait for the garabage collector to delete it. Stringbuilder however gets past this and is much more memory efficent when dealing with strings.

Hmm... I haven't learned about stringbuilder yet in the book i'm learning from. thanks though.

Also. can you give me an example of the stringbuilder command?

EDIT: I saw this on a forum from some searching around.

"The String object is immutable. Every time you use one of the methods in the System.String class, you create a new string object in memory, which requires a new allocation of space for that new object. In situations where you need to perform repeated modifications to a string, the overhead associated with creating a new String object can be costly. The System.Text.StringBuilder class can be used when you want to modify a string without creating a new object. For example, using the StringBuilder class can boost performance when concatenating many strings together in a loop."

So, to answer the question, at what point is it more efficient, I wrote a little tester. The StringBuilder returned Sub-10ms times (DateTime not capable of registering < 10ms) at 1000 or fewer iterations of a loop simply appending the string "string". The += method of concatenation takes 15+ms for the same iterations. At 5000 iterations, the += method was becoming much slower at 218ms. versus the StringBuilder which was still sub 10ms.Even at 10000 iterations, the SB was sub 10, while the concat method had gone over 1.2 seconds. My test code is as follows:

private void UpdateTime()

{

int iterations = Int32.Parse(txtIterations.Text);

string theString = txtTheString.Text;

DateTime strCall = DateTime.Now;

string targetString = null;

for(int x = 0 ; x < iterations ; x++)

{

targetString += theString;

}

TimeSpan time = (DateTime.Now - strCall);

txtConcatTime.Text = time.TotalMilliseconds.ToString();

//StringBuilder

DateTime inCall = DateTime.Now;

StringBuilder sb = new StringBuilder(theString);

for(int x = 0 ; x < iterations ; x++)

{

sb.Append(theString);

}

time = (DateTime.Now - inCall);

txtStringBTime.Text = time.TotalMilliseconds.ToString();

MessageBox.Show("done");

}

 

[Kallex]

Hello!

Few things to improve your code (moreover abstract guidelines):
- Never use goto. When you really know how to properly use it, you know never ever use it. It's really painful to try to figure out what a block of code does (especially if there's some bug in it) when there are few gotos tossing the control flow all around with god-knows what variable values are set on every round.

There are enough different tools for adjusting the control flow properly without having to rely on "goto". If goto is needed, then it simply is an implication that the code control flow has to be refactored so that other means become available.

Info: About controlling the code flow I mean the if/else blocks and the while(), for(), foreach() do/while() and alike blocks.


- Keep reading the book about the methods; in general write your logic operations in form of logically independent methods that perform understandable logical functions and that can be then called from the higher level functions that control the overall flow.

Appropriately dividing the functionality to methods will be one of the tools to avoid "duplicate code", that introduces multiple fixing points when there is a problem.

Usually if you are doing copy-pasting of code, it is a clear sign that you actually should be isolating the copy-pasted code within logical method(s) that you can call in all the places that you need the functionality.


- After getting basic grasp with the methods go on and read about object orientation and grouping functionality within independent objects. During that you will learn to use methods when dealing with particular object instances.

Some methods are neutral (static) such as they simply perform block of code without any relevation to any particular instance. Other methods in C# are running within the scope of the object that they're called from.



Those should get you bit further in the process; you should get the tools to start writing bigger yet still logically simple programs.