5
\$\begingroup\$

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?

\$\endgroup\$
2
  • \$\begingroup\$ I feel like any review I could give would be a bit incomplete because we can't see the CardDisplay class. For example, I don't know why it'd be necessary to do this: _item.mMasterCardID = _item.mMasterCard.mCardID; \$\endgroup\$ Oct 9, 2014 at 2:25
  • \$\begingroup\$ Ben: it's because it's a class I construct with many other pieces and objects, and sometimes, I forget to set the mastercardid property, so that's why :) \$\endgroup\$
    – hsim
    Oct 9, 2014 at 12:12

4 Answers 4

7
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ CardData has many other methods, properties, fields, etc. The example just show a bit of my code so that you mostly understand the structure. \$\endgroup\$
    – hsim
    Oct 9, 2014 at 14:43
5
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you very much. To answer: yes, the DatabaseEntity is my object giving access to the whole database: the sample of code make it looks badly simply because the sample is too small to be complete. I used parts of both classes to show mostly the structure and understand how to do better code. \$\endgroup\$
    – hsim
    Oct 9, 2014 at 14:47
  • \$\begingroup\$ As for the exception, well, I'd be curious to see examples of those. I think that you are suggesting to create a custom exception manager depending on the location of the errors, right? \$\endgroup\$
    – hsim
    Oct 9, 2014 at 14:53
  • \$\begingroup\$ @HerveS No, I'm not suggesting you should handle exceptions based on where they were thrown from. I'm saying that knowing where they were thrown from can be very helpful for debugging exceptions that indicate bugs in your code ("boneheaded exceptions" in Eric Lippert's classification). \$\endgroup\$
    – svick
    Oct 9, 2014 at 15:06
  • 1
    \$\begingroup\$ @HerveS And if you show us only parts of your classes, then it's hard to correctly suggest how should they be actually structured. \$\endgroup\$
    – svick
    Oct 9, 2014 at 15:10
  • \$\begingroup\$ Yeah, I understand, but the parts I've shown and the comments / answers I have received so far have already greatly helped me and did exactly what was required. \$\endgroup\$
    – hsim
    Oct 9, 2014 at 15:11
3
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ I'd love to see and example of that. I know that interfaces exists, but I have never used one before. \$\endgroup\$
    – hsim
    Oct 9, 2014 at 15:13
  • \$\begingroup\$ Not a whole lot to it, but here's an example: ideone.com/jPMDnb \$\endgroup\$
    – jmoreno
    Oct 9, 2014 at 20:20
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the Linq line, I'll adopt it. As for your other comments, this is because I've learned to work with list before using IEnumerable (yes indeed!), so that's why. :) \$\endgroup\$
    – hsim
    Oct 9, 2014 at 14:16
  • \$\begingroup\$ Then I would definitely recommend swapping them other. LINQ queries don't actually execute until you enumerate them, which means calls to First() or Any() etc only cause a subset of queries to be executed. Calling ToList() wastefully executes every query. In this case your for loop / a RemoveAll would cause the query to be fully enumerated anyway, so it's unlikely you'd get a performance boost, but it's a good habit to develop. \$\endgroup\$
    – Nick Udell
    Oct 9, 2014 at 14:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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