views:

131

answers:

4

Please, help me understand, what's wrong with this code. (I am trying to build a string, taking parts of it line by line from a text file).

I get a runtime error "In the instance of the object reference not set to an object" on the line strbuild.Append(str);

        StreamReader reader = new StreamReader("buf.txt", System.Text.Encoding.ASCII);
        StringBuilder strbuild = new StringBuilder();
        strbuild = null;

        while (reader.Peek() >= 0)
        {
            string str = null;
            str = reader.ReadLine().ToString();

            string segment = str.Substring(0, 1);

            if (segment == "A")
            {
                strbuild.Append(str); //here  i get an error
            }
            else if (segment == "B")
            {
                strbuild.Append("BET");
            }

        }
        printstr = strbuild.ToString();
        reader.Close();

        MessageBox.Show(printstr);
+6  A: 

You are setting the stringbuilder to null after initialization.

Change

StringBuilder strbuild = new StringBuilder(); 
strbuild = null; 

to

StringBuilder strbuild = new StringBuilder(); 

leaving out the line

strbuild = null;
astander
+10  A: 

Look at these lines:

StringBuilder strbuild = new StringBuilder();
strbuild = null;

What do you expect to happen when you then call strbuild.Append(...)? Why are you setting strbuild to null at all?

You seem to be fond of two-line variable initialization - here's another example:

string str = null;
str = reader.ReadLine().ToString();

This would be more easily readable as just:

string str = reader.ReadLine();

(ReadLine already returns a string, so you don't need to call ToString() on the result.)

However, I do suggest that you use a using statement for the StreamReader - otherwise when an exception is thrown, you'll be leaving the reader open.

One nice thing about TextReader.ReadLine() is that it returns null when you're done. You don't need to peek and then read.

Finally, if you're only testing a single character you don't need a substring - just use the string indexer to get a char. So, you could have:

StringBuilder builder = new StringBuilder();

// Consider using File.OpenText
using (StreamReader reader = new StreamReader("buf.txt", Encoding.ASCII))
{
    string line;
    // Normally side-effect + test is ugly, but this is a common and
    // neat idiom
    while ((line = reader.ReadLine()) != null)
    {
        // TODO: What do you want to happen for empty lines?
        char segment = str[0];
        if (segment == 'A')
        {
            builder.Append(line);
        }
        else if (segment == 'B')
        {
            builder.Append("BET");
        }
    }
}
MessageBox.Show(builder.ToString());
Jon Skeet
Thanks a lot for the great answer!
rem
+2  A: 

Change

  StringBuilder strbuild = new StringBuilder();
  strbuild = null;

to

  StringBuilder strbuild = null;
  strbuild = new StringBuilder();

or, to prevent this kind of error:

  StringBuilder strbuild = new StringBuilder();
Henk Holterman
+1  A: 

There are some many errors within your example, here is a first corrected version:

StringBuilder strbuild = new StringBuilder();

// Put resource into using statements, for deterministic cleanup
using (TextReader reader = new StreamReader("buf.txt", System.Text.Encoding.ASCII))
{
    string line;

    //Maybe looks a little ugly the first time, but is commonly used to
    //process all the lines of a file (.ReadToEnd() can cause memory problems
    //on really big files)
    while ((line = reader.ReadLine()) != null)
    {
        //Instead of if, else if, else if, etc just take a switch
        //statement. Makes it much easier to read.
        switch (line[0])
        {
            //If you need case insensitivity put both versions of the character
            //before your first line of code
            case 'A':
            case 'a':
                strbuild.Append(line);
                break;
            //Otherwise just use the lower or upper case version you like
            case 'B':
                strbuild.Append("BET");
                break;
        }
    }
}

//Put the result of the StringBuilder directly into the Show() function
MessageBox.Show(strbuild.ToString());
Oliver
While it's nice that you cleaned it up for them, you didn't explain why you made the changes or what was causing the problem in the first place.
Joshua
@Joshua: So added some comments to the code. Hope this helps ;-)
Oliver
A much better answer! :)
Joshua
+1 for turning attention to possible using of "switch" statement. Thanks!
rem