3

In Robert Martin's Clean Code, I encountered the following method on p. 195:

private void parseSchemaElement(String element) throws ArgsException {
    char elementId = element.charAt(0);
    String elementTail = element.substring(1);
    validateSchemaElementId(elementId);
    if (elementTail.length() == 0) marshalers.put(elementId, new BooleanArgumentMarshaler());
    else if (elementTail.equals("*")) marshalers.put(elementId, new StringArgumentMarshaler());
    else if (elementTail.equals("#")) marshalers.put(elementId, new IntegerArgumentMarshaler());
    else if (elementTail.equals("##")) marshalers.put(elementId, new DoubleArgumentMarshaler());
    else if (elementTail.equals("[*]")) marshalers.put(elementId, new StringArrayArgumentMarshaler());
    else throw new ArgsException(INVALID_ARGUMENT_FORMAT, elementId, elementTail);
}

Just a few pages earlier (p. 157), the book advocates use of dependency injection (DI) in order to structure our program better and facilitate testing in isolation.

Would the above example be the case where dependency injection should be used in order to test the given class in isolation, i.e. without also testing all the *ArgumentMarshaller classes?

We could have a factory object, which would create an appropriate *ArgumentMarshaller where needed - this would be consistent with the DI recommendations and also facilitate testing with mocks/doubles.

..or perhaps did I misunderstand something and the dependency injection applied here is not a good idea?

  • 1
    Can't you already test this class in isolation? Create an object, call parseSchemaElement and then check that the correct object was put in its marshalers array? It seems to me that the method you are asking about is a factory function... – Daniel T. Mar 27 '16 at 17:07
  • 1
    As a private method, agrguably you dont need to unit test. Wow, this function does far more than one thing. – user949300 Dec 4 '17 at 1:07
  • @user949300: It does one thing: it parses a schema element. SRP isn't about methods anyway. – Robert Harvey Dec 4 '17 at 18:04
  • @Robert Harvey Agree on SRP, but I didn't mention that. Nor am I a fanatice Robert Martineer, but this is his code This methods doesn't "do just one thing and do it well". It validates something, it creates the proper XXXMarshaller, and it adds said Marshaller somewhere. The create part should be separated, which would make that part testable since it is no longer a void method. – user949300 Dec 4 '17 at 21:39
7

Simply put, it depends.

Even though I am all for dependency injection in classes containing business logic, for this specific case and without knowing the context, even though the Marshalers are instantiated directly, it seems the only work that is done with them is adding them do a dictionary based on some rule, but not actually performing any operations on them. And for that I would be OK with instantiating the classes directly in place.

If you, however, wanted to decouple the creation of Marshalers in question, the factory approach is the correct one.

Dependency injection is generally recommended for classes which contain methods performing operations with side effects, such as writing to a database. In that case you want to know what the dependencies are so you are not surprised when a method work suddenly wipes your entire database even though, based on public API of the class, it shouldn't have access to it.

The only real improvement I'd have for this code would be to rename the method to parseSchemaElementAndAddItToMarshalerCollection.

  • 2
    That's a mighty long name. A better approach would be myMarshalerCollection.ParseAndAdd(string); – Robert Harvey Mar 27 '16 at 17:00
  • @RobertHarvey That's a very nice suggestion. – Andy Mar 27 '16 at 17:53
  • @Robert Harvey Very good name, because it hints at a potential flaw/"smell": why should a collection know how to parse a string? – user949300 Dec 4 '17 at 22:50
  • @user949300: Presumably its a specialized collection, and not an ordinary generic collection like a linked list or array. – Robert Harvey Dec 4 '17 at 23:07
4

Yes, you could use dependency injection (DI) here, but let’s consider how it would be achieved.

The method you quote is essentially a factory class creating the correct type of marshaller based on a string input.

Therefore injecting a factory class isn't going to help us much. It separates the logic out into its own class, which could improve the code if it's a large class, but really it just moves the offending if block and hard links 'down a level'.

In order to abstract out the string x = type y logic, we would have to inject a dictionary of IMarshallerFactory each with an associated string key.

This is a neat approach and allows you to test the consuming class without testing the *marshaller classes as you state.

However you still need to instantiate the dictionary in your IoC setup somewhere, plus add a host of factory classes and interfaces.

Unless your code requires this level of genericism and the ability to have varying marshaller mappings at run time it could be considered overkill.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.