When is it appropriate to assign values to an instance variable in Java?

Question

This is a question on best practice. When taking an object oriented approach, I've come up with three different ways that I might do the same thing. To my untrained eye, none of them seem "wrong", but I know every language and style has its best practices, and I want to know if any of these three ways violate some "best practice" that I haven't learned yet.

Way 1: (declare, then assign in constructor)

public class CCipher {

    private String alphabet;
    private String shiftedAlphabet;
    private int mainKey;

    public CCipher(int key){

        mainKey = key;
        alphabet = "abcdefghijklmnopqrstuvwxyz";
        shiftedAlphabet = alphabet.substring(mainKey) 
                        + alphabet.substring(0, mainKey);
    }

Way 2: (declare and assign at the same time)

public class CCipher {

    private String alphabet = "abcdefghijklmnopqrstuvwxyz";
    private String shiftedAlphabet;
    private int mainKey;

    public CCipher(int key){

        mainKey = key;
        shiftedAlphabet = alphabet.substring(mainKey) 
                        + alphabet.substring(0, mainKey);
    }

Way 3: (Some things get initialized in a non get/set method)

public class CCipher {

    private String alphabet;
    private String shiftedAlphabet;
    private int mainKey;

    public CCipher(int key){

        mainKey = key;
        alphabet = "abcdefghijklmnopqrstuvwxyz";

    }

    public String encrypt(String input){
        shiftedAlphabet = alphabet.substring(mainKey) 
                        + alphabet.substring(0, mainKey);
        // ... code to encrypt input ...
    }

    public String decrypt(String input){
        shiftedAlphabet = alphabet.substring(26 - mainKey) 
                + alphabet.substring(0, 26 - mainKey);
        // ... code to decrypt input
    }
}

Personally, for this specific homework assignment, I really like the third way best, because it flows with the logic of the problem I'm trying to solve. But it it's wrong, well it's wrong...

Your third method leaves shiftedAlphabet as null when the CCipher object is initialized; this may end up causing issues for whoever uses it and expects the value to not be null. The other two are mostly personal preference. — Jacob G.
– Jacob G., Commented Dec 19, 2019 at 16:32
For the third way, the shiftedAlphabet is only ever used internally in the method which assigns to it, so there is no need for it to be a field; better to make it a local variable. — kaya3
– kaya3, Commented Dec 19, 2019 at 16:32
Why dont you declare alphabet = "abcdefghijklmnopqrstuvwxyz"; as private static final String? Is there any chance it changes later? — NickAth
– NickAth, Commented Dec 19, 2019 at 16:33
Do you need to have a mainKey field in the first place? It doesn't seem to be used anywhere except in the constructor. — JB Nizet
– JB Nizet, Commented Dec 19, 2019 at 16:34
Hi @NickAth, I doubt it would change later unless I spontaneously learn Russian. I haven't learned about "final" yet, though my IDE has been poking me to use it. — rocksNwaves
– rocksNwaves, Commented Dec 19, 2019 at 16:35

Joop Eggen · Accepted Answer · 2019-12-19 16:45:58Z

3

The second version seems okay. But with the constant indeed as static final String.

public class CCipher {

    private static final String ALPHABET = "abcdefghijklmnopqrstuvwxyz";
    private final String shiftedAlphabet;
    private final int mainKey;

    public CCipher(int key) {
        mainKey = key;
        shiftedAlphabet = ALPHABET.substring(mainKey) 
                        + ALPHABET.substring(0, mainKey);
    }

Christopher Schneider pointed out that in for encrypting and decrypting different shiftedAlphabets are used. As a CCipher object in reality probably either encrypts or decrypts, make it a local variable.

One would need two different non-final lazily initialized fields which is cumbersome.

public class CCipher {

    private static final String ALPHABET = "abcdefghijklmnopqrstuvwxyz";
    //private final String shiftedEncryptAlphabet;
    //private final String shiftedDecryptAlphabet;
    private final int mainKey;

    public CCipher(int key) {
        mainKey = key;
        shiftedAlphabet = ALPHABET.substring(mainKey) 
                        + ALPHABET.substring(0, mainKey);
    }

    public String encrypt(String input){
        String shiftedAlphabet = alphabet.substring(mainKey) 
                    + alphabet.substring(0, mainKey);
        // ... code to encrypt input ...
    }

    public String decrypt(String input){
        String shiftedAlphabet = alphabet.substring(26 - mainKey) 
            + alphabet.substring(0, 26 - mainKey);
        // ... code to decrypt input
    }

edited Dec 19, 2019 at 16:45

answered Dec 19, 2019 at 16:32

Joop Eggen

110k8 gold badges89 silver badges142 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

Christopher Schneider Over a year ago

Exactly what I was going to say. If properties don't change, mark them final and initialize in the constructor.

Joop Eggen Over a year ago

@ChristopherSchneider indeed. Even in 3 the mainKey seems to be fixed. Hence prepare the constants so everything is optimal.

Christopher Schneider Over a year ago

I missed this, since it's a class property and not a local variable, but looks like shiftedAlphabet is a changing variable. What it should probably be is two variables, e.g. shiftedAlphabetEncrypt and Decrypt

Joop Eggen Over a year ago

@ChristopherSchneider I oversaw that. Then a local variable maybe would be best, as it is unlikely that the same instance would do both, encrypting and decrypting.

rocksNwaves Over a year ago

@JoopEggen Thanks. I'm thinking of using a combination of all your answers. Keep alphabet and make it final. Get rid of the shiftedAlphabet variable and make it local. Let the mainKey be filled in by the constructor...

Mureinik · Accepted Answer · 2019-12-19 16:35:59Z

1

With regard to alphabet - it if never changes, the second snippet probably comes closest to the best practice, but it's still not quite there - it should be a private static final "constant":

public class CCipher {   
    private static final String alphabet = "abcdefghijklmnopqrstuvwxyz";
    // ...
}

With regard to shiftedAlphabet - the third snippet is definitely less favorable (albeit not technically "wrong") - on each call to encrypt or decrypt you recalculate the shiftedAlphabet which is not impacted by the input in any way. It may not be wrong, but it is wasteful (in other words - as a teacher, I'd definitely deduce points for that, even if the code does work).

To sum this up - the second snippet is probably the best of all three, but I'd fix alphabet's modifiers to private static final.

answered Dec 19, 2019 at 16:35

Mureinik

316k54 gold badges402 silver badges406 bronze badges

4 Comments

rocksNwaves Over a year ago

Well, it's not being "re-calculated" necessarily, seeing as how it never got initialized in the constructor or when it was declared. That's why I like this way, because each decrypt requires on shifted alphabet and encrypt requires a totally different shifted alphabet . Perhaps the solution (from a comment on the question) would be to just make shiftedAlphabet a local variable in the methods that use it?

rocksNwaves Over a year ago

And I haven't learned about "final" yet, though my IDE has been poking me to use it with a warning highlight. I wanted to hold off, but since you mention it I'll skip ahead a little :)

JB Nizet Over a year ago

@rocksNwaves most readers, me included, missed the fact that your third example shows that you actually need two different versions of the shifted alphabet: one for decryption, and one for encryption. So use two local variables, or use two (final) fields. But don't use a single field and change the value every time one of the methods is invoked. That makes the code not-thread-safe, and extremely confusing.

rocksNwaves Over a year ago

@JBNizet That makes sense to me! That's what I'm going to do now, thank you!

Collectives™ on Stack Overflow

When is it appropriate to assign values to an instance variable in Java?

2 Answers 2

5 Comments

4 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

5 Comments

4 Comments

Your Answer

Sign up or log in

Post as a guest

Related