Immutable employee class

Question

I have created an employee object to be immutable.

package com.immutable.ex01; import java.io.Externalizable; import java.io.IOException; import java.io.ObjectInput; import java.io.ObjectOutput; import java.text.DateFormat; import java.text.SimpleDateFormat; import java.util.ArrayList; import java.util.Calendar; import java.util.Date; import java.util.List; public final class Employee implements Cloneable, Externalizable{ private final Integer age; private final String name; private final List<String> companies; private final Date dob; public Employee(Integer age, String name,List<String> companies, Date dob) { this.age = age; this.name = name; this.companies = companies; this.dob = dob; } public Integer getAge() { return this.age; } public String getName() { return this.name; } public Date getDob() { Calendar cal = Calendar.getInstance(); cal.setTime(this.dob); return cal.getTime(); } public List<String> getCompanies() { List<String> clone = new ArrayList<String>(this.companies.size()); for(String companyName : this.companies) { clone.add(companyName); } return clone; } @Override public String toString() { StringBuffer strb = new StringBuffer(); strb.append("AGE").append(" ").append(this.age) .append(", NAME ").append(this.name) .append(", DOB ").append(this.getDateString()) .append(", COMPANIES WORKED IN "); for(String companyName : this.companies) { strb.append(companyName).append(","); } strb.deleteCharAt(strb.length() - 1); return strb.toString(); } @Override public Object clone() throws CloneNotSupportedException{ throw new CloneNotSupportedException("Cannot be cloned"); } @Override public void writeExternal(ObjectOutput out) throws IOException { throw new IOException("This operation is not supported"); } @Override public void readExternal(ObjectInput in) throws IOException { throw new IOException("This operation is not supported"); } private String getDateString() { String date = null; DateFormat df = null; try { df = new SimpleDateFormat("dd MMM yyyy"); date = df.format(this.dob); }catch(Exception e) { e.printStackTrace(); } return date; } }

My main method class

package com.immutable.ex01; import java.text.DateFormat; import java.text.SimpleDateFormat; import java.util.ArrayList; import java.util.Date; import java.util.List; public class MainTest01 { public static void main(String [] args) { Employee emp = null; List<String> companyList = null; Date date = null; try { companyList = new ArrayList<String>(); companyList.add("IBM"); companyList.add("Google"); companyList.add("Norton"); companyList.add("Seable"); companyList.add("Nekki"); date = toDate("28 Oct 1981"); emp = new Employee(23, "John Molkovich", companyList, date); date = emp.getDob(); date = new Date(); companyList = emp.getCompanies(); companyList.add("Toyo Corp"); System.out.println(emp); }catch(Exception e) { e.printStackTrace(); } } private static Date toDate(String dateString) throws Exception{ Date d = null; DateFormat df = new SimpleDateFormat("dd MMM yyyy"); d = df.parse(dateString); return d; } }

Please review my code and check if I have achieved immutability for my employee class.

If you create an immutable Employee, there's no need to clone it, never. So implementing Cloneable doesn't make sense. — Ralf Kleberhoff
– Ralf Kleberhoff, Commented Mar 31, 2019 at 15:04

AJNeufeld · Accepted Answer · 2019-03-31 15:02:23Z

Your Employee is frozen in time; they can never get any older!

Age should be calculated from date-of-birth and “today”, not stored in an immutable field.

The getCompanies() method could be written simply as:

return new ArrayList<>(this.companies);

Or perhaps:

return Collections.unmodifiableList(this.companies);

Carcigenicate · Accepted Answer · 2019-03-31 23:28:25Z

First, I'll note that your class is not immutable currently. You allow the user to pass in a List of companies, but don't make a copy of them. If the user modifies the list after passing it in, it will be modified inside the Employee as well. Just make a copy of it to remedy this:

public Employee(Integer age, String name, List<String> companies, Date dob) { this.age = age; this.name = name; this.companies = new ArrayList<>(companies); this.dob = dob; }

And you can do the same in getCompanies as @AJ noted.

And I'll mention some warnings that pop up as soon as I pasted this into IntelliJ:

Externalizable class 'Employee' has no 'public' no-arg constructor

At class Employee....

Iteration can be replaced with bulk 'Collection.addAll' call

In getCompanies in the add loop. You can just pass a collection to addAll and have it do the looping.

'StringBuffer strb' may be declared as 'StringBuilder'

In toString. StringBuffer is a synchronized version of StringBuilder. You don't need synchronization here though.

@Override is not allowed when implementing interface method

At the @Override annotations for the Externalizable methods.

You can also simplify the stringification of companies in toString using StringJoiner and String.join:

public String toString() { StringJoiner joiner = new StringJoiner(", "); joiner.add("AGE " + this.age) .add("NAME " + this.name) .add("DOB " + this.getDateString()) .add("COMPANIES WORKED IN: " + String.join(", ", this.companies)); return joiner.toString(); }

AGE 23, NAME John Molkovich, DOB 28 Oct 1981, COMPANIES WORKED IN: IBM, Google, Norton, Seable, Nekki

StringJoiner is like StringBuffer/StringBuilder, but it automatically adds a ", " between each addition, and String.join adds a ", " between each company. No more needing to manually add commas for each field, and needing to delete the trailing company comma.

And don't worry about using append methods instead of + at every opportunity like you were doing. Iirc, + is automatically optimized to a StringBuilder with append calls when possible, and even when it's not, the overhead you'll suffer from using + likely won't be a problem. Go for the readable code here instead of the over-optimized code.

And what exception are you anticipating being thrown in main? You should get rid of the whole try. Let the sucker crash if it's going to crash. It's usually easier to get information about an exception when the IDE is allowed to process the actual crash instead of just a printout.

And a small note, I don't see any reason to have this.age be a wrapped Integer. It would be better for it to be a primitive int (or even a short). I'd change the field type to int, along with the Employee constructor parameter and the return type of getAge.

You could even get rid of the getters for your immutable fields like age. They can't be overwritten anyways since they're final:

class Immutable { public final int age; public Immutable(int age) { this.age = age; } public static void main(String[] args) { Immutable i = new Immutable(10); i.age = 4; System.out.println(i.age); } }

Error:(11, 10) java: cannot assign a value to final variable age

A getter is necessary for cases like getCompany where the field is mutable, but numbers are immutable. There's a discussion on the topic here.

Stack Exchange Network

Immutable employee class

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Immutable employee class

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions