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
-
@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"?
-
@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.
-
catch(Exception ex) { } finally { throw new Exception("Everything may or may not have completed"); }
-
@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
-
@RaceProUK said in Filehandling is even harder:
I can go one better:
catch { } finally { }
:P
I misses that tasty unused variable warning!
-
@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".
-
@Jaloopa said in Filehandling is even harder:
catch(Exception ex) { } finally { throw new Exception("And then the murders began"); }
-
@Vault_Dweller Throwing from a finaly?
That's legal?
-
@PleegWat VS2015 likes it:
-
@PleegWat Why wouldn't it be?
-
@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. :)