*sniff* I'm going to miss this project...



  • I submit to you, for your viewing pleasure, what is probably the last hunk of retard-o-matic code from this job.  Although, at the rate bad code pops up around here I might get a few good ones in by Friday after all...

     

    private string ValidateFileName(string fileNameIn)
        {
            string newFileName = fileNameIn;
            string savePath = Server.MapPath("~/uploads/files/") + VarianceID + "/";
            string fullFilePath = savePath + fileNameIn;
            while (System.IO.File.Exists(fullFilePath))
            {
                String[] strArr = newFileName.Split('.');
                string tmpFileName = strArr[0];
                string numberStr = tmpFileName.Substring(tmpFileName.Length - 3, 3);
                if (numberStr[0] == '(' && numberStr[2] == ')' && IsInteger(numberStr[1].ToString()))
                {
                    int newInt = System.Convert.ToInt32(numberStr[1].ToString());
                    newInt = newInt + 1;
                    newFileName = tmpFileName.Substring(0, tmpFileName.Length - 3) + "(" + System.Convert.ToString(newInt) + ")" + "." + strArr[1];
                }
                else
                {
                    newFileName = tmpFileName + "(1)" + "." + strArr[1];
                }
    
            fullFilePath = savePath + newFileName;
        }
    
        return newFileName;
    }
    
    public  bool IsInteger(string theValue)
    {
        try
        {
            Convert.ToInt32(theValue);
            return true;
        }
        catch
        {
            return false;
        }
    } 
    



  • Last time I tried returning in a catch statement, I got another exception thrown (thread was being aborted).

    I do love the Convert.ToInt32 call in the larger function...Let's try-catch the Conversion once, and if it succeeds, do it again!



  • Where to begin..?  Um.

    1. File name is lost if there is more than one '.' in it.

    2. Too many unhandled exceptions for index out of bounds to even count

    3. Legitimately named foo(1).bar becomes foo(2).bar, not foo(1)(1).bar, but I'll forgive this one.

    4. Only one character is checked in between brackets, so after foo(9).bar, we get foo(10).bar, then foo(10)(1).bar

    5. IsInteger, but that was already covered. 

    It's a fair assumption that this code was written for a specific set of files, but we all know that someone will eventually see this code and attempt to reuse it (like any good programmer would try to do).  Once that happens.... well, glad you won't be there anymore to see what'll happen :)



  • [quote user="skippy"]

    It's a fair assumption that this code was written for a specific set of files, but we all know that someone will eventually see this code and attempt to reuse it (like any good programmer would try to do).  Once that happens.... well, glad you won't be there anymore to see what'll happen :)

    [/quote]

     It's for general file uploads in a web app.  Anything is possible with user input.  :)  I proposed a much different method of dealing with uploads, but I didn't get the work assigned to me.



  • [quote user="djork"]

     It's for general file uploads in a web app.  Anything is possible with user input.  :)  I proposed a much different method of dealing with uploads, but I didn't get the work assigned to me.

    [/quote]

    As long as the web app maps the original file name with the saved name, then I guess it'll "work" for most cases (ignoring filenames with < 3 characters).  <rhetoricalquestion>Of course if this mapping is in place, then why they didn't just use mktemp or a guid in the first place?</rhetoricalquestion>  It's less work, less code, and less error prone.

    Hope the new job works out for ya (I assume you have a new one lined up) 



  • [quote user="skippy"]

    3. Legitimately named foo(1).bar becomes foo(2).bar, not foo(1)(1).bar, but I'll forgive this one.

    [/quote]

     most browser do that too, especially interesting when downloading programs with the version in the filename.

    I was using firefox 0.9 to download 1.0+, but the first download was corrupted, and for some reason I downloaded it again later, so I ended up with the files:

    firefox-1.0+_en-US.exe

    firefox-2.0+_en-US.exe

    firefox-3.0+_en-US.exe
     

    or something like that. 



  • All that WTF processing, and not even a check for "../". Which is usually one of the points of filtering filenames, make sure nothing evil will happen on write...


Log in to reply