dennisdoomen / CSharpGuidelines

A set of coding guidelines for C# 9.0, design principles and layout rules for improving the overall quality of your code development.
https://www.csharpcodingguidelines.com
Other
746 stars 271 forks source link

Proposal for new rule: Write code that is easy to debug #206

Closed bkoelman closed 4 years ago

bkoelman commented 4 years ago

I sometimes see developers write lines like this:

string result = ConvertToXml(ApplyTransforms(ExecuteQuery(GetConfigurationSettings(source))));

in an attempt to keep the number of statements within a method below the required threshold. I would prefer the code to read like this instead (using var for brevity):

var configurationSettings = GetConfigurationSettings(source);
var queryResult = ExecuteQuery(configurationSettings);
var transformed = ApplyTransforms(queryResult);
var xml = ConvertToXml(transformed);

Because now it becomes possible to add breakpoints to the individual steps and inspect their outcomes without the need to step into them.

Thoughts?

bkoelman commented 4 years ago

Note this is not about chaining method calls, which is a common pattern in fluent APIs.

dennisdoomen commented 4 years ago

Yeah, I think there's a balance to uphold to.

Something I like about Rider is that when you press F11 (step into), it allows you to choose the method in that nested call you want to dive into. It's call Smart Steps.

bkoelman commented 4 years ago

Wow, that's an awesome Rider feature! Nice, I did not know about that.

Do you still think this rule should be added?

dennisdoomen commented 4 years ago

Yes, it makes sense. But again, we want to make sure there's a balance. Just putting everything on a separate line is a bit too much for me.

bkoelman commented 4 years ago

Sounds good to me. How would you phrase the rule then?

dennisdoomen commented 4 years ago

I'm in doubt between "Avoid overuse of nested calls", which is very specific, and your original title.

bkoelman commented 4 years ago

We can have both. The goal is easy to debug (title) and the way to achieve that is by not overusing nested calls (in the text).

dennisdoomen commented 4 years ago

Let's do that.

bkoelman commented 4 years ago

I've submitted a PR earlier, but now I started thinking some more about this. While Smart Steps prevents us from needing to step-into/step-out multiple times, it doesn't help in showing intermediate return values. How I would do that:

  1. Step into the method
  2. Scroll to the closing curly of the method body and select Run to here
  3. Inspect the magical $ReturnValue in watch/locals/auto window

Alternatively I may paste the method call in Intermediate window and see the outcome there. Assuming the method has no side effects, which I need to verify first.

Is there an easier way I'm unaware of? Can you provide an example where nested method calls (not properties) would be a good thing? I can't think of any.

dennisdoomen commented 4 years ago

Given this piece of code:

image

As soon as I complete that nested call, this is what Rider shows me:

image

Visual Studio 2019 does the same thing:

image

bkoelman commented 4 years ago

Looks like I missed something indeed! :)

dennisdoomen commented 4 years ago

But do you still stand behind your rule?

bkoelman commented 4 years ago

I think the rule as currently formulated is still valid (do not overuse). Now I understand why "not overuse" is more appropriate than saying each call result must be put into a separate variable, which I believed to be best earlier.