Made IsFileCsv into an extension method and added tests#17
Conversation
- Moved `IsFileCsv` out of `FileService` and into String extensions - Added unit tests in `StringExtensionsTests.cs` to validate the method with various file path scenarios. - Updated `FileService.cs` to use the new `IsFileCsv` extension method.
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
bechtd
left a comment
bechtd
left a comment
There was a problem hiding this comment.
System.IO.Path is NOT System.IO.File
This is a common mistake to make because they look similar. You would think both talk to the file system. Path does not.
| : Result.Fail("Selected file is not a CSV file."); | ||
| } | ||
|
|
||
| private bool IsFileCsv(string filePath) => fileSystem.Path.GetExtension(filePath)?.ToLower(CultureInfo.CurrentCulture) == ".csv"; |
There was a problem hiding this comment.
- I know it feels like
System.IO.Pathis part of the "file system", especially since you can access it that way. BUT when it comes to using these methods, such asGetExtension, they are independent of the OS, to the best of my knowledge. This is a static method. That signifies that this is most likely a "pure function" with no hard dependencies.
You can inspect the actual library and see for yourself:
There was a problem hiding this comment.
Since System.IO.Path (as best as I can tell) NEVER TALKS to the OS, you do not have to put it inside a Service, to use the "heavy" fileSystem object.
Yes, it is there, but I think they did that for "completeness" of their Abstraction library.
It's static, so you can treat it as a pure function (an isolated algorithm that only works internally inside C#).
Therefore instead of putting this Is CSV checker in a Service, we make it an extension and then Unit Test it.
There was a problem hiding this comment.
- String comparison should be done using
string.Equals( "string1", "string2", StringComparison.IgnoreCase). Don't useToLower()orToUpper(). It is best to use this function for comparing two strings, always, using one of the ignore case flags, if you plan to ignore case. - Now that this is in an extension, we can test it extensively (raising the coverage numbers).
| [InlineData("fileWith.csv.txt", false)] | ||
| [InlineData("fileWith.txt.csv", true)] |
There was a problem hiding this comment.
try to trick it
it's not fooled
jdubar
added
refactor
AND a |
3 participants
Made
IsFileCsvinto an extension method and added testsIsFileCsvout ofFileServiceand into String extensionsStringExtensionsTests.csto validate the method with various file path scenarios.FileService.csto use the newIsFileCsvextension method.