dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.37k stars 971 forks source link

Refactor classes with multiple private boolean fields to use flags enum #10827

Open elachlan opened 7 months ago

elachlan commented 7 months ago

I performed a search for "private bool _" and filtered out classes with 4 or less fields, the collated and ordered results are in the table below.

I believe we can convert these to use BitVector32 to save runtime memory usage.

elachlan commented 7 months ago

@JeremyKuhne before I go crazy with this, does the team prefer:

Also, I was thinking of changing the private fields to properties to reduce the surface area of the refactor. That way we only touch the fields.

JeremyKuhne commented 7 months ago

Bools only take a byte per so there really is no point for doing anything for 4 or less.

Really we should just use a [Flags] enum for cases where we have several bools in a class. It is much easier to manage and probably results in the smallest code.

EnumExtensions.ChangeFlags<T> is the helper I wrote to efficiently change flags. The idea was to get rid of all of our other Get/Set flag helpers. It should probably move to Core.

elachlan commented 7 months ago

I'll go through and do all the classes with 5 or more properties. I'll switch the fields to properties to hide the logic and reduce the surface area of the change. I'll also move the extension method so it is in core.

JeremyKuhne commented 7 months ago

@elachlan so each class would have something like {ClassName}Flags nested at an appropriate underlying type size (byte, short, int, long).

elachlan commented 7 months ago

@JeremyKuhne yes. I'll also put the enum in a separate file.

elachlan commented 7 months ago

@JeremyKuhne can we use a byte based flags enum for 2-8 bools? That way we cover the 2-4 bool field cases as well.

JeremyKuhne commented 7 months ago

@elachlan I wouldn't bother as the extra type info and complexity are probably eating any benefit.

elachlan commented 7 months ago

It looks like in other places we have gone with {ClassName}States, so I will go with that naming convention?