2
\$\begingroup\$

I have an Engineer which doesn't look very good. How can I make him better organized? What about good practices?

Interface:

public interface Engineer {
    String HOMELESS = "homeless";
    String ENGINEER = "engineer";
    String SOFTWARE_ENGINEER = "softwareEngineer";
    String TEST_ENGINEER = "testEngineer";
    String TEST_AUTOMATION_ENIGNEER = "testAutomationEngineer";

    static final int WORK = 1;
    static final int LOOKING_FOR_NEW_JOB = 2;
    static final int BUSY = 3;
    static final int FREE = 4;
    static final int SLEEP = 5;

    boolean isHappy();
    String getEmployer();
    String getType();

    String makeOffer(int salary, String employer);

    void fire();
    void setView(EngineerImpl.View view);
    /**
     * Sets employer
     * @param name
     * @return
     */
    void setEmployer(String name);
    String getName();
    void startWork(long time);

    /**
     * Stops {@link Engineer}
     * @return
     */
    int stopWork(long time);

    /**
     * Sets {@link Engineer} into hibernate mode for {@code seconds}
     * @param seconds
     */
    void sleep(int seconds);
    boolean eatPizza(int pieces);    
}

Implementation:

public class EngineerImpl implements Engineer {

    private View view;

    static class EngineersFactory {
        public static Engineer getEngineer(String name, String type) {
            return new EngineerImpl(name, type);
        }

        public static Engineer getEngineer(String name) {
            return getEngineer(name, ENGINEER);
        }

        public static Engineer getSoftwareEngineer(String name) {
            return getEngineer(name, SOFTWARE_ENGINEER);
        }

        public static Engineer getTestEngineer(String name) {
            return getEngineer(name, TEST_ENGINEER);
        }

        public static Engineer getTestAutomationEngineer(String name) {
            return getEngineer(name, TEST_AUTOMATION_ENIGNEER);
        }
    }

    interface View {
        void showSmile();
    }

    private boolean isHappy = false;
    private String employer;
    private int salary;
    private String name;
    private int status;
    private long workStartedAt;
    private int previousStatus;
    private final String type;

    public EngineerImpl(String name, String type) {
        isHappy = true;
        this.name = name;
        status = WORK;
        this.type = type;
    }

    @Override
    public String getType() {
        return type;
    }

    @Override
    public void setView(View view) {
        this.view = view;
    }

    @Override
    public boolean isHappy() {
        return isHappy;
    }

    @Override
    public String getEmployer() {
        return employer;
    }

    @Override
    public String makeOffer(int salary, String employer) {
        if (shouldIAcceptNewOffer(salary, employer)) {
            acceptOffer(salary, employer);
            return employer;
        }
        return this.employer;
    }

    @Override
    public void fire() {
        this.salary = 0;
        this.employer = HOMELESS;
        isHappy = false;
        status = LOOKING_FOR_NEW_JOB;
    }

    @Override
    public void setEmployer(String name) {
        this.employer = name;
    }

    @Override
    public String getName() {
        return name;
    }

    @Override
    public void startWork(long time) {
        status = BUSY;
        workStartedAt = time;
    }

    @Override
    public int stopWork(long time) {
        status = FREE;
        view.showSmile();
        return (int) (time - workStartedAt);
    }

    @Override
    public void sleep(int seconds) {
        previousStatus = status;
        status = SLEEP;
        try {
            Thread.sleep(seconds);
        } catch (InterruptedException e) {
            // ...
        }
        status = previousStatus;
    }

    @Override
    public boolean eatPizza(int pieces) {
        isHappy = ((pieces % 2) == 0 && status == FREE);
        if (isHappy) {
            view.showSmile();
        } else {
            sleep(10);
        }
        return isHappy;
    }

    @Override
    public boolean equals(Object engineer) {
        if (this == engineer) {
            return true;
        }
        Engineer e = (Engineer) engineer;
        return this.isHappy == e.isHappy() && hashCode() == e.hashCode();
    }

    @Override
    public int hashCode() {
        return getName().hashCode()
                + getEmployer().hashCode()
                + getType().hashCode();
    }

    private boolean shouldIAcceptNewOffer(int salary, String employer) {
        return salary > this.salary && employer.equals(this.employer);
    }

    private void acceptOffer(int salary, String employer) {
        this.salary = salary;
        this.employer = employer;
        isHappy = true;
        view.showSmile();
    }
}

Some explicit questions:

  1. Is it OK to put constants to interface and use them in the implementation?
  2. Should I make constants static final if they are defined in an interface? Which constants should I prefer, int or String?
  3. Can I make some additional actions in setters?
  4. When should I write a JavaDoc?
\$\endgroup\$
12
\$\begingroup\$

This code is, depending on your point of view, either over-engineered or just wrong.

The whole point of separating the interface from the implementation would be to make the interface independent of the implementation. Yet, your interface mentions EngineerImpl.View, which defeats the whole purpose of that separation.

The fix, if you consider the code to be over-engineered, is to forget about splitting the interface and implementation, and just make an Engineer class. If you don't intend to write any other implementations of the interface, that would be the way to go. Alternatively, you could fix it by moving EngineerImpl.View to Engineer.View.


In addition, the EngineersFactory is in a weird place. A factory could be useful if you want the caller not to instantiate objects directly — for example, if the caller doesn't know what class to instantiate. Putting the factory as an inner class of EngineerImpl seems to suggest that the factory is meant to only produce EngineerImpl objects, though.

The naming of the methods in the factory is also off. "Get" implies fetching an existing instance (like a singleton), not creating a new one. For a factory, names like createEngineer(), makeEngineer(), newEngineer(), or just engineer(…) would be more appropriate.

Again, I'm not sure what value the factory adds. The EngineerImpl(name, type) constructor is already public, as are the string constants for the types. If you abandon the interface-implementation separation, then you would be better off eliminating the factory as well. Alternatively, keep the factory, but make the EngineerImpl constructor private so that there is one right way to instantiate objects.

\$\endgroup\$
  • \$\begingroup\$ Nice caught on factory issue and View interface placement. \$\endgroup\$ – Iaroslav Karandashev Oct 15 '15 at 17:28
  • \$\begingroup\$ This code is just collection of common places I see daily. \$\endgroup\$ – Iaroslav Karandashev Oct 15 '15 at 17:29
8
\$\begingroup\$
static final int WORK = 1;
static final int LOOKING_FOR_NEW_JOB = 2;
static final int BUSY = 3;
static final int FREE = 4;
static final int SLEEP = 5;

A few things specific to this

First, using int is a bad idea. Allow me to demonstrate

engineer.sleep(LOOKING_FOR_NEW_JOB);

I don't think engineer.getSpouse() is going to buy the notion that napping counts has job hunting.

engineer.eatPizza(FREE);

The poor schmuck trying to debug things later is likely to get many confused -- where did the engineer find free pizza?

It looks very much like what you are trying to describe is the "state" of the engineer. Enums are a good tool to use for that.

public enum EngineerState {
    WORK, LOOKING_FOR_NEW_JOB, BUSY, FREE, SLEEP
}

Second point to make is that these look a lot like implementation details; notice that no part of your interface uses these ideas at all. That being the case, they shouldn't be defined in the interface.

You might also consider that if you are trying to track state, then you should have a state machine (somewhere). Make that explicit -- state machines are a very powerful idea, and if you aren't familiar with them you should correct that. I happen to think stateless4j is a good library for that.

Turning the over-engineering up to "11": right now your Engineer interface seems to be serving two different roles.

void startWork(long time);
int stopWork(long time);

void sleep(int seconds);
boolean eatPizza(int pieces);    

These methods all look like they are part of an Engineer model; they are a bunch of methods that tell your in memory representation of an engineer how to change to match the real world.

String makeOffer(int salary, String employer);
void setEmployer(String name);
void fire();

These also look like methods in a model; but the spelling of the method names is wrong. makeOffer and fire are things the boss does. For a model of an engineer, they should probably be receiveOffer and loseJob, or something similar. setEmployer is a weak name -- in a model, we don't touch properties, but instead mimic behavior. acceptJob is the verb that causes the employer to change.

boolean isHappy();
String getEmployer();
String getType();
String getName();

These methods all look like an Engineer projection - a bunch of questions you might ask about the engineer at a particular point in time. These shouldn't be part of the same interface, because they serve a completely different role.

(It would, of course, be reasonable for the EngineerImpl to implement both interfaces in a simple program.)

\$\endgroup\$
5
\$\begingroup\$

The makeOffer() method could be cleaned up. Since the shouldIAcceptNewOffer() and acceptOffer(), both are only used once for this single method it would make more sense to handle all the logic in one method.

Like so;

 public String decideOnOffer(int salary, String employer) {
    if (salary > this.salary && employer.equals(this.employer)) {
        this.salary = salary;
        this.employer = employer;
        isHappy = true;
        view.showSmile();
    }       
}

This method saves passing through the parameters salary and employer again unnecessarily and is a more neater/conventional approach.

\$\endgroup\$
5
\$\begingroup\$

In addition to all the other excellent answers...

Seeking new employment

@Override
public void fire() {
    this.salary = 0;
    this.employer = HOMELESS;
    isHappy = false;
    status = LOOKING_FOR_NEW_JOB;
}

I'm pretty sure HOMLESS can't be an employer. Also, does calling this method imply the engineer is fired, or the engineer has decided to fire the employer? Should there be a difference between resign() and retrench()?

Dates

Using long primitive to represent 'times' is an archaic concept from the old java.util.Date-based classes. In case you are not on Java 8 (with its new java.time.* APIs), you should look at the JodaTime library to better represent chronological events/periods. The long primitive also makes it questionable what is the epoch you are using. As a last resort, even a Date object will be better as it clearly represents a date-time, and uses the well-understood Unix epoch so that there is no ambiguity there.

equals(Object) comparison

@Override
public boolean equals(Object engineer) {
    if (this == engineer) {
        return true;
    }
    Engineer e = (Engineer) engineer;
    return this.isHappy == e.isHappy() && hashCode() == e.hashCode();
}

You will also need to return false when the input is not an Engineer implementation. This is partly why for the equals(Object) method, the argument name used here is usually something along the lines of object, obj, other or just o. It cannot be assumed that the casting will always work.

Answering your questions

  1. Is it OK to put constants to interface and use them in the implementation?

Certainly, so long that it makes sense for them to 'belong' to the interface.

edit: After some Googling, I think it's prudent to clarify that this is not an endorsement of the constant interface pattern, and I hope you are not alluding to that. An interface should be used for its original purpose, foremost. It should not be used purely to attach constants to classes implementing the interface. When implementations require some constants to ease inter-implementation operability, or said constants are abstract enough not to be regarded as implementation-specific, then adding them to the interface 'makes sense'.

  1. Should I make constants static final if they are defined in an interface? Which constants should I prefer, int or String?

As mentioned in @VoiceOfUnreason's answer, sometimes you should consider enums instead of 'plain' int and String values. Enums are usually more expressive and thus a better fit for enumerated values. Constants should always be declared as static final to communicate the intent that they are unmodifiable, and that they can be used without class instantiation.

  1. Can I make some additional actions in setters?

The idea here is to be consistent. If your setEmployer() method calls signRenumerationPackage() and the optional (I hope) burnBridges() methods, then you have to make sure modifying the employer reference always goes through the setEmployer() setter. As long as you have consistency in place, having additional actions in setters are OK and sometimes even encouraged.

  1. When should I write a JavaDoc?

As early as possible. :)

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