2

I am writing a program that describes different properties on a single Management Company's plot of land. For this program there are 3 overloaded addProperty method's. My question is I can reuse the more general addProperty(Property p) method within the other 2 addProperty methods.

public int addProperty(Property p) {
    // From Assignment: Return -1(the property is null),
    // -2(array is full),
    // -3(plot for property !encompassed by MGMCP plot),
    // -4(if plot overlaps any other property plot)

    if(p == null)//what if p's Plot is 0?
        return -1;
    if(properties.length == MAX_PROPERTY)
        return -2;
    if(!plot.encompasses(p.getPlot()))
        return -3;
    if(overlapsExsistingProperties(p.getPlot()))
        return -4;

    properties[currentPropertyIndex] = new Property(p);

    return currentPropertyIndex++;
}// addProperty

public int addProperty(String name, String city, double rent, String owner) {
    return addProperty(new Property(name,city,rent,owner));
}// addProperty

public int addProperty(String name, String city, double rent, String owner, int x, int y, int width, int depth) {
    return addProperty(new Property(name,city,rent,owner,x,y,width,depth));
}// addProperty overload

Is the above code seen as good practice? I'm mainly considering how calling the 1st addProperty method within the other's requires 2 object initializations. While if I rewrote the method body for each addProperty it would only require 1 object initialization.

This is a pretty basic example, but in larger applications would this make a difference to performance or slow down garbage collection?

  • 1
    Unrelated but important: int is a totally inappropriate return type for this context. All the negative return values should be replaced by error objects, exception throws, or enum values. – Alexander - Reinstate Monica Nov 10 '19 at 23:21
  • Is there a reason why you are actually cloning the property instead of just passing it by reference? – Tulains Córdova Nov 11 '19 at 23:05
2

It is common practice to have overloads with a different number of parameters (the ones with fewer parameters calling the ones with more parameters, using common defaults). Or different kind of parameters (like a stream or a file path) Your way of doing it is a little odd though. The 2nd and 3rd method are of little use and may just be confusing. The client code can just as easily create that object itself and pass it to the 1st method.

I do not see the extra instantiations you mention. But creating a simple object that is just a data container would not be terribly expensive or worrying (assuming you are not in a tight loop).

  • 2
    While I agree it is weird in the current situation, if Property were a private/internal class that is not meant to be exposed, those methods make sense (but then the first method doesn't, of course). – Flater Nov 10 '19 at 22:48
0

In short

This common practice has little impact on performance. It creates however an unnecessarily coupling between two classes. Limit the use of this approach to the cases where extra-convenience really outweighs coupling drawbacks.

More details

The design constraints are the real problem here:

  • The interface of your addProperty()-overloads is strongly coupled with the interface of Property's constructor.
  • This infringes the Single Responsibility Principle, that says that a class should have only one reason to change. Here the PlotLand class has 2 reasons to change: its own core requirements on one side, the Property's interface on the
  • In consequence, if one day you'd decide to make another attribute mandatory for the Property construction at construction, you'd have to change your interface, and this might propagate changes to everywhere your interface is used: you'd have to change every piece of code where your overloads are used.
  • This also goes against the principle of the least knowledge, which aims at a proper separation of concerns.

The performance impact of this practice can in general be neglected:

  • in all cases you would have to create a property based on all the parameters needed for its construction.
  • In the overload, you'd pass these parameters once more, and have an extra call for the "reuse". But since build-in types are compact and class instances are passed by reference in java, the extra effort is certainly only a question of nanoseconds.
  • Would it be in a core game loop with the risk of slowing down the FPS rate, I'd prudently say to make a benchmark. But in a business application, this extra effort is nothing in comparison to the time you'll need to store the data in the database.

But principles are there to guide us, not to command us. So ultimately it's up to you to decide if the extra-convenience outweighs the extra-constraints.

  • His class is already tightly coupled to Property via the first method (look at the code inside). The overloads don't change that much. – user949300 Nov 12 '19 at 22:46
  • @user949300 There is a huge difference between coupling and coupling. If a class implementation depends on the interface of another ,it’s a loose coupling:I can change the implementation of the other class without any impact. Changing interface of the other class, I may need to change the implementation of my class, but that’s all. But here it’s more serious: one interface depends on another interface. The coupling is much stronger: any change in one class interface will propagate to the interface of the other class, and all their using classes. – Christophe Nov 12 '19 at 23:17
0

My question is I can reuse the more general addProperty(Property p) method within the other 2 addProperty methods.

In general, yes. There is nothing inherently wrong with having one method effectively wrap another (regardless of whether it's an overload or not). If a significant portion of the overloaded method bodies would otherwise be repeating themselves anyway, doing this actually follows DRY principles to a tee.

However, in this case, the second and third method shouldn't exist because they are nothing more than a wrapper around a Property constructor. This violates DRY: if the Property constructor changes, so must this method, for no apparent reason other than "because I created this method".

Whoever would call any of these methods (i.e. the consumer) would have access to the Property constructors, which means they are already able to figure out which parameters are needed. Your second and third method are just repeating the same information. It adds nothing of value, but it becomes a WET spot in case the Property constructor changes.

I'm mainly considering how calling the 1st addProperty method within the other's requires 2 object initializations.

Even if your consumer calls the first method, they are still doing two initializations:

  1. The object that will the passed into the addProperty(Property p) method parameter
  2. The new Property(p) that you instantiate inside this method

Your concern may be valid but it's the same for all three methods, so it does not matter for your particular question here.

I'm not quite sure why you do

properties[currentPropertyIndex] = new Property(p);

instead of

properties[currentPropertyIndex] = p;

I suspect you're cloning the object so that the consumer can no longer make changes to it after it was added. Whether that's necessary of not isn't clear to me. If it is necessary, then that second instantiation is the cost of implementing the cloning.

While if I rewrote the method body for each addProperty it would only require 1 object initialization.

You could avoid a second instantiation if you only use the second or third method, but good practice tends to suggest wrapping a set of data values (that belong together) in an object anyway. Complying with good practice inherently leads you back to your double initialization, and this double initialization isn't a red flag for performance (assuming there's no heavy lifting in the constructor's body).

Your performance argument of avoiding one object instantiation pretty much bleaks in comparison with the readability and general not-nice-to-develop consequences of using methods with long signatures.

in larger applications would this make a difference to performance or slow down garbage collection?

Not to a degree that you should generally steer away from it. Unless you are squeezing for performance, these kinds of considerations add an unnecessary complexity the codebase for negligent gains (if any).

Don't optimize prematurely. Optimize when there is a performance issue. It's better to solve the issues that occur in reality rather than any issue you can think of, because the latter is going to lead you to wasting effort on fixing things that maybe never ended up becoming a problem anyway.

0

It isn't clear to me why you are cloning the Property object instead of just assigning it.

Anyways, instantiations of concrete classes should not be done inside "add" methods.

Creation of objects should be done in a factory/creator/builder class or method which should perform the checks you're doing in addProperty(Property p) and throw appropriate exceptions when mandatory stuff is missing.

The add methods should not be tightly coupled to concrete implementations. Passed parameters should be abstract/interfaces and only the factory/builder would be tightly coupled to the concrete class via the new reserved word.

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.