Fetching data from your database in a constructor

Question

I have this item (an item is an object linked to a xml view), to build it I need 2 things, get labels from codes, and get text from the strings.xml android file.

please ignore the long to int and int to long convertion, for some reason the database value where randomly set to long and int but they could all be int, I will refactor this later.

refManager is calling a DAOto fetch data from the database.

I have a for loop, building several OperationInstallItem

//constructor

public OperationInstallItem(final Resources resources, final RefManager refManager,
            final ModuleCategoryEnum category, final ModeleOperationEntity modeleOperationEntity,
            boolean isMandatory) {
        //basic init
        this.category = category
        this.isMandatory = isMandatory;
        this.refTypeOperation = modeleOperationEntity.getRefTypeOperation();
        this.refTypeInstallation = Long.valueOf(modeleOperationEntity.getRefTypeInstallation());

        //label init
        this.operationLabel = refManager.getOperationLabel(Ints.checkedCast(refTypeOperation));
        this.installationShortLabel = parametrageManager.getInstallationShortLabelt(refTypeInstallation);
        this.installationLongLabel = parametrageManager.getInstallationLongLabel(refTypeInstallation);
        this.title = resources.getString(R.string.operation_intallation_title)

        //icons specific to this class
        CodeTypeOperation codeTypeOperation =
                CodeTypeOperation.getEnumByCode(Ints.checkedCast(refTypeOperation));
        addModuleAdapterIconId = codeTypeOperation.getIcon();
        moduleAdapterIconId = codeTypeOperation.getIcon();
        moduleAdapterIconselectedId = codeTypeOperation.getIconSelected();
        moduleFragmentClass = TDBAddModuleFragment.class;
    }

Most of my collueages are uneasy with passing a manager as a constructor arguments but noone of them can explain me why. "it's just not done this way"

same thing with Resources resources that allow you to get String values from the strings.xml file doing this resources.getString(R.string.operation_intallation_title), according to them, it is bad to pass it in the constructor.

I don't get it, I need those labels/title, if I do this before the instanciation and pass it as parameters it's the exact same thing. no performance gain, nothing is different.

Or is there something I'm missing?

Answer 1

First of all, it violates the "tell don't ask" principle: if your OptionInstallItem is not inherently coupled to the database and its actions (and as you do not even record the reference to the refmanager, it obviously isn't) it should not go about scavenging in your whole application to get its job done. Always give every class exactly what it needs to operate, not the "master key" for self service.

Then, your way makes it unnecessarily hard to test. If you have a simple object along the lines of

public MyClass(String myLabel) {
    this.myLabel = myLabel;
}

Testing is as simple as can be:

MyClass testObject = new MyClass("test");
Assert....

On the other hand, if you have

public MyClass(DatabaseInterfaceThingy database) {
    this.myLabel = database.doSomeQueryForMyLabel("me");
}

... say hello to mockito to even instantiate that object.

And send your colleagues over to Codereview to learn a few things and be able to tell you "why" the next time ... ;-)

Answer 2

In addition to mtj's answer:

Why your code example is bad in my opinion:

• The main job of a constructor is not "to do stuff", that's the job of methods - that's why their names should have at least one verb in them. Sometimes, yes, it's more complicated and takes more effort, to do work not in the constructor, but imo that's quite rare and there's patterns to work around this problem.
• Of course, testing. Setting things up for the simplest test cases takes way too much effort. Just for that reason I try to keep constructors as simple as possible. I don't know anyone who once had a really hard time to write test cases for types with bloated constructors who do not try to keep their constructors tidy afterwards.
• This type is tightly coupled to its dependencies, it can't work without them and it is dependent on their correct behavior. And since this type which is, if I understand correctly, is part of the presentation layer and provides data to show to the ui, it also violates the SRP principle. It does provide data for the ui and it loads that data.
• Inheritance: If you'd like to use some inheritance to extend some behavior, you will have a hard time, especially if you want to change the behavior. Which is, in that case, close to impossible.
• For some reason, there are creational patterns, and I guess this is the main point, e.g. the Builder pattern: "The Builder is a design pattern designed to provide a flexible solution to various object creation problems in object-oriented programming. The intent of the Builder design pattern is to separate the construction of a complex object from its representation" [wikipedia]. So basically: If you have a hard time to construct your object, e.g. using a lot of parameters, do a lot of calculations, just decouple the construction.