Mystery Meat

JimLahey

In keeping with fleshy current affairs, our codebase has thrown up a WTF not fit for human consumption. Ladies and gentlemen, allow me to present the Mystery Meat pattern:

public IController CreateForm(Type formType, IModel model)
{
	try
	{
		Type formType = registeredTypes[formType.Name];

		ConstructorInfo constructor = formType.GetConstructor(new Type[] { typeof(IModel) });

		IController controller = (IController)constructor.Invoke(new object[] { model });

		return controller;
	}
	catch (Exception e)
	{
		throw new ArgumentException("The given type is not a valid implementation of IController", "formType", e);
	}
}

What this pattern does is obfuscate the instantiation of the only Form Controller in the entire application that implements IController so you have to step through thousands of lines by hand to find it. Extra marks for zero input parameter validation and an exception handler that will send you in completely the wrong direction if the implementation doesn't actually have that constructor, which you'll only find out at runtime. Called Mystery Meat for the reason that you don't know where the fuck it's from or what the fuck is in it but when you find out, you'll really wish you hadn't.

To think we've got several devs who won't go near an IoC container or interfaces because "we can't quite see what's going on". Nice one.

Inspired me for a google search for "I know. I'll use Reflection".

Results are as expected...

MiffTheFox

Some people, when confronted with a problem, think “I know, I’ll quote Jamie Zawinski.” Now they have two problems.

Seriously, I dislike that guy with a passion. If I felt like it I'd make a Windows port of Xscreensaver to piss him off. The webpage would start with a quote from the GPL.

ubersoldat

No matter the language, when you find ".GetConstructor" you should stop and think if there's a better way of doing what you're about to do... and yes, IoC containers are better than what you're about to do.

TheCPUWizard

@ubersoldat said:

No matter the language, when you find ".GetConstructor" you should stop and think if there's a better way of doing what you're about to do... and yes, IoC containers are better than what you're about to do.

Only in that established IoC containers (and DI in general) is likely to be fairly well debugged, documented, etc.. Under the covers nearly all of them use a similar implementation.

zelmak

@MiffTheFox said:

If I felt like it I'd make a Windows port of Xscreensaver to piss him off. The webpage would start with a quote from the GPL.

I was feeling enterprising/enterprisey one day and ported a bunch of them to Java.

What? Hasn't everybody done that?

FrostCat

@MiffTheFox said:

Some people, when confronted with a problem, think “I know, I’ll quote Jamie Zawinski.” Now they have two problems.

Seriously, I dislike that guy with a passion. If I felt like it I'd make a Windows port of Xscreensaver to piss him off. The webpage would start with a quote from the GPL.

Ha.

JimLahey

@TheCPUWizard said:

Only in that established IoC containers (and DI in general) is likely to be fairly well debugged, documented, etc.. Under the covers nearly all of them use a similar implementation.

Established containers are commonly developed by entire teams of people, peer reviewed and a number of generations old, which all counts for nothing when you suffer from NIH and you think you know better. The argument against using IoC from some of our lot is based around another person picking up the code and having to learn the IoC framework which would "unnecessarily eat up time". They somehow think their homebrewed, undocumented implementation that has no unit tests whatsoever and has never been peer reviewed is easier and quicker to pick up, understand and if needs be modify to fit new requirements.

error

I'm sorry but are you suggesting the .NET Framework reflection API is not proven? Why bring in a whole IoC framework for something this simple?

Kaosadvokit

@joe.edwards said:

I'm sorry but are you suggesting the .NET Framework reflection API is not proven? Why bring in a whole IoC framework for something this simple?

Possible answers:

Because everybody knows that 3rd party libraries are more efficient than 1st party libraries.
Because I already know an IoC framework and I don't know reflection.
Because I want to blame "that crappy 3rd party library" for all those failures we haven't been able to figure out instead of admitting that I may have made a mistake and can't find it.
Because admitting I don't know reflection means admitting I didn't actually read that C# book I expensed to the company.
Because I feel like an idiot for tracing through thousands of lines when a simple error->search->breakpoint->debug would have accomplished the same in minutes.

Maybe there is some context that we are missing that makes it a genuine WTF, but I'm with joe.edwards on this; I see a simple solution to a simple problem. I see a reasonable level of error checking and reporting. There could be more, but the code pretty clearly spells out the requirements for a "valid IController"; the type must be in registeredTypes and have a constructor that takes a single IModel parameter. There are more robust or elegant solutions, but I wouldn't say it is a WTF.

this_code_sucks

@Kaosadvokit said:

@joe.edwards said:
I'm sorry but are you suggesting the .NET Framework reflection API is not proven? Why bring in a whole IoC framework for something this simple?

Possible answers:
Because everybody knows that 3rd party libraries are more efficient than 1st party libraries.
Because I already know an IoC framework and I don't know reflection.
Because I want to blame "that crappy 3rd party library" for all those failures we haven't been able to figure out instead of admitting that I may have made a mistake and can't find it.
Because admitting I don't know reflection means admitting I didn't actually read that C# book I expensed to the company.
Because I feel like an idiot for tracing through thousands of lines when a simple error->search->breakpoint->debug would have accomplished the same in minutes.
Maybe there is some context that we are missing that makes it a genuine WTF, but I'm with joe.edwards on this; I see a simple solution to a simple problem. I see a reasonable level of error checking and reporting. There could be more, but the code pretty clearly spells out the requirements for a "valid IController"; the type must be in registeredTypes and have a constructor that takes a single IModel parameter. There are more robust or elegant solutions, but I wouldn't say it is a WTF.

I don't think the wtf is using this over a IoC framework, I think the WTF is that no one opted for the simplest solution:

Factory class:

static IController CreateFoo(IModel model) { return new Foo(model); }

Please don't take this as me being anti-IoC framework, but I think most of the time they are overkill. They should be used when there is a real need for them.

JimLahey

@this_code_sucks said:

I don't think the wtf is using this over a IoC framework, I think the WTF is that no one opted for the simplest solution

That is the essence of the WTF, yes, supplemented by some totally misleading exception handling. The IoC reference was just to illustrate the irony that some of my colleagues will readily complain about certain language features and/or third party libraries being less than transparent, whilst writing even less transparent code themselves.