5
\$\begingroup\$

In a course that I'm taking on learning object oriented programming in java, I have completed an assignment for modeling a home entertainment system and the ability for a user to be able to use a single remote control to control multiple digital devices such as a TV or sound system etc.

I don't feel too confident about this design so I wanted to run it past the experts. Below is my code. Any feedback you can provide on improving this design and why would be super helpful!

package user;

import remotes.RemoteControl;
import devices.Projector;
import devices.TV;

public class Person {

    public static void main (String args[]){

        //Devices in the home
        Projector niceProjector = new Projector();
        TV niceLargeScreenTV = new TV();

        // The user's control
        RemoteControl control;

        control = RemoteControl.connectWithdevice(niceProjector);
            control.clickOnButon();

        control = RemoteControl.connectWithdevice(niceLargeScreenTV);
            control.clickOffButon();

    }

}

RemoteControl class:

package remotes;

import devices.Device;

public class RemoteControl {

    private Device theDevice= null;

    private RemoteControl(){

    }

    public static RemoteControl connectWithdevice(Device aDevice){
        RemoteControl remote = new RemoteControl();
        remote.theDevice = aDevice;
        return remote;
    }

    public void clickOffButon(){
        theDevice.turnOnDevice();
    }

    public void clickOnButon(){
        theDevice.turnOnDevice();
    }

}

One of the device classes:

package devices;


public class TV implements Device{

    @Override
    public void turnOnDevice() {
        SwitchToFavoriteChannel();
        System.out.println("TV has been turned on");
    }

    @Override
    public void turnOffDevice() {
        turnOnTVAlarm();
        System.out.println("TV has been turned off");
    }


    // below are additional steps that may be required for you to operate your device
    private void SwitchToFavoriteChannel(){
        System.out.println("Switched to your favorite Channel");
    }

    private void turnOnTVAlarm(){
        System.out.println("TV Alarm set for you to wake up in the morning");
    }


}
\$\endgroup\$
1
  • \$\begingroup\$ I would recommend you to search and read "Head First Design Patterns", if you don't know about it. It's a book about design patterns. I recommend you because examples are written in java and also it has an example of a remote controller, I'm pretty sure you'll find some help there also. \$\endgroup\$
    – Marius.C
    Feb 6, 2015 at 16:04

3 Answers 3

1
\$\begingroup\$
@Override
public void turnOffDevice() {
    turnOnTVAlarm();
    System.out.println("TV has been turned off");
}

Welcome to the hell that never ends.

Just kidding. But please don't make TVs that automatically turn on an alarm to wake you up in the morning. I have alarm clocks for that, which are beating resistant.

Method naming

    control = RemoteControl.connectWithdevice(niceProjector);
        control.clickOnButon();

    control = RemoteControl.connectWithdevice(niceLargeScreenTV);
        control.clickOffButon();

Did you mean clickOnButton and clickOffButton?

Actually, your method naming could use some work. Let's go over some of them. But first, Java convention. It's Java convention that your function names are camelCase.

So, let's look at your method names:

connectWithdevice

This should be connectWithDevice, as each word after the first starts with a capital.

SwitchToFavoriteChannel

This should be switchToFavoriteChannel, as the first word does not have a capital. (That's how we easily distinguish classes and methods from each other).

turnOnTVAlarm

This one is tricky. For now, I'd let it stay... TV is only two characters. But when you have an acronym, treat it as one word. That is, turnOnTvAlarm. This because you end up with class names like HTTPConnection later, and getHttpConnection reads a lot better than getHTTPConnection. Especially if you're not familiar with the acronym.

Instantiating RemoteControl

private Device theDevice= null;

private RemoteControl(){

}

public static RemoteControl connectWithdevice(Device aDevice){
    RemoteControl remote = new RemoteControl();
    remote.theDevice = aDevice;
    return remote;
}

Have you considered passing the device in as constructor argument instead?

Then you can do something like this:

private final Device device;

public RemoteControl(Device aDevice){
    device = aDevice;
}

And people will always be forced to set a device.

With Objects.requireNotNull you can even make them not pass in a null value (you can also write the check yourself, but it's easier with Objects.requireNotNull.

private final Device device;

public RemoteControl(Device aDevice){
    device = Objects.requireNotNull(aDevice, "A remote control requires a device to work with.");
}

Design

Good designs feel natural.

Let's see how natural your design feels.

I have a Projector. And I have a TV. And I may have a RemoteControl... wait, I need to ask RemoteControl to connect to a device so it can send messages to the device? Why can't I just grab the remote control for the TV?

There's two options that I'm aware of in real life:

  • You take a programmable remote and set it to control a specific device. This would be like passing a different Device into an existing RemoteControl.
  • You pick up the remote that belongs to the TV. We'll assume we're clean, orderly people who store the remote with the device (far more likely for projectors than for TV's...). This would be like asking the Device to give a RemoteControl.

Both are more natural than wishing for a RemoteControl for a device.

\$\endgroup\$
4
  • \$\begingroup\$ Nice answer. Instead off writing my own answer, I'll comment what I would add to this: you could add that the op could go ahead a bit further in his imagination and deny the turnOffDevice if the device wasn't on and vice versa for turnOnDevice, which could potentially lead to develop a generic abstract class for all devices (in place perhaps of the interface) \$\endgroup\$ Feb 6, 2015 at 15:51
  • \$\begingroup\$ @BrunoCosta The problem of a RemoteControl is that it doesn't know the state of the Device. It just sends commands. \$\endgroup\$
    – Pimgd
    Feb 6, 2015 at 15:53
  • \$\begingroup\$ That's why I mentioned the abstract class. \$\endgroup\$ Feb 6, 2015 at 15:55
  • \$\begingroup\$ @BrunoCosta I'm not quite sure what you mean... maybe you could elaborate in an answer of your own? \$\endgroup\$
    – Pimgd
    Feb 6, 2015 at 15:55
1
\$\begingroup\$

@Pimgd already made the review of your code and I don't think he missed anything at all. So I'll be a bit critic about the enthusiasm (imagination), you could have about your devices.

You could make Device an abstract class instead and make it store the state of the device (if it's powered or not):

public abstract class Device{
    private boolean powered = false;

    public void turnOnDevice() {
        if(powered){
            return;
        }
        powered = true;
        turnOndeviceImpl();
    }

    abstract void turnOndeviceImpl();

    public void turnOffDevice() {
        if(!powered){
            return;
        }
        powered = false;
        turnOffdeviceImpl();
    }

    abstract  void turnOffdeviceImpl();
}

Then your Tv (and all your other devices would derive from this:

public class Tv extends Device{

    @Override
    void turnOnDeviceImpl() {
        SwitchToFavoriteChannel();
        System.out.println("TV has been turned on");
    }

    @Override
    void turnOffDeviceImpl() {
        turnOnTVAlarm();
        System.out.println("TV has been turned off");
    }

    private void SwitchToFavoriteChannel(){
        System.out.println("Switched to your favorite Channel");
    }

    private void turnOnTVAlarm(){
        System.out.println("TV Alarm set for you to wake up in the morning");
    }
}
\$\endgroup\$
1
\$\begingroup\$

Device interface is a good idea because we can have different implementations of devices each with common behaviour.

public interface Device {
    void turnOn();
    void turnOff();
}

No need of *Device suffix in method names because the name of the interface give you the context of the operations.

About the RemoteControl

public interface RemoteControl {
    void turnOn();
    void turnOff();
}

In my opinion it should be interface with common methods for different kinds (implementations) of remote controls.

One possible implementation could be

 public class ProjectorRemoteControl implements RemoteControl {
    private final Device projector;

    public ProjectorRemoteControl(Device projector) {
        this.projector = projector;
    }

    @Override
    public void turnOn() {
       projector.turnOn();
    }

    @Override
    public void turnOff() {
        projector.turnOff();
    }
}

Implementation of Device

public class Projector implements Device {
    private final Map<String,Operation> operations;

    public Projector(Map<String, Operation> operations) {
        this.operations = operations;
    }

    @Override
    public void turnOn() {
        execute("turnOn");
    }

    @Override
    public void turnOff() {
        execute("turnOff");
    }

    private void execute(String operationName) {
        Operation operation = operations.get(operationName);
        operation.execute();
    }
}

It use another abstraction Operation

public interface Operation {
    void execute();
}

We can have different operations

public class ProjectorTurnOnOperation implements Operation {
    @Override
    public void execute() {
        System.out.println("projector is turned on");
    }
}


public class ProjectorTurnOffOperation implements Operation {
    @Override
    public void execute() {
        System.out.println("projector is turned off");
    }
}

Usage

public class Person {
    public static void main(String[] args) {
        Device projector = new Projector(
                new HashMap<String, Operation>(){{
                    put("turnOn",new ProjectorTurnOnOperation());
                    put("turnOff",new ProjectorTurnOffOperation());
                }}
        );

        RemoteControl projectorRemoteControl = new ProjectorRemoteControl(projector);
        projectorRemoteControl.turnOn();
        projectorRemoteControl.turnOff();

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