2
\$\begingroup\$

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>() }           
    };
}
\$\endgroup\$
  • 2
    \$\begingroup\$ where is the aditem class? you haven't pasted it in? \$\endgroup\$ – BKSpurgeon Jun 7 '17 at 12:40
  • \$\begingroup\$ The downvote is for the missing AdItem. \$\endgroup\$ – t3chb0t Jun 9 '17 at 4:16
2
\$\begingroup\$

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() }
    };
}
\$\endgroup\$
  • 1
    \$\begingroup\$ However, we should ask ourselves if it is really necessary? For me it is even less readable than it was before and adds more code to be tested. \$\endgroup\$ – MaLiN2223 Jun 5 '17 at 19:13
  • 1
    \$\begingroup\$ I downvoted it for the IntList and its public new void Add(int value). Why do you need it? \$\endgroup\$ – t3chb0t Jun 9 '17 at 4:15
  • 1
    \$\begingroup\$ You're right. It sucks. Eliminating. \$\endgroup\$ – Jesse C. Slicer Jun 11 '17 at 4:06
  • \$\begingroup\$ DV suck too ;-] undone. \$\endgroup\$ – t3chb0t Jun 11 '17 at 11:08
1
\$\begingroup\$

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?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.