Creating if in setter/getter

Question

Quite new with Java and wanted some help with storing passenger details whilst using setters/getters and creating an output with the toString method.

The problem I have is, say I am storing the passengers phone number and don't want their phone number to contain any characters, have a length of 10 numbers and start with 1 and return "Not Valid" if one of these occur.

I have tried to create if statements in the setter but it is not returning the "Not Valid". This is what I have so far

public class Passenger {

    private String name;
    private String location;
    private String phoneNumber;

    public Passenger(String name, String location, String phoneNumber) {
        this.name = name;
        this.location = location;
        this.phoneNumber = phoneNumber;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public String getLocation() {
        return location;
    }

    public void setLocation(String location) {
        this.location = location;
    }

    public String getPhoneNumber() {
        return phoneNumber;
    }

    public void setPhoneNumber(String phoneNumber) {
        if (phoneNumber.matches("[a-zA-z]+)")) {
            phoneNumber = "Not Valid";
        } 
        else if (phoneNumber.length() > 10) {
            phoneNumber = "Not Valid";
        }

        else if (phoneNumber.startsWith("1")){
            phoneNumber = "Not Valid";
        }
        else {
        this.phoneNumber = phoneNumber;
        }
    }

    public String toString() {
        return " Name: " + name + "\n Location: " + location + "\n Phone Number: " + phoneNumber;

  public class Test {

    public static void main(String[] args) {

        Passenger one = new Passenger("John John", "China", "1231231234");
        Passenger two = new Passenger("John John", "China", "A");
        Passenger three = new Passenger("John John", "China", "2323232323");
        Passenger four = new Passenger("John John", "China", "123123123123");
        System.out.println(one);
        System.out.println(two);
        System.out.println(three);
        System.out.println(four);
    }

For passenger two, three and four I would expect phone number to show Not Valid, but they are showing the values which were put in.

Any help would be grateful

Not an answer to this, but you might be better off throwing an exception if you detect an invalid phone number, rather than setting it to `"Not Valid"`. If you come to check if the account contains a valid phone number, you might use `if (user.getPhoneNumber().equals("not valid"))` which would not behave as expected due to case sensitivity. You may also make a typo. At the very least, define a `public static final String INVALID_STRING = "Not Valid";` in your class, and use that. — cameron1024, Aug 01 '19 at 13:32
You are mixing 3 responsibilities within one class: printing (a class `PassengerPrinter` with a method `void print(Passenger)`), validating (a class `PhoneValidator` with a method `void validate(String)` [if you will be throwing an exception], or a method `boolean isValid(String)`), and storing (your class `Passenger`). — Andrew Tobilko, Aug 01 '19 at 17:49

MC Emperor · Answer 1 · 2019-08-02T06:59:29.523

One could of course, call the setPhoneNumber method within the constructor. But the problem is that you are calling an overridable method within the constructor.

This can lead to problems.

One way is to make a private method containing the validation:

private void validateAndSetPhoneNumber(String phoneNumber) {
    this.phoneNumber = ...
}

And then call it from both the constructor

public Passenger(String name, String location, String phoneNumber) {
    this.name = name;
    this.location = location;
    this.phoneNumber = validateAndSetPhoneNumber(phoneNumber);
}

as from the setter:

public void setPhoneNumber(String phoneNumber) {
    validateAndSetPhoneNumber(phoneNumber);
}

Furthermore, a few notes:

You say you "don't want their phone number to contain any characters". Assuming that you mean "any character other than a digit", your validation is not quite correct. Your current code most certainly throws a PatternSyntaxException because your regex contains an unmatched ). If the regex were [a-zA-z]+, then still the validation were incorrect. For instance, the phone number input abc4def would be considered valid. That is because String.matches tries to match the entire region.

According to your current requirements, the following would suffice:

if (phoneNumber.matches("[02-9]\\d{0,9}")) {
    this.phoneNumber = phoneNumber;
}
else {
    this.phoneNumber = "Not Valid";
}

or just

this.phoneNumber = phoneNumber.matches("[02-9]\\d{0,9}") ? phoneNumber : "Not Valid";

However, I agree with cameron1024's comment.

score 2 · Answer 2 · answered Aug 01 '19 at 13:28

2

Use the method setPhoneNumber in constructor of your class

public Passenger(String name, String location, String phoneNumber) {
    this.name = name;
    this.location = location;
    this.setPhoneNumber(phoneNumber);
}

answered Aug 01 '19 at 13:28

Giuseppe Milicia

56
2

2

it would be better to copy the logic to the constructor, then. Having a constructor of a non-final class call a non-final setter, might lead to issues if the class is subclassed. – Stultuske Aug 01 '19 at 13:32

score 1 · Answer 3 · answered Aug 01 '19 at 14:05

Alright so a few things. In your constructor you're not calling your setter method, so those if statements were never being touched. Also in the setter you forgot to use 'this.phoneNumber'. This was causing the data to be null rather than "Not Valid".

You mention in the last statement that you expect case 1 to be correct but one of your if statements states else if (phoneNumber.startsWith("1")) So in case 1 the number is not valid, just a side note.

Here is the code:

public class Passenger {

private String name;
private String location;
private String phoneNumber;

public Passenger(String name, String location, String phoneNumber) {
    this.name = name;
    this.location = location;
    setPhoneNumber(phoneNumber);
}

public String getName() {
    return name;
}

public void setName(String name) {
    this.name = name;
}

public String getLocation() {
    return location;
}

public void setLocation(String location) {
    this.location = location;
}

public String getPhoneNumber() {
    return phoneNumber;
}

public void setPhoneNumber(String phoneNumber) {
    if (phoneNumber.matches("[a-zA-z]+")) {
        this.phoneNumber = "Not Valid";
    } 
    else if (phoneNumber.length() > 10) {
        this.phoneNumber = "Not Valid";
    }

    else if (phoneNumber.startsWith("1")){
        this.phoneNumber = "Not Valid";
    }
    else {
    this.phoneNumber = phoneNumber;
    }
}

public String toString() {
    return " Name: " + name + "\n Location: " + location + "\n Phone Number: " + phoneNumber;
}
}

Hope this helps

T A · Answer 4 · 2019-08-01T13:44:38.860

For passenger two, three and four I would expect phone number to show Not Valid, but they are showing the values which were put in.

You are printing the current set values generated by your constructor. Your constructor does not access your get and set methods during the creation of your object, those are for accessing your fields after creation of your object.

If you want to edit your fields during the creation of the object, you should do so in the constructor:

public Passenger(String name, String location, String phoneNumber) {
    this.name = name;
    this.location = location;
    if (phoneNumber.matches("[a-zA-z]+)")) {
        this.phoneNumber = "Not Valid";
    } 
    else if (phoneNumber.length() > 10) {
        this.phoneNumber = "Not Valid";
    }
    else if (phoneNumber.startsWith("1")){
        this.phoneNumber = "Not Valid";
    }
    else {
        this.phoneNumber = phoneNumber;
    }
}

You could also incorporate the changes into your toString() method if you don't actually want to change the fields but just the output:

public String toString() {
    return " Name: " + this.getName() + "\n Location: " + this.getLocation() + "\n Phone Number: " + this.getPhoneNumber();
}

score 0 · Answer 5 · answered Aug 01 '19 at 13:48

You get good answers about which is missing in your current code : setPhoneNumber() is never invoked.

But I think that adding specific logic in a standard setter (setFieldName()) is not necessarily the best thing to do : in most of cases, developers expect that a setter sets a value as it is passed.
They cannot guess that setPhoneNumber()means setPhoneNumberOrUseDefaultValueIfInvalid() without inspecting the implementation.

So I would rather rename it to make the intention explicit :

private static final String DEFAULT_PHONE_NUMBER = "Not Valid";

public void setPhoneNumberOrUseDefaultValueIfInvalid(String phoneNumber) {
        if (phoneNumber.matches("[a-zA-z]+)")) {
            phoneNumber = DEFAULT_PHONE_NUMBER;
        } 
        else if (phoneNumber.length() > 10) {
            phoneNumber = DEFAULT_PHONE_NUMBER;
        }

        else if (phoneNumber.startsWith("1")){
            phoneNumber = DEFAULT_PHONE_NUMBER;
        }
        else {
            this.phoneNumber = phoneNumber;
        }
}

and use it :

public Passenger(String name, String location, String phoneNumber) {
    this.name = name;
    this.location = location;
    setPhoneNumberOrUseDefaultValueIfInvalid(phoneNumber);
}

Creating if in setter/getter

5 Answers5

This can lead to problems.