8
\$\begingroup\$

So I started this question, I believe i have synchronized the methods correctly, need some insight on whats wrong and what should be done. I have to modify ths class Purse so that the method AddCoin is synchronized and implement a synchronized RemoveCoin method with a Coin argument, I believe that is done just fine. Where im really having trouble is implementing thread that adds pennies, another thread that adds quarters, and another thread that removes and prints randomly selected coins, pennies, quarters, any one. Then finally synchronize the threads to wait for each other when necessary. Here is what I have so far:

Purse.cs

using System;
using System.Collections;
using System.Threading;

namespace TestingProject
{
    /// <summary>
    /// A purse holds a collection of coins.
    /// </summary>
public class Purse
{
    ///  Constructs an empty purse.
    public Purse()
    {
        coins = new ArrayList();

    }

    ///   Add a coin to the purse.
    ///   @param aCoin the coin to add
    public void Add(Coin aCoin)
    {
        Monitor.Enter(coins);
        coins.Add(aCoin);
        
        Monitor.Exit(coins);
    }
    public void RemoveCoin(Coin aCoin)
    {
        Monitor.Enter(coins);
        coins.Remove(aCoin);
        
        Monitor.Exit(coins);
    }
    

    ///  Get the total value of the coins in the purse.
    ///  @return the sum of all coin values
    public double GetTotal()
    {
        double total = 0;
        for (int i = 0; i < coins.Count; i++)
        {
            Coin aCoin = (Coin)coins[i];
            total = total + aCoin.GetValue();
        }
        return total;
    }

    private ArrayList coins;
}



}

Coin.cs

using System;
using System.Collections;

namespace TestingProject
{
    /// <summary>
    /// A coin with a monetary value.
    /// </summary>
public class Coin   {

    /// Constructs a coin.
    /// @param aValue the monetary value of the coin
    /// @param aName the name of the coin
    public Coin(double aValue, String aName) 
    { 
        value = aValue; 
        name = aName;
    }

    public Coin()
    {
    }

    /// Gets the coin value.
    /// @return the value
    public double GetValue()    
    {
        return value;
    }

    /// Gets the coin name.
    /// @return the name
    public String GetName() 
    {
        return name;
    }
    
    public override bool Equals(Object otherObject)
    {
        Coin other = (Coin)otherObject;
        return name==other.name
            && value == other.value;
    }

    // C# requirement: 
    // since we override Equals, MUST also override GetHashCode ( !! )
    public override int GetHashCode()
    {
        return base.GetHashCode ();
    }

    private double value;
    private string name;
}
}

pursetest.cs

using System;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.VisualBasic;


namespace TestingProject
{


class PurseTest
{
    public static void Main(string[] args)
    {
        

        // Random object used by each thread
        Random random = new Random();

        
        Purse purse =new Purse();

        Coin coin = new Coin();

        // output column heads and initial buffer state
        Console.WriteLine("{0,-35}{1,-9}{2}\n",
           "Operation", "Buffer", "Occupied Count");


        
        Thread purseThread =
           new Thread(new ThreadStart(purse.Add))
           {
               Name = "Purse"
           };

        Thread coinThread =
           new Thread(new ThreadStart(coin.GetValue));
        coinThread.Name = "coin";

        // start each thread
        purseThread.Start();
        coinThread.Start();

    }
}
}
\$\endgroup\$
7
  • 4
    \$\begingroup\$ 1. ArrayList should only be used if it's before 2005. Read up on generics (List<Coin>). 2. Don't use Monitor.Enter/Exit manually, investigate the lock keyword. \$\endgroup\$ Aug 3, 2020 at 20:08
  • \$\begingroup\$ familiar with list<coin> but I had to use ArrayList for some reason on this program \$\endgroup\$
    – tyler j
    Aug 3, 2020 at 20:15
  • 2
    \$\begingroup\$ Is this a conversion of a Java program? The code has a distinctly Java feel to it... Anyway, are you limited to an early .Net framework? Some of the advice I'd give isn't applicable if you're stuck on an old version. \$\endgroup\$
    – RobH
    Aug 4, 2020 at 7:24
  • 1
    \$\begingroup\$ For anything regarding money or currency, you should prefer Decimal over double. double is a binary floating point and its an approximation to a value, whereas Decimal is base-10 floating point and represents exact values. This is particularly critical with decimal places. \$\endgroup\$
    – Rick Davin
    Aug 4, 2020 at 15:53
  • 1
    \$\begingroup\$ Can you provide a stronger justification as to why you believe you must use ArrayList? \$\endgroup\$
    – Rick Davin
    Aug 4, 2020 at 15:55

2 Answers 2

8
\$\begingroup\$

Taking a look at your Coin class, there are a few things that aren't quite idiomatic C#.

The most obvious thing is that you're using what looks nearly like Java Doc comments. C# uses XML doc comments so this:

/// Constructs a coin.
/// @param aValue the monetary value of the coin
/// @param aName the name of the coin

Should be:

/// <summary>Constructs a Coin.</summary>
/// <param name="aValue">The monetary value of the Coin.</param>
/// <param name="aName">The name of the Coin.</param>

In C#, we have properties which mean you don't need to write explicit GetXyz and SetXyz. I'm not sure whether you're using an old version of .Net (e.g. using ArrayList) but it's normal to have those as auto-implemented properties like this:

 public double Value { get; }
 public string Name { get; }

If you can't use auto-properties, you'll have to do a bit more typing. I'm going to assume you're stuck on an older .Net and try to avoid newer features.

private readonly double value;
public double Value 
{
    get
    {
        return value;
    }
}

That means you get rid of GetValue and GetName methods. I'd also suggest that you remove the empty constructor to force creation with a value and a name. That only leaves your Equals and GetHashcode implementations.

Let's consider a couple of test cases:

new Coin().Equals("blah"); // InvalidCastException :(
new Coin().Equals(null); // NullReferenceException :(

Oh dear, we've got a problem here! Let's fix those bugs:

public override bool Equals(Object otherObject)
{
    return Equals(otherObject as Coin);
}

public bool Equals(Coin coin)
{
    if (ReferenceEquals(null, coin))
         return false;

    return coin.Value == this.Value && coin.Name == this.Name;
}

That's nice and clear and most importantly, is correct! This is a common pattern and you can implement IEquatable at the same time if you want. I think a coin should have a name in your model so you should validate that in the constructor.

So we're up to here:

/// <summary>
/// A coin with a monetary value.
/// </summary>
public class Coin : IEquatable<Coin>
{
    private readonly double value;
    
    public double Value
    {
        get
        {
            return value;
        }
    }
    
    private string name;
    
    public string Name 
    {
        get
        {
            return name;
        }
    }

    /// <summary>Constructs a coin.</summary>
    /// <param name="value">The monetary value of the Coin</param>
    /// <param name="name">The name of the Coin</param>
    public Coin(double value, string name)
    {
        if (name == null)
            throw new ArgumentNullException("name");
        this.value = value;
        this.name = name;
    }

    public override bool Equals(Object otherObject)
    {
        return Equals(otherObject as Coin);
    }

    public bool Equals(Coin coin)
    {
        if (ReferenceEquals(null, coin))
            return false;

        return coin.Value == this.Value && coin.Name == this.Name;
    }
    
    // C# requirement: 
    // since we override Equals, MUST also override GetHashCode ( !! )
    public override int GetHashCode()
    {
        return base.GetHashCode();
    }
    
}

So, only one thing left to talk about: GetHashCode. We must override GetHashCode and implement it correctly. I'll link you to this SO post: https://stackoverflow.com/questions/371328/why-is-it-important-to-override-gethashcode-when-equals-method-is-overridden

Your GetHashCode should look like this:

public override int GetHashCode()
{
    int hash = 13;
    hash = (hash * 7) + Value.GetHashCode();
    hash = (hash * 7) + Name.GetHashCode();
    return hash;
}

Now we've got a class that correctly implements equality.

ETA:

As has been pointed out in the comments, value is a contextual keyword so you may want to think twice before calling a field value. I don't see the problem myself as syntax highlighting in VS, for example, would know it wasn't a keyword. Calling it theValue would be worse for readability in my opinion. If you're on C# 6 or later, you should prefer the auto property so you don't need the field. Either way, if you write this class well at the beginning, the field is an implementation detail and you'll never read this source code again.

\$\endgroup\$
5
  • \$\begingroup\$ value is a keyword, you cannot use it as a field name. \$\endgroup\$
    – Flater
    Aug 4, 2020 at 12:29
  • \$\begingroup\$ I can't find when it was introduced right now but more recent versions of .NET have a System.HashCode struct for generating hashcodes like this. \$\endgroup\$
    – Vivelin
    Aug 4, 2020 at 12:50
  • \$\begingroup\$ @Vivelin - I've only seen that used in .Net Core so I think it's pretty new. It was definitely after Tuple got introduced as people were using that to get a good hash. It's definitely a good one to know though! \$\endgroup\$
    – RobH
    Aug 4, 2020 at 13:06
  • \$\begingroup\$ @Flater - it's a contextual keyword like async so it's valid. Perhaps it is bad to use here (and I typed it into CR so didn't even think about it). It's the same reason you can do this: class async { public async Do(async async) => async; } \$\endgroup\$
    – RobH
    Aug 4, 2020 at 13:34
  • \$\begingroup\$ Actually seen in code at one point: float @double = (int)3.1415m; Just because you can doesn't mean you should... \$\endgroup\$ Aug 4, 2020 at 21:06
5
\$\begingroup\$

The Purse class could be rewritten in the following way:

public class Purse
{
    private ArrayList coins = new ArrayList();
    private readonly object lockObject = new object();

    public void Add(Coin aCoin)
    {
        lock (lockObject)
        {
           coins.Add(aCoin);
        }
    }
    public void RemoveCoin(Coin aCoin)
    {
        lock (lockObject)
        {
            coins.Remove(aCoin);
        }
    }

    public double GetTotal()
    {
        lock (lockObject)
        {
            return coins.Cast<Coin>().Sum(aCoin => aCoin.GetValue());
        }
    }
}

Some remarks regarding the code:

  1. As the OP stated ArrayList could not be change. So here we can't use neither generic collections nor concurrent collections.
    1.1) In order to be able to use System.Linq on an ArrayList, first we have to call the Cast<T> operator
  2. Syncronization is needed not just for the write operations but also for read operations as well.
    2.1) All shared resource access should be protected when you want to expose thread-safe methods
    2.2) You should consider to use Read Write Lock if you want to allow multiple read access at the same time
  3. You should use a dedicated lock object as the Guidlines says
    3.1) Visual Studio can warn you if you forgot to protect one of the shared resource access Missing lock statement
\$\endgroup\$
2
  • 1
    \$\begingroup\$ ad 3) that's a misreading of the guidelines. The full quote is "lock on a dedicated object instance [...] or another instance that is unlikely to be used as a lock object by unrelated parts of the code. Avoid using the same lock object instance for different shared resources" (emphasis mine). Locking a collection when you access that specific collection is consistent with this guideline, and far less confusing. And since it's a private, never-exposed variable, it is very unlikely to be used by unrelated parts of the code. \$\endgroup\$ Aug 4, 2020 at 12:34
  • \$\begingroup\$ @SebastianRedl Yes, your observation is correct. In this particular case all requirements are satisfied by the coins private field. Personally I prefer to have a separate lock object. I also try to strive to have at most one shared resource per type. \$\endgroup\$ Aug 4, 2020 at 14:03

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.