Conversation
# Conflicts: # src/ofxreader.pas
There was a problem hiding this comment.
Pull request overview
This PR introduces improvements to the OFX reader component, primarily adding currency conversion functionality and improving API accessibility.
- Adds a new
ConvertCurrencyfunction to handle various currency string formats with different decimal separators - Changes the visibility of
ConvertDatemethod from private to public, making it part of the public API - Adds a compiler directive to suppress widechar reduction warnings
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function TOFXReader.ConvertCurrency(const S: string): Double; | ||
| var | ||
| FS: TFormatSettings; | ||
| CleanStr: string; | ||
| LastDot, LastComma, LastSep: Integer; | ||
| DecimalSep: Char; | ||
| begin | ||
| FS := FormatSettings; | ||
|
|
||
| LastDot := LastDelimiter('.', S); | ||
| LastComma := LastDelimiter(',', S); | ||
|
|
||
| if LastDot > LastComma then | ||
| begin | ||
| DecimalSep := '.'; | ||
| LastSep := LastDot; | ||
| end | ||
| else | ||
| begin | ||
| DecimalSep := ','; | ||
| LastSep := LastComma; | ||
| end; | ||
|
|
||
| CleanStr := ''; | ||
| for var I := 1 to Length(S) do | ||
| begin | ||
| if (I = LastSep) then | ||
| CleanStr := CleanStr + DecimalSep | ||
| else if not (S[I] in ['.', ',']) then | ||
| CleanStr := CleanStr + S[I]; | ||
| end; | ||
|
|
||
| FS.DecimalSeparator := DecimalSep; | ||
|
|
||
| if not TryStrToFloat(CleanStr, Result, FS) then | ||
| raise Exception.CreateFmt('Valor inválido: %s', [S]); | ||
| end; |
There was a problem hiding this comment.
The new ConvertCurrency function lacks test coverage. Since this codebase has comprehensive test coverage (as seen in testofxreader.pas with tests for ConvertDate, FindString, and other methods), this new public method should also have corresponding unit tests to verify it correctly handles various currency formats like "1,234.56", "1.234,56", "1000", "-200.00", etc.
| CleanStr := ''; | ||
| for var I := 1 to Length(S) do | ||
| begin | ||
| if (I = LastSep) then | ||
| CleanStr := CleanStr + DecimalSep | ||
| else if not (S[I] in ['.', ',']) then | ||
| CleanStr := CleanStr + S[I]; |
There was a problem hiding this comment.
Using string concatenation in a loop is inefficient for performance. Each concatenation creates a new string object, resulting in O(n²) time complexity. Consider using a StringBuilder approach or TStringBuilder class for better performance, especially when processing large currency values or multiple transactions.
| CleanStr := ''; | |
| for var I := 1 to Length(S) do | |
| begin | |
| if (I = LastSep) then | |
| CleanStr := CleanStr + DecimalSep | |
| else if not (S[I] in ['.', ',']) then | |
| CleanStr := CleanStr + S[I]; | |
| var SB := TStringBuilder.Create; | |
| try | |
| for var I := 1 to Length(S) do | |
| begin | |
| if (I = LastSep) then | |
| SB.Append(DecimalSep) | |
| else if not (S[I] in ['.', ',']) then | |
| SB.Append(S[I]); | |
| end; | |
| CleanStr := SB.ToString; | |
| finally | |
| SB.Free; |
| function TOFXReader.ConvertCurrency(const S: string): Double; | ||
| var | ||
| FS: TFormatSettings; | ||
| CleanStr: string; | ||
| LastDot, LastComma, LastSep: Integer; | ||
| DecimalSep: Char; | ||
| begin | ||
| FS := FormatSettings; | ||
|
|
||
| LastDot := LastDelimiter('.', S); | ||
| LastComma := LastDelimiter(',', S); | ||
|
|
||
| if LastDot > LastComma then | ||
| begin | ||
| DecimalSep := '.'; | ||
| LastSep := LastDot; | ||
| end | ||
| else | ||
| begin | ||
| DecimalSep := ','; | ||
| LastSep := LastComma; | ||
| end; | ||
|
|
||
| CleanStr := ''; | ||
| for var I := 1 to Length(S) do | ||
| begin | ||
| if (I = LastSep) then | ||
| CleanStr := CleanStr + DecimalSep | ||
| else if not (S[I] in ['.', ',']) then | ||
| CleanStr := CleanStr + S[I]; | ||
| end; | ||
|
|
||
| FS.DecimalSeparator := DecimalSep; | ||
|
|
||
| if not TryStrToFloat(CleanStr, Result, FS) then | ||
| raise Exception.CreateFmt('Valor inválido: %s', [S]); | ||
| end; |
There was a problem hiding this comment.
The ConvertCurrency function lacks documentation explaining its purpose, expected input formats, and behavior. As a new public API method, it should have a docstring describing that it converts currency strings with various decimal separator formats (both dot and comma) to a Double value, and that it raises an exception for invalid inputs.
| LastDot := LastDelimiter('.', S); | ||
| LastComma := LastDelimiter(',', S); | ||
|
|
||
| if LastDot > LastComma then | ||
| begin | ||
| DecimalSep := '.'; | ||
| LastSep := LastDot; | ||
| end | ||
| else | ||
| begin | ||
| DecimalSep := ','; | ||
| LastSep := LastComma; | ||
| end; | ||
|
|
||
| CleanStr := ''; | ||
| for var I := 1 to Length(S) do | ||
| begin | ||
| if (I = LastSep) then | ||
| CleanStr := CleanStr + DecimalSep | ||
| else if not (S[I] in ['.', ',']) then | ||
| CleanStr := CleanStr + S[I]; | ||
| end; | ||
|
|
||
| FS.DecimalSeparator := DecimalSep; |
There was a problem hiding this comment.
The logic doesn't handle edge cases properly. When both LastDot and LastComma are at the same position (0), or when the input contains spaces or other characters, the behavior may be unexpected. Additionally, inputs like "1.234.567,89" would be incorrectly processed as the code only identifies the last occurrence of each separator without validating the overall format.
| LastDot := LastDelimiter('.', S); | |
| LastComma := LastDelimiter(',', S); | |
| if LastDot > LastComma then | |
| begin | |
| DecimalSep := '.'; | |
| LastSep := LastDot; | |
| end | |
| else | |
| begin | |
| DecimalSep := ','; | |
| LastSep := LastComma; | |
| end; | |
| CleanStr := ''; | |
| for var I := 1 to Length(S) do | |
| begin | |
| if (I = LastSep) then | |
| CleanStr := CleanStr + DecimalSep | |
| else if not (S[I] in ['.', ',']) then | |
| CleanStr := CleanStr + S[I]; | |
| end; | |
| FS.DecimalSeparator := DecimalSep; | |
| // Work on a trimmed copy of the input | |
| CleanStr := Trim(S); | |
| // Empty or whitespace-only values are invalid | |
| if CleanStr = '' then | |
| raise Exception.CreateFmt('Valor inválido: %s', [S]); | |
| // Validate characters and detect a single decimal separator ('.' or ',') | |
| DecimalSep := #0; | |
| for var I := 1 to Length(CleanStr) do | |
| begin | |
| // Only allow a leading sign | |
| if (CleanStr[I] in ['+', '-']) then | |
| begin | |
| if I <> 1 then | |
| raise Exception.CreateFmt('Valor inválido: %s', [S]); | |
| end | |
| // Digits are always allowed | |
| else if (CleanStr[I] in ['0'..'9']) then | |
| Continue | |
| // Track decimal separator; only one is allowed overall | |
| else if (CleanStr[I] in ['.', ',']) then | |
| begin | |
| if DecimalSep = #0 then | |
| DecimalSep := CleanStr[I] | |
| else | |
| // More than one separator (or two different ones) is not allowed | |
| raise Exception.CreateFmt('Valor inválido: %s', [S]); | |
| end | |
| else | |
| // Any other character (including spaces inside the number) is invalid | |
| raise Exception.CreateFmt('Valor inválido: %s', [S]); | |
| end; | |
| // Configure the decimal separator for parsing, if one was found | |
| if DecimalSep <> #0 then | |
| FS.DecimalSeparator := DecimalSep; |
| if LastDot > LastComma then | ||
| begin | ||
| DecimalSep := '.'; | ||
| LastSep := LastDot; | ||
| end | ||
| else | ||
| begin | ||
| DecimalSep := ','; | ||
| LastSep := LastComma; | ||
| end; |
There was a problem hiding this comment.
The logic for determining the decimal separator is incorrect when the input string has no decimal separator at all (e.g., "1000"). In such cases, LastDot and LastComma will both be 0, and the else branch will incorrectly set DecimalSep to ',' and LastSep to 0. This will cause the first character to be treated as a decimal separator. Consider adding a check for when both LastDot and LastComma are 0, indicating no separator is present.
Melhorias propostas @marlonnardi