Don't immediately throw in ConstructorInjectionConvention

rather let the ModelFactory throw as the convention is applied twice in hosting case
natemcmaster · Nov 26, 2018 · a80c375 · a80c375
1 parent a0e8d52
commit a80c375
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 36 deletions.
diff --git a/src/CommandLineUtils/Conventions/ConstructorInjectionConvention.cs b/src/CommandLineUtils/Conventions/ConstructorInjectionConvention.cs
@@ -44,11 +44,11 @@ public virtual void Apply(ConventionContext context)
                 return;
             }
 
-            s_applyMethod.MakeGenericMethod(context.ModelType).Invoke(this, new object[] { context });
+            s_applyMethod.MakeGenericMethod(context.ModelType).Invoke(this, new object[] {context});
         }
 
         private static readonly MethodInfo s_applyMethod
-         = typeof(ConstructorInjectionConvention).GetRuntimeMethods().Single(m => m.Name == nameof(ApplyImpl));
+            = typeof(ConstructorInjectionConvention).GetRuntimeMethods().Single(m => m.Name == nameof(ApplyImpl));
 
         private void ApplyImpl<TModel>(ConventionContext context)
             where TModel : class
@@ -62,41 +62,28 @@ private void ApplyImpl<TModel>(ConventionContext context)
                 throw new InvalidOperationException(Strings.NoAnyPublicConstuctorFound(typeof(TModel)));
             }
 
-            var (matchedCtor, matchedArgs) = (constructors.Length == 1)
-                // shortcut for single constructor
-                ? FindMatchedConstructor(constructors, context.Application,
-                    throwIfNoParameterTypeRegistered: true)
-                // find the constructor with the most parameters first
-                : FindMatchedConstructor(constructors, context.Application,
-                    throwIfNoParameterTypeRegistered: false);
+            var factory = FindMatchedConstructor<TModel>(constructors, context.Application,
+                constructors.Length == 1);
 
-            var parameterLessCtor = Array.Find(constructors, c => c.GetParameters().Length == 0);
-
-            if (matchedCtor == null && parameterLessCtor != null)
-            {
-                return;
-            }
-
-            if (matchedCtor == null && parameterLessCtor == null)
+            if (factory != null)
             {
-                throw new InvalidOperationException(Strings.NoMatchedConstructorFound(typeof(TModel)));
-            }
-
-            if (matchedCtor != null)
-            {
-                (context.Application as CommandLineApplication<TModel>).ModelFactory =
-                    () => (TModel)matchedCtor.Invoke(matchedArgs);
+                ((CommandLineApplication<TModel>) context.Application).ModelFactory = factory;
             }
         }
 
-        private (ConstructorInfo matchedCtor, object[] matchedArgs) FindMatchedConstructor(
+        private static Func<TModel> FindMatchedConstructor<TModel>(
             ConstructorInfo[] constructors,
             IServiceProvider services,
             bool throwIfNoParameterTypeRegistered = false)
         {
             foreach (var ctorCandidate in constructors.OrderByDescending(c => c.GetParameters().Length))
             {
                 var parameters = ctorCandidate.GetParameters().ToArray();
+                if (parameters.Length == 0)
+                {
+                    return null;
+                }
+
                 var args = new object[parameters.Length];
                 for (var i = 0; i < parameters.Length; i++)
                 {
@@ -106,22 +93,25 @@ private void ApplyImpl<TModel>(ConventionContext context)
                     {
                         if (!throwIfNoParameterTypeRegistered)
                         {
-                            continue;
+                            goto nextCtor;
                         }
-                        throw new InvalidOperationException(Strings.NoParameterTypeRegistered(ctorCandidate.DeclaringType, paramType));
+
+                        return () =>
+                            throw new InvalidOperationException(
+                                Strings.NoParameterTypeRegistered(ctorCandidate.DeclaringType, paramType));
                     }
+
                     args[i] = service;
                     if (i == parameters.Length - 1)
                     {
-                        var matchedArgsLength = args.Count(x => x != null);
-                        if (parameters.Length == matchedArgsLength)
-                        {
-                            return (ctorCandidate, args);
-                        }
+                        return () => (TModel) ctorCandidate.Invoke(args);
                     }
                 }
+
+                nextCtor: ;
             }
-            return (null, null);
+
+            return () => throw new InvalidOperationException(Strings.NoMatchedConstructorFound(typeof(TModel)));
         }
     }
 }
diff --git a/test/CommandLineUtils.Tests/ConstructorInjectionConventionTests.cs b/test/CommandLineUtils.Tests/ConstructorInjectionConventionTests.cs
@@ -149,9 +149,9 @@ public TestAppWithoutMatchedConstructor(TestConsole console)
         public void ThrowsWhenNoMatchedConstructorFound()
         {
             var app = new CommandLineApplication<TestAppWithoutMatchedConstructor>();
-            var ex = Assert.Throws<TargetInvocationException>(() => app.Conventions.UseConstructorInjection());
-            Assert.IsType<InvalidOperationException>(ex.InnerException);
-            Assert.Equal(Strings.NoParameterTypeRegistered(typeof(TestAppWithoutMatchedConstructor), typeof(TestConsole)), ex.InnerException.Message);
+            app.Conventions.UseConstructorInjection();
+            var ex = Assert.Throws<InvalidOperationException>(() => app.ModelFactory());
+            Assert.Equal(Strings.NoParameterTypeRegistered(typeof(TestAppWithoutMatchedConstructor), typeof(TestConsole)), ex.Message);
         }
 
         private class TestAppWithAlternativeConstructor