NullPointerException while indexing variable String parameters

Question

I'm new to the idea of using an ellipsis. I'm almost certain my error is caused by improperly declaring or initializing "String[] authors", but I don't know how to do this an still have my setAuthors method work.

import java.util.*;
public class Book {
    private String[] authors; //I'm guessing this line should end with "= new String..."
                              //but not sure how to w/o specifying an array dimension
    private int authorsSize;

    //Receives variable # of String parameters and indices them into String[] "authors"
    public void setAuthors(String... authors) {
        authorsSize = authors.length;
        for(int i=0;i<authorsSize;i++)
            this.authors[i] = authors[i];
    }

//getAuthors method:

    public String getAuthors(){
         String s = "";
         authorsSize = authors.length;
              for(int i=0;i<authorsSize;i++)
              s = s+authors[i] + ", ";
         printAuthors = s;
         return s;
    }

getAuthors method: public String getAuthors(){ String s = ""; authorsSize = authors.length; for(int i=0;i<authorsSize;i++) s = s+authors[i]; printAuthors = s; return s; } — aweeeezy
– aweeeezy, Commented Nov 17, 2012 at 8:18
You need to cater for authors being null in your get method... Also, what is printAuthors? — Brian
– Brian, Commented Nov 17, 2012 at 9:57
I have now added an adjusted getAuthors method in my answer below. I've taken the liberty of improving it by having comma's only between elements and also by using the more efficient StringBuilder — Brian
– Brian, Commented Nov 17, 2012 at 10:10

Jon Skeet · Accepted Answer · 2012-11-16 08:36:35Z

7

The simplest approach would just be to clone the array:

public void setAuthors(String... authors){
    this.authors = (String[]) authors.clone();
}

After all, you're overwriting the previous data anyway, and you can't know the size before the method call. You don't need the authorsSize variable at this point - you've got the authors array which knows its own length.

(If you were able to use immutable collections, you wouldn't even need to bother cloning, of course.)

EDIT: As noted in comments, this method will throw an exception if you pass in a null reference. You shouldn't automatically decide that this is a situation you should "handle" - it's entirely legitimate to document that the parameter must not be null. I would suggest documenting the behaviour around nullity either way.

Indeed, if you do this you might also want to initialize your instance field like this:

private String[] authors = new String[0];

That way you always know you'll have a non-null reference in that field, so you can use it with impunity. I would prefer this over having to check for null on every use.

edited Nov 16, 2012 at 8:36

answered Nov 16, 2012 at 8:19

Jon Skeet

1.5m893 gold badges9.3k silver badges9.3k bronze badges

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

8 Comments

Bhavik Ambani Over a year ago

What is the null is passed as an argument ? Your code will throw NullPointerException and it will fail. For more info check my code below.

Bhavik Ambani Over a year ago

What if the NULL is passed then exception will be thrown and the previous values will be unchanged to the old values. rather it should be set null.

Jon Skeet Over a year ago

@BhavikAmbani: If it throws a NullPointerException, that's quite possibly the correct behaviour - we don't know whether the document is intended to handle a null reference.

Bhavik Ambani Over a year ago

@JonSkeet But the old value will be unchanged, rather the old values should be set to null, i.e. this.authors should be set with null as null is passed as authors.

Jon Skeet Over a year ago

@BhavikAmbani: If the method has been called incorrectly, the state of the class shouldn't be changed. That's general best practice. See my edit - explicitly declaring that null is an invalid parameter value seems like it's more likely to be a good thing, IMO.

|

PermGenError · Accepted Answer · 2012-11-16 08:26:51Z

3

you never initialized authors your array . you need to initialize it before you use it .

String[] authors = new String[size];

//but not sure how to w/o specifying an array dimension

The best way is to use an List implementing classes as they are dynamic Arrays. i.e., you dont need to specify the size.

List<String> authors = new ArrayList<String>();

edited Nov 16, 2012 at 8:26

answered Nov 16, 2012 at 8:19

PermGenError

46.5k8 gold badges90 silver badges107 bronze badges

Comments

Bhavik Ambani · Accepted Answer · 2012-11-16 08:31:52Z

1

You have to currect your setAuthors method as described below

public void setAuthors(String... authors) {
    if (authors != null && authors.length > 0) {
        authorsSize = authors.length;
        authors = new String[authorsSize];
        for (int i = 0; i < authorsSize; i++)
            this.authors[i] = authors[i];
    }else{
        this.authors = null;
    }
}

edited Nov 16, 2012 at 8:31

answered Nov 16, 2012 at 8:22

Bhavik Ambani

6,66515 gold badges58 silver badges87 bronze badges

8 Comments

Brian Over a year ago

Well done on checking null and length, but the problem is that this.authors will not change if the authors is null or 0 and will hold onto old data...

Brian Over a year ago

Slightly different outcome to mine below, depending on what you want to happen in the case of a null or 0

Bhavik Ambani Over a year ago

As I said please find my updated answer, where I checked null or 0 value

Brian Over a year ago

yes, I found your updated answer, I was just pointing out that the eventual outcome of this.authors would be different between our 2 code snippets as you would make this.authors = null and I would make this.authors = new String[]

Brian Over a year ago

I can spend as long as I like, on whatever I like... Especially if I feel there is more to be added.

|

happy77 · Accepted Answer · 2012-11-16 09:17:10Z

1

Because the declaration of "private String[] authors" is before that of "public void setAuthors(String... authors)", you can not use the format like "String[] authors = new String[authorsSize]". This will make the size of authors always be 0.

The better way is to use the dynamic initialization: List authors = new ArrayList(); Then use this.authors.add(authors[i]) to pass parameters.

answered Nov 16, 2012 at 9:17

happy77

1811 gold badge1 silver badge5 bronze badges

Comments

Brian · Accepted Answer · 2012-11-17 10:13:55Z

you could also adjust your code like so:

import java.util.*;
public class Book{

    private String[] authors; 
    private int authorsSize;

    public void setAuthors(String... authors){

        //check for null, you could also set this.authors = new String[] if you prefer.
        if(authors == null){
            this.authors = null;
        }else{
            authorsSize = authors.length;

            //also add this line...
            this.authors = new String[authorsSize];

            for(int i=0;i<authorsSize;i++)
                this.authors[i] = authors[i];
        }
    }

    public String getAuthors(){
         if(authors == null){
             return ""; //could also return null here if you would prefer
         }

         StringBuilder s = new StringBuilder();
         authorsSize = authors.length;
         for(int i=0;i<authorsSize;i++){
             if(i > 0)
                 s.append(",");

             s.append(authors[i]);
         }

         //printAuthors = s;
         return s.toString();
    }
}

Collectives™ on Stack Overflow

NullPointerException while indexing variable String parameters

5 Answers 5

8 Comments

Comments

8 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

8 Comments

Comments

8 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related