CardManager class and a data access layer for it

Question

Ok, I am working with 2 layers of class: somekind of a "manager" class which task is to take an object, link all items attached to it, and return it to the caller, and a "data" class which task is to call the database on a specific request.

Here's an example of how I work. Here's a "manager" class:

public class CardManager
{
    private static readonly CardData mCardDAL = new CardData();

    public List<CardDisplay> ListCardsToShow(int _pageNumber, string _queryString, string _rarity, string _type, string _color, out int _totalCount)
    {
        List<CardDisplay> listToReturn = mCardDAL.ListCardsToShow(_pageNumber, _queryString, _rarity, _type, _color, out _totalCount);

        LinkListCardDisplayData(listToReturn);

        return listToReturn;
    }


    /// <summary>
    /// Method links the card set with each cards of the list.
    /// </summary>
    /// <param name="_listToReturn"></param>
    private static void LinkListCardDisplayData(IEnumerable<CardDisplay> _listToReturn)
    {
        try
        {
            foreach (CardDisplay item in _listToReturn)
            {
                LinkCardDisplayData(item);
            }
        }
        catch (Exception ex)
        {
            throw new Exception(ex.Message);
        }
    }

    private static void LinkCardDisplayData(CardDisplay _item)
    {
        _item.mMasterCardID = _item.mMasterCard.mCardID;

        ImagesManager.GetCardImages(_item);

        if (_item.mChildCard != null)
        {
            _item.mChildCardID = _item.mChildCard.mCardID;
        }
    }
}

And here's a "data" class, namely the CardData class in this occurrence:

public class CardData
{
    internal List<CardDisplay> ListCardsToShow(int _pageNumber, string _queryString, string _rarity, string _type, string _color, out int _totalCount)
    {
        using (DatabaseEntity db = new DatabaseEntity())
        {
            db.Database.Connection.Open();

            List<CARD> cardData;

            List<CardInfo> listCards;

            if (!String.IsNullOrWhiteSpace(_queryString))
            {
                var predicate = GetCardPredicate(_queryString);

                if (_rarity != "All")
                {
                    predicate = predicate.And(_item => _item.CARD_RARTY == _rarity);
                }

                if (_color != "All")
                {
                    predicate = predicate.And(
                            _item => _item.CARD_MANA_COST.Contains(_color) || _item.CARD_COLOR.Contains(_color));
                }

                if (_type != "All")
                {
                    predicate = predicate.And(_item => _item.CARD_TYPE.Contains(_type));
                }

                var cardQry = from c in db.CARD.AsExpandable().Where(predicate)
                              select c;

                _totalCount = cardQry.Count();

                int pageCount = _pageNumber - 1;

                cardData = cardQry.OrderBy(_x => _x.CARD_IDE).Skip(pageCount * 20).Take(20).ToList();

                for (int i = 0; i < cardData.Count; i++)
                {
                    CARD card = cardData[i];

                    if (cardData.Any(_item => _item.CARD_MASTER_IDE == card.CARD_IDE))
                    {
                        cardData.Remove(card);
                    }
                }

                listCards = DataConverter.ListCardDATAToListCardInfo(cardData);
            }
            else
            {
                // If we are here then the user browsed to get the 300 latest entries available.

                Expression<Func<CARD, bool>> cardPredicate = PredicateBuilder.True<CARD>();

                if (_rarity != "All")
                {
                    cardPredicate = cardPredicate.And(_item => _item.CARD_RARTY == _rarity);
                }

                if (_type != "All")
                {
                    cardPredicate = cardPredicate.And(_item => _item.CARD_TYPE.Contains(_type));
                }

                if (_color != "All")
                {
                    cardPredicate =
                        cardPredicate.And(
                            _item => _item.CARD_MANA_COST.Contains(_color) || _item.CARD_COLOR.Contains(_color));
                }

                var cardQry = (from c in db.CARD.AsExpandable().Where(_item => !_item.CARD_NAME.Contains("(Foil)"))
                                select c).OrderByDescending(_x => _x.CARD_SET.CARD_SET_RELES_DATE).Take(300);

                cardQry = cardQry.Where(cardPredicate);

                _totalCount = cardQry.Count();

                int pageCount = _pageNumber - 1;

                cardData = cardQry.Skip(pageCount * 20).Take(20).ToList();

                for (int i = 0; i < cardData.Count; i++)
                {
                    CARD card = cardData[i];

                    if (cardData.Any(_item => _item.CARD_MASTER_IDE == card.CARD_IDE))
                    {
                        cardData.Remove(card);
                    }
                }

                listCards = DataConverter.ListCardDATAToListCardInfo(cardData);
            }

            List<CardDisplay> listToReturn = MakeListCardDisplay(listCards);

            return listToReturn;
        }
    }
}

My question is not on "how" I do my code (but feel free to make a constructive comment as I love to learn), but rather on how it is structured. I'd like, for example, that a CardData class be exclusively instantiated by a "manager" class, meaning that I could not, for example, create a public static readonly CardData mCardDAL = new CardData(); object in a controller.

Is there a way to isolate my class in such a manner that anyone is "forced" to pass through the manager to get the object? And I am working in a good way, is my code good?

Answer 1

You could use a private nested class:

public class CardManager
{
    public CardManager()
    {
        var card = new CardData();
    }

    private class CardData
    {
    }
}

CardData will not be able to be instantiated outside CardManager. For example, this will not compile

public class Other
{
    public Other()
    {
        CardData card = new CardData();
    }
}

---

I don't understand why CardData is a class. It's got one method, no fields, no properties. Its one method, at 103 lines, is almost certainly doing too much and should be split up into smaller methods.

---

Parameter names should not start with an underscore, by convention.

---

    try
    {
        foreach (CardDisplay item in _listToReturn)
        {
            LinkCardDisplayData(item);
        }
    }
    catch (Exception ex)
    {
        throw new Exception(ex.Message);
    }

This has lost the stack trace. The try-catch should be removed entirely.

Answer 2

I don't really see a reason why CardData should be a separate class here. In fact, your requirement (“that a CardData class be exclusively instantiated by a "manager" class”) makes it sound like the single method from CardData should be part of CardManager. Another option would be to make CardData a private class nested inside CardManager.

---

Your classes don't have any instance state at all. Because of that, I think they should be made into static classes.

---

public List<CardDisplay> ListCardsToShow(int _pageNumber, string _queryString, string _rarity, string _type, string _color, out int _totalCount)

I've seen underscores used for private fields, but never for parameters. You shouldn't use this convention, since it's not consistent with the rest of the .Net world.

---

catch (Exception ex)
{
    throw new Exception(ex.Message);
}

The only thing this code does is to remove information about the cause of the exception, making debugging harder. If you want to throw some specific exception type, pass the original exception as innerException. This way, you only add information, not remove it:

throw new CardManagerException("Could not link card display data.", ex);

---

ImagesManager.GetCardImages(_item);

GetX methods should return X. Depending on what exactly does this method do, something like PopulateCardImages might be a better name.

---

if (_item.mChildCard != null)
{
    _item.mChildCardID = _item.mChildCard.mCardID;
}

This looks like mChildCardID should be a property that returns _item.mChildCard.mCardID. Or maybe it shouldn't exist at all, I don't see what value does it add.

Also, the m prefix is sometimes used to indicate fields and those should never be public. If that's the case, change those fields to properties and drop the m prefix.

---

using (DatabaseEntity db = new DatabaseEntity())

The name DatabaseEntity makes it sound like it's, well, an entity stored in some database (a single row in a table, or something like that), not some object used to access the whole DB.

---

if (_rarity != "All")
{
    predicate = predicate.And(_item => _item.CARD_RARTY == _rarity);
}

if (_color != "All")
{
    predicate = predicate.And(
            _item => _item.CARD_MANA_COST.Contains(_color) || _item.CARD_COLOR.Contains(_color));
}

if (_type != "All")
{
    predicate = predicate.And(_item => _item.CARD_TYPE.Contains(_type));
}

This code seems to be repeated in both branches of the condition. You should refactor it into a separate method, so that you Don't Repeat Yourself.

Other parts of those branches also seem very similar, you might be able to extract those too.

---

var cardQry = from c in db.CARD.AsExpandable().Where(predicate)
              select c;

I don't see any reason to use that query syntax here, the following code should behave the same:

var cardQry = db.CARD.AsExpandable().Where(predicate);

The same applies to cardQry in the other branch.

Also, programming is not about typing the least number of characters, so you should spell words in full (e.g. cardQuery instead of cardQry) to make it clearer.

Answer 3

A couple of things that I haven't seen others mention.

One is that access modifiers are not security. You should want to control who creates your object, not because you don't want someone creating it when they shouldn't, but because they shouldn't HAVE to know how to create your object -- whether the "someone" is a fellow developer, a class that you are writing or even you while working on another class, that knowledge is a burden you don't want them to bear.

Secondly, as others have mentioned, the way to do this is to create a nested inner class. What I haven't seen others mention, perhaps thinking it too basic, is how do you let others interact with your inner class -- and the answer is to create an interface.

Answer 4

One thing I don't think anybody's spotted yet is this:

for (int i = 0; i < cardData.Count; i++)
{
    CARD card = cardData[i];

    if (cardData.Any(_item => _item.CARD_MASTER_IDE == card.CARD_IDE))
    {
        cardData.Remove(card);
    }
}

Could be swapped for:

cardData.RemoveAll(card=> cardData.Any(card2=> card.CARD_IDE == card2.CARD_MASTER_IDE));

And finally I see no reason why cardData is a List and not an IEnumerable. Then you wouldn't need so many calls to ToList(), which wastefully enumerates the entire query.