search

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

Change rule AV1130 (remove reference to .NET 4.5) #215

Closed pauljansen42 closed 4 years ago

pauljansen42 commented 4 years ago

Rule AV1130 states "Return an IEnumerable or ICollection instead of a concrete collection class". The intention of the rule is to make sure that no mutable collection classes are returned. However, ICollection is mutable. The only reason why it is mentioned in the rule is that users who run a .NET framework version lower than 4.5 have no other choice than to use ICollection.

Since almost everybody is using at least .NET framework 4.5 (distributed with Visual Studio 2012), I recommend to rephrase the rule and make it a bit stricter. So I suggest to rename the rule to:

Return unchangeable interface collection classes (AV1130)

And allow only

IEnumerable, IReadOnlyCollection, IReadOnlyList or IReadOnlyDictionary<TKey, TValue>, ImmutableArray, ImmutableList or ImmutableDictionary<TKey, TValue>

So without ICollection. The reason why I am asking this is because a lot of people ask us why ICollection is allowed because the rule states "You generally don’t want callers to be able to change an internal collection".

I have discussed this already with Dennis Doomen and he told me "Kan ik me op zich wel in vinden", which means "I can live with that".

bkoelman commented 4 years ago

This makes perfect sense to me. However if you rename the rule as proposed, we should keep the exception for ImmutableArray and friends, as they are not interfaces. I agree that in general it would still be good to prefer interfaces when available.

Perhaps the next name is more correct (some collections are structs): Return interfaces to unchangeable collections (AV1130)

pauljansen42 commented 4 years ago

I am fine with that! Please go ahead.

bart-degreed commented 4 years ago

I don't intend to create a PR for this, but I'm willing to review yours.

pauljansen42 commented 4 years ago

Hi Bart,

I don't understand what you mean. I have already created a ticket for this.

Regards,

Paul

-- Paul Jansen - TIOBE Software, https://www.tiobe.com Victory House II Esp 401 5633 AJ Eindhoven, the Netherlands Phone: +31 40 400 2800 Mobile: +31 617 400 620 TIOBE Software - The Software Quality Company

On 16-Aug-20 7:23 PM, Bart Koelman wrote:

I don't intend to create a PR for this, but I'm willing to review yours.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dennisdoomen/CSharpGuidelines/issues/215#issuecomment-674553527, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ3MFJLHEKF3JG5LBH5DZKTSBAI2DANCNFSM4QA3P75Q.

bkoelman commented 4 years ago

An issue was created by you to propose and discuss changes. I'm fine with them, but more importantly Dennis (the owner of this repo) is too. So next, a pull request needs to be created that contains the actual changes, which Dennis can then approve and merge. Just like you, I'm not an owner of this repro. But I'm willing to provide feedback on such a PR, similar to how I'm giving feedback here.

Hope that helps.

pauljansen42 commented 4 years ago

Hi Bart,

OK, thanks.

@Dennis, would you like me to propose something? Or are you going to prepare the pull request?

Regards,

Paul

-- Paul Jansen - TIOBE Software, https://www.tiobe.com Victory House II Esp 401 5633 AJ Eindhoven, the Netherlands Phone: +31 40 400 2800 Mobile: +31 617 400 620 TIOBE Software - The Software Quality Company

On 16-Aug-20 7:52 PM, Bart Koelman wrote:

An issue was created by you to propose and discuss changes. I'm fine with them, but more importantly Dennis (the owner of this repo) is too. So next, a pull request needs to be created that contains the actual changes, which Dennis can then approve and merge. Just like you, I'm not an owner of this repro. But I'm willing to provide feedback on such a PR, similar to how I'm giving feedback here.

Hope that helps.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dennisdoomen/CSharpGuidelines/issues/215#issuecomment-674556571, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ3MFJPINEM736KRTOO2KQTSBAMEXANCNFSM4QA3P75Q.

bkoelman commented 4 years ago

Aw, I see I'm mixing up work/personal accounts again. Sorry for any confusion, both are from me.

dennisdoomen commented 4 years ago

@pauljansen42 if you're willing to provide us with a PR, that would be great. Otherwise, I'll do it somewhere this week.

pauljansen42 commented 4 years ago

Hi Dennis,

I will ask somebody within our company to write a proposal. Hold on.

Regards,

Paul

-- Paul Jansen - TIOBE Software, https://www.tiobe.com Victory House II Esp 401 5633 AJ Eindhoven, the Netherlands Phone: +31 40 400 2800 Mobile: +31 617 400 620 TIOBE Software - The Software Quality Company

On 16-Aug-20 9:47 PM, Dennis Doomen wrote:

@pauljansen42 https://github.com/pauljansen42 if you're willing to provide us with a PR, that would be great. Otherwise, I'll do it somewhere this week.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dennisdoomen/CSharpGuidelines/issues/215#issuecomment-674569418, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ3MFJJOQTQRIX2L7T4FFY3SBAZU7ANCNFSM4QA3P75Q.

maikelsteneker commented 4 years ago

I've created a pull request (#216) for this.