Basic Auction Software

Question

I'm currently working on doing some fundamental practice around Objects after spending years coding in C# but in a very procedural way, I'm also starting to try and learn SOLID. My main concerns on my code are the validation rules inside the Auction class and throwing so many exceptions as it feels wrong, I feel like I should be implementing some sort of List<ValidationResults> or even a PlaceBidResult class but then that feels like I'm not encapsulating the logic behind placing a bid in the right class?

Auction Class - This is basically the brain of the whole system and allows for placing and removing bids, setting reserve prices etc.

    public class Auction
    {
        public string ItemName { get; private set; }
        public double ReservePrice { get; private set; } = 0;

        public DateTime StartDate { get; private set; }
        public DateTime EndDate { get; set; }

        List<Bid> bids;
        public IEnumerable<Bid> Bids
        {
            get
            {
                return bids.AsEnumerable();
            }
        }


        public int CurrentHighestBid
        {
            get
            {
                return bids.Max(c => (int?)c.Value) ?? 0;

            }

        }
        private const int REPUTATION_REQUIRED_TO_BID = 10;
        private const double MAXIMUM_ALLOWED_DURATION = 10;

        public Auction(string itemName, DateTime startDate, DateTime endDate)
        {
            if (string.IsNullOrEmpty(itemName))
                throw new NullReferenceException("Must provide a valid item name");

            this.ItemName = itemName;

            if (startDate < DateTime.Now.AddMinutes(-10)) //Gives a ten minute grace to prevent this failing if DateTime.Now is passed in.
                throw new ArgumentOutOfRangeException(nameof(startDate), startDate, "Auctions can't start in the past");

            this.StartDate = startDate;

            if (endDate < DateTime.Now || endDate > StartDate.AddDays(MAXIMUM_ALLOWED_DURATION))
            {
                throw new ArgumentOutOfRangeException(nameof(endDate), endDate, $"Auctions can't end in the past and the duration of the auction must be less than {MAXIMUM_ALLOWED_DURATION} days");
            }
            this.EndDate = endDate;


            bids = new List<Bid>();


        }

        public void PlaceBid(double value, User user)
        {
            //Check that the bid is valid. I.e are their bids that are higher than us already
            ValidateBid(value, user);
            bids.Add(new Bid(value, user));

            Console.WriteLine("Bid Added " + value);

        }

        private void ValidateBid(double value, User user)
        {
            if (CurrentHighestBid > value)
                throw new ArgumentException("There are already higher bids than this", nameof(value));

            if (value < 0)
                throw new ArgumentException("You cannot bid negative amounts", nameof(value));

            if (value < ReservePrice)
                throw new ArithmeticException("Bid value cannot be lower than the reserve price of an item");

            if (StartDate > DateTime.Now)
                throw new ArgumentException("Auction hasn't started yet!");

            if (user.Reputation < REPUTATION_REQUIRED_TO_BID) 
                throw new ArgumentException("User does not have enough reputation to bid on these", nameof(user.Reputation)); //Is throwing an exception here the correct way to go?
        }

        public void RemoveBid(double value, User user)
        {
            var bid = bids.Where(c => c.Value == value).FirstOrDefault();           

            if (bid != null)
                bids.Remove(bid);           
        }

        public void SetReservePrice(double value)
        {
            if (value < 0)
                throw new ArithmeticException("Reserve price cannot be below zero");

            this.ReservePrice = value;
        }       


    }
}

Bid Class - right now this is basically a data bag or "anemic domain model" could this be improved with some kind of logic of its own?

    public class Bid
{
    public double Value { get; private set; }
    public User User { get; private set; }

    public Bid(double value, User user)
    {
        this.Value = value;
        this.User = user;
    }
}

Answer 1

You problem is very open ended and you can implement in many ways so unfortunately my answer will be rather open ended too. To start, a few general remarks:

• I normally use string.IsNullOrWhitespace(blah) instead of string.IsNullOrEmpty(blah)
• Don't throw NullReferenceException. Throw ArgumentNullException(nameof(argumentName)). That's what it's there for.
• C# typically uses PascalCase for constant names: ReputationRequiredToBid.
• ArithmeticException - you're not supposed to use this class as per documentation.
• While I don't agree with your (over)use of exceptions, you should use InvalidOperationException instead of ArgumentExceptions when the current state breaks the code, and not the otherwise valid and well formed argument(s). For example a negative value should throw an ArgumentOutOfRangeException or simply an ArgumentException, however CurrentHighestBid > value should throw an InvalidOperationException or a custom exception (I don't like custom exceptions though - keep reading for alternatives).

Anyway you're definitely overusing exceptions in my opinion. Consider creating a separate validation class and moving the validation code there - single responsibility principle. Consider returning a ValidationResult object with details about what went wrong, potentially in a IEnumerable<string> Errors property. I would probably still throw ArgumentExceptions for nonsensical arguments, like negative bid.

If I need to throw an exception, in 99% of the cases it's a variation of ArgumentException, and occasionally InvalidOperationException. The end result is that the code I write uses them quite rarely, and in predictable places, such as the entry point of the method etc. If I review a piece of code that throws any other exceptions, I'm expecting to see or hear a very strong reason for that. And an even stronger reason if the exception is a custom exception. In short, especially if you don't have experience, stay away from doing anything crazy with exceptions. They're not bad per se, but there are more elegant and readable solutions out there.

Bid class - parameter validation maybe?

Answer 2

You don't check for StartDate > EndDate:

This is not caught:

      Auction auction = new Auction("AAA", new DateTime(2019, 3, 2), new DateTime(2019, 3, 1));

---

public int CurrentHighestBid
{
  get
  {
    return bids.Max(c => (int?)c.Value) ?? 0;
  }
}

Instead of return bids.Max(c => (int?)c.Value) ?? 0;, you can write return bids.Max(c => (int?)c.Value).GetValueOrDefault();. But why do you cast to int?

---

In ValidateBid(double value, User user) you check the start date but you don't check the end date.

---

In

public void RemoveBid(double value, User user)
{
  var bid = bids.Where(c => c.Value == value).FirstOrDefault();       
  if (bid != null)
    bids.Remove(bid);
}

you're not using the user argument to anything. I would expect the condition to be something like (you can skip the Where-call):

var bid = bids.FirstOrDefault(b => b.Value == value && b.User == user);

Futher: Remove(...) returns a boolean value to indicate if something was removed or not. You could return that value from RemoveBid() to let the client know...:

public bool RemoveBid(double value, User user)
{
  var bid = bids.Where(c => c.Value == value).FirstOrDefault();
  if (bid != null)
    return bids.Remove(bid);
  return false;
}

---

According to the use of exceptions and validation of input, I think you have to distinguish between input/states that hinders a normal or valid execution of the program, for instance a null reference or a StartDate > EndDate, and violations of business rules that don't cause the program to stop working, but make invalid or unwanted results - like value < ReservePrice. The first type should be thrown as exceptions while the latter should be handled with useful return values and/or messages. Ideally seen.

But it can be difficult to determine the difference (for instance: is StartDate > EndDate a business rule or an exception?), so it is often seen, that exceptions is used in both cases. What ever you choose do it consistently and document which exceptions are thrown where and why. Never fail to throw an exception, just because you feel you have too many :-). If you think you have to throw too many exception, you should maybe consider if the overall design is right.

Answer 3

Couple of other thoughts on this:

ValidateBid is probably making some checks which it shouldn't be making. The top 3 conditions are all related to the validation of a bid, but the latter two are not. I would be inclined to move the "auction not started yet" check and "User reputation" check elsewhere. It seems like they should come before the bid validation just thinking through it procedurally.

• If the auction isn't ongoing then nobody can bid.
• If the auction is ongoing but the user hasn't got enough reputation they can't bid.
• If auction is ongoing, the user is good, but their bid is too low they can't bid.

Seems like the natural flow of checks.

The remaining three conditions are all variations on "bid is below minimum" so you could just cut out the very specific error messages and include one which states "bid is below minimum value: $x"

Your validate bid method then looks something like

private void ValidateBid(double bid)
{
    var minimumBid = new int[] {(CurrentHighestBid + 0.01), ReservePrice}.Max();
    if (bid < minimumBid)
        throw new ArgumentException($"Bid is below minimum value ${minimumBid}");
}

Also note that in the question, you're allowing a user to submit a bid which is the same value as the current highest bid. You're also allowing a user to bid 0 as long as there's no reserve and no bids higher than 0. Might be desired behaviour but I guess probably not.

Last thing - I'd recommend some unit tests for this class. There are a lot of different reasons you may not accept a user's bid and it's gonna be easy for some to slip through the net as you refactor.