Refactoring fixed static adding items to new List

Question

I am not sure how to approach this it fixed item adding to the list of object. I have the slot of ads, that have to be prepulated so they could be selected based on front end parameter. So basically few of these ads can be picked on the website and be positioned. How could this list be simplified, because now it is block of repeated code.

private static List<AdItem> GetAdSlotItems()
{
    var s = "";
    return new List<AdItem>()
    {
        new AdItem { Type = AdType.LeftSky, Container = "div-ad-skyscraper-l", Sizes = new List<List<int>> { new List<int>() { 120, 600 }, new List<int>() { 160, 600 } }, Position = new List<Position>() { new Position() { Id = AdType.LeftSky.ToString(), Value = AdPosition.left.ToString() } } },
        new AdItem { Type = AdType.RightSky, Container = "div-ad-skyscraper-r", Sizes = new List<List<int>> { new List<int>() { 120, 600 }, new List<int>() { 160, 600 } }, Position = new List<Position>() { new Position() { Id = AdType.RightSky.ToString(), Value = AdPosition.right.ToString() } } },
        new AdItem { Type = AdType.LeaderBoard, Container = "div-ad-leaderboard", Sizes = new List<List<int>> { new List<int>() { 300, 250 } }, Position = new List<Position>() { new Position() { Id = AdPosition.inline.ToString(), Value = AdPosition.inline.ToString() } } },
        new AdItem { Type = AdType.BillBoard, Container = "div-ad-billboard", Sizes = new List<List<int>> { new List<int>() { 728, 90 }, new List<int>() {970, 250 }, new List<int>() { 300, 50 } }, Position = new List<Position>() },
        new AdItem { Type = AdType.MPU1, Container = "div-ad-rectangle-e1", Sizes =  new List<List<int>> { new List<int>() {300, 250 }, new List<int>() { 300, 600 } }, Position = new List<Position>() { new Position() { Id = AdType.MPU1.ToString(), Value = AdPosition.mpu1.ToString() } } },
        new AdItem { Type = AdType.MPU1, Container = "div-ad-rectangle-e2", Sizes =  new List<List<int>> { new List<int>() {300, 250 }, new List<int>() { 300, 600 } }, Position = new List<Position>() { new Position() { Id = AdType.MPU2.ToString(), Value = AdPosition.mpu2.ToString() } } },
        new AdItem { Type = AdType.MPU1, Container = "div-ad-rectangle-e3", Sizes =  new List<List<int>> { new List<int>() {300, 250 }, new List<int>() { 300, 600 } }, Position = new List<Position>() { new Position() { Id = AdType.MPU3.ToString(), Value = AdPosition.mpu3.ToString() } } },
        new AdItem { Type = AdType.WallPaper, Container = "div-ad-wallpaper", Sizes = new List<List<int>> { new List<int>() { 1, 1 } }, Position = new List<Position>() }           
    };
}

Answer 1

Well, we can cut the boilerplate down a little bit with some helpers and changes to your original classes like this:

using System.Collections.Generic;

using static AdPosition;
using static AdType;

public class AdItem
{
    public AdType Type { get; set; }

    public string Container { get; set; }

    public SizeList Sizes { get; set; }

    public PositionList Position { get; set; }
}

public sealed class AdItemList : List<AdItem>
{
    public void Add(AdType type, string container, SizeList sizes, PositionList position)
    {
        this.Add(new AdItem { Type = type, Container = container, Sizes = sizes, Position = position });
    }
}

public sealed class SizeList : List<List<int>>
{
    public void Add(params int[] values)
    {
        var intList = new List<int>(values.Length);

        foreach (var value in values)
        {
            intList.Add(value);
        }

        this.Add(intList);
    }
}

public sealed class PositionList : List<Position>
{
    public void Add(string id, string value)
    {
        this.Add(new Position { Id = id, Value = value });
    }
}

then your method shrinks up like so:

private static AdItemList GetAdSlotItems()
{
    var s = "";
    return new AdItemList
    {
         { LeftSky, "div-ad-skyscraper-l", new SizeList { { 120, 600 }, { 160, 600 } }, new PositionList { { LeftSky.ToString(), left.ToString() } } },
         { RightSky, "div-ad-skyscraper-r", new SizeList { { 120, 600 }, { 160, 600 } }, new PositionList { { RightSky.ToString(), right.ToString() } } },
         { LeaderBoard, "div-ad-leaderboard", new SizeList { { 300, 250 } }, new PositionList { { inline.ToString(), inline.ToString() } } },
         { BillBoard, "div-ad-billboard", new SizeList { { 728, 90 }, { 970, 250 }, { 300, 50 } }, new PositionList() },
         { MPU1, "div-ad-rectangle-e1", new SizeList { { 300, 250 }, { 300, 600 } }, new PositionList { { MPU1.ToString(),mpu1.ToString() } } },
         { MPU1, "div-ad-rectangle-e2", new SizeList { { 300, 250 }, { 300, 600 } }, new PositionList { { MPU2.ToString(),mpu2.ToString() } } },
         { MPU1, "div-ad-rectangle-e3", new SizeList { { 300, 250 }, { 300, 600 } }, new PositionList { { MPU3.ToString(),mpu3.ToString() } } },
         { WallPaper, "div-ad-wallpaper", new SizeList { { 1, 1 } }, new PositionList() }
    };
}

Answer 2

The least you can do is press Enter a couple of times :) Compare this:

new AdItem { Type = AdType.LeftSky, Container = "div-ad-skyscraper-l", Sizes = new List<List<int>> { new List<int>() { 120, 600 }, new List<int>() { 160, 600 } }, Position = new List<Position>() { new Position() { Id = AdType.LeftSky.ToString(), Value = AdPosition.left.ToString() } } },

to this:

new AdItem 
{ 
    Type = AdType.LeftSky, 
    Container = "div-ad-skyscraper-l",
    Sizes = new List<List<int>> 
    { 
        new List<int>() { 120, 600 }, 
        new List<int>() { 160, 600 } 
    }, 
    Position = new List<Position>() //should be "Positions", plural
    {
        new Position() { Id = AdType.LeftSky.ToString(), Value = AdPosition.left.ToString() } 
    } 
},

I think it is obvious, that second option looks way better.

---

Another thing to consider is whether you really want to hard-code those values. Maybe it is better to move those settings to some xml/json configuration file and parse/deserialize it instead?