5

We have a method called attachDevice(Device device) which has only one argument. We had a situation to overload this method with one more parameter as like attachDevice(Device device, String deviceName).

with single argument

public void attachDevice(Device device)
{
    ..
    ..        
}

with double arguments

public void attachDevice(Device device, String deviceName)
{
    ..
    device.setName(deviceName);
    genericDeviceMap.put(deviceName, device);
    ..
}

Actually, my Team Lead asked me to make these two methods into a single generic call. The overloaded method only have two additional lines than the single method (which are shown as above). I can pass empty string instead of passing the deviceName, because the invocation of overloaded method will be very lesser than the invocation single argument call. But how bad it is that if passing null value since i don't set the name if the argument will be null. Which would be the best practice for this scenario?

Any suggestions are highly appreciated.

Note: The actual problem I stated here is, that

i) I wanted to merge overloaded methods into one method which should be generic to avoid code duplication

ii) Need a recommended value to pass to the method (either empty string or null value) to stick with best coding practices.

From the given answers, I got the best solution for my problem. The linked question which has been referred this question duplicate also provides info about handling null value, but not covered the first point I mentioned in the note. And I didn't write bad code by passing null as parameter, since I am aware that NPE will thrown sometimes.

Thanks.

  • Possible duplicate of Best way to handle nulls in Java? – gnat Jul 11 '16 at 13:45
  • 2
    mutating the device argument seems like a poor design choice in the first place, why would attching a device change the name of the dvice im attaching? – jk. Jul 11 '16 at 14:33
  • @jk, We have a such scenario that, device name can be set by default programmatically when it boots up or it can be set by getting the input from user. – S.K. Venkat Jul 11 '16 at 14:35
  • If the user can change device name you will need to be very careful about using device name as a key in a map – jk. Jul 11 '16 at 14:56
  • Yeah. You are absolutely correct and that has been taken care of. – S.K. Venkat Jul 11 '16 at 14:57
9

I would discourage you to ever use null since it can lead to a further NPE, which are hard to debug (and cost a lot if they occur in production code).

Solution 1 (overload method)

If no deviceName is provided, you can provide a default one instead. The biggest disadvantage from this approach is the danger in genericDeviceMap.put(deviceName, device) because it can silently override the entry whose key is the default name (therefore, losing track of the previous Device).

public void attachDevice(Device device)
{
    attachDevice(device, "DefaultName");       
}

public void attachDevice(Device device, String deviceName)
{
    ..
    device.setName(deviceName);
    genericDeviceMap.put(deviceName, device);
    ..
}

Solution 2 (extract method)

Maybe that with your current architecture it doesn't make sense to add an entry to genericDeviceMap when attachDevice is called without a name. If so, a good approach is to only extract the common behaviour between the two attachDevice into private methods. I personnally don't like this approach for 2 reasons:

  • The behaviour between the two attachDevice is not the same, one has a side-effect (device.setName(deviceName)) and the other not
  • The side-effect in itself who often lead to subtle bugs because you alter an object who's coming from an outside scope

Code:

public void attachDevice(Device device)
{
    preAttachDevice();
    postAttachDevice();      
}

public void attachDevice(Device device, String deviceName)
{
    preAttachDevice();
    device.setName(deviceName);
    genericDeviceMap.put(deviceName, device);
    postAttachDevice();
}

private void preAttachDevice()
{
    ...
}

private void postAttachDevice()
{
    ...
}

Solution 3 (remove method)

My favorite, but the hardest. Ask yourself if you really need these two methods ? Does it make really sense to be able to call attachDevice either with a name or not ? Shouldn't you be able to say that attachDevice must be called with a name ?

In this case the code is simplified to only one method

public void attachDevice(Device device, String deviceName)
{
    ..
    device.setName(deviceName);
    genericDeviceMap.put(deviceName, device);
    ..
}

Or on the other hand, do you really need to maintain a Map of devices and devices names and set the device's name ? If not, you can get rid of the second method and only keep the first one.

public void attachDevice(Device device)
{
    ...
    ...     
}
  • @Spotted, I know using null would be awkward thing, but the app has been designed such way to make things bit complicated. I'll pick up the 1st approach as solution for the current scenario. Thank you very much. – S.K. Venkat Jul 11 '16 at 13:32
4

I'd argue that your team leader is asking you to create bad code. Having to pass some arbitrary value in as a name parameter when you do not want to specify a name is messy and confusing for any devs looking to use the method.

Instead, keep your two methods (though I'd rename your latter one to something like attachNamedDevice to make it clear it does something different to attachDevice, then move the common code into private methods:

public void attachDevice(Device device)
{
    preDeviceNameSetup(device);
    postDeviceNameSetup(device);        
}

public void attachNamedDevice(Device device, String deviceName)
{
    preDeviceNameSetup(device);
    device.setName(deviceName);
    genericDeviceMap.put(deviceName, device);
    postDeviceNameSetup(device);        
}

private void preDeviceNameSetup(Device device)
{
    ...
}

private void postDeviceNameSetup(Device device)
{
    ...
}

That way, you keep the API clean, but avoid code duplication in the implementation.

  • 1
    I do prefer that solution, since with an overloaded methods you don't necessarily know that different arguments produce different behaviour. If I have a Device that has a name and call attachDevice(Device) then that name can possibly get nuked. Or not - who knows. Hard to tell. If course that method could just call attachDevice(device, device.hasName() ? device.getName() : "Default name") (ore equivalent) but then it seems like the two methods are still redundant. attachNamedDevice more clearly marks it as separate. – VLAZ Jul 11 '16 at 16:25
  • A problem with requiring that public API calls use two separate methods is that it compels code duplication in derived or wrapper classes. If one exposes a static final string YourClassName.nameNotSpecified and the a courtesy overload which is final and is documented as chaining to the normal one while passing that string, then derived classes or wrapper classes will have a single point of attachment. – supercat Jul 11 '16 at 16:30
  • @vld, if user want to update the name, that can be done using the overloaded methods. We have rest api call to invoke these methods based on the input from user. – S.K. Venkat Jul 11 '16 at 17:09
1

I would do the following:

  1. Remove the second function with the String as second parameter.
  2. Call device.setName(deviceName) before calling attachDevice(device). It is not the responsibility of the attachDevice method to set the name of the device.
  3. Change the attachDevice method to add the device to the genericDeviceMap if it has a name, as shown in the code snippet below.

If you also need to add the device to the genericDeviceMap when it does not have a name, you are probably better off setting such a default in the constructor of the Device class itself or something like that.

public void attachDevice(Device device)
{
    // ...
    if (device.getName())
    {
        genericDeviceMap.put(deviceName, device);
    }
    // ...
}

This keeps your concerns separated and helps you stay away from functions with side effects.

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.