0
\$\begingroup\$

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?

\$\endgroup\$
1
\$\begingroup\$

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 ... ;-)

\$\endgroup\$
  • \$\begingroup\$ allright it's harder to test but is it better to write code in order to make easier unit testing?, I really don't understand how it breaks the tell don't ask principe? where am I extracting data from an object and then making a decision? \$\endgroup\$ – sliders_alpha Aug 3 '18 at 23:33
  • 1
    \$\begingroup\$ @sliders_alpha The object actively asks the rest of the application for data it needs. This makes unnecessary coupling between the object and the other services. And passing in just what you need is not going out of your way for testability (which is in itself a good reason to do it), it is also isolating the object from change. If sometimes you database interface changes, you'll have to rewrite all the objects you created that way. Introducing unnecessary coupling always makes change hard, and code changes over time. Thus, isolation is a good general principle. \$\endgroup\$ – mtj Aug 4 '18 at 6:09
  • \$\begingroup\$ for the tell don't ask I still don't understand, I am not asking about internal data (property) of the oarametrageManager and taking a decision, I'm asking for a database value, if I do that outside of the constructor I'm still asking for that data. and for the coupling with the DB, if I do the call before the construction then when the DB interface change I will have to do modification everywhere I'm makine a "new" whereas here I will just have to do it in the constructor, to me it seems like coupling is less important. \$\endgroup\$ – sliders_alpha Aug 4 '18 at 7:28
  • \$\begingroup\$ @sliders_alpha Sorry, if you think that a widely agreed-upon principle in the software development community is utter bullcrap, I cannot see what I can do to convince you. Maybe this simply comes from experience, so try what you like, get your nose bloodied in a few projects, and then revisit this question. \$\endgroup\$ – mtj Aug 4 '18 at 7:33
  • \$\begingroup\$ No I agree with the principle, but not with its interpretation here. the "tell don't ask" principle says that you do not want to use a getter of an internal property of an object to then make a decision. I am not doing this here. \$\endgroup\$ – sliders_alpha Aug 4 '18 at 7:49
0
\$\begingroup\$

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.
\$\endgroup\$

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.