rubberduck-vba / Rubberduck

Every programmer needs a rubberduck. COM add-in for the VBA & VB6 IDE (VBE).
https://rubberduckvba.com
GNU General Public License v3.0
1.92k stars 301 forks source link

Redundant ByRef modifier or Implicit ByRef parameter? #3174

Closed A9G-Data-Droid closed 6 years ago

A9G-Data-Droid commented 7 years ago

Really though, which is it?

If I include "ByRef" on a parameter then code inspection tells me that "it is not necessary to include the ByRef modifier declaration". If I remove "ByRef" I get a code quality warning that states "specify ByRef explicitly when passing parameters by reference"

If you click fix on one then the other appears. You could fix it back and forth for eternity and it would never be truly "fixed".

This is a case where the advice conflicts with itself. This cognitive dissonance might be an expression of internal strife between the Rubberduck team members. We really need to make a final decision on this.

I think being explicit here is the proper way. It's the same advice given for implicitly public members and implicitly variant variables. Be explicit so we can tell that you did that on purpose. Therefore, I recommend removing the "Redundant ByRef modifier" Code Quality warning.

Rubberduck v2.1.2008 Internal Version 2.0.14.35826

rkapka commented 7 years ago

You can ignore one inspection and fix the other one. Basically, you don't need ByRef (so you can choose to disable the corresponding inspection if you would like to omit the modifier), but on the other hand you might want to always specify the modifier (and in that case you would disable the other inspection).

A9G-Data-Droid commented 7 years ago

Are you suggesting that both warning are valid? Both advices are the officially correct solution? The non-warning state exists in a superposition between the two?

retailcoder commented 7 years ago

This is by design. Some critique said RD was "too pedantic", so we decided to leave it up to the user to decide whether they wanted explicit or implicit ByRef. IMO you are right, explicit is better (hence the vast majority of RD inspections favoring explicitness). Yet explicit ByRef is indeed redundant as far as VBA is concerned.

AAA-grade tools such as JetBrains' ReSharper (RD's biggest inspiration) do the same thing: you could spend the rest of your life "fixing" an inspection result only to get another, contradicting one - the bottomline is that these essentially boil down to personal preference and that we decided to make Rubberduck more flexible in enforcing these preferences, regardless of whether the preference is for explicit or implicit modifiers.

So yeah, one of the two is meant to be set to DoNotShow, but we didn't want to force a default. Or perhaps we should?

A9G-Data-Droid commented 7 years ago

Aha, Thanks. I didn't realize it was intentional. Maybe it could be a radiobutton, explicit or implicit, so that you will never be banished to the land where both of your available choices are wrong. It's upsetting at a very basic instinctual level.

retailcoder commented 7 years ago

BTW thanks for your continued feedback, support and usage!

A9G-Data-Droid commented 7 years ago

Sure thing! I use Rubberduck everyday at work now. It has helped me learn a lot about VBA and I am a better coder for it.

daFreeMan commented 7 years ago

You and me both, @A9G-Data-Droid - you and me both!

A9G-Data-Droid commented 6 years ago

I think this was fixed but I don't know when. In my settings I see that redundant 'ByRef' is set to "Do Not Show" and "Implicit ByRef parameter" is set to "Hint". If those are the default settings then this issue can be closed.