C# - Use of unassigned local variable 'stringnamehere'

Question

Here's my code:

        private void startButton_Click(object sender, EventArgs e)
    {
        DriveInfo[] drives = DriveInfo.GetDrives();
        string drivenames;
        for (int i = 0; i < drives.Count(); i++)
        drivenames = drives[i].Name;
        MessageBox.Show(drivenames); // --> For debug purposes only
        strCmdText = "del /f /s " + drivenames + "*.sfk";
        System.Diagnostics.Process.Start("CMD.exe", strCmdText);
    }

I'm trying to get the drive letters on the computer to a string, however, when I'm trying to use the string, it says Use of unassigned local variable 'drivenames'. What's the problem here?

Couldn't agree more with @JeroenVannevel - It NEVER appears to me to be worth it getting rid of them, even when it's simply a one-line statement under a simple `if` — , Aug 14 '14 at 14:06
I think he wants the Messagebox in the loop also, so he definitely needs the brackets. — David Crowell, Aug 14 '14 at 14:07
@DeeMac Same thoughts. I've been coding for years, never wrote a single block without `{}` — Sriram Sakthivel, Aug 14 '14 at 14:08
I disagree. Single-statement `if`s and `while` loops are completely fine. Just don't put the statement on the next line, or use a coding tool that will fix indent errors. — Clever Neologism, Aug 14 '14 at 14:17
@CleverNeologism It is not about indentation. It is all about avoiding careless mistakes. — Sriram Sakthivel, Aug 14 '14 at 14:22
@SriramSakthivel It's exactly about indentation. It's almost impossible to make this mistake in properly indented code, and given VS will be able to automatically indent any compilable program correctly, there is no need whatsoever to add lots of superfluous brackets to prevent this type of error. — Servy, Aug 14 '14 at 14:34
@Sriram Thus the option of not putting the single statement following the if on the next line. Unless you are in the habit of putting multiple statements on a single line, you can't carelessly add a second statement, thinking it's part of your `if` statement. Thus, your code is pretty well segregated: all single-statement `if` statements are one line without braces (and looking at code, it's obvious if there's something after the `if`), all multistatement `if`s are multiline and use braces (of course). It's hard to make a mistake, because it won't "look right". — Clever Neologism, Aug 14 '14 at 14:59

score 7 · Answer 1 · edited May 23 '17 at 12:05

7

If drives.Count() is 0 the loop will not be entered. Then the variable is still unassigned. That's why the compiler doesn't like MessageBox.Show(drivenames).

You could assign "" or null:

string drivenames = "";
// ...

The compiler just wants to help you to avoid bugs.

If you instead want to show the MessageBox for every drive you have to move it into the loop as David has shown. You could also use this LINQ query which does not need an explicit loop:

private void startButton_Click(object sender, EventArgs e)
{
    DriveInfo[] drives = DriveInfo.GetDrives();
    string drivenames = string.Join(Environment.NewLine, drives.Select(d => d.Name));
    MessageBox.Show(drivenames); 
}

edited May 23 '17 at 12:05

Community

1
1

answered Aug 14 '14 at 14:03

Tim Schmelter

450,073
74
686
939

If I edit my 'string drivenames' to 'string drivenames = null;', the MessageBox shows up G:\ , but I actually don't even have a drive called G:\. – user3669879 Aug 14 '14 at 14:05
@user3669879: have you used the debugger? – Tim Schmelter Aug 14 '14 at 14:07
And what about the case when I have to enter the drive letters into the command prompt for a command? For instance, I want to run "del /f /s DRIVELETTERSHERE*.filetypehere"? – user3669879 Aug 14 '14 at 14:16

David Crowell · Answer 2 · 2014-08-14T14:24:37.593

5

I'm assuming you want to show a messagebox for each drive. If that's the case, you want something more like this:

private void startButton_Click(object sender, EventArgs e)
{
    DriveInfo[] drives = DriveInfo.GetDrives();
    string drivenames = string.Empty;
    for (int i = 0; i < drives.Count(); i++)
    {
        drivenames = drives[i].Name;
        MessageBox.Show(drivenames); // --> For debug purposes only
    }
}

EDIT Based on comments, this could be dratically simplified to:

private void startButton_Click(object sender, EventArgs e)
{
    foreach (var drive in DriveInfo.GetDrives())
    {
        // todo: write some code here
        MessageBox.Show(drive);
    }
}

Isn't that easier to read?

edited Aug 14 '14 at 14:24

answered Aug 14 '14 at 14:06

David Crowell

3,711
21
28

1

I much personally prefer this, using an empty string, over assigning a null value. It will prevent any unexpected behavior associated with the null value. – Tyress Aug 14 '14 at 14:09
And what about the case when I have to enter the drive letters into the command prompt for a command? For instance, I want to run "del /f /s DRIVELETTERSHERE\*.filetypehere"? – user3669879 Aug 14 '14 at 14:10
2

@Tyress There is no need for the variable to exist outside of the scope of the loop in the first place. The variable shouldn't exist *at all* when it doesn't have a valid value, rather than having some "preferred" default value. Scoping the variable appropriately, or removing it entirely, is far safer. – Servy Aug 14 '14 at 14:11
Also, Use `foreach` wherever you can. Here there is no need of `for` loop. Use `Array.Length` instead of `Count()` extension method. – Sriram Sakthivel Aug 14 '14 at 14:13
@Tyress: what is unexpected, that it is still `null` after the loop? Then it is good to know, isn't it? An empty string could fail silently whereas a `null` value may fail loudly(exception) which is a good thing. (In this case `MessageBox.Show` would not throw an exception) – Tim Schmelter Aug 14 '14 at 14:19
@Servy er, the for loop and the string usage in the original post IS shady. My point was simply assigning null to a string. Not really sure why you'll ever want a string with a null value - I consider it an exception waiting to happen. But that could be a matter of preference. – Tyress Aug 14 '14 at 14:20
@Tyress The code here would function perfectly fine with `null`. There are also plenty of situations in which having a `null` string is fine, normally where you want to make it clear that the value *shouldn't* be used until given a value, and that you *want* it to complain loudly if you are using the value inappropriately rather than silently continuing on despite being in an invalid state. – Servy Aug 14 '14 at 14:22
@TimSchmelter Pretty sure `MessageBox.Show` doesn't throw when passed a null string. – Servy Aug 14 '14 at 14:22
@Servy: ... as i've mentioned in the comment. I just wanted to note that it's more likely that you get an unexpected behaviour(f.e. exception) if you use `null` instead of an empty string which is a good thing (as opposed to Tyres opinion). Using a wrong value is not the best way to handle unexpected behaviours. – Tim Schmelter Aug 14 '14 at 14:25
Well, if the null value works for you, it works for you. I personally don't like it. Most of the time, a string.Empty is what you want to represent a null value too, especially if you're getting data from databases which you will need to coalesce with an empty string to display on some other medium outside of MessageBox.Show(), or write to a different data-interchange format. On those occasions it will complain, and it doesn't have to. As I said, it is preference. I _prefer_ to use a string.Empty. – Tyress Aug 14 '14 at 14:34
If you want to _display_ something you often don't want to see `NULL`. But whether a `null` value or an empty string is more appropriate as default value in general depends on the context. In most cases it is better to treat a currently not defined value as `null` instead of empty. The latter is a valid value. You're also not able anymore to differentiate between the unassigned value and a value that is just empty. If you get a `NullReferenceException` it's a good thing, it helps to make the code more robust. An empty string is likely to fail silently (f.e the user gets incorrect result). – Tim Schmelter Aug 14 '14 at 14:58
What I'm trying to say is I do not necessarily check the context anymore. I just go with string.Empty. Makes life simpler for me. You are correct with a loud NullReferenceException to make sure values are filled, but if I had critical data using strings,I would make other checks just so I don't have to use a string with a null assignent. Here's actually a question on this very topic, and I like the answer: http://stackoverflow.com/questions/6689876/string-empty-vs-null-which-one-do-you-use . – Tyress Aug 14 '14 at 15:09

score 0 · Answer 3 · answered Aug 14 '14 at 14:09

Your code as it stands does not compile, you can make it compile by, as Tim suggests, assigning string drivenames = null.

Your program will then display the last drive name as on each iteration, the previous value will be overridden by the previous and if you don't have any drives then null will be passed to MessageBox.Show which will at best show an empty text box, at worst throw a runtime exception.

Assuming you only want one message box to show, may I suggest the following change to your code:

DriveInfo[] drives = DriveInfo.GetDrives();

IEnumerable<string> driveNames = drives.Select(drive => drive.Name);

string output = String.Join(", ", driveNames);

MessageBox.Show(output); // will display "C, D, E" or similar

C# - Use of unassigned local variable 'stringnamehere'

3 Answers3