diff --git a/Extras.cs b/Extras.cs index b7af0ab..9d463fe 100644 --- a/Extras.cs +++ b/Extras.cs @@ -6,6 +6,8 @@ namespace BankParserEff { public class Extras { + // These should be properties, not fields + // See: https://stackoverflow.com/a/295109 public string _numarReferinta; public string _iban; public string _nrExtras; @@ -38,7 +40,7 @@ public class Extras public string _informatiiPentruClientSoldDisponibil; - + // What is all this whitespace here for? diff --git a/ExtrasParser.cs b/ExtrasParser.cs index 71080bc..0c75376 100644 --- a/ExtrasParser.cs +++ b/ExtrasParser.cs @@ -5,15 +5,25 @@ namespace BankParserEff { public class ExtrasParser { + // It's default, but you should still mention that the field is "private" Extras _extras; + + // This one should either BE a property or be wrapped by one. Preferrably the first, in this case. public int _lastProcessedLineIndex; + // First off, *stop it* with the arbitrary empty lines + // You shouldn't need more than one line of empty space at once, and you should + // always have an empty line between methods for example + // All these methods, why are they public? If you don't expect to use them from outside the class, make all of them private + // Only keep public what you expect to be used by consumers of the class, this OOP basics. + + // Is this method even used anymore? public Extras Tag20(string sir) - { + {// Why an empty line below? sir = sir.Replace("\n", "").Replace("\r", ""); - sir = $"\n\n\n" + sir; + sir = $"\n\n\n" + sir;// Why the $ (verbatim) prefix? You're still doing concatenation unsing '+' ; also, why are you adding three newlines? _extras._numarReferinta = sir; return _extras; } @@ -21,6 +31,12 @@ public Extras Tag25(string sir) { string iban = sir; + // Look up newline variations; MT940 seems to always have its lines ended in CR+LF (so \r\n), never just \r or just \n so you can just + // replace \r\n in one go. + // + // Also, do the lines you're processing even still have line feeds at the end, after using ReadAllLines? + // Because the "remarks" section of the documentation says they don't and it's lazy to not check the docs or with a debugger: + // https://docs.microsoft.com/en-us/dotnet/api/system.io.file.readalllines?view=netcore-3.1 _extras._iban = iban.Replace("\n", "").Replace("\r", ""); return _extras; } @@ -39,7 +55,7 @@ public Extras Tag60(string sir) { if (sir.Contains(":86:")) - { + { // Does this branch of the "if" EVER get used? Isn't it from when you were still not parsing line-by-line? Same goes for others like it _extras._codSoldInitial = sir.Substring(0, 1); _extras._dataSoldInitial = (DateTime.ParseExact("20" + sir.Substring(1, 6), "yyyyMMdd", null)); @@ -62,8 +78,21 @@ public Tranzactie Tag61(string sir) { Tranzactie tranzactie = new Tranzactie(); + // I don't like the code in the "if/else" + // + // Any code that is COMMON should sit outside the "if/else" and be written ONLY ONCE, you're just cloning code here + // and making it extremely hard for anyone to follow what's going on ; most of the if and the else parts share the + // same exact code + // + // This applies for ALL these if/else blocks. + // + // By the way, you might know now, but I sure as hell don't, how the :61: tag looks, so why not write an explanation + // in a code comment about its structure? You end up _needing_ two displays otherwise. if (sir.IndexOf(":86:") - sir.IndexOf("/") <= 52) { + // I'm not sure this code, which is probably rolled over by from when you had just one long string, behaves as it should + // Especially since you probably haven't checked how it handles :61: tags that are on two lines + // tranzactie._dataTranzactie = (DateTime.ParseExact("20" + sir.Substring(0, 6), "yyyyMMdd", null)).Date; tranzactie._codTranzactie = sir.Substring(10, 1); tranzactie._sumaTranzactie = Convert.ToDecimal(sir[11..sir.IndexOf(',')] + sir.Substring(sir.IndexOf(','), 3)) / 100; @@ -72,7 +101,7 @@ public Tranzactie Tag61(string sir) tranzactie._detaliiTranzactie = ""; tranzactie._informatiiPentruClient = sir.Substring(sir.IndexOf(":86:") + 4); } - + // Should this not be just an "else"? if (sir.IndexOf(":86:") - sir.IndexOf("/") > 52) { tranzactie._dataTranzactie = (DateTime.ParseExact("20" + sir.Substring(0, 6), "yyyyMMdd", null)).Date; @@ -161,7 +190,8 @@ public Extras Tag65(string sir) } - +//... +// Why the random whitespace though? @@ -169,9 +199,13 @@ public Extras InternalParse(string[] liniiIntrare, int indexLiniePrimita) { _extras = new Extras(); - string randNetichetat = ""; + string randNetichetat = ""; // *Ne_e_tichetat bool isExtrasValid = false; + // You don't really need linieActuala; you could write "for (;indexLiniePrimita < ....; indexLiniePrimita += 1)" + // as you're only interesting in moving "indexLiniePrimita" forward + // + // Though it is usually a _bad idea_ to modify a received parameter, you should probably copy it to a local variable for (int linieActuala = indexLiniePrimita; linieActuala < liniiIntrare.Length; linieActuala += 1) { if (String.IsNullOrEmpty(liniiIntrare[linieActuala])) @@ -180,7 +214,7 @@ public Extras InternalParse(string[] liniiIntrare, int indexLiniePrimita) _lastProcessedLineIndex = indexLiniePrimita; } - else { break; } + else { break; } // Why not invert the "if" check? Then you don't even need the "else" as the code after "break" won't be executed } for (int linieCurenta = indexLiniePrimita; linieCurenta < liniiIntrare.Length; linieCurenta += 1) @@ -210,12 +244,24 @@ public Extras InternalParse(string[] liniiIntrare, int indexLiniePrimita) if (liniiIntrare[linieCurenta].StartsWith(":60F:")) { randNetichetat = ConcatenareRanduriDetalii(liniiIntrare, linieCurenta); + // This is a very bad idea as *you're modifying the source array* when you set + // this changed value on it. And you probably didn't even register this issue. + // + // WHY NOT JUST USE A LOCAL VARIABLE to store your intermediate data?! + // For some unknwon reason you avoid them and it makes no sense. + // + // Also applies to the rest of the places like this one liniiIntrare[linieCurenta] = liniiIntrare[linieCurenta] + randNetichetat; Tag60(liniiIntrare[linieCurenta].Substring(5)); } if (liniiIntrare[linieCurenta].StartsWith(":61:")) { + // Be aware that the :61: tag is the only tag other than :86: that can + // have two lines, with the second line being "supplementary details" + // I'm not sure you're processing this correctly here + // And I think you've not checked that since you haven't created for yourself + // input data that contains that sort of :61: tag randNetichetat = ConcatenareRanduriDetalii(liniiIntrare, linieCurenta); liniiIntrare[linieCurenta] = liniiIntrare[linieCurenta] + randNetichetat; _extras._tranzactii.Add(Tag61(liniiIntrare[linieCurenta].Substring(4))); @@ -223,6 +269,25 @@ public Extras InternalParse(string[] liniiIntrare, int indexLiniePrimita) if (liniiIntrare[linieCurenta].StartsWith(":62F:")) { + // + // . + // . + // . + // Okay, after scratching my head, I figured out what you're doing here, you're concatenating + // several lines into one long line, and then using the code you previously had to parse the + // entire line in one go. This is fine, it's one solution you could handle implementing. + // + // But it _is_ wasteful as you still end up going over any :86: fields you encounter, you should + // have tried to skip ahead as well. + // + // And if you don't know how to return multiple values from a method, well then + // + // a) https://lmgtfy.com/?q=how+to+return+multiple+values+c%23 (not that hard was it?) + // + // or + // + // b) ASK! + // randNetichetat = ConcatenareRanduriDetalii(liniiIntrare, linieCurenta); liniiIntrare[linieCurenta] = liniiIntrare[linieCurenta] + randNetichetat; Tag62(liniiIntrare[linieCurenta].Substring(5)); @@ -244,6 +309,8 @@ public Extras InternalParse(string[] liniiIntrare, int indexLiniePrimita) if (liniiIntrare[linieCurenta].StartsWith("-}") || liniiIntrare[linieCurenta].Equals("")) + // "-}" should be saved as a constant field on the class level, as you should always avoid "magic strings" + // Also, we don't compare a string to "", we use string.IsNullOrWhitespace(myStr) instead { _lastProcessedLineIndex = linieCurenta + 1; break; @@ -290,7 +357,7 @@ public List Parse(string[] liniiIntrare) public string ConcatenareRanduriDetalii(string[] liniiIntrare, int linieCurenta) { - string randNetichetat = ""; + string randNetichetat = ""; // Ne_e_tichetat for (int linieFaraTagSauCuTag86 = linieCurenta + 1; linieFaraTagSauCuTag86 < liniiIntrare.Length; linieFaraTagSauCuTag86 += 1) { if (!liniiIntrare[linieFaraTagSauCuTag86].StartsWith(":6") && !liniiIntrare[linieFaraTagSauCuTag86].StartsWith("-}") && !String.IsNullOrWhiteSpace(liniiIntrare[linieFaraTagSauCuTag86])) diff --git a/Program.cs b/Program.cs index 821b40f..5b5eaa3 100644 --- a/Program.cs +++ b/Program.cs @@ -10,6 +10,8 @@ class Program static void Main() { ExtrasParser extrasParser = new ExtrasParser(); + // I'm sorry, but MY computer doesn't have the same paths as yours (as is usually the case) + // Either use relative paths or figure out a different way to load the data string[] randuri = File.ReadAllLines(@"C:\Users\User1\Downloads\bin\STA\3.STA"); extrasParser.Parse(randuri); diff --git a/Tranzactie.cs b/Tranzactie.cs index 5916284..e516328 100644 --- a/Tranzactie.cs +++ b/Tranzactie.cs @@ -4,7 +4,13 @@ namespace BankParserEff { public class Tranzactie { - + // Why the wrong indenting? + + // These should be properties + // https://stackoverflow.com/a/295109 + + // Also ONLY PRIVATE fields should be prefixed with _ (as mentioned) + // And also fields should never need to be private, we have properties for that public DateTime _dataTranzactie; public string _codTranzactie; public decimal _sumaTranzactie; @@ -15,6 +21,8 @@ public class Tranzactie public string _informatiiPentruClient; - +// The indenting here is wrong +// Visual Studio has a code cleanup feature, USE IT! LEARN THE SHORTCUTS! +// https://docs.microsoft.com/en-us/visualstudio/ide/code-styles-and-code-cleanup?view=vs-2019#apply-code-styles } }