Tag Archives: refactoring

Coupling: The factory method

One of the challenges we face with coding is dealing with coupling. Coupling is an important aspect of programming, it tells us how much our code is tangled. When coupling is too high, we can’t easily re-use code. When the coupling is too low it does little. You can measure coupling, there are several metrics for it even (for instance “Coupling between Objects, CBO”).

In this blog post I’d like to talk about a subtle introduction of coupling: when you introduce a factory method.

Consider you have an interesting piece of code, and this piece of code has quite a lot of properties:

[sourcecode language=”java”]
class Person {
private String firstName;
private String lastName;
// .. more properties here

public void subcribeTo(Subscription subscription) {
// do something interesting here
}

}
[/sourcecode]

The problem is, because of the amount of properties and other dependencies, we’d like to simplify its creation by introducing a Factory method. In this case we are building a web application, so we take the Request as input to read the parameters:

[sourcecode language=”java”]
class Person {
private String firstName;
private String lastName;
// .. more properties here

public static Person create(HttpServletRequest request, .. more arguments here .. ) {
this.firstName = request.getParameter("firstName");
// .. read more properties
// .. set up dependencies, etc.
}

public void subcribeTo(Subscription subscription) {
// do something interesting here
}

}
[/sourcecode]

In the code that uses Person, it becomes easier to construct the Person and we’re happy with that. However, we have introduced coupling on several levels:

  • We construct the object with specific parameters in the create method. If we want to create from different parameters, we cannot use it. There is a coupling between the parameters and the properties.
  • The object is constructed using a Request object. We cannot now move the class to an application that does not use the web. A person has nothing to do with a request, it is just convenience that we put the factory method in the Person class. There is a coupling between the code of Person and the dependency delivering the Request object.

There are several ways to deal with this. But lets start with the last reason of coupling. It is easy to fix this coupling by creating a Factory class within your web application. From there you can generate the Person object out of a request. The Person class has no create method anymore, and thus is not tightly coupled to a Request class. The newly created Factory however is coupled to the Request, which is fine as it is meant to convert Requests into Person objects. Hence we could even name it that way:

[sourcecode language=”Java”]
class Person {
private String firstName;
private String lastName;
// .. more properties here

Person(String firstName, String lastName, …) {
this.firstName = firstName;
this.lastName = lastName;
// …
}

public void subcribeTo(Subscription subscription) {
// do something interesting here
}

}

class PersonFromRequestFactory {

// .. dependencies here

public Person create(HttpServletRequest request) {
Person person = new Person(request.getParameter("firstName"), )
// .. read more properties
// .. set up dependencies in Person, etc.
}

}
[/sourcecode]

Once we have this Factory, you can take it a step further:
If you have different kind of request parameters to create the same object you could create different methods in the new Factory:

[sourcecode language=”Java”]
class PersonFromRequestFactory {

// .. dependencies here

public Person createFromRegistrationForm(HttpServletRequest request) {
Person person = new Person(request.getParameter("firstName"), )
// .. read more properties
// .. set up dependencies in Person, etc.
}

public Person createFromSubscriptionForm(HttpServletRequest request) {
Person person = new Person(request.getParameter("givenName"), )
// .. read more properties
// .. set up dependencies in Person, etc.
}

}
[/sourcecode]

You could also create a Parameter object and go from there. For instance, if your web application uses Spring, you could wire your request parameters to an object (called “Form binding“) automagically and use that object as input in your Factory. This way it is type safe:

[sourcecode language=”Java”]
class PersonFromRequestFactory {

// .. dependencies here

public Person create(RegistrationForm form) {
Person person = new Person(form.getFirstName(), …)
// .. read more properties
// .. set up dependencies in Person, etc.
}

public Person createFromSubscriptionForm(SubscriptionForm form) {
Person person = new Person(form.getGivenName(), )
// .. read more properties
// .. set up dependencies in Person, etc.
}

}
[/sourcecode]

But how do you test all this?
Did you notice the Person has private fields, and no get/set methods? The only way to set the fields is using the Person constructor. How do you test the correct construction of this Person class from the request? Since we are not able to read the properties, we have to use other ways to test that code. I’ll cover that in the next blog post.

Refactoring

In one of my previous posts I blogged about refactoring I did for my hobby project.

Robert C Martin has a nice video showing refactoring, using IntelliJ. Watch it here.

You can also see why unit testing is so important, as with each change the unit tests are being ran in order to check if anything got broken.

How to fry your co-developers brain; and then make it better

Here is a little example of code I’ve been faced with (not written by me), that struck me. Although the syntax is correct (it is javascript), it took me a little while to actually understand what is going on.

Here is the code:
[sourcecode language=”javascript”]
someObject: function(data) {
return data.json ? data.json.stateObject ? data.json.stateObject : {} : {};
},
[/sourcecode]

And here is how I refactored it.
[sourcecode language=”javascript”]
someObject: function(data) {
if (data.json) {
if (data.json.stateObject) {
return data.json.stateObject;
}
}
return {};
}
[/sourcecode]

So I mentioned brianpower in a while ago. Michael Feathers though calls such a thing “forming a mental model“. Which is exactly what I meant. You also hear this in the scene of Usability; the less your mental model matches with the actual object you use (expectations), the less likely you’ll probably understand it, let alone *use it*.

So tell me, which one is easier to understand? And how much impact do you think this has?

Tiny refactorings? Compose Method!

I blogged about tiny refactorings not too long ago. I’ve even added an example showing such a refactoring in my game.

I’m reading Refactoring to Patterns, and figured that the refactorings I’ve mentioned have a name; it’s called the Compose Method. In the book it even has its own chapter (Chapter 7 “Simplification”), where it also refers to other great patterns to help you improve your maintainability of your code.

Creating (default) test instances of a class, without exposing default constructor.

Did you ever need to just have an instance of a class you cannot instantiate because the default constructor is not available? Do you want to create a test instance just to be used in unit tests? Don’t want to break up the design of your code just for testing? This post might help you:

Sample code:


public MyClass {
final int someField;

private MyClass() {
// may not use this
someField = -1;
}

public MyClass(int someFieldValue) {
someField = someFieldValue;
}

int getSomeField() {
return someField;
}
}

So lets say this class is used in a lot of places. And all we want is have a default test instance. We don’t care about the internals. Lets say we only use it to toss around using an interface which we mock in our tests. Like so:


public void someUnitTestUsingMyClassAndMockingAnInterface() {
MyClass instance = new MyClass(-1); // this works, but its not really descriptive
MyClass someotherMyClass = new MyClass(-1); // like, what is -1? we don't want to be bothered by this
EasyMock.expect(someinterface.doSomethingWithMyClass(instance)).andReturn(someotherMyClass);
...
}

There are two ways to approach this problem:

  • Make private constructor public
  • Use a Factory or a creation method

So, just making the default constructor public sounds easy and is used very often. And, because we want to let our fellow developers know it is just to be used in unit-tests we place a comment in them. This mostly results in something like this:


public MyClass {
final int someField;

public MyClass() {
// Use this only in tests!
someField = -1;
}

}

However, this has a few drawbacks:

  • You cannot see if the default constructor should only be used for tests, unless you look into the code itself and read that comment line
  • You broke with the initial design decision to have only objects instantiated with a given “someField”

So how do we prevent this?

We use a factory, or a creation method. Lets start with the first, the factory: If there is a factory, just use it to get a test instance. If the factory lacks a ‘createTestInstance()’ method, then add it. Like so:

public MyClassFactoryImpl implements MyClassFactory {

public MyClass createTestInstance() {
return new MyClass(-1);
}

}

We could discuss about wether you want the method to be factory method to be static here, and so forth, but the idea is clear.

However, when there is no factory present it is unlikely you want to create a factory just for your unit tests. In that case I suggest you use the creation method, also refered to as the factory method pattern. Simply add a public static method that returns a new instance. The final result would be:

public MyClass {
final int someField;

public static MyClass createTestInstance() {
return new MyClass();
}

private MyClass() {
// may not use this
someField = -1;
}

public MyClass(int someFieldValue) {
someField = someFieldValue;
}

int getSomeField() {
return someField;
}
}

The benefits are clear:

  • The default constructor remains private, so the design is left untouched
  • It is clear what the createTestInstance method does. No comments needed.

And in your unit test (using EasyMock as well) it looks like this:


public void someUnitTestUsingMyClassAndMockingAnInterface() {
MyClass instance = MyClass.createTestInstance();
MyCLass someotherMyClass = MyClass.createTestInstance();
EasyMock.expect(someinterface.doSomethingWithMyClass(instance)).andReturn(someotherMyClass);
...
}

Note: When you wind up with multiple create methods you should consider using the Factory pattern instead. Yes, creating a factory is the best thing once creation logic starts to dominate your class. (ie, think of the Single Responsibility Principle).
Note 2: If you’re a design purist, you might even debate that the public constructor of MyClass should be a creation method. It improves consistency as all instances are created by creation methods. It also has the benefit of describing what kind of instance you get (ie, what the field means). Such improvements are valid and should be made. I consider these improvements small refactorings. They should never be undervalued.