CODEWITHSUNDEEP
c#
Aaron Thomas
Aaron Thomas

Reputation: 5281

C# List<T>.Add not adding properly

Using .Add to add an instance of a class to a generic list is not working.

To illustrate the problem, here are two simple example classes:

public class WorkOrder
{
    private List<Note> _Notes;
    public List<Note> Notes
    {
        get
        {
            return _Notes ?? new List<Note>();
        }
        set
        {
            _Notes = value;
        }
    }
}

public class Note
{
    public string NoteText { get; set; }
    public System.DateTime Time { get; set; }
    public string User { get; set; }
}

You may notice the coding in get on the WorkOrder.Notes property. I put this in so the property wouldn't be initialized with a null value (ref an answer to another question I posted on SO here).

To utilize these classes: 

public void Test()
{
    WorkOrder tempWorkOrder = new WorkOrder();
    Note tempNote = new Note()
    {
        User = "Aaron",
        Time = DateTime.Now,
        NoteText = "Work Order pulled from CSV Excel report."
    };
    tempWorkOrder.Notes.Add(tempNote);
}

I would expect the last line in Test() to add tempNote to the list of Note in tempWorkOrder. However, tempWorkOrder.Notes is null after this line completes. No errors or exceptions are thrown.

I'm using VS2013 Express.

What am I doing wrong?

Upvotes: 7

Views: 16071

Answers (9)

xanatos
xanatos

Reputation: 111840

private List<Note> _Notes;
public List<Note> Notes
{
    get
    {
        return _Notes ?? new List<Note>();
    }
    set
    {
        _Notes = value;
    }
}

The get is wrong. It should be:

    get
    {
        if (_Notes == null) {
            _Notes = new List<Note>();
        }
        return _Notes;
    }

because otherwise you don't save the new List<Note>() you created and every time you use the get you recreate it (the get returns a new List<Note>() but doesn't modify _Notes, so every get checks _Notes, see it's null and return a new List<Note>())

You can compact the get to:

return _Notes ?? (_Notes = new List<Note>());

(see Ternary/null coalescing operator and assignment expression on the right-hand side?) I don't hate enough the world (and my fellow programmers) to do it :-)

Upvotes: 19

Dennis Sulejman
Dennis Sulejman

Reputation: 21

It should be possible to use the null-coalescing assignment if you are using C# 8, like so:

get => _Notes ??= new List<Note>();

With brackets:

get { return _Notes ??= new List<Note>(); }

Upvotes: 1

Gauravsa
Gauravsa

Reputation: 6522

Late to the party, you can create a small extension method which can guard against null or empty list: 

public static bool NotNullAndEmpty<T>(this IEnumerable<T> source)
{
  if (source != null && source.Any())
    return true;
  else
    return false;
}

Also, if you are using database, then its advisable to use IEnumerable and do all modifications with IEnumerable. Once done, call .ToList() which will result in a single call to the database.

Upvotes: 0

Stig
Stig

Reputation: 1323

public class WorkOrder
{
    public List<Note> Notes {get;set;}

    public WorkOrder()
    {
        Notes = new List<Note>();
    }
}

But in C# 6.0 you should be able to do the following:

public class WorkOrder
{
    public List<Note> Notes {get;set;} = new List<Note>();                
}

Upvotes: 0

toadflakz
toadflakz

Reputation: 7944

You're not initializing _Notes.

So while you get a List<Note> back when _Notes is null, it is not assigning the object to _Notes. Each time you access the public property, it is returning a different List<Note> which is why the Add() call appears to not work.

You should rather use:

get 
{ 
  if (_Notes == null) 
     _Notes = new List<Note>(); 
  return _Notes; 
}

Upvotes: 1

Renan Gemignani
Renan Gemignani

Reputation: 2793

The problem is your get method:

    get
    {
        return _Notes ?? new List<Note>();
    }

Since you don't assign the reference of the object you're creating to _Notes, it keeps being null, and you assigned to a list that isn't referenced anywhere else.

This is what you can do instead:

    get
    {
        if (_Notes == null)
            _Notes = new List<Note>();
        return _Notes;
    }

Upvotes: 0

adv12
adv12

Reputation: 8551

In the getter for Notes, you're doing nothing to save a reference to the newly-created list. Therefore, every time you access that getter, you'll get a fresh, empty list. So this:

tempWorkOrder.Notes.Add(tempNote);

...is adding tempNote to a List<Note> that is immediately thrown away.

Upvotes: 0

Jeffrey Wieder
Jeffrey Wieder

Reputation: 2376

You never assign _Notes

Do this instead

    private List<Note> _Notes;
    public List<Note> Notes
    {
        get
        {
            if(_Notes == null)
                 _Notes = new List<Note>();
            return _Notes;
        }
        set
        {
            _Notes = value;
        }
    }

Upvotes: 1

David Watts
David Watts

Reputation: 2289

You have not created the list yet there. You need to add a constructor to the WorkOrder as you cannot add to a collection that does not exist. This way, whenever you create a Work Order, you will have an empty list in the `_Notes' field.

It would look something like this: 

WorkOrder(){
    _Notes = new List<Note>();
}

Upvotes: 2

Related Questions