CODEWITHSUNDEEP
c#arraylistforeach
Kyle
Kyle

Reputation: 5557

ArrayList is null when there's only one item

I have a block of code that works but is, I think, a little inelegant. advertiserList is null when there's only one item. Can't C# treat it as a loop with one item? How can I clean up this code? Thank.

The inner parts of the if{} else{} both do the same things. I'm just interested in getting advertiser info.

// build the advertiser loop
var advertiserList = campaignAdvertiserContainer["Advertiser"] as ArrayList;
if (advertiserList != null) // if multiple advertisers exist per campaign
{
    foreach (Dictionary<string, object> advertiser in advertiserList)
    {
        Dictionary<string, object> multipleCampaignAdvertiserLookup = new Dictionary<string, object>();
        multipleCampaignAdvertiserLookup.Add("CampaignId", campaign["id"].ToString());
        multipleCampaignAdvertiserLookup.Add("AdvertiserId", advertiser["id"].ToString());
        multipleCampaignAdvertiserLookup.Add("MediaCode", advertiser["MediaCode"].ToString());
        multipleCampaignAdvertiserLookup.Add("BusinessKey", advertiser["BusinessKey"].ToString());
        multipleCampaignAdvertiserLookup.Add("CreatedBy", System.Reflection.Assembly.GetExecutingAssembly().FullName.ToString());
        multipleCampaignAdvertiserLookup.Add("CreatedDt", DateTime.Now.ToString());

        campaignAdvertiserLookupArray.Add(multipleCampaignAdvertiserLookup);
    }
}
// there's only one advertiser, no need to loop
else
{
    Dictionary<string, object> singleAdvertiser = (Dictionary<string, object>)campaignAdvertiserContainer["Advertiser"];
    Dictionary<string, object> singleCampaignAdvertiserLookup = new Dictionary<string, object>();
    singleCampaignAdvertiserLookup.Add("CampaignId", campaign["id"].ToString());
    singleCampaignAdvertiserLookup.Add("AdvertiserId", singleAdvertiser["id"].ToString());
    singleCampaignAdvertiserLookup.Add("MediaCode", singleAdvertiser["MediaCode"].ToString());
    singleCampaignAdvertiserLookup.Add("BusinessKey", singleAdvertiser["BusinessKey"].ToString());
    singleCampaignAdvertiserLookup.Add("CreatedBy", System.Reflection.Assembly.GetExecutingAssembly().FullName.ToString());
    singleCampaignAdvertiserLookup.Add("CreatedDt", DateTime.Now.ToString());

    campaignAdvertiserLookupArray.Add(singleCampaignAdvertiserLookup);
}

Upvotes: 0

Views: 101

Answers (3)

Sergey Kalinichenko
Sergey Kalinichenko

Reputation: 726779

The problem is with the code that is calling your method: it passes a single item differently from passing multiple items.

The preferred solution would be to change that, and pass an array list with a single item rather than passing a "naked" item when there's only one. If it is not an option, change your code to make an array list yourself, like this:

var advertiserList = campaignAdvertiserContainer["Advertiser"] as ArrayList;
if (advertiserList == null) {
    advertiserList = new ArrayList {
         campaignAdvertiserContainer["Advertiser"]
    };
}

At this point, advertiserList is what the positive branch of your if statement expects. You can replace both branches of the if with its first branch (i.e. the one containing the for loop).

Upvotes: 3

Servy
Servy

Reputation: 203836

If you had a collection with one item in it, then yes, you could just use a foreach loop no matter what, but that's not what you have. You have the actual item stuck into that container rather than a collection of size one.

You should modify whatever code puts a value into this object so that it always puts in a collection. If there are no items it should put in an empty collection, if there is one item it should put in a collection with one item, if there is more than one item it should put in all of those items. If you do that you can always just foreach over the result, without needing so much as a null check. Doing this is to write programs that are validated at compile time due to the C# type system. Not only are they easier to write, but you know that so long as it compiles, it'll work.

You should also avoid using ArrayList, instead you should use the generic List<T>. Much like avoiding using an object variable to hold onto either an item or a list of items, you should always strive to have statically constrain your types to what you need them to be.

Upvotes: 3

John Saunders
John Saunders

Reputation: 161781

First, this has nothing to do with how C# treats things. It's the code.

campaignAdvertiserContainer["Advertiser"] is apparently a single item. The code that put that there should put an ArrayList of the single item into there.

Second, ArrayList is obsolete. Instead, the code should use List<Dictionary<string,object>>.

Upvotes: 2

Related Questions