Code cleanup in System.ComponentModel.TypeConverter by bmeverett · Pull Request #14154 · dotnet/corefx

bmeverett · 2016-12-01T18:01:08Z

Resolves #12160 to clean up code in System.ComponentModel.TypeConverter
cc @AlexGhiondea

I created an individual commit for each improvement, didn't know if that would make review any easier. I tried to search the project and make sure everything was accounted for. This is a decent size project so something may have been missed. Feel free to comment and I'll be glad to fix it.

dnfclas · 2016-12-01T18:01:13Z

Hi @bmeverett, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

dnfclas · 2016-12-01T18:07:50Z

@bmeverett, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

AlexGhiondea · 2016-12-02T04:37:07Z

@bmeverett thanks for your contribution! I am going to take a closer look at this next week when I get back in the office!

stephentoub · 2016-12-02T16:57:42Z

            //

-            Attribute[] attributes = null;
+            Attribute[] attributes = Array.Empty<Attribute>();


Does this need to be assigned at all? It's set in both the subsequent if/else branch, so it should be definitely assigned without needing this initializer.

stephentoub · 2016-12-02T16:59:40Z

-                _onListChanged -= value;
-            }
-        }
+        public event ListChangedEventHandler ListChanged; 


This doesn't look like a safe change. The _onListChanged field was annotated as [NonSerialized], but the backing field generated for this event won't have such an attribute on it.

stephentoub · 2016-12-02T17:00:54Z

-            _dataSource = null;
-            _dataMember = null;
+            DataSource = null;
+            DataMember = null;


Are these lines necessary at all? null is the default value.

stephentoub · 2016-12-02T17:01:11Z

-            _dataSource = dataSource;
-            _dataMember = null;
+            DataSource = dataSource;
+            DataMember = null;


Is this line necessary? null is the default value.

stephentoub · 2016-12-02T17:02:15Z

+                        foreach (var val in GetStandardValues(context))
                        {
-                            CultureInfo info = (CultureInfo)e.Current;
+                            CultureInfo info = (CultureInfo)val;


This can be changed to just:

foreach (CultureInfo info in GetStandardValues(context))

stephentoub · 2016-12-02T17:02:27Z

+                        foreach (var val in _values)
                        {
-                            CultureInfo info = (CultureInfo)e.Current;
+                            CultureInfo info = (CultureInfo)val;


stephentoub · 2016-12-02T17:05:40Z

                else
                {
-                    format = formatInfo.ShortDatePattern + " " + formatInfo.ShortTimePattern;
+                    format = $"{formatInfo.ShortDatePattern} {formatInfo.ShortTimePattern}";


This should be reverted. It's less expensive the way it was originally.

stephentoub · 2016-12-02T17:25:59Z

-            get
-            {
-                // ComCtl32.dll V6 (WindowsXP) provides a nice black circle but we don't want to attempt to simulate it 
-                // here to avoid hard coding values.  MaskedTextBox picks up the right value at run time from comctl32.


Why was this comment deleted?

stephentoub · 2016-12-02T17:27:32Z

            _nameHash = _name.GetHashCode();

-            ArrayList newArray = new ArrayList();
+            List<Attribute> newArray  = new List<Attribute>();


Nit: extra space before the =

stephentoub · 2016-12-02T17:32:37Z

-            }
-
-            return obj;
+            return obj ?? (obj = Activator.CreateInstance(objectType, args));


Why does obj need to be assigned? e.g. why not just:

return obj ?? Activator.CreateInstance(objectType, args);

?

stephentoub · 2016-12-02T17:36:38Z

            }

-            return instance;
+            return instance ?? (instance = NodeFor(objectType).CreateInstance(provider, objectType, argTypes, args));


Why does instance need to be assigned?

stephentoub

Thanks. Overall the changes look good, but there are some issues that will need to be addressed.

bmeverett · 2016-12-02T18:14:23Z

@stephentoub Thanks for the feedback! Especially about string interpolation. I will keep that in mind. I will fix up your comments and make another commit.

AlexGhiondea

Thanks for all this work @bmeverett!!!

Overall it looks great! I had a couple of minor comments that we should consider doing before taking this change.

Again -- thanks for doing this!!

AlexGhiondea · 2016-12-05T18:26:41Z

-                return typeof(Byte);
-            }
-        }
+        internal override Type TargetType => typeof(Byte);


Byte -> byte

AlexGhiondea · 2016-12-05T18:29:32Z

-                {
-                    return s_cultureInfoNameMap[cultureInfoDisplayName];
-                }
+            private static readonly Dictionary<string, string> s_cultureInfoNameMap = CreateMap();


Was the removal of volatile intentional?

I used the example from the issue and you had it removed there. Should it be added back in?

With this change, volatile isn't necessary anymore... it's almost the opposite of readonly, where readonly effectively says "this will never change so feel free to cache it as long as you like" and volatile kind of says "this can change at any moment and it's important you use the absolute latest".

AlexGhiondea · 2016-12-05T18:33:17Z

-                return typeof(Decimal);
-            }
-        }
+        internal override Type TargetType => typeof(Decimal);


Decimal -> decimal

AlexGhiondea · 2016-12-05T18:33:41Z

        public DefaultBindingPropertyAttribute()
        {
-            _name = null;
+            Name = null;


This is not required

AlexGhiondea · 2016-12-05T18:35:07Z

-                return _name;
-            }
-        }
+        public string Name => _name;


Can we remove _name?

AlexGhiondea · 2016-12-05T18:56:54Z

-                return typeof(SByte);
-            }
-        }
+        internal override Type TargetType => typeof(SByte);


SByte -> sbyte

AlexGhiondea · 2016-12-05T18:57:38Z

-                return typeof(Single);
-            }
-        }
+        internal override Type TargetType => typeof(Single);


Single -> float

AlexGhiondea · 2016-12-05T18:59:15Z

-                return typeof(UInt16);
-            }
-        }
+        internal override Type TargetType => typeof(UInt16);


Uint16 -> ushort

AlexGhiondea · 2016-12-05T18:59:51Z

-                return typeof(UInt32);
-            }
-        }
+        internal override Type TargetType => typeof(UInt32);


UInt32- > uint

AlexGhiondea · 2016-12-05T19:01:24Z

-                return typeof(UInt64);
-            }
-        }
+        internal override Type TargetType => typeof(UInt64);


UInt64 -> ulong

safern

I left some comments.

Thanks for doing this!

safern · 2016-12-05T19:30:44Z

        private class Site : ISite
        {
-            private IComponent _component;
            private Container _container;


Nit: Just such as you did it with _component we should delete _container and just use the property Container with it's default get.

safern · 2016-12-05T19:53:44Z

-                return _commandID;
-            }
-        }
+        public virtual int ID => _commandID;


Why are we here not using the same approach as in the other classes that you deleted the private fields and used the property only with a public get?

safern · 2016-12-05T19:54:40Z

-                return _oldDesigner;
-            }
-        }
+        public IDesignerHost OldDesigner => _oldDesigner;


Same here, can we remove _oldDesigner and _newDesigner?

safern · 2016-12-05T21:08:22Z

-                return invalidIndex;
-            }
-        }
+        public static int InvalidIndex => invalidIndex;


Should we update all constants from this class to be uppercase? e.g invalidIndex to INVALID_INDEX ?

safern · 2016-12-05T21:11:59Z

-                    return _container;
-                }
-            }
+            public IContainer Container => _container;


Can we delete _container and just use Container ?

safern · 2016-12-05T21:31:06Z

@@ -69,49 +67,27 @@ public ToolboxItemFilterAttribute(string filterString, ToolboxItemFilterType fil
        {
            if (filterString == null) filterString = String.Empty;


Instead of doing

if (filterString == null) filterString = String.Empty

we could simplify this to one line when initializing FilterString :

FilterString = filterString ?? string.Empty;

safern · 2016-12-05T21:36:57Z

+                    }
+                    else
+                    {
+                        return site.Name;


We could simplify this to:

return nestedSite != null ? nestedSite.FullName : site.Name;

Or I think we could use ?? operator as well:

return (nestedSite?.FullName) ?? site.Name;

converted remaining properties to auto properties

bmeverett · 2016-12-07T22:44:39Z

I think the tests may have gotten stuck. Not sure if it was something I did.

safern · 2016-12-07T22:49:40Z

The details of Innerloop CentOS7.1 are not found on the link provided.
Innerloop OSX Debug has an error, I'm taking a look on that one.
Innerloop OSX Release actually passed but never updated the status here on the PR.

@mmitche

mmitche · 2016-12-07T22:54:41Z

@safern I beleive this is because the lab is being moved. Please ignore.

safern · 2016-12-07T22:57:00Z

Ok then I'm going to go ahead and restart both tests with failures.

test Innerloop CentOS7.1 Debug Build and Test please
test Innerloop OSX Debug Build and Test please

safern

Looks good to me! Thanks for doing this.

stephentoub · 2016-12-09T14:18:22Z

@dotnet-bot test this please

stephentoub

Thanks, @bmeverett!

bmeverett · 2016-12-09T15:53:04Z

Thanks guys!

* remove explicit base calls * auto implemented properties * use expression body members * use ? for null checking * use string interpolation * remove unnecessary initialization * replace empty arrays with array.empty * simplify event handler declarations * readonly modifier for sync objects * use foreach * cache char[] into static readonly field * simplify CultureInfoMapper * reaplce non generic collections with generic collections * fix code review comments a lot dealing with string interpolation. * add space to datetimeconverter and datetimeoffsetconverter to fix tests * additional modifications converting types * uppercase constants in MaskedTextProvider converted remaining properties to auto properties

…otnet#14154.

) * remove explicit base calls * auto implemented properties * use expression body members * use ? for null checking * use string interpolation * remove unnecessary initialization * replace empty arrays with array.empty * simplify event handler declarations * readonly modifier for sync objects * use foreach * cache char[] into static readonly field * simplify CultureInfoMapper * reaplce non generic collections with generic collections * fix code review comments a lot dealing with string interpolation. * add space to datetimeconverter and datetimeoffsetconverter to fix tests * additional modifications converting types * uppercase constants in MaskedTextProvider converted remaining properties to auto properties Commit migrated from dotnet/corefx@6f48f19

Commit migrated from dotnet/corefx@9c18f4b

bmeverett added 13 commits November 30, 2016 09:00

remove explicit base calls

b27527c

auto implemented properties

cac3e70

use expression body members

9810ee5

use ? for null checking

1dfc195

use string interpolation

74e4ed5

remove unnecessary initialization

f69d431

replace empty arrays with array.empty

fda4d31

simplify event handler declarations

47d0db4

readonly modifier for sync objects

a19292d

use foreach

31a4e89

cache char[] into static readonly field

a7dc4f3

simplify CultureInfoMapper

e74cfe0

reaplce non generic collections with generic collections

03d5720

dnfclas added the cla-required label Dec 1, 2016

dnfclas added cla-signed and removed cla-required labels Dec 1, 2016

karelz added the area-System.ComponentModel label Dec 1, 2016

karelz assigned bmeverett, AlexGhiondea and safern Dec 1, 2016

stephentoub reviewed Dec 2, 2016

View reviewed changes

bmeverett added 2 commits December 2, 2016 14:34

fix code review comments a lot dealing with string interpolation.

977ffbf

add space to datetimeconverter and datetimeoffsetconverter to fix tests

9b3eaef

AlexGhiondea reviewed Dec 5, 2016

View reviewed changes

additional modifications converting types

1066efe

safern reviewed Dec 5, 2016

View reviewed changes

karelz modified the milestone: 1.2.0 Dec 6, 2016

uppercase constants in MaskedTextProvider

c3bca8b

converted remaining properties to auto properties

safern approved these changes Dec 8, 2016

View reviewed changes

stephentoub approved these changes Dec 9, 2016

View reviewed changes

stephentoub merged commit 6f48f19 into dotnet:master Dec 9, 2016

bmeverett deleted the code-cleanup branch December 9, 2016 15:53

pjanotti pushed a commit to pjanotti/corefx that referenced this pull request Sep 28, 2017

active supress coreclr dotnet#14154

c95b6d4

pjanotti pushed a commit that referenced this pull request Sep 28, 2017

active supress coreclr #14154 (#24305)

9c18f4b

poizan42 added a commit to poizan42/corefx that referenced this pull request Oct 3, 2017

Make FileSystemWatcher tests tolorant of spurious/extra events. Fixes d…

8215a85

…otnet#14154.

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

active supress coreclr dotnet/corefx#14154 (dotnet/corefx#24305)

6dca35f

Commit migrated from dotnet/corefx@9c18f4b

		@@ -69,49 +67,27 @@ public ToolboxItemFilterAttribute(string filterString, ToolboxItemFilterType fil
		{
		if (filterString == null) filterString = String.Empty;

Conversation

bmeverett commented Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Dec 1, 2016

Uh oh!

dnfclas commented Dec 1, 2016

Uh oh!

AlexGhiondea commented Dec 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

bmeverett commented Dec 2, 2016

Uh oh!

AlexGhiondea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bmeverett commented Dec 1, 2016 •

edited

Loading

safern commented Dec 7, 2016 •

edited

Loading