xaml-math icon indicating copy to clipboard operation
xaml-math copied to clipboard

Establish code style standards

Open ForNeVeR opened this issue 6 years ago • 1 comments

Discussion extracted from #116

@alexreg I also noticed that for properties the this. prefix is not being used. This is good practice, and convention throughout the existing codebase. We need to normalise this.


@ForNeVeR I disagree, let's discuss that. I think that there's guideline to avoid this. in some of the popular C# coding guidelines I'm personally trying to follow when writing in C#:

I can see that this. is more or less consistently used in the existing code base, although I try to remove any unnecessary code from the methods I change. this. falls into that category.

After all, in properly written code this. should almost never be necessary: when you see var x = <something>, it's usually unambiguous:

  • either <something> starts with _, and then it's a field: var x = _field
  • or it starts with a capital, and then it's a property (or a static readonly field per ReSharper guideline)

The only place when this. is "absolutely necessary" is when you're calling an extension method on it (and this is uncommon).


@alexreg

I disagree, let's discuss that. I think that there's guideline to avoid this. in some of the popular C# coding guidelines I'm personally trying to follow when writing in C#:

The official VS (Roslyn) guideline recommends it, last time I checked. In any case, it makes code a lot more readable, since you can determine instance properties just by looking at them. Huge win. You also don't have to get involved in any shit like underscore prefixing, which plagues Java.


@ForNeVeR Java doesn't generally use underscore prefixing, and that's the reason they prefer this.. I think you've actually mistook C# and Java here: in C#, people are often "plague" their code with underscores, and in Java they never do that :)


@ForNeVeR Also, the Roslyn repo now references the same style document I linked before (the same that recommends against this.).


@alexreg

Java doesn't generally use underscore prefixing, and that's the reason they prefer this.. I think you've actually mistook C# and Java here: in C#, people are often "plague" their code with underscores, and in Java they never do that :)

I've seen quite the opposite, and am quite sure of it! I guess we've had different experiences. Anyway, I think I've offered good rationale here; either way though, we need to be consistent about usage.


@alexreg

Also, the Roslyn repo now references the same style document I linked before (the same that recommends against this.).

You're confusing the guidelines for Roslyn itself, and Roslyn style analysers, which insist on using this. for all properties (and possibly more, I forget).


@ForNeVeR Alright, I agree. I'll extract all the messages about code style to a separate issue later. We'll establish a code style document and add a linter or style analyzer to check it for us. this. will be mandatory.

Alexander, thanks for discussion!


@alexreg @ForNeVeR Okay, thanks for hearing me out! Sounds like a good plan for the future. We'll leave it for a separate issue, so we don't pollute this PR discussion any more. :-)

Thus we should:

  • [ ] establish a coding standard and mention it in our README / CONTRIBUTING files
    • [ ] the standard should be based on the corefx guide
    • [ ] although this. should be mandatory
  • [ ] add a Roslyn analyzer, a linter or something we could automate that will check our usage of this. and other issues with coding standard
  • [ ] clean up all the existing code according to the standard
  • [ ] require our contributors to follow the guide (by executing the linter on the CI server)

ForNeVeR avatar Mar 20 '18 15:03 ForNeVeR

I second this plan of action.

alexreg avatar Mar 20 '18 17:03 alexreg