Save as is taking the filename provided, but is changing to null later in the code - which is causing the file never to be created

Question

So, I had a question last time about syntax, and now that the syntax error is fixed, I have a problem that even after my professor looked it over, he doesn't know how to fix. We went through my code line by line, and with the initial save as dialog, everything looks good and the filename/path shows up in the debugger. It passes down to the create file line, and then to the code that I had to add to make my syntax work - It then proceeds to where I'm trying to open the file to be able to use the writeline command with my random number generator - And there instead of opening the appropriate file, it becomes "null" as a value! It doesn't stop there though, it continues on to the random number generator, and rolls the required number of random numbers, but of course since the opening value showed "null" it doesn't save to file like it's supposed to. Oh, and the code that was in my textbook is what generated the first syntax error without providing a way to fix it. Here's the code, sorry if it's long/difficult to read.

using System.IO; // Added to be able to use StreamWriter variable type

namespace Random_Number_File_Writer
{
public partial class Form1 : Form
{ 
    StreamWriter randomNumberFile; //Name streamwriter
    decimal numbers; //Variable to insert the number up down value into
    Random rand1 = new Random(); //Random number generator
    int writeitem; // Variable to insert random number into, to write.

    public Form1()
    {

        InitializeComponent();
    }
    public void saveFileDialog1_FileOk(object sender, CancelEventArgs e)
    {
    }

    private void generateButton_Click(object sender, EventArgs e)
    {
        try
        {
            //Initial opening point for save file dialogue
            saveFileDialog1.InitialDirectory = @"C:\Users\Heather\Documents\Visual Studio 2010\Projects\Random Number File Writer";
            //Save As popup - Opening the file for writing usage once it's created.
            if(saveFileDialog1.ShowDialog() == DialogResult.OK)
            {
                randomNumberFile = File.CreateText(openFileDialog1.FileName);
            }
            else // Popup informing user that the data will not save to a file because they didn't save.
            {
                MessageBox.Show("You elected not to save your data.");
            }

            numbers = numericUpDown1.Value; //Gathering the number of numbers to generate from the number box.

            while (numbers > 0) // Loop counting down to 0 to give the user the appropriate number of requested random numbers.
            {

                writeitem = rand1.Next(101); // Random number generated.
                randomNumberFile.WriteLine(writeitem); //Random number written to file
                numbers--; // Initial number for user input decremented so that loop will have an ending and user only gets the amount of randoms asked for.
            }
            randomNumberFile.Close();
        }

I included just the parts that are pertinent - I do have a little bit after the fact but that's just for the exit / clear buttons and the debugger doesn't jump to them at all so i clipped out the superfluous.

You're trying to write to the file even if the user said not to. When you're printed the "You elected not to save your data" you should return from the method... — Jon Skeet, Nov 21 '12 at 15:44

score 4 · Answer 1 · answered Nov 21 '12 at 15:43

4

You are using openFileDialog1.Filename with File.CreateText but above you use saveFileDialog1

answered Nov 21 '12 at 15:43

Mario The Spoon

4,799
1
24
36

Also, punch your professor in the face, as he clearly doesn't understand the importance of scope. You could have avoided this if it had been written properly. – Steffan Donal Nov 21 '12 at 15:46
Yah seriously, it scares me if a professor stepping line by line through a debugger couldn't see this... – Jesse Carter Nov 21 '12 at 15:47
Mario, consider yourself hugged with a squeal of delight. It works! Thank you! – Heather T Nov 21 '12 at 15:53
@HeatherT take a look at my answer as well as you are asking for trouble with the way your method is set up at the moment. Try cancelling the save file dialog and your program is still going to try to write the random numbers to a text file which is not good – Jesse Carter Nov 21 '12 at 15:55

Jesse Carter · Accepted Answer · 2012-11-21T16:19:06.353

0

It makes no sense to me that your while loop for numbers sits outside the if else logic of your save file dialog. If they didn't select a file then why are you still trying to write out the random numbers to a file? Move the while loop inside your if statement.

Also as Mario points out you are using mismatched filenames from 2 different dialogs so that is the root issue of the problem but I suggest you fix both to avoid future headaches.

try
    {
        //Initial opening point for save file dialogue
        saveFileDialog1.InitialDirectory = @"C:\Users\Heather\Documents\Visual Studio 2010\Projects\Random Number File Writer";
        //Save As popup - Opening the file for writing usage once it's created.
        if(saveFileDialog1.ShowDialog() == DialogResult.OK)
        {
            randomNumberFile = File.CreateText(saveFileDialog1.FileName);
            numbers = numericUpDown1.Value; //Gathering the number of numbers to generate from the number box.
            while (numbers > 0) // Loop counting down to 0 to give the user the appropriate number of requested random numbers.
            {
                writeitem = rand1.Next(101); // Random number generated.
                randomNumberFile.WriteLine(writeitem); //Random number written to file
                numbers--; // Initial number for user input decremented so that loop will have an ending and user only gets the amount of randoms asked for.
                randomNumberFile.Close();
            }
        }
        else // Popup informing user that the data will not save to a file because they didn't save.
        {
            MessageBox.Show("You elected not to save your data.");
        }
    }

edited Nov 21 '12 at 16:19

answered Nov 21 '12 at 15:43

Jesse Carter

20,062
7
64
101

While you address a valid problem, large `if` bodies aren't recommended and won't solve the original problem. – CodeCaster Nov 21 '12 at 15:45
I realize that my answer doesn't solve the root issue of the problem and addressed that. However, adding the while loop to the if that checks for a valid save file is certainly not creating a "large" if block not to mention that the logic here requires it. If he makes it through the else where the user did not pick a save file he is still trying to write random numbers to the text file! – Jesse Carter Nov 21 '12 at 15:46
Which can be solved by making the `else` `return`. Any `if` block larger than one line can be considered large. – CodeCaster Nov 21 '12 at 15:48
While this might be an issue of preference I consider a random return in an else statement that prevents further code from executing to be extremely bad form and I think a lot of people on here would agree with me. – Jesse Carter Nov 21 '12 at 15:50
I think you're referring to single entry, single exit, which indeed was considered a Good Thing in the 60's. Today it's more like fail fast. Large `if` blocks aren't merely a matter of style, they're accidents waiting to happen. What if there's another condition? Nest another `if`? – CodeCaster Nov 21 '12 at 15:51
If there is another condition then sure, we might take a look at breaking it apart and avoiding complex nested logic. But for this example the logic is pretty clear... IF the user picks a file we want to save to it. So put the save logic in the if block. There is nothing "big" or "nested" or "complex about that. And as it sits it IS an error in his coding logic. He could use a return in his else but he hasn't yet so this is still wrong and I am addressing an issue that needs to be fixed regardless of how he decides to do so. – Jesse Carter Nov 21 '12 at 15:54
You're suggesting to nest a `while` in an `if`, which [_will_ make the code more complex](http://en.wikipedia.org/wiki/Cyclomatic_complexity). Besides that, the `if` is the wrong way around. The code should _expect_ the user to click OK, the `if` should detect if OK was _not_ clicked and if so, return from the method to do nothing. Code expecting the "happy case" that checks its parameters on beforehand is less error-prone and complex and thus more robust. – CodeCaster Nov 21 '12 at 15:58
She. @JesseCarter and thanks for the input, I'll shift that loop into the appropriate spot. Y'all are just a bundle of information this morning ^_^ – Heather T Nov 21 '12 at 15:58
You're arguing semantics with me when a bug exists in his code the way it is currently written. I have provided a solution for that. If you don't appreciate my method then feel free to post an ACTUAL ANSWER to the OP helping him fix the problem that we both agree exists here. I work in an environment with a very strict coding standards that require the single entry single exit so I am biased here. But you're not helping the OP at all through this argument – Jesse Carter Nov 21 '12 at 16:09
My point is that the bug you refer to is easier fixed by putting a `return;` in the `else` block than by moving the whole `while` block into the `if`. While your point is valid, I tried to point out _why_ the `return` solution is better. Applying SESE might work for you at your work environment, but in general it doesn't offer much benefits (on the contrary even), that's what I wanted to warn for. Furthermore OP's problem itself was already answered before I commented on your answer. – CodeCaster Nov 21 '12 at 16:17
Thank you both for your help, it's all helpful information. I do feel the need to poke at one thing though... Jesse, I'm a she, not a he. As it's past the end of class I need to scoot now, and thank all of you again for your help. – Heather T Nov 21 '12 at 16:16
I have updated my answer to show the fixed code. I think you need to take another look at the cyclomatic complexity link you sent me as this implementation DOES NOT add any complexity whatsoever. Having a while loop inside an if does not create another execution branch it simply says if the user picked a file we're going to write random numbers to that file. While your else return point may be valid depending on the coding standards of where you work you are wrong regarding "added complexity" – Jesse Carter Nov 21 '12 at 16:21
I am perfectly willing to create a new SO question regarding Cyclomatic Complexity to point out the fact that this solution is no more complex – Jesse Carter Nov 21 '12 at 16:24
No, you are right, I stand corrected on the cyclomatic complexity. Thank you. – CodeCaster Nov 21 '12 at 16:35
No problem :P Thanks for being a good sport about it. I enjoyed the debate and to be honest had never even heard about Cyclomatic Complexity until you provided the link which is very interesting. I don't always agree with enforcing SESE at my workplace but am forced to deal with it – Jesse Carter Nov 21 '12 at 16:39
@HeatherT Sorry Heather I only just realized that you had commented as part of our discussion here pointing out that you were female. I know its sexist sometimes but I take for granted that there are women programmers out there too considering there are a few incredibly talented ones in my program as well! Thanks for marking this as the answer too :D – Jesse Carter Nov 21 '12 at 18:39
Not a problem you went above and beyond what I asked. It's still not running quite right but I'm going to fiddle with it before asking another question. :) -- Oh, and as to the girl thing it's not a problem either. If I remember correctly I started the question with the default username of "user + number" so it's understandable. ;) – Heather T Nov 22 '12 at 14:02

Save as is taking the filename provided, but is changing to null later in the code - which is causing the file never to be created

2 Answers2