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?

2
  • 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
    Nov 10, 2019 at 23:21
  • Is there a reason why you are actually cloning the property instead of just passing it by reference? Nov 11, 2019 at 23:05

3 Answers 3

3

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

1
  • 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, 2019 at 22:48
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 and acknowledge you have read our privacy policy.

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