How not to handle file upload



  • string fullPath = Path.Combine(SavePath, fupFileUpload.FileName);
    
    // Snip some irrelevant stuff
    
    if (!Directory.Exists(SavePath))
        Directory.CreateDirectory(SavePath);
    
    try {
        fupFileUpload.SaveAs(fullPath);
        OnFileUploaded(fullPath, fileSize);
    } catch {
        valError.ErrorMessage = "Unable to save file";
        valError.IsValid = false;
    }

    So if saving the file throws an exception, we report an error, but if the previous step of creating the directory we're saving it into throws an exception, we don't catch it.

    Oh, and if there is a problem writing to the disk, we don't log that anywhere or notify anyone other than the end user.



  •  They probably felt there was more that could go wrong with uploading a file than creating a directory. Would the create directory line even be executed often?



  • @ASheridan said:

    Would the create directory line even be executed often?


    Potentially as much as every time; in practice, probably about 25% of the time or more.



  • @pjt33 said:

    fupFileUpload

    Hahahaha I do love Hungarian notation sometimes.



  • I just hope for your sake that nobody uploads a file called "..\..\..\web.config".

    Joking aside, I'd give changing the file name to something you control (like a GUID) a higher priority than a missed exception. Also, if the Directory.CreateDirectory throws an exception, you're MORE likely to have a log of that because it will bubble up to ASP.NET logging it to the Windows Event log.



  • Does CreateDirectory return an error if the directory already exists? If it does, what if another client created the same directory in between the calls to Exists() and CreateDirectory()? If does not, why even check if it exists? If the directory could not be created, SaveAs() will fail anyway.

    So what's your problem?

     



  • @pjt33 said:

    string fullPath = Path.Combine(SavePath, fupFileUpload.FileName);

    // Snip some irrelevant stuff

    if (!Directory.Exists(SavePath))
    Directory.CreateDirectory(SavePath);

    try {
    fupFileUpload.SaveAs(fullPath);
    OnFileUploaded(fullPath, fileSize);
    } catch {
    valError.ErrorMessage = "Unable to save file";
    valError.IsValid = false;
    }

    So if saving the file throws an exception, we report an error, but if the previous step of creating the directory we're saving it into throws an exception, we don't catch it.

    Oh, and if there is a problem writing to the disk, we don't log that anywhere or notify anyone other than the end user.

    Here is the optimal way to do it:

    
    try
    {
      Directory.CreateDirectory(SavePath);
    CreateFile:
      fupFileUpload.SaveAs(fullPath);
      OnFileUploaded(fullPath, fileSize);
    }
    catch(IOException)
    {
      goto CreateFile;
    }
    catch(Exception)
    {
      valError.ErrorMessage = "Unable to save file and/or create directory";
      valError.IsValid = false;
    }
    
    

    This design pattern is called "the backwash" and it will be necessary until C# gets a retry keyword (or the on error resume next clause).

    Ok the real interesting part of this code is that knowing if this compiles or not is a good interview question.



  • @Speakerphone Dude said:

    @pjt33 said:
    string fullPath = Path.Combine(SavePath, fupFileUpload.FileName);

    // Snip some irrelevant stuff

    if (!Directory.Exists(SavePath))
    Directory.CreateDirectory(SavePath);

    try {
    fupFileUpload.SaveAs(fullPath);
    OnFileUploaded(fullPath, fileSize);
    } catch {
    valError.ErrorMessage = "Unable to save file";
    valError.IsValid = false;
    }

    So if saving the file throws an exception, we report an error, but if the previous step of creating the directory we're saving it into throws an exception, we don't catch it.

    Oh, and if there is a problem writing to the disk, we don't log that anywhere or notify anyone other than the end user.

    Here is the optimal way to do it:

    try
    {
      Directory.CreateDirectory(SavePath);
    CreateFile:
      fupFileUpload.SaveAs(fullPath);
      OnFileUploaded(fullPath, fileSize);
    }
    catch(IOException)
    {
      goto CreateFile;
    }
    catch(Exception)
    {
      valError.ErrorMessage = "Unable to save file and/or create directory";
      valError.IsValid = false;
    }
    
    

    This design pattern is called "the backwash" and it will be necessary until C# gets a retry keyword (or the on error resume next clause).

    Ok the real interesting part of this code is that knowing if this compiles or not is a good interview question.

    Eww. Also, isn't that just going to create an infinite loop on a CreateFile IOException?



  • @morbiuswilters said:

    @Speakerphone Dude said:
    @pjt33 said:
    string fullPath = Path.Combine(SavePath, fupFileUpload.FileName);

    // Snip some irrelevant stuff

    if (!Directory.Exists(SavePath))
    Directory.CreateDirectory(SavePath);

    try {
    fupFileUpload.SaveAs(fullPath);
    OnFileUploaded(fullPath, fileSize);
    } catch {
    valError.ErrorMessage = "Unable to save file";
    valError.IsValid = false;
    }

    So if saving the file throws an exception, we report an error, but if the previous step of creating the directory we're saving it into throws an exception, we don't catch it.

    Oh, and if there is a problem writing to the disk, we don't log that anywhere or notify anyone other than the end user.

    Here is the optimal way to do it:

    try
    {
      Directory.CreateDirectory(SavePath);
    CreateFile:
      fupFileUpload.SaveAs(fullPath);
      OnFileUploaded(fullPath, fileSize);
    }
    catch(IOException)
    {
      goto CreateFile;
    }
    catch(Exception)
    {
      valError.ErrorMessage = "Unable to save file and/or create directory";
      valError.IsValid = false;
    }
    
    

    This design pattern is called "the backwash" and it will be necessary until C# gets a retry keyword (or the on error resume next clause).

    Ok the real interesting part of this code is that knowing if this compiles or not is a good interview question.

    Eww. Also, isn't that just going to create an infinite loop on a CreateFile IOException?

    The IOException is raised only when the directory to be created already exists (unless the "SaveAs" method throws an IOException which would be its own WTF). All other exceptions go to the second leg. So no endless loop, just a "clever" use of gotos.



  • @Speakerphone Dude said:

    (unless the "SaveAs" method throws an IOException which would be its own WTF).

    Why would that be a WTF? That's exactly what I'd expect on a failed write.



  • @Speakerphone Dude said:

    <Weird-ass code involving goto>

    This design pattern is called don't-be-retarded:

    try
    {
      try { Directory.CreateDirectory(SavePath); } catch(IOException) {} // Directory already exists; ignore exception
    

    fupFileUpload.SaveAs(fullPath);
    OnFileUploaded(fullPath, fileSize);
    }
    catch(Exception)
    {
    valError.ErrorMessage = "Unable to save file and/or create directory";
    valError.IsValid = false;
    }

    If you don't care that a specific statement fails or not, don't use crazy-ass logic with gotos to skip it; just ignore exceptions from that one statement,



  • @Salamander said:

    @Speakerphone Dude said:

    <Weird-ass code involving goto>

    This design pattern is called don't-be-retarded:

    try
    {
      try { Directory.CreateDirectory(SavePath); } catch(IOException) {} // Directory already exists; ignore exception
    

    fupFileUpload.SaveAs(fullPath);
    OnFileUploaded(fullPath, fileSize);
    }
    catch(Exception)
    {
    valError.ErrorMessage = "Unable to save file and/or create directory";
    valError.IsValid = false;
    }

    If you don't care that a specific statement fails or not, don't use crazy-ass logic with gotos to skip it; just ignore exceptions from that one statement,

    Jesus titty-fucking Christ people... Ever heard of Directory.Exists? Also, you should be beaten with a bat if you're using a try/catch as an if/else block.

    If you get a security exception because the account doesn't have create permissions on the directory, then throw the exception because the create file will also fail.


  • @Salamander said:

    @Speakerphone Dude said:

    <Weird-ass code involving goto>

    This design pattern is called don't-be-retarded:

    try
    {
      try { Directory.CreateDirectory(SavePath); } catch(IOException) {} // Directory already exists; ignore exception
    

    fupFileUpload.SaveAs(fullPath);
    OnFileUploaded(fullPath, fileSize);
    }
    catch(Exception)
    {
    valError.ErrorMessage = "Unable to save file and/or create directory";
    valError.IsValid = false;
    }

    If you don't care that a specific statement fails or not, don't use crazy-ass logic with gotos to skip it; just ignore exceptions from that one statement,

    I don't know what is worse: that you did not get that my code was a joke or that you actually suggest a nested try/catch in this situation instead of verifying that the directory exists. In any case, bravo!



  • @C-Octothorpe said:

    Ever heard of Directory.Exists?

    You're assuming the filesystem is never accessed by more than one program/thread at a time.

    Even if you check the directory exists beforehand, you should still assume that the call to create the directory will fuck up. Because it can.



  • @Salamander said:

    @C-Octothorpe said:
    Ever heard of Directory.Exists?

    You're assuming the filesystem is never accessed by more than one program/thread at a time.

    Even if you check the directory exists beforehand, you should still assume that the call to create the directory will fuck up. Because it can.

    That's a good point, however you should still use Exists rather than blindly using a try/catch...

    Or better yet, you can also create a thread-safe version where if the directory is found to not exist, lock, check again (someone else could have beat him to it), then create the directory. All subsequent threads will then find that it exists and move along, all without using the try/catch as a crutch.



  • @C-Octothorpe said:

    @Salamander said:
    @C-Octothorpe said:
    Ever heard of Directory.Exists?

    You're assuming the filesystem is never accessed by more than one program/thread at a time.
    Even if you check the directory exists beforehand, you should still assume that the call to create the directory will fuck up. Because it can.

    That's a good point, however you should still use Exists rather than blindly using a try/catch...

    Or better yet, you can also create a thread-safe version where if the directory is found to not exist, lock, check again (someone else could have beat him to it), then create the directory. All subsequent threads will then find that it exists and move along, all without using the try/catch as a crutch.
    Except that the .Exists only returns true or false and not the reason why it could not find it.  Using a try catch when trying to accessing it directly will result in an exception with details that might be useful for logging.  In several of my previous program's applications we had to remove .Exists checks from several services because they were hiding actual error information.


  • @Anketam said:

    @C-Octothorpe said:

    @Salamander said:
    @C-Octothorpe said:
    Ever heard of Directory.Exists?

    You're assuming the filesystem is never accessed by more than one program/thread at a time.
    Even if you check the directory exists beforehand, you should still assume that the call to create the directory will fuck up. Because it can.

    That's a good point, however you should still use Exists rather than blindly using a try/catch...

    Or better yet, you can also create a thread-safe version where if the directory is found to not exist, lock, check again (someone else could have beat him to it), then create the directory. All subsequent threads will then find that it exists and move along, all without using the try/catch as a crutch.
    Except that the .Exists only returns true or false and not the reason why it could not find it.  Using a try catch when trying to accessing it directly will result in an exception with details that might be useful for logging.  In several of my previous program's applications we had to remove .Exists checks from several services because they were hiding actual error information.
    So, you're saying that using a try/catch as an if/else block is good practice? Aren't you still going to get that same error message if Exists returns false and you try to create the directory? I'm curious how the use of Exists somehow prevented you from capturing (or even getting) the same exception. Maybe I just don't get your specific use-case...

    This reminds me of people who use the safe-cast (as) and don't check for null immediately afterwards.


  • @C-Octothorpe said:

    Or better yet, you can also create a thread-safe version where if the directory is found to not exist, lock, check again (someone else could have beat him to it), then create the directory. All subsequent threads will then find that it exists and move along, all without using the try/catch as a crutch.

    This only works if you're assuming every directory interaction will have from the same process.

    The solution is: if (.Exists) { try/catch } and then try to swallow the "directory exists" exception and rethrowing the others. This isn't a very complicated problem, people. Also, I should point out that some of the confusion comes from the platform: I shouldn't have to do a check and a try/catch just to handle the case of a directory existing or being created after my check (almost every fucking OO language I've worked with does this wrong). At the very least, there should be a CreateIfNotExists method. This is one of the very first utility methods I implement when I start using a new language..



  • @morbiuswilters said:

    @C-Octothorpe said:
    Or better yet, you can also create a thread-safe version where if the directory is found to not exist, lock, check again (someone else could have beat him to it), then create the directory. All subsequent threads will then find that it exists and move along, all without using the try/catch as a crutch.

    This only works if you're assuming every directory interaction will have from the same process.

    The solution is: if (.Exists) { try/catch } and then try to swallow the "directory exists" exception and rethrowing the others. This isn't a very complicated problem, people. Also, I should point out that some of the confusion comes from the platform: I shouldn't have to do a check and a try/catch just to handle the case of a directory existing or being created after my check (almost every fucking OO language I've worked with does this wrong). At the very least, there should be a CreateIfNotExists method. This is one of the very first utility methods I implement when I start using a new language..

    I agree with the pattern, however the Create method will do nothing if the directory exists, but will throw on bad path, permissions, etc.

    Just thinking about it now, you wouldn't even need the Exists call, just call the Create, and if it blows it'll be because of something that is out of your control (permissions, invalid path, unmapped drive, etc.), and if that's the case, log the error and blow up because there is nothing that you can do to recover.



  • @C-Octothorpe said:

    I agree with the pattern, however the Create method will do nothing if the directory exists, but will throw on bad path, permissions, etc.

    Just thinking about it now, you wouldn't even need the Exists call, just call the Create, and if it blows it'll be because of something that is out of your control (permissions, invalid path, unmapped drive, etc.), and if that's the case, log the error and blow up because there is nothing that you can do to recover.

    I take it back. I've done very little with C# so I didn't know Microsoft had implemented a sensible Directory.Create method.



  • @Speakerphone Dude said:

    I don't know what is worse: that you did not get that my code was a joke or that you actually suggest a nested try/catch in this situation instead of verifying that the directory exists. In any case, bravo!

    You started to talk about Design Patterns (going so far as to name them). We just assumed you were one of "those guys"; the kind that have a favorite (usually incorrect and/or non-needed) pattern for every problem. Those guys exist and they're a pain.

    They're almost as bad as the guys that chalk every error up to not doing unit testing (especially when a unit test would not have caught the bug).



  • @Gazzonyx said:

    @Speakerphone Dude said:
    I don't know what is worse: that you did not get that my code was a joke or that you actually suggest a nested try/catch in this situation instead of verifying that the directory exists. In any case, bravo!

    You started to talk about Design Patterns (going so far as to name them). We just assumed you were one of "those guys"; the kind that have a favorite (usually incorrect and/or non-needed) pattern for every problem. Those guys exist and they're a pain.

    They're almost as bad as the guys that chalk every error up to not doing unit testing (especially when a unit test would not have caught the bug).

    Pfft.. unit testing.. The compiler will catch all of my bugs!



  • @morbiuswilters said:

    @Gazzonyx said:
    @Speakerphone Dude said:
    I don't know what is worse: that you did not get that my code was a joke or that you actually suggest a nested try/catch in this situation instead of verifying that the directory exists. In any case, bravo!

    You started to talk about Design Patterns (going so far as to name them). We just assumed you were one of "those guys"; the kind that have a favorite (usually incorrect and/or non-needed) pattern for every problem. Those guys exist and they're a pain.

    They're almost as bad as the guys that chalk every error up to not doing unit testing (especially when a unit test would not have caught the bug).

    Pfft.. unit testing.. The compiler will catch all of my bugs!


    I know you're being sarcastic, but sometimes it does. At the least, all modern IDEs will give you warnings (and they are customizable). My problem is when I've got a concurrency bug (or a bug in another library) and someone tells me that if I had been doing until testing, I wouldn't have this problem. These people seem to also have an uncanny ability to ask about until tests before they fully understand a problem. I guess they never read any of Frederick Brooks' thoughts on silver bullets.



  • @Gazzonyx said:

    @morbiuswilters said:
    @Gazzonyx said:
    @Speakerphone Dude said:
    I don't know what is worse: that you did not get that my code was a joke or that you actually suggest a nested try/catch in this situation instead of verifying that the directory exists. In any case, bravo!

    You started to talk about Design Patterns (going so far as to name them). We just assumed you were one of "those guys"; the kind that have a favorite (usually incorrect and/or non-needed) pattern for every problem. Those guys exist and they're a pain.

    They're almost as bad as the guys that chalk every error up to not doing unit testing (especially when a unit test would not have caught the bug).

    Pfft.. unit testing.. The compiler will catch all of my bugs!


    I know you're being sarcastic, but sometimes it does. At the least, all modern IDEs will give you warnings (and they are customizable). My problem is when I've got a concurrency bug (or a bug in another library) and someone tells me that if I had been doing until testing, I wouldn't have this problem. These people seem to also have an uncanny ability to ask about until tests before they fully understand a problem. I guess they never read any of Frederick Brooks' thoughts on silver bullets.

    Sometimes it catches all of your bugs?

    But I wasn't referring to syntax errors or warnings. Usually when people say "the compiler will catch bugs" they mean "static/strong typing", which is what I was referencing.


  • ♿ (Parody)

    @morbiuswilters said:

    @Gazzonyx said:
    They're almost as bad as the guys that chalk every error up to not doing unit testing (especially when a unit test would not have caught the bug).

    Pfft.. unit testing.. The compiler will catch all of my bugs!

    Sucker. Just write FOSS and let the world find the bugs for you!



  • @morbiuswilters said:

    But I wasn't referring to syntax errors or warnings. Usually when people say "the compiler will catch bugs" they mean "static/strong typing", which is what I was referencing.

    When someone creates a compilers that understands/interprets intent, we're all out of a job. :) Until then, I think we're stuck with unit tests, integration testing and being careful.
    Come to think of it, weakly typed languages are the only reason that I think Hungarian notation should be around any more. The IDE gives us the rest of the context, typing, etc. That being said, I hate Hungarian notation.



  • @boomzilla said:

    @morbiuswilters said:
    @Gazzonyx said:
    They're almost as bad as the guys that chalk every error up to not doing unit testing (especially when a unit test would not have caught the bug).

    Pfft.. unit testing.. The compiler will catch all of my bugs!

    Sucker. Just write FOSS and let the world find the bugs for you!

    I don't want dope-smoking Marxists using my bugs without paying for them! I'll just wrap my bugs in a sleek case, jack up the price 40% and rake in the bucks. It works for Apple.


  • ♿ (Parody)

    @morbiuswilters said:

    I don't want dope-smoking Marxists using my bugs without paying for them! I'll just wrap my bugs in a sleek case, jack up the price 40% and rake in the bucks. It works for Apple.

    If you throw in a javascript debugger you might be able to make it 45%.



  • @boomzilla said:

    @morbiuswilters said:
    I don't want dope-smoking Marxists using my bugs without paying for them! I'll just wrap my bugs in a sleek case, jack up the price 40% and rake in the bucks. It works for Apple.

    If you throw in a javascript debugger you might be able to make it 45%.

    If I leave out a Javascript debugger then people have to go through the Bug Store to get their native bugs. And I get a cut of the revenue. It's worth way more than a measly 5% to make things work poorly.


  • Discourse touched me in a no-no place

    @Gazzonyx said:

    That being said, I hate Hungarian notation.
    Which one? Apps Hungarian or Systems Hungarian (which is largely redundant in strongly typed languages, and requires you to rename variables if you change the underlying type)?


  • ♿ (Parody)

    @PJH said:

    @Gazzonyx said:
    That being said, I hate Hungarian notation.
    Which one? Apps Hungarian (which is largely redundant in strongly typed languages, and requires you to rename variables if you change the underlying type) or Systems Hungarian?

    Or the other way around. In case your care about that sort of thing.


  • Discourse touched me in a no-no place

    @boomzilla said:

    Or the other way around. In case your care about that sort of thing.
    Oh bollocks, of course. I'll just go and edit it....



  • @Gazzonyx said:

    @Speakerphone Dude said:
    I don't know what is worse: that you did not get that my code was a joke or that you actually suggest a nested try/catch in this situation instead of verifying that the directory exists. In any case, bravo!

    You started to talk about Design Patterns (going so far as to name them). We just assumed you were one of "those guys"; the kind that have a favorite (usually incorrect and/or non-needed) pattern for every problem. Those guys exist and they're a pain.

    They're almost as bad as the guys that chalk every error up to not doing unit testing (especially when a unit test would not have caught the bug).

    Good news for your: there is a magnificent design pattern that always apply, it's called Design by Design Pattern. What is beautiful is that if you always use Design by Design Pattern there will never be a case where a bug is not caught by unit testing; if this happens it simply means that one of the Design Patterns that has been selected in the process of doing Design by Design Pattern is suboptimal, which is an implementation problem and has nothing to do with the Design by Design Pattern approach. To solve an implementation pattern one can use the Implement by Implementation Pattern pattern, or even better, use Test Driven Executable Specifications and blame the Business Analyst who signed off on the specifications whenever unit testing is unable to catch a bug (which really becomes a non-compliance incident).



  • @Speakerphone Dude said:

    @Gazzonyx said:
    @Speakerphone Dude said:
    I don't know what is worse: that you did not get that my code was a joke or that you actually suggest a nested try/catch in this situation instead of verifying that the directory exists. In any case, bravo!

    You started to talk about Design Patterns (going so far as to name them). We just assumed you were one of "those guys"; the kind that have a favorite (usually incorrect and/or non-needed) pattern for every problem. Those guys exist and they're a pain.

    They're almost as bad as the guys that chalk every error up to not doing unit testing (especially when a unit test would not have caught the bug).

    Good news for your: there is a magnificent design pattern that always apply, it's called Design by Design Pattern. What is beautiful is that if you always use Design by Design Pattern there will never be a case where a bug is not caught by unit testing; if this happens it simply means that one of the Design Patterns that has been selected in the process of doing Design by Design Pattern is suboptimal, which is an implementation problem and has nothing to do with the Design by Design Pattern approach. To solve an implementation pattern one can use the Implement by Implementation Pattern pattern, or even better, use Test Driven Executable Specifications and blame the Business Analyst who signed off on the specifications whenever unit testing is unable to catch a bug (which really becomes a non-compliance incident).


    I hate you so very much right now.



  • @aihtdikh said:

    @pjt33 said:
    fupFileUpload

    Hahahaha I do love Hungarian notation sometimes.

    Oh you made me want some lángos (hungarian bread) and luckily I stumbled across some on Saturday! First time in years! But now I want some more :/



  • @Zemm said:

    @aihtdikh said:
    @pjt33 said:

    fupFileUpload
    Hahahaha I do love Hungarian notation sometimes.

    Oh you made me want some lángos (hungarian bread) and luckily I stumbled across some on Saturday! First time in years! But now I want some more :/

    We should all have had Hungarian food for Cinco de Mayo.

     



  • @da Doctah said:

    We should all have had Hungarian food for Cinco de Mayo.


    Why? Do Hungarians hate the French too?



  • @pjt33 said:

    @da Doctah said:

    We should all have had Hungarian food for Cinco de Mayo.

    Why? Do Hungarians hate the French too?
    Doesn't everybody?


     



  • WHEN WERE YOU IN MY KITCHEN!



  • @da Doctah said:

    @pjt33 said:

    @da Doctah said:

    We should all have had Hungarian food for Cinco de Mayo.


    Why? Do Hungarians hate the French too?
    Doesn't everybody?


     

    Dude, what the fuck is wrong with that rancid-ass mayo?


Log in to reply