2
\$\begingroup\$

In this class, the order of the last 3 statements in the constructor is important. If you call OrderedCards = OrderCards(cards, wildcardsCount); last, it will crash with NRE.

Is this a bad idea? If so, how should I fix it?

/// <summary>
/// The abstract result of analysis describing the combination of cards
/// </summary>
public abstract class TichuTrick
{

    public List<CardData> OrderedCards { get; private set; }
    public readonly int CardCount;
    public readonly int TrickValue;
    public readonly int HighestCardValue;
    public readonly int WildcardsCount;

    /// <summary>
    /// Constructs a response of analysis
    /// </summary>
    /// <param name="cards">This class works only with 1 wildcard in each trick.</param>
    internal TichuTrick(List<CardData> cards, int cardCount, int wildcardsCount)
    {
        if (wildcardsCount > 1)
        {
            throw new System.ArgumentException("More than one wildcards support, is not yet implemented", nameof(wildcardsCount));
        }
        CardCount = cardCount;
        OrderedCards = OrderCards(cards, wildcardsCount);
        TrickValue = CalculateTrickValue();
        HighestCardValue = CalculateHighestCardValue();
    }

    protected virtual List<CardData> OrderCards(List<CardData> cards, int WildcardCount)
    {
        return cards.OrderBy(x => x.NumericalValue).ToList().CopyList();
    }

    protected virtual int CalculateTrickValue()
    {
        return CalculateHighestCardValue();
    }

    protected virtual int CalculateHighestCardValue()
    {
        return OrderedCards.Last().NumericalValue;
    }

}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Everything is in specific order in a program .. \$\endgroup\$
    – Denis
    Jan 11 '17 at 21:26
  • \$\begingroup\$ @denis I agree, but this way, I'm hiding a dependency between functions. CalculateHighestCardValue assumes that OrderCards has already been called. \$\endgroup\$ Jan 11 '17 at 21:28
  • \$\begingroup\$ How to solve the order problem? Simple: pass the required data via arguments: CalculateHighestCardValue(IEnumerable<CardData> cards). Calling virtual methods from a constructor is a bit troublesome: in a child class that overrides them, the child implementations will be called - which will happen before the child constructor is executed, because the base constructor is executed first. \$\endgroup\$ Jan 12 '17 at 19:00
4
\$\begingroup\$

The order is not the problem here but rather the fact that you have multiple virtual calls in your constructor. This is really bad because in your derived class, when they are being instantiated, your constructor will first call the base implementation of the virtual functions your base class constructor, which might result in unexpected behaviour form t he virtual functions.

Also you shouldn't be using fields, use properties instead.

Here is how you can avoid all of your current issues :

public abstract class TichuTrick
{
    private readonly List<CardData> _cards;
    public List<CardData> OrderedCards => OrderCards(_cards, WildcardsCount);

    public int CardCount { get; }

    public int TrickValue => CalculateTrickValue();
    public int HighestCardValue => CalculateHighestCardValue();
    public int WildcardsCount { get; }

    internal TichuTrick(List<CardData> cards, int cardCount, int wildcardsCount)
    {
        if (wildcardsCount > 1)
        {
            throw new System.ArgumentException("More than one wildcards support, is not yet implemented", nameof(wildcardsCount));
        }
        CardCount = cardCount;
        _cards = cards;
        WildcardsCount = wildcardsCount;
    }

    protected virtual List<CardData> OrderCards(List<CardData> cards, int WildcardCount)
    {
        return cards.OrderBy(x => x.NumericalValue).ToList().CopyList();
    }

    protected virtual int CalculateTrickValue()
    {
        return CalculateHighestCardValue();
    }

    protected virtual int CalculateHighestCardValue()
    {
        return OrderedCards.Last().NumericalValue;
    }
}

I also don't think this parameter should be a parameter at all cardCount you can access the card count from the array, but since I'm not 100% sure that this is correct I left it there.

\$\endgroup\$
2
  • \$\begingroup\$ In C#, unlike C++, the derived methods will be called, not the base methods. The problem is that at that point the derived constructor has not been executed yet. \$\endgroup\$ Jan 12 '17 at 19:13
  • \$\begingroup\$ Yes you're correct, the base constructor will be executed first. \$\endgroup\$
    – Denis
    Jan 12 '17 at 19:56
3
\$\begingroup\$
protected virtual List<CardData> OrderCards(List<CardData> cards, int WildcardCount)
{
    return cards.OrderBy(x => x.NumericalValue).ToList().CopyList();
}

This method should be a static one because it doesn't require any instance data to work and much less protected. Are you really going to override it?


protected virtual int CalculateTrickValue()
{
    return CalculateHighestCardValue();
}

protected virtual int CalculateHighestCardValue()
{
    return OrderedCards.Last().NumericalValue;
}

and

TrickValue = CalculateTrickValue();
HighestCardValue = CalculateHighestCardValue();

Why would you need two methods doing the same thing but under different names?

The interesting thing about them is that if you override the CalculateHighestCardValue and it calls the base.CalculateTrickValue then you'll have an infinite loop and the StackOverflowException will be thrown.


return cards.OrderBy(x => x.NumericalValue).ToList().CopyList();

You don't need to copy the list again. You've just created a new one.

\$\endgroup\$
2
\$\begingroup\$

Way too many dependencies

Trick and Highest are the same

You don't even use WildcardCount in OrderedCards

Why not just return Max rather than OrderedCards.Last

My take is you are getting tricky for the sake of getting tricky

public abstract class TichuTrick
{ 
    public int CardCount        { get; private set; }
    public int WildcardsCount   { get; private set; }
    public List<CardData> Cards { get; private set; }
    protected virtual IEnumerable<CardData> OrderedCards 
    {
        get 
        {
            return Cards.OrderBy(x => x.NumericalValue);
        }
    }

    protected virtual int CalculateTrickValue()
    {
        return CalculateHighestCardValue();
    }

    protected virtual int CalculateHighestCardValue()
    {
        return OrderedCards.Last().NumericalValue;
        // or just use Max
    }
    /// <summary>
    /// Constructs a response of analysis
    /// </summary>
    /// <param name="cards">This class works only with 1 wildcard in each trick.</param>
    internal TichuTrick(List<CardData> cards, int cardCount, int wildcardsCount)
    {
        if (wildcardsCount > 1)
        {
            throw new System.ArgumentException("More than one wildcards support, is not yet implemented", nameof(wildcardsCount));
        }
        CardCount = cardCount;
        WildcardsCount = wildcardsCount;
        Cards = cards;
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ I can't use properties, because of Unity's Editor :P. I wanted OrderedCards.Last() because the cards are already ordered, but Max() seems more intuitive! \$\endgroup\$ Nov 8 '18 at 14:01

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.