3
\$\begingroup\$

I am trying to model person in OOPS using immutability.
I have created an object bond with age as 25. To change the age I have created a new object and returned it.

Is there any better way to change the age instead of passing all the values of object again? Is this a correct way to do?

import java.io.*;
import java.util.*;


class Solution
{
    public static void main(String args[])
    {
        System.out.println("TestMessage");      
        Person bond = new Person("bond",25,"100 High","Park St","10000","NewYork");
        System.out.println("The age of bond is "+bond.getAge());     
        //If I want to change age of John,create new object as the preson object is immutable.
        bond = bond.changeAge(50);
        System.out.println("New age of bond is "+bond.getAge());      

    }
}

//Immutable class....
//Rule 1:No setter..getters can be present
//Rule 2:Ensure that the class can’t be extended
//Rule 3:Make all fields final, To avoid problems in multithreaded env.
//Rule 4:Make all fields private
//Rule 5:Ensure exclusive access to any mutable components
final class Person
{
  private final String name;
  private final int age;
  private final String streetAddr1;
  private final String streetAddr2;
  private final String pinCode;
  private final String city;

  private int salary;
  private float bonusPercentage;

  public Person(String name,int age,String streetAddr1,String streetAddr2,String pinCode,String city)
  {
    this.name=name;
    this.age=age;
    this.streetAddr1 = streetAddr1;
    this.streetAddr2 = streetAddr2;
    this.pinCode = pinCode;
    this.city = city;
  }  
  public int getAge()
  {
    return this.age;
  }  

  public Person changeAge(int age)
  {
    return new Person(this.name,age,this.streetAddr1,this.streetAddr2,this.pinCode,this.city);
  }
}
\$\endgroup\$
  • \$\begingroup\$ There's no reason to make the fields private and add getters, as they are already final. Unless, of course, you are reasonably sure the data won't stay a field. YAGNI applies. \$\endgroup\$ – Deduplicator Oct 14 '15 at 21:32
  • \$\begingroup\$ @Deduplicator If there are no getters,how can I access the fields like age. If I want to find the age. \$\endgroup\$ – mc20 Oct 14 '15 at 21:34
  • \$\begingroup\$ As I said, make it public and remove the getter. \$\endgroup\$ – Deduplicator Oct 14 '15 at 21:36
  • \$\begingroup\$ @Deduplicator while that is true, a common practice in Java is to have private fields and getters, even if the fields are immutable. \$\endgroup\$ – Simon Forsberg Oct 14 '15 at 21:38
  • 1
    \$\begingroup\$ There is reason for using accessors over public final fields. This preserves the flexibility to change the class internal representation... \$\endgroup\$ – MAG Oct 14 '15 at 23:32
7
\$\begingroup\$

General Review

Bad Practices

First, you import the whole package:

import java.io.*;
import java.util.*;

It is usually better practice to import specific classes. Also, I don't even see the use of any of these packages. Don't import what you don't need: it will affect performance.

Formatting

Your second class seems to have two-space indents. Java standard conventions suggests four-space indents.

Your spacing is also inconsistent. Consider:

this.name=name;
this.age=age;
this.streetAddr1 = streetAddr1;
this.streetAddr2 = streetAddr2;
this.pinCode = pinCode;
this.city = city;

You have some lines with spaces, some lines without. Choose a style, and stick with it. I suggest the spaces version, because it is easier to read...

Spacing here:

public Person(String name,int age,String streetAddr1,String streetAddr2,String pinCode,String city)

and here:

Person bond = new Person("bond",25,"100 High","Park St","10000","NewYork");

Put spaces after the commas; this will make it easier to read.

Naming

Usually, setters' method names begin with set. It is also suggested that it is so. changeAge should be setAge.

Immutable Class Suggestions

Is this a correct way to do?

Well, read on!

Immutable classes should not have any set methods. This is because an immutable class should only represent objects that don't change. If a person needs to be changed (not likely though), then the most obvious way to do so is to directly call the Constructor for a new Object, as a person with a different characteristic would not be the same person.

\$\endgroup\$
  • 3
    \$\begingroup\$ I think the problem with his change method is one of wording, not principle. Suppose that the class has lots of fields, more than you want to type out into a constructor every time you want a new Person just like your old one, but with a different name? Equally important case: suppose the class expands at some point, adding new fields; having a way to construct objects based on previously constructed objects saves you a lot of revisions in this case. \$\endgroup\$ – sqykly Oct 15 '15 at 1:52
  • 1
    \$\begingroup\$ Having to reconstruct the object manually isn't the "most obvious way" to me. Immutable classes can have methods such as withAge(int newAge), only these methods spawn an independent instance of the object (with a new age and the rest of data same as original). Case in point: msdn.microsoft.com/en-us/library/… \$\endgroup\$ – Konrad Morawski Oct 15 '15 at 13:34
  • \$\begingroup\$ @konradmorawski exactly my point : ) \$\endgroup\$ – sqykly Oct 15 '15 at 14:47
  • \$\begingroup\$ @sqykly yeah, true, i actually read your comment and left a +1 after i wrote mine. FWIW, some languages, like JVM-compatible Kotlin, offer this thing out of the box, see kotlinlang.org/docs/reference/data-classes.html - under "Copying" \$\endgroup\$ – Konrad Morawski Oct 15 '15 at 14:50
  • 1
    \$\begingroup\$ @mc20 You're doing it right already, just naming it wrong. changeAge makes it sound like the object itself will change. withAge, as Konrad suggested above, is more clear. \$\endgroup\$ – sqykly Oct 15 '15 at 16:12
6
\$\begingroup\$

I usually create an inner class, Copier, which is responsible for copying all values from the original object to the new one. And the Copier class also provides setters that are used to override values. This approach is fairly similar the fluent builder API. A note that you have to firstly copy original values in the constructor in order to make sure all setters override them properly.

final class Person {
    private final String name;
    private final int age;
    private final String streetAddr1;
    private final String streetAddr2;
    private final String pinCode;
    private final String city;

    private int salary;
    private float bonusPercentage;

    public Person(String name, int age, String streetAddr1, String streetAddr2, String pinCode, String city) {
        this.name = name;
        this.age = age;
        this.streetAddr1 = streetAddr1;
        this.streetAddr2 = streetAddr2;
        this.pinCode = pinCode;
        this.city = city;
    }

    public int getAge() {
        return this.age;
    }

    public Person changeAge(int age) {
        return new Copier().withAge(age).copy();
    }

    private class Copier {
        private String name;
        private int age;
        private String streetAddr1;
        private String streetAddr2;
        private String pinCode;
        private String city;

        public Copier() {
            name = Person.this.name;
            age = Person.this.age;
            streetAddr1 = Person.this.streetAddr1;
            streetAddr2 = Person.this.streetAddr2;
            pinCode = Person.this.pinCode;
            city = Person.this.city;
        }

        public Copier withAge(int age) {
            this.age = age;
            return this;
        }

        public Person copy() {
            return new Person(name, age, streetAddr1, streetAddr2, pinCode, city);
        }
    }

}
\$\endgroup\$
4
\$\begingroup\$

This 2 attributes are not doing anything.

  private int salary;
  private float bonusPercentage;

They should be final as well. And you should probably want an overload for your constructor or some methods similar to your changeAge. If you plan to add more optional parameter in the future. Effective Java builder pattern may be of use.

\$\endgroup\$
4
\$\begingroup\$

Is there any better way to change the age instead of passing all the values of object again? Is this a correct way to do?

There are two approaches that are typically used. Both are employed by the final class String, so we'll use that as our reference.

The first is to add a method to the final class which creates a new instance as a result of the method call. An example would be String#concat(String) which concatenates two Strings and returns a new immutable object with the result. This is the approach you came up with above.

The other is to create a new class called a mutable peer. String actually has two mutable peers, StringBuilder and StringBuffer. You supply the peer with your immutable object, change the state of the peer, and then create a new immutable from it. As long as your API only takes your immutable class you can leverage the benefits of immutability, while clients can freely modify immutable peer instances. The Builder paradigm is often used as mutable peer.

Which approach is right? It depends. If you need a lot of flexibility to change the immutable, then go with a mutable per. If there are only a small number of mutation operations, it's cleaner and more convenient if they're directly on the object.

\$\endgroup\$
1
\$\begingroup\$

I think you missed the point by making this representation of a Person immutable.

Explanation: the age isn't something that is immutable by nature (as you already see you had to add a changeAge method). So when I read your code, if I had no idea what a Person would be in real life, I would think that when a Person has its age changed, it just disappears and let place to a new similar Person with only another age. Sounds weird...

So what ? I personnaly would modelize an immutable Person in this way:

final class Person {
    private final String name;
    private final LocalDate birthdate;
    private final String streetAddr1;
    private final String streetAddr2;
    private final String pinCode;
    private final String city;

    private int salary;
    private float bonusPercentage;

    public Person(String name, LocalDate birthdate, String streetAddr1, String streetAddr2, String pinCode,
            String city) {
        this.name = name;
        this.birthdate = birthdate;
        this.streetAddr1 = streetAddr1;
        this.streetAddr2 = streetAddr2;
        this.pinCode = pinCode;
        this.city = city;
    }

    public int getAge()
    {
        return Period.between(birthdate, LocalDate.now()).getYears();
    }

    public static void main(String args[]) {
        Person bond = new Person("James Bond", LocalDate.of(1930, Month.AUGUST, 25), "100 High", "Park St", "10000", "NewYork");
        System.out.println("The age of Bond is " + bond.getAge());

        // Yep, even if he also lives in the same place, has the same name, he's definitely not the same person as he's born 23 years later !
        Person anotherBond = new Person("James Bond", LocalDate.of(1953, Month.MAY, 16), "100 High", "Park St", "10000", "NewYork");
        System.out.println("Age of the other Bond is " + anotherBond.getAge());
    }
}

What did I do ? I just replaced the age by the birthdate. A birthdate is by definition immutable (because it's an instant that occured in the past and cannot be altered). Then I can automatically infer the age on demand.

Note: as the OP only asked for a review of the "age concept", I didn't alter the rest of Person.

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