Filehandling is even harder



  • Same legacy application. The goal is to import a CSV-file into the database. First, we need to check the file like this:

    public bool CheckFile(FileInfo file) {
        int linecount = 0;
    
        try {
            Logger("Check file :" + file.FullName);
    
            DataSet1TableAdapters.QueriesTableAdapter queries = new DataSet1TableAdapters.QueriesTableAdapter();
    
            bool OK = true;
            double checksum = 0.0d;
            string sAmount = "";
            double dAmount = 0.0d;
    
            using (CsvReader csv = new CsvReader(new StreamReader(file.FullName), false, ';')) {
                while (csv.ReadNextRecord()) {
                    // if last line is reached
                    if (String.IsNullOrEmpty(csv[0].Trim()) || csv[1].Trim().ToUpper() == "SUM" || csv[2].Trim().Contains(";;;;;")) {
                        Logger("Checksum: " + csv[5].Trim());
                        break;
                    }
    
                    //increase counter
                    linecount++;
    
                    // skip first line
                    if (linecount == 1) {
                        continue;
                    }
    
                    // Check sum
                    sAmount = csv[5].Trim();
    
                    // No decimal places?
                    if (!sAmount.Contains(",")) {
                        OK = false;
                        Logger("File has wrong format, line: " + linecount + ", Amount: " + sAmount);
                        continue;
                    }
    
                    string[] splitAmount = sAmount.Split(',');
    
                    // No decimal places, or not 2 decimal places, cancel any further checking!!
                    if (splittedAmount.Length != 2) {
                        OK = false;
                        Logger("File has wrong format: 2 decimal places expected!! Line: " + linecount + ", Amount: " + sAmount);
                        continue;
                    }
                    // Decimal places != 2
                    if (splittedAmount[1].Length != 2) {
                        OK = false;
                        Logger("File has wrong format: 2 decimal places expected!! Line: " + linecount + ", Amount: " + sAmount);
                        continue;
                    }
    
                    // Convert and Save
                    dAmount = Convert.ToDouble(sAmount);
                    checksum += dAmount;
    
                    // Check customer number
                    string cuNo = (string)queries.GetCustNumber(csv[2].Trim());
                    if (String.IsNullOrEmpty(cuNo)) {
                        OK = false;
                        Logger("Customer number is unknown: " + csv[2].Trim());
                        continue;
                    }
                    else {
                        // Account Number Check 
                        int bankAcc = (int)queries.CountAccNo(cuNo);
                        if (bankAcc < 1) {
                            OK = false;
                            Logger("No bank account found for customer: " + cuNo);
                            continue;
                        }
                    }
                }
            }
            return OK;
        }
        catch (Exception ex) {
            throw ex;
        }
    }
    

    Don't you just love it when the comments are lying to you? Note how it says "cancel further checking" and inside the if it says "continue".

    Anway, what happens if this actually does return "true"? Well, of course we do the whole checking again! Maybe someone with malicious intend changed the file while we where reading it?

    public int ImportFile(FileInfo file, DateTime bookDate) {
            int linecount = 0;
    
            try {
                if (CheckFile(file)) {
                    DataSet1TableAdapters.QueriesTableAdapter queries = new DataSet1TableAdapters.QueriesTableAdapter();
                    Logger("Import started");
    
                    int importedRecords = 0;
                    double checksum = 0.0d;
                    long checksumCENT = 0;
                    string sAmount = "";
                    double dAmount = 0.0d;
    
                    using (CsvReader csv = new CsvReader(new StreamReader(file.FullName), false, ';')) {
                        while (csv.ReadNextRecord()) {
                            // last line? 
                            if (String.IsNullOrEmpty(csv[0].Trim()) || csv[1].Trim().ToUpper() == "SUM" || csv[2].Trim().Contains(";;;;;")) {
                                Logger("File imported.");
                                break;
                            }
    
                            linecount++;
    
                            if (linecount == 1) {
                                continue;
                            }
    
                            sAmount = csv[5].Trim();
    
                            // No decimal places! Cancel import!! 
                            if (!sAmount.Contains(",")) {
                                Logger("File has wrong format, line:" + linecount + ", Amount: " + sAmount);
                                throw new Exception("Wrong format in amount col");
                            }
    
                            string[] splitAmount = sAmount.Split(',');
    
                            // No decimal places or different amount than 2, cancel import!! 
                            if (splitAmount.Length != 2) {
                                Logger("File has wrong format, line: " + linecount + ", Amount: " + sAmount);
                                throw new Exception("Wrong format in amount col");
                            }
    
                            // Convert and Insert to DB
                            dAmount = Convert.ToDouble(sAmount);
                            checksum += dAmount;
    
                            string mapping = file.Name.Trim().Replace(" ", "").Replace(".csv", "").Replace("CustomerFile", "") + "-" + linecount;
    
                            // Check custNo
                            string custNo = (string)queries.GetCustNoForCust(csv[2].Trim());
                            string bankAccount = (string)queries.GetBankAccount((string)queries.GetCustNoForCust(csv[2].Trim()));
    
                            long iAmount = Convert.ToInt64(sAmount.Replace(".", "").Replace(",", ""));
    
                            DateTime authDate = Convert.ToDateTime(csv[4].Trim());
                            int pId = (int)queries.GetId(csv[0].Trim());
    
                            long insert = (long)queries.InsertIntoDb(bankAccount, pId, authDate, iAmount, csv[6].Trim(), bookDate, mapping, bookDate, file.Name, iAmount, iAmount, csv[6].Trim(), false, DateTime.Now, "DKV", csv[2].Trim(), Convert.ToInt32(csv[7].Trim()));
    
                            checksumCENT += iAmount;
    
                            importedRecords++;
                        }
                    }
    
                    Logger("File was imported: " + file.Name);
                    Logger("Line count: " + linecount);
                    Logger("Count of imported records: " + importedRecords);
                    Logger("Newly calculated import sum: " + checksum);
                    Logger("Import sum in cent: " + checksumCENT);
    
                    return importedRecords;
                }
                else {
                    throw new Exception("Error in file " + filename);
                }
    
            }
            catch (Exception ex) {
                throw ex;
            }
        }
    

    So, we read this file TWICE to import it into the database, how about that?
    Best thing is that the user has to manually correct errors that could easily be fixed inside the program. No decimal places? Add ",00". Only one decimal place? Add "0". Done.

    At least the whole process is cancelled once it runs into an error. Phew!

    Too bad there are still records imported. And because the file name is stored inside the database right at the beginning of the whole process (actually right after the user clicks the "import" button), he or she has to call IT to get the filename and any records that might have been imported deleted from the database.



  • @Hans_Mueller And another case of 'on exception, strip stack trace and rethrow'.

    Honestly that's one of the worst things in C#, since who would know that

    catch(Exception e)
    {
      throw e;
    }
    

    tosses the stack out, but

    catch(Exception e)
    {
      throw;
    }
    

    does not?

    Either way, there shouldn't be a catch here.



  • @Magus Yes, sadly the whole app is written with "error handling" like that. At least it's not just

    catch(Exception ex)
    {
        MessageBox.Show(ex.Message);
    }
    

    like in other programs I have to maintain.



  • @Hans_Mueller said in Filehandling is even harder:

    Same legacy application.

    For reference: Exception handling is hard


  • Impossible Mission Players - A

    @Hans_Mueller said in Filehandling is even harder:

    Note how it says "cancel further checking" and inside the if it says "continue".

    ... Maybe they meant "cancel further checking for just this row"?


  • area_pol

    @Hans_Mueller said in Filehandling is even harder:

    @Magus Yes, sadly the whole app is written with "error handling" like that. At least it's not just

    catch(Exception ex)
    {
        MessageBox.Show(ex.Message);
    }
    

    like in other programs I have to maintain.

    That's dangerously informative, it could lead to identifying the source of the problem.

    Let's refactor that:

    catch(Exception ex)
    {
             MessageBox.Show("Something went wrong");
    }
    

    Good good, not bad at all, bonus warning for unused variable is nice too, but it still can be made better:

    catch(Exception ex)
    {
    }
    

    Yesss, readable, fast and unobtrusive for the user. It's almost perfect...

    catch(Exception ex)
    {
    }
    finally
    {
    }
    

    Aaand my work is done.


  • kills Dumbledore

    @MrL

    catch(Exception ex)
    {
    }
    finally
    {
        throw new Exception("Everything may or may not have completed");
    }
    

  • SockDev

    @MrL said in Filehandling is even harder:

    Yesss, readable, fast and unobtrusive for the user. It's almost perfect...

    catch(Exception ex)
    {
    }
    finally
    {
    }
    

    Aaand my work is done.

    I can go one better:

    catch
    {
    }
    finally
    {
    }
    

    :P


  • area_pol

    @RaceProUK said in Filehandling is even harder:

    I can go one better:

    catch
    {
    }
    finally
    {
    }
    

    :P

    I misses that tasty unused variable warning!


  • kills Dumbledore

    @MrL said in Filehandling is even harder:

    I misses that tasty unused variable warning!

    catch
    {
        var iNeedABreakPoint = "";
    }
    finally
    {
        var iNeedABreakPointAgain = 0L;
    }
    


  • @RaceProUK Much better because now you won't get compiler warnings about that unused variable "ex".


  • Notification Spam Recipient

    @Jaloopa said in Filehandling is even harder:

    @MrL

    catch(Exception ex)
    {
    }
    finally
    {
        throw new Exception("And then the murders began");
    }
    


  • @Vault_Dweller Throwing from a finaly?

    That's legal?


  • SockDev

    @PleegWat VS2015 likes it:
    0_1490275680811_upload-4df6766f-209b-45f9-b36a-34964eba9e2a


  • Impossible Mission - B

    @PleegWat Why wouldn't it be?


  • Discourse touched me in a no-no place

    @Hans_Mueller said in Filehandling is even harder:

    like in other programs I have to maintain

    Let me guess; they run inside a webserver?



  • @dkf Luckily, they don't. :)


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.