Java Encapsulation & OOP Best Practice

Question

I implemented a class called mobileCall. I create several objects from this class and populate the String variables of this object with values coming from an XML having several mobileCalls for specific person. I need to group and count all calls made by this person (i.e. national calls: 11 mins; international calls: 15 mins; data: 20 MB)

So, I implemented several public methods inside the class to check the type of the call to return true or false. And in the main class I called these methods to check if they satisfy the criteria I count up the specific counter.

Someone professional saw my code and said that this is not a good practice and OOP was designed to eliminate such 'what are you' methods. And that there are better ways to implement this behavior. I tried to read through OOP and Encapsulation but could not figure out a better way to do it. I feel he has a point, tho.

Code Example

public class MobileCall {

    String callType;
    String callDuration;
    String callAmount;
    String callerID;
    String calleID;
    ....
    public boolean isNational(){

        if (callType.compareTo("National")==0)
            return true;
        else
            return false;
    }

    public boolean isInternational(){

        if (callType.compareTo("international")==0)
            return true;
        else
            return false;
    }
    ...
}

In Main Method

int nationalCounter;
int internationalCounter;
MobileCall mobileCall = new MobileCall();

if(mobileCall.isNational())
    nationalCounter = nationalCounter + mobileCall.getCallDuration();
else if (mobileCall.isInternational())
    internationalCounter = internationalCounter + mobileCall.getDuration();
....

Answer 1

There are many issues here ... If you talk about a good design , you might need to re-code this thing. TO point out few of the issues:

I will try to explain you the meaning of encapsulation which seems to be your basic doubt:

Encapsulation means: I have a purpose and I am designed to perform a job and I don't know anything other than my job.

I do not allow others to change my property without informing me and I have full control on my properties and I am fully responsible for what you get out of me.

Here me is any object.

Just to make it clearer, an object must perform a single job and must not try to do lot many things.

Now how do you achieve the above behavior?

You need to restrict the access to the properties, give proper/controlled access methods for the properties.

Make call types as enums as they are the best candidate for the type and value safety checks.

There are many more suggestions which I want to write, if you can share some more code pieces.

A good link to understand the basics of OOPS is here.

Answer 2

private String callType;

....
public boolean isNational(){

    if (callType.compareTo("National")==0)
        return true;
    else
        return false;
}

public boolean isInternational(){

    if (callType.compareTo("international")==0)
        return true;
    else
        return false;
}

Is the relevant code. Here's the point (now that callType is now strongly encapsulated).

Three months from now, someone complains that the program is taking up too much memory. You revisit your code and decide that maybe strings are not the best way of determining the nationality of the call, after all it is either international or not, right? That means you need at most one bit to store the data.

you make the change

private boolean nationalCall;

and then you change the methods that use this data

public boolean isNational() {
  return nationalCall;
}

public boolean isInternational() {
  return !nationalCall;
}

And because you properly encapsulated the "national call data", you can rest assured that you no longer need to hunt through the rest of your program to make sure other bits of your program will work correctly. The "effects" of a change are "encapsulated" at the "boundary" of the class's "interface" which is actually implemented by a number of "member methods".

Now code like this

if (mobileCall.isNational()) {
    nationalCounter = nationalCounter + mobileCall.getCallDuration();
} else if (mobileCall.isInternational()) {
    internationalCounter = internationalCounter + mobileCall.getDuration();
}

looks to be suspicious. Note that this block of code seems very concerned with the data and methods of a different class. Perhaps it is externally managing the class to such a degree that the location of the behavior misplaced?

If you added the methods to the "Call" class

public int getNationalMinutes() {
  if (national) {
    return callDuration;
  }
  return 0;
}

public int getInternationalMinutes() {
  if (!national) {
    return callDuration;
  }
  return 0;
}

Then you could reduce your accumulation code to

while (Call call : client.getAllCalls()) {
  internationalMinutes += call.getInternationalMinutes();
  nationalMinutes += call.getNationalMinutes();
}

Note that this last bit of work isn't 100% encapsulation in the strict data sense, as we added interface items (methods); but it does sort-of do some encapsulation in the behavior sense as we no longer need determine if the call minutes are international or national based on a series of queries to determine the class's internal state.

A series of checks against a class to determine how to use the class's date is an external migration of the behavior outside of the "Call" class. So it moving this behavior back into the class may be considered an encapsulation of the behavior of the class, unlike the previous example, which was a strict encapsulation of the data of the class.

Too many times, people forget that classes encapsulate both data and behavior. Thank you for finding an excellent example where both the points of encapsulation could be presented in the same question.

Answer 3

Well, it's not very extendable if you want to separate local calls from international calls (or separate international in more country groups). From an OO point of view you could create subtypes of MobileCall but this would be an overkill (especially if there is no functionality associated with the different types).

I would create an enum

public enum CallType { NATIONAL, INTERNATIONAL, DATA, ... } 

and map your string identifier to the enum values (if you choose wisely, you don't have do do much for that). Create an MobileCallTally class (backed by a Map

mobileCallTally.inc(call.getType(), call.getAmount());

(Of course, you could just use the Strings as Key and avoid the enum altogether, but since it's a 'type' personally I like to have it 'typed'.)

Answer 4

methods like isNational are perfectly fine for determining an object's state, but if you're using them to determine the object's type, than it's probably better to use an inherited type.

public class NationalMobileCall extends MobileCall 
{
}

Answer 5

Firstly, all instance variable should be declared as private. This encapsulates each instance variable, or attribute, of a particular class. This is the whole idea behind encapsulation, where the object's attributes are only accessible within that particular class.

For example, change String callType; to private String callType;

The advantage of declaring instance variables as private is that you can prevent other classes from manipulating the attributes of a particular object and you can provide safety nets so that they are not set to erroneous values. Imagine if a person had the ability to change the physical attributes of another person; that would be absolutely ridiculous. In the same way, attributes should be private to adhere to the principle of encapsulation.

The way that attributes/instance variables can be accessed is through the use of setters and getters. e.g.:

public void setCallType(String callType) {
    this.callType = callType;
}

public String getCallType() {
    return this.callType;
}

In this way, you adhere to the OOP principle of encapsulation.