Simplify code in EventListenerTextWriter #3309

jnm2 · 2019-07-04T19:22:13Z

The first commit fixes the whitespace in EventListenerTextWriter. Probably best to avoid including this in the diffs you look at.

The remaining commits show each step in the process of removing duplicated code in EventListenerTextWriter.

oznetmaster · 2019-07-04T19:47:46Z

Why??

You really think this code is easier to read and maintain?

I have no problem with removing/refactoring duplicated code.

It is the rest of it that concerns me.

oznetmaster · 2019-07-04T19:58:36Z

Once again, just because the C# team has decided to add some new feature to C# does not mean that it is good/maintainable coding practice to use it.

There is much controversy and disagreement over many of the features.

I certainly see no need to start retrofitting existing code to use them, for no other reason then they exist.

I am sure that this is one of those divisive issues.

jnm2 · 2019-07-04T20:35:55Z

@oznetmaster

Why??

You really think this code is easier to read and maintain?

Are you referring to the first commit, "This is the way I wanted to write it originally," or the second commit, "Less verbosity"?

For the first, it makes sense to me that I should be allowed to contribute the code I originally wanted to contribute and for which I asked that we allow C# 7. In the case of the switch especially, it's now three times as readable in my opinion.

For the second, it's changing three lines to one line. If this one commit is that big of a deal to you, I'm happy to drop it but very confused at your strong reaction. This isn't "many of the features." This is a single C# 6 feature, the ?. operator. We use it many other places in NUnit already.

What was specifically rejected was the mass changes for the sake of changes. Not C# 7 syntax. I fully intend to use it judiciously in all future PRs of any nature.

I certainly see no need to start retrofitting existing code to use them, for no other reason then they exist.

I object to this characterization. This is not happening in any line of this PR.

jnm2 · 2019-07-04T20:53:31Z

Oops, didn't mean to click Close. Sorry!

jnm2 · 2019-07-04T21:09:52Z

There is much controversy and disagreement over many of the features.

Since you make this point, please mention specifically which lines of code in this PR you believe to be controversial to @nunit/framework-team. I listened to the feedback from all of you in the two C# 7 PRs and made sure that this PR doesn't fall into any specific issue that anyone mentioned. If I missed something, please point to a particular line of code. That's much less confounding than a blanket statement that any C# 7 is bad.

(For one thing, we decided as a team to allow C# 7 syntax. For another, the syntax itself had positive comments in other PRs from multiple members. Lastly, the vehemence against C# 7 in the general programming world which you've cited a few times is not visible in the slice of the world that I have access to. I'll take it into consideration, but on a personal level I have no option but to consider it in proportion to the other evidence around me.)

Please also clarify if you'd like me to drop the second commit and/or move the first commit to its own PR.

oznetmaster · 2019-07-04T23:57:41Z

For example, I asked a number of my colleagues about:

                var context = SynchronizationContext.Current as SingleThreadedTestSynchronizationContext;

                if (context == null)
                    throw new InvalidOperationException("This strategy must only be used from a SingleThreadedTestSynchronizationContext.");

vs.

                var context = SynchronizationContext.Current as SingleThreadedTestSynchronizationContext
                    ?? throw new InvalidOperationException("This strategy must only be used from a SingleThreadedTestSynchronizationContext.");

and I could not find a single one who did not think that the second one was inferior to the first for readability and maintainability.

Obviously, we work in totally different programming environments.

jnm2 · 2019-07-05T02:42:09Z

@oznetmaster I'm willing to drop that to keep the peace, but since nothing tells me why this is less readable or maintainable, I will certainly use it in future PRs. That makes sense, right?

CharliePoole · 2019-07-05T03:06:01Z

@jnm2 I vowed to keep out of this set of refactoring PRs but I'll just butt in to say that the standard for what you might do in a PR for a different purpose is (and should be) biased towards your preferences. The person who does the fix should get to decide, within limits of a standard, how to do it. Other folks may have opinions, which you should pay attention to, but you get to decide.

On the other hand, when you do a global fix just for its own sake, you open a can of worms. Everyone won't agree that what you are doing is desirable and, by definition, it's not necessary.

Whether (for example) using something other than a value after ?? is clear or unclear is a matter of opinion. (*) It's not worth putting forth arguments as to why your opinion is right, because there is no right in matters of style. There's only agreement or non-agreement.

I understand that putting forth your own view strongly can sometimes work to get a productive discussion going. So far, I'm not seeing that happening here. Usually, we are better able to do this in issues than PRs I think.

(*) My opinion, it's absolutely not clear on first reading.

jnm2 · 2019-07-05T05:58:48Z

@CharliePoole Thanks, those are good points.

@CharliePoole and @oznetmaster I hope it's clear that there are no global refactorings being discussed or in progress. Today's set of refactoring PRs has nothing to do with code style, with the exception of the two commits which I have removed. I may bring one back in a separate PR because the goto case is really harder to read than the C# 7 when clause, but it's definitely separable from this PR.

jnm2 · 2019-07-05T06:00:35Z

(ready for review of the remaining code)

CharliePoole · 2019-07-05T09:01:55Z

@jnm2 Sorry, since I wasn't reviewing this one, I actually only read @oznetmaster 's comment and took it to be a continuation of our standards discussions. I see now it's a specific item you felt needed to be refactored. In doing so, you used certain features as is your choice as the person doing it. IMO anyone who reviews should specifically indicate lines you changed and say something like "This seems confusing to me...."

Sorry I missed that. However, I think most of us are out here like stateless services, responding to the specific comments we see when we see them and therefore sometimes missing some context.

Now that I'm clear about what you are doing, I would suggest adding (in future refactoring PRs) a sentence or two that says why you are doing this particular thing right now. I don't think the title is sufficient. Removing duplication is good as is simplifying code. But if you were not already working in EventListenerTextWriter, there was some motivation as to why you wanted to do this right now. IMO it helps to tell us, even if all there is to tell is "I got tired of seeing this" or "I'm finding this code hard to follow." Then anyone who reviews can feel that they are helping you do something you want to accomplish.

At least, this is what I would try to do and recommend to anyone else. YMMV.

CharliePoole

This looks good to me. I didn't see the commits you removed, but I suspect I would agree with @oznetmaster on the positioning of the throw inside an expression.

CharliePoole · 2019-07-05T09:08:17Z

src/NUnitFramework/framework/Internal/Execution/EventListenerTextWriter.cs

+        private string FormatForListener(object value)
+        {
+            return
+                value is null ? string.Empty :


I have always lined up and indented the ternary op like so...

return value is null ? string.Empty : value is IFormattable formattable ? formattable.ToString(null, FormatProvider) : value.ToString();

I'm not sure if we have a standard, but that seems much clearer to me.

I'm as yet unclear about the inline declaration of formattable. I suspect I'll like it after I get used to it though.

I go back and forth between that formatting and this one depending on how I want to look at it: flat and linear like a switch statement, or as a tree. The way it is now, it lines up all the cases after ? characters and it ends all the lines with an outcome. I usually switch to the tree structure when it can't be thought of as flat, but I see that situation as more complex. Happy to change this though.

Only a suggestion. I could try to look at it as a switch statement and see how that works.

Okay, I might lean towards leaving it as-is then.

Speaking of switching, here is C# 8's switch expression—not sure yet if I like it as much as my flat ternary though:

return value switch { null => string.Empty, IFormattable formattable => formattable.ToString(null, FormatProvider), _ => value.ToString() };

CharliePoole · 2019-07-05T09:09:43Z

src/NUnitFramework/framework/Internal/Execution/EventListenerTextWriter.cs

-                        _defaultWriter.WriteLine(value);
-                }
-            }
+            if (!TrySendLineToListener(FormatForListener(value)))


I like how it cleans up the duplication.

Only a suggestion. I could try to look at it as a switch statement and see how that works.

jnm2 · 2019-07-05T11:58:15Z

@CharliePoole

Now that I'm clear about what you are doing, I would suggest adding (in future refactoring PRs) a sentence or two that says why you are doing this particular thing right now. I don't think the title is sufficient.

Thanks, this seems like a great idea. In the case of these six PRs, these are places where I ran across the code for unrelated reasons. The code raised a flag in my mind that it could be cleaner or that in some cases the implementation could be improved.

ChrisMaddock

I don't know what's since been removed, but what's left looks like a solid improvement to me. Thanks Joseph!

jnm2 added is:refactor awaiting:review labels Jul 4, 2019

jnm2 requested a review from a team July 4, 2019 19:22

jnm2 closed this Jul 4, 2019

jnm2 reopened this Jul 4, 2019

jnm2 added 9 commits July 4, 2019 22:50

Fix whitespace

0d9f92a

Deduplicate invocation of default writer

f6dd5a3

Deduplicate calls to TrySendToListener

a307211

Convert if statement to conditional expression

d66b966

Declare temporary variable inline

639ced6

Move listener object formatting to helper method

bcc4481

Merge if statements

22e7db8

Invert if statements

f18612c

Use helper method to replace the duplicate implementation below

991ac7e

jnm2 force-pushed the csharp_simplifications branch from e7f56d0 to 991ac7e Compare July 5, 2019 02:52

jnm2 changed the title ~~C# simplifications~~ Simplify code in EventListenerTextWriter Jul 5, 2019

CharliePoole approved these changes Jul 5, 2019

View reviewed changes

ChrisMaddock approved these changes Jul 6, 2019

View reviewed changes

ChrisMaddock merged commit 743ffdd into nunit:master Jul 6, 2019

ChrisMaddock added the closed:done label Jul 6, 2019

ChrisMaddock added this to the 3.13 milestone Jul 6, 2019

ChrisMaddock assigned jnm2 Jul 6, 2019

jnm2 deleted the csharp_simplifications branch July 7, 2019 02:34

jnm2 removed the awaiting:review label Jul 7, 2019

Simplify code in EventListenerTextWriter #3309

Simplify code in EventListenerTextWriter #3309

Uh oh!

Conversation

jnm2 commented Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oznetmaster commented Jul 4, 2019

Uh oh!

oznetmaster commented Jul 4, 2019

Uh oh!

jnm2 commented Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnm2 commented Jul 4, 2019

Uh oh!

jnm2 commented Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oznetmaster commented Jul 4, 2019

Uh oh!

jnm2 commented Jul 5, 2019

Uh oh!

CharliePoole commented Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnm2 commented Jul 5, 2019

Uh oh!

jnm2 commented Jul 5, 2019

Uh oh!

CharliePoole commented Jul 5, 2019

Uh oh!

CharliePoole left a comment

Choose a reason for hiding this comment

Uh oh!

CharliePoole Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

jnm2 Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnm2 Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

CharliePoole Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

CharliePoole Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

jnm2 commented Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisMaddock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnm2 commented Jul 4, 2019 •

edited

Loading

jnm2 commented Jul 4, 2019 •

edited

Loading

jnm2 commented Jul 4, 2019 •

edited

Loading

CharliePoole commented Jul 5, 2019 •

edited

Loading

jnm2 Jul 5, 2019 •

edited

Loading

jnm2 commented Jul 5, 2019 •

edited

Loading