2

JAVA 8

I have a POJO class:

class User {
    private String name;
    private String password;

    //... Getters / Setters ...
}

Which i will use as an entity class.

In the getter/setter for password, I want to add decryption/encryption logic.

public String getPassword() {
    return EncryptionFactory.decryption(password);
}

public void setPassword(String password) {
    this.password = EncryptionFactory.encryption(password);
}

EncryptionFactory is a utility class which encrypts/decrypt a String.

My question is

Based on the General Java coding guidelines, if I add logic to alter password, does it break design or its a bad design?

While using it i got bad design feedback from my professors.

Community
  • 1
  • 1
Sawan Kumar
  • 327
  • 1
  • 5
  • 13
  • 1
    You are using get/setter then just only get and set value with the variable, do not do any things else. Because the design of it need to be like this, if you need to process some things then just do it separately. Just image you public your pojo class for others people, they don't know that you have encrypt the password, then they do it again. That's bad – TuyenNTA Jun 17 '17 at 03:51
  • 3
    Take a look at https://stackoverflow.com/questions/1568091/why-use-getters-and-setters. The top answer mentions your use case - using a different representation than is exposed by your interface. – Cameron Skinner Jun 17 '17 at 03:55
  • @CameronSkinner , that's mean we should do it. right ? – Sawan Kumar Jun 17 '17 at 04:13
  • @TuyenNguyen : Good point, but if we can do encryption/decryption on getter/setter, we need to change one place rather than calling encryption decryption process every where we do access user pojo. – Sawan Kumar Jun 17 '17 at 04:17
  • 1
    That means "it depends". You have to use best judgement in a given context. Since we only have a very small snippet of code it is difficult to definitely say what is best design. Did your professor say why it is bad design? If he said that it is bad design without explaining why then it is a wrong statement. – tsolakp Jun 17 '17 at 04:20

1 Answers1

6

From the wiki article on mutators:

Often a setter is accompanied by a getter (also known as an accessor), which returns the value of the private member variable

Since getters are intended for giving access to private field values, it would violate the principle of least astonishment: an exception could be thrown, the impl for EncrpytionFactory may become invalid at some point, etc... When developers are expecting to simply access a value.

Whether you consider this bad design depends on how strict your design standards are. The best way to determine is to look at the cons:

  • Unable to obtain encrypted password
  • Forced to use String for plain-text password to store password (since setPassword automatically encrypts)
  • Introduces the dependency of EncrpytionFactory within the type that contains get/setPassword

So in your specific case, it's bad design.

Although there are cases where it may be preferred by some developers. For example, if you had a Person#getAge() : int but used a Date property to manage the person's age:

class Person {
    private Date dateOfBirth;

    public int getAge() {
        Date now = ...; //grab date/time right now
        int age = ...; //calculate age using dateOfBirth and now

        return age;
    }

Some would argue that the calculation should be decoupled, or getAge should be named calculateAge.

From the wiki article:

Accessors conversely allow for synthesis of useful data representations from internal variables while keeping their structure encapsulated and hidden from outside modules. A monetary getAmount accessor may build a string from a numeric variable with the number of decimal places defined by a hidden currency parameter.

In your case, there is more to worry about.

Do you REALLY want to decrypt the password, as in, is there any reason to? Assuming you want to maintain security, there are safer ways to maintain a password.

Anytime you want to compare an entered password to a stored password, simply encrpyt the entered password and compare the results to the stored password.

If someone wants to "recover" their password, it would be a security risk to send the user their password in plain-text. This is why large businesses require you to reset your password if you forget it.

Lets say you wanted to keep the decryption.

Do you really need to decrypt everytime someone calls getPassword()? What if someone wants the encrpyted value?

You should keep decryption separate from accessing. Seeing how you already have a EncryptionFactory:

User user = ...;
String encryptedPassword = user.getPassword();
String decryptedPassword = EncryptionFactory.decryption(encryptedPassword);

There should only be few times where your password actually exists decrypted (some may argue it should never exist decrypted in memory). Point is, it should be completely optional.

The password should be set already encrypted:

String encryptedPassword = EncryptionFactory.encrpytion(...);

User user = ...;
user.setPassword(encryptedPassword);

It should be accessible in encrpyted form. You should never force security risks like this (decrypting a password), make it optional.

Lastly, if you must expose a decrypted getPassword alternative, you should rename it to decryptPassword():

public String decryptPassword() {
    return EncrpytionFactory.decryption(getPassword());
}

Although this introduces unneccesary coupling betweeing User (or whatever type your'e using) and EncryptionFactory.

You should really look deeper into the security aspects. You should not be maintaining decrypted passwords as String, and it's safest if no one but the user knows the password.

Community
  • 1
  • 1
Vince
  • 14,470
  • 7
  • 39
  • 84