Move and Update conditional logic in TimeChip.razor.cs#14
Conversation
Updated the `OnParametersSet` method to replace the pattern matching syntax `is >= 20m || is < 6m` with standard comparison operators `>= 20m || < 6m`.
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
Simplified `TimeChip` logic by delegating time-based icon and color determination to new extension methods in `DecimalExtensions`. Added `IsAtNight`, `GetIconAsSvg`, and `GetIconColor` methods to encapsulate reusable logic. Introduced `DecimalExtensionsTests` to validate the new methods: - Verified `ToFormattedString` formats decimals correctly. - Tested `IsAtNight` for accurate nighttime detection. - Ensured `GetIconAsSvg` and `GetIconColor` return appropriate values based on time. These changes improve code readability, reusability, and test coverage.
bechtd
changed the title
jdubar
added
refactor
There was a problem hiding this comment.
I've always found test names to be a bit...awkward. Writing things like SomeMethodName_ValidOrInvalidInput_ReturnsSomeValue just never...felt right to me. I can totally see becoming apathetic with the naming (because I've been there!!) Obviously, you've experienced the same as well.
But this - and I think you've shown me this before, but it didn't sink in until I saw your pic that you attached to the PR description. THIS, I like. It looks ORGANIZED!
I ❤️ organization!!
Consider me a fresh convert. I dig it.
There was a problem hiding this comment.
yes, the Test naming convention, with _ (underscores) kinda hurts the eyes. _ isn't used a lot in C#, though clearly it is a valid symbol for a method name (unlike the - dash), which means, then, it's preference.
As I say a lot, inside Test Projects, slightly different "set of styling rules."
The REAL "sexy" way to name your tests is to use a Name property in the Fact decorator. Let me check.
AH, in xUnit, it's DisplayName. NUnit uses TestName I think.
Take a look:
And this is what that looks like in the test runner:
just like with your quote, you can have some fun with it.
To encourage me using this "English Naming" approach, I had started naming the tests
public void Test001public void Test002
But the Decorator had the REAL NAME, in English, written out in as clean as I want.
Another great reason to put your tests inside Method-named Sub-classes ... Each "sub-class restarts the Test counter", so you don't drive yourself nuts trying to figure out which number you're up to in a class with lots of methods.
There was a problem hiding this comment.
Lots of languages name their tests this way, with strings, so that it's much more readable and friendly for other devs to truly understand "what's this test supposed to do?"
In C#, these abilities are there, but I found so few people bother to explore the libraries. They miss out on having a much better Unit Test writing and reading experience. There's much more to these libraries than I know. I'm sure of it.
| { | ||
| public static string ToFormattedString(this decimal value) => value.ToString("00.00"); | ||
|
|
||
| public static bool IsAtNight(this decimal time) => time is >= 20m or < 6m; // time >= 20m || time < 6m; |
| Icon = Time.GetIconAsSvg(defaultValue: Icon); | ||
| IconColor = Time.GetIconColor(defaultValue: IconColor); |
There was a problem hiding this comment.
Love seeing the shorthand approach using extension methods.
There was a problem hiding this comment.
Love seeing the shorthand approach using extension methods.
AWESOME! I love refactoring. I can't help it ... my brain is just built for tinkering. I love "folding" and "collapsing" code (BUT UNDER TEST, so that I ensure I don't bust things up ... at least not TOO severely ... [cough] Downloads [cough].)
Part of this is "dumbing down the UI" ... even a little.
Even splitting the logic. At first you think "OH, I'm evaluating that TWICE now!" but that's mis-placed optimization.
Even though both Icon and IconColor are making a decision on the same exact logic, it's not that expensive to break them up ... extract the common logic out into a boolean-returning method both can then use.
It's always tempting to try to "use the same loop" to do 9 different things, but that leads to being "overwhelmed". Readability and Testability PLUMMET. In the end, in my experience, THAT is SO MUCH MORE Costly, than (we evaluated the Time twice, or We looped through this same list 9 times, setting 9 fields).
It seems inefficient, but complexity theory says otherwise. Sure, it's a little more overhead, but the savings to understanding for "the next dev" (who might even be you, 6 to 12 months later) when you don't have a "mega-loop" to do so many different things ... that's the REAL TRUE Cost of development.
Moving towards Functional programming, I find, tends to help you follow code a lot faster ... and it tends to be super simple to unit test.
There's still code reuse. You just approach it a different way (I'd argue, the more correct way).
3 participants
First Pass
Fix conditional logic in TimeChip.razor.cs
Updated the
OnParametersSetmethod to replace the pattern matching syntaxis >= 20m || is < 6mwith standard comparison operators>= 20m || < 6m.Second Pass
Then I realized what you were trying to do.
is >= 20m or < 6mThis is the newer "range" syntax that I'm not used to, though I do love the compactness of it.
However, the problem we have is that I can't easily prove these are logically equivalent without tests. And this code was in razor code-behind.
This is one of those instances where you have to "dumb down" the UI. Although this code does have UI specific aspects:
For now, it's OK to place it under the Extension folder. If the program were larger, I would probably isolate it to a "UI/Extensions" folder, so that I know that this is for UI-dependent functions only (to keep UI libraries from bleeding into the lower logic layer).
Now, can use tests to verify the logic of our range, and we can compare the results of both the classic notation (commented out) and the newer notation, and confirm that they produce the exact same results in the tests. This is how we can prove to ourselves that these two different notations are logically equivalent (to the best of our knowledge). No ambiguity, no guesses, no having to run the program, setting a specific time, to see what Icon and Color you get.
This is one of the toughest parts of extracting all logic out of the UI, so that you can unit test it. It's not always easy nor obvious that you can. Assignment is especially tricky when you ONLY seem to have a "when true" case. The classic way to handle that is to have a way to "hold onto the original value, by passing it along as the default value." In this way, you are assigning the same value back to itself, effectively causing no change. As long as you don't have any sort of "set value triggers" this should be OK to do.
If it's not OK, then we probably get most of what we want isolated, by only moving the
IsAtNightlogic into the Extension class. This is the more interesting portion of logic to verify. Now,if (bool)would be all that's left, which is so "boring" you could probably get away with no test. I mean, theGetIconAsSvgandGetIconColortests are pretty boring. It's eithertrueor itsfalse. You go left or you go right. It's rare you make mistakes on something this simple, but it happens.Again, the more interesting tests are the range verification, especially using the newer notation.
Sub classes in Tests
This is a notation I prefer, but only for
consts and in Tests.Instead of using
#regions.AND instead of having to repeat the name of the method being tested over and over in every single test name.
This is especially annoying as I change my mind about method names often. In fact, I changed the name of
GetIconAsSvgafter I tested it. When the method name is in every class, and then I Refactor->Rename it, now I have to rename all the tests, if I even remember (hint, most Devs don't because many don't run tests regularly). Instead of having to rename the Tests (which could be dozens, depending on how complex the method is), I now only change the sub-class name, which I always name as the Method.This was something I picked up at a .NET Coding Convention about 8 years ago. Even Visual Studio was built to understand this notation, as the test runs now look like this:
It appends the sub-class name onto the main class name, with a
+(see highlighted)But, with the older style (circled in red), I have that method name repeated 3 times. If I were to ever rename that, I'd have to rename each method name ... OR I rename the class once, and it's easy, because I copy the method from the top test, and paste it over the (old) sub-class name.
It makes for a lot less work maintaining tests.
AND I do not need regions!
AND I ensure I keep my tests (for the same function) together!!! This is another classic problem. As Devs find new issues with a function and add more tests, they tend to add them at the end of the test class file, or just anywhere random (I've seen it). This encourages them to keep them together. They don't have to. The test will still run outside of the sub-class with the matching name, but it's a clear sign of organization and intent.
And just to be extra sure we didn't cause problems, sure we run the program testing the edges:
Unit tests work. They help you work faster without having to constantly run your program. Yes, they can't easily verify everything, and you can mess up, and leave gaps (which you can usually fill, if you can figure out how to test for it), but the more you get used to thinking of things this way, and looking for flaws, testing the "edges," the more confident you will feel about the "correctness" and stability of your code.