7
\$\begingroup\$

Recently, as part of an interview, I was given a (seemingly) simple code prompt: To simulate a Bank with 5 customers and one teller, allowing them to deposit to and withdraw from their accounts, but with each customer on its own thread while only allowing one to access the teller at any given time.

I've a CompSci degree, VERY limited experience with multi-threading and fairly new to C#. I would just like someone to critique my code a bit. I got some general feedback but would like some more specific insight.

I was told I:

  1. showed incomplete understanding of spawning threads and when to lock a thread
  2. the program contains dead code (which I can't find, other than a single accessor that did not get used)

namespace BankSimulationThreaded
{
    class Program
    {
        // Logs the number of completed transactions.
        static int transactions = 0;
        // Creates a new teller that will be shared by all customers.
        static Teller tel = new Teller();
        // Creates a random number generator, ran, that will be used to create random integers for the transactions.
        static Random ran = new Random();

        static void Main(string[] args)
        {
            // Initializes a new thread for each customer.
            Thread bankCustomer1 = new Thread(new ThreadStart(new BankCustomer("Joe").runCustomer));
            Thread bankCustomer2 = new Thread(new ThreadStart(new BankCustomer("Bob").runCustomer));
            Thread bankCustomer3 = new Thread(new ThreadStart(new BankCustomer("Steve").runCustomer));
            Thread bankCustomer4 = new Thread(new ThreadStart(new BankCustomer("Frank").runCustomer));
            Thread bankCustomer5 = new Thread(new ThreadStart(new BankCustomer("Jess").runCustomer));
            // Starts the treads.
            bankCustomer1.Start();
            bankCustomer2.Start();
            bankCustomer3.Start();
            bankCustomer4.Start();
            bankCustomer5.Start();
            // Prevents program from closing so that user may read output.  
            if (transactions == 100)
            {
                Console.WriteLine("The Bank is now closed.  Press any key to exit.");
                Console.ReadKey();
            }
        }

        class BankCustomer
        {
            // Stores the current balance in the BankCustomer account. Default value is 1000.
            int balance;
            string name;
            // Creates a BankCustomer object with the default balance of 1000.
            public BankCustomer(String name)
            {
                this.name = name;
                balance = 1000;
            }
            // Returns the current account balance for the BankCustomer object.
            public int getBal()
            {
                return balance;
            }
            // Subtracts the given amount from the current balance.  Returns false if amount exceeds balance.
            public bool withdraw(int amount)
            {
                // If amount does not exceed balance or amount in vault, subtract amount from balance. 
                if (amount > balance)
                {
                    Console.Write(this.name + "'s account has ");
                    return false;
                }
                if (!tel.withdraw(amount))
                {
                    Console.Write("Vault has ");
                    return false;
                }
                balance -= amount;
                return true;
                // If amount exceeds balance, return false;
            }
            // Adds the given amount to the current balance.
            public void deposit(int amount)
            {
                balance += amount;
            }
            public string depositOrWith(bool deposit)
            {
                if (deposit)
                    return "deposit";
                return "withdrawal";
            }
            public void runCustomer()
            {
                // When deposit is true, make deposit.  Else, make withdrawal. 
                bool deposit = true;
                // amt is used to determine the amount of the transaction.  It is a random int between 1 and 5000.
                int amt;
                // Transact until total transactions exceed hit the specified value.
                while (transactions < 100)
                {
                    // Locks the resource so that it may only be used by one thread at a time.
                    lock(tel)
                    {
                        Console.WriteLine(this.name + " approaches the window.  "+ this.name + " is making a " + depositOrWith(deposit) + ".");
                        Thread.Sleep(100);
                        // Assign amt a random number, using Random ran, between 1 and 5000.
                        amt = ran.Next(1, 5000);
                        if (deposit)
                        {
                            // Deposit amount deposited into this threads account, the vault, and set next transaction to be a withdrawal.
                            this.deposit(amt);
                            tel.deposit(amt);
                            // Next transaction will be a withdrawal.
                            deposit = false;
                            transactions++;
                            Console.WriteLine(this.name + " deposited $" + amt + " into their account.");
                        }
                        else
                        {
                            // If a withdrawal is possible, prints the amount and the name of the transacting customer.  
                            if (this.withdraw(amt))
                            {
                                Console.WriteLine(this.name + " withdrew $" + amt + " from their account.");
                            }
                            else
                            {
                                Console.WriteLine("insufficient funds.  Attempted withdrawal of " + amt + ".");
                            }
                            // Else increments transactions and flags next transaction as a deposit.
                            transactions++;
                            deposit = true;
                        }
                        // Prints the customer's name, balance of their account, and the balance of the vault after each transaction.
                        Console.WriteLine(this.name + "'s balance: $" + this.balance + " " + " Vault Balance: $" + tel.getBal() + "\r\n");
                        // Puts this thread to sleep before reliquishing control to allow the user to view transactions as they occur.  
                        Thread.Sleep(250);
                    }
                }
            }



        }// End of class BankCustomer
    }// End of class Program

    class Teller
    {
        // The amount of money that is left in the vault.  Default value is 20000.
        int vaultBalance; 

        // Creates a Teller object with the default balance of 20000.
        public Teller()
        {
            vaultBalance = 20000;
        }
        // Returns the current vault balance.
        public int getBal()
        {
            return vaultBalance;
        }
        // Subtracts amount from the current vault total.  Returns false if the vault has insufficient funds. 
        public bool withdraw(int amount)
        {
            // If amount does not exceed vault, transaction occurs.
            if (amount <= vaultBalance)
            {
                vaultBalance -= amount;
                return true;
            }
            //If not, returns false.
            return false;
        }
        // Adds the given deposit amount to the current vault total. 
        public void deposit(int amount)
        {
            vaultBalance += amount;
        }

    }// End of class Teller
}
\$\endgroup\$
  • 1
    \$\begingroup\$ TL;DR... Sleep inside lock sounds like awful idea irrespective what the rest of the code does. \$\endgroup\$ – Alexei Levenkov Apr 2 '15 at 4:41
  • \$\begingroup\$ As far as whether you understand about spawning threads, I don't know about that. I mean, if the interviewer told you to dedicate one thread to each customer, I don't know what that means other than to start a new thread for each customer as you did here. I suppose the interviewer might be taking issue with the verbosity of your code, explicitly creating instances of ThreadStart when C# would infer that. And did the interviewer also expect you to create a sixth thread, to represent the teller, with the active customer thread communicating with the teller thread? Not clear. \$\endgroup\$ – Peter Duniho Apr 2 '15 at 4:52
  • \$\begingroup\$ to be honest: I don't like all the global variables (the classes depend on) and the fact that you made no effort to create thread-safe classes - I don't 100% agree with the feedback but I think you have to work on your OOP skills first \$\endgroup\$ – Carsten Apr 2 '15 at 7:15
  • 2
    \$\begingroup\$ That's an interesting bank, where the customers form more of a mob than a queue. \$\endgroup\$ – 200_success Apr 2 '15 at 21:44
8
\$\begingroup\$

A list of things you could improve:

  • Do not write comments like this: // End of class Teller It’s very easy to see in Visual Studio where a class starts and ends.
  • Add XML comments to your classes, method, properties, fields etc. See: https://msdn.microsoft.com/en-us/library/vstudio/b2s063f7%28v=vs.100%29.aspx
  • In C# Classes, Methods, Properties and events are written in Pascal case: GetBal not getBal
  • Do not use ‘uncommon’ abbreviations if the full word is not very long: GetBalance not GetBal
  • Always specify access modifiers https://msdn.microsoft.com/en-us/library/ms173121.aspx Not class BankCustomer write public class BankCustomer
  • Use auto Properties instead of global variables: public String Name { get; set; } And use the appropriate access modifier public Int32 Balance { get; private set; }
  • Make as much methods as possible private or protected (Keep interfaces slim)
  • Use inline if statements (aka ternary operator): return deposit ? "Deposit" : "Withdrawal";
  • Use String.Format for creating formatted strings or writing to the console: Console.Write( "{0}'s account has ", Name ); https://msdn.microsoft.com/en-us/library/system.string.format%28v=vs.110%29.aspx
  • Don’t use redundant qualifier this, base this.Deposit( amt ); Deposit( amt ); would be better
  • The if at the end of the main method doesn’t work. Your program will close immediately (if the threading stuff is done right). You should use a while loop: while ( _transactions < 100 ) Thread.Sleep( 100 ); Console.WriteLine("The Bank is now closed. Press any key to exit."); Console.ReadKey();
  • Use Regions to structure your code: #region Properties
  • Invert if statements to reduce nesting: if ( amount > ValueBalance ) return false;
  • Do not create classes inside of other classes. ( only do this if you need a private class which is only used in the scope of the other class )
  • Do not use Thread.Sleep inside of a lock statement
  • Use the var keyword if possible
  • Use Environment.NewLine instead of "\r\n"
  • Create the customers as a collection so you can start the threads inside of a loop: //Create some customers var customers = new List<BankCustomer> { new BankCustomer { Name = "Joe" }, new BankCustomer { Name = "Bob" }, new BankCustomer { Name = "Steve" }, new BankCustomer { Name = "Frank" }, new BankCustomer { Name = "Jess" } }; //Run the customers customers.ForEach( x => Task.Run( () => x.RunCustomer() ) );

Here is an improved version of your code. (This is not a perfect solution… there is still a lot to improve)

public static class ObjectStorage
{
    // Logs the number of completed transactions.
    static ObjectStorage()
    {
        Teller = new Teller();
        Rnd = new Random();
    }

    public static Int32 Transactions { get; set; }

    // Creates a new teller that will be shared by all customers.
    public static Teller Teller { get; private set; }

    // Creates a random number generator, _rnd, that will be used to create random integers for the transactions.
    public static Random Rnd { get; private set; }
}

public class Program
{
    static void Main( String[] args )
    {
        //Create some customers
        var customers = new List<BankCustomer>
        {
            new BankCustomer { Name = "Joe" },
            new BankCustomer { Name = "Bob" },
            new BankCustomer { Name = "Steve" },
            new BankCustomer { Name = "Frank" },
            new BankCustomer { Name = "Jess" }
        };

        //Run the customers
        customers.ForEach( x => Task.Run( () => x.RunCustomer() ) );

        // Prevents program from closing so that user may read output.  
        while ( ObjectStorage.Transactions < 100 )
            Thread.Sleep( 100 );
        Console.WriteLine( "The Bank is now closed.  Press any key to exit." );
        Console.ReadKey();
    }
}

public class BankCustomer
{
    #region Properties

    /// <summary>
    ///     Gets the balance of the customer.
    /// </summary>
    /// <value>The balance of the customer.</value>
    public Int32 Balance { get; private set; }

    /// <summary>
    ///     Gets or sets the name of the customer.
    /// </summary>
    /// <value>The name of the customer.</value>
    public String Name { get; set; }

    #endregion

    #region Ctor

    /// <summary>
    ///     Creates a new instance of the <see cref="BankCustomer" /> class.
    /// </summary>
    public BankCustomer()
    {
        Balance = 1000;
    }

    #endregion

    #region Public Members

    public void RunCustomer()
    {
        // When Deposit is true, make Deposit.  Else, make withdrawal. 
        var deposit = true;
        // amt is used to determine the amount of the transaction.  It is a random int between 1 and 5000.
        // Transact until total transactions exceed hit the specified value.
        while ( ObjectStorage.Transactions < 100 )
        {
            Console.WriteLine("{0} approaches the window.  {0} is making a {1}.", Name, DepositOrWith(deposit));

            // Assign amt a random number, using Random _rnd, between 1 and 5000.
            var amt = ObjectStorage.Rnd.Next(1, 5000);

            // Locks the resource so that it may only be used by one thread at a time.
            lock ( ObjectStorage.Teller )
            {
                if ( deposit )
                {
                    // Deposit amount deposited into this threads account, the vault, and set next transaction to be a withdrawal.
                    Deposit( amt );
                    ObjectStorage.Teller.Deposit( amt );
                    // Next transaction will be a withdrawal.
                    deposit = false;
                    ObjectStorage.Transactions++;
                    Console.WriteLine( Name + " deposited $" + amt + " into their account." );
                }
                else
                {
                    // If a withdrawal is possible, prints the amount and the name of the transacting customer.  
                    if ( Withdraw( amt ) )
                        Console.WriteLine( Name + " withdrew $" + amt + " from their account." );
                    else
                        Console.WriteLine( "insufficient funds.  Attempted withdrawal of " + amt + "." );
                    // Else increments transactions and flags next transaction as a Deposit.
                    ObjectStorage.Transactions++;
                    deposit = true;
                }
                // Prints the customer's name, balance of their account, and the balance of the vault after each transaction.
                Console.WriteLine("{0}'s balance: $ {1}  Vault Balance: $ {2}{3}", Name, Balance, ObjectStorage.Teller.ValueBalance, Environment.NewLine);
            }

            Thread.Sleep(1);
        }
    }

    #endregion

    #region Private Members

    /// <summary>
    ///     Subtracts the given amount from the current balance.
    /// </summary>
    /// <param name="amount">The amount to subtract.</param>
    /// <returns>Returns false if amount exceeds balance. </returns>
    private Boolean Withdraw( Int32 amount )
    {
        // If amount does not exceed balance or amount in vault, subtract amount from balance. 
        if ( amount > Balance )
        {
            Console.Write( "{0}'s account has ", Name );
            return false;
        }
        if ( !ObjectStorage.Teller.Withdraw( amount ) )
        {
            Console.Write( "Vault has " );
            return false;
        }
        Balance -= amount;
        return true;
    }

    /// <summary>
    ///     Adds the given amount to the current balance.
    /// </summary>
    /// <param name="amount">The amount to add.</param>
    private void Deposit( Int32 amount )
    {
        Balance += amount;
    }

    private String DepositOrWith( Boolean deposit )
    {
        return deposit ? "Deposit" : "Withdrawal";
    }

    #endregion
}

/// <summary>
///     Description of the <see cref="Teller" /> class.
/// </summary>
public class Teller
{
    #region Properties

    /// <summary>
    ///     Gets the amount of money that is left in the vault.
    /// </summary>
    public Int32 ValueBalance { get; private set; }

    #endregion

    #region Ctor

    /// <summary>
    ///     Initialize a new instance of the<see cref="Teller" /> class.
    /// </summary>
    public Teller()
    {
        ValueBalance = 20000;
    }

    #endregion

    #region Public Members

    /// <summary>
    ///     Subtracts amount from the current vault total.
    /// </summary>
    /// <param name="amount">The amount to subtract.</param>
    /// <returns>Returns false if the vault has insufficient funds. </returns>
    public Boolean Withdraw( Int32 amount )
    {
        // If amount does not exceed vault, transaction occurs.
        if ( amount > ValueBalance )
            return false;
        ValueBalance -= amount;
        return true;
        //If not, returns false.
    }

    /// <summary>
    ///     Adds the given Deposit amount to the current vault total.
    /// </summary>
    /// <param name="amount">The amount to add.</param>
    public void Deposit( Int32 amount )
    {
        ValueBalance += amount;
    }

    #endregion
}

Edit: This is a better solution.

Differences: - Teller is a singleton - The lock happens in the Teller class - Teller class does the transaction counting

 #region Usings

 using System;
 using System.Collections.Generic;
 using System.Threading;
 using System.Threading.Tasks;

 #endregion

 namespace Bank
 {
     public class Program
     {
         static void Main( String[] args )
         {
             //Create some customers
             var customers = new List<Customer>
             {
                 new Customer { Name = "Joe" },
                 new Customer { Name = "Bob" },
                 new Customer { Name = "Steve" },
                 new Customer { Name = "Frank" },
                 new Customer { Name = "Jess" }
             };

             //Run the customers
             customers.ForEach( x => Task.Run( () => x.MakePayments() ) );

             // Prevents program from closing so that user may read output.  
             while ( Teller.Instance.IsWorking )
                 Thread.Sleep( 100 );
             Console.WriteLine( "The Bank is now closed.  Press any key to exit." );
             Console.ReadKey();
         }
     }

     public static class RandomHelper
     {
         private static readonly Random _random = new Random();

         public static Int32 Next( Int32 start, Int32 end )
         {
             return _random.Next( start, end );
         }
     }

     /// <summary>
     ///     Class representing a teller.
     /// </summary>
     public sealed class Teller
     {
         #region Fields

         /// <summary>
         ///     Lazy used to create a teller.
         /// </summary>
         private static readonly Lazy<Teller> Lazy = new Lazy<Teller>( () => new Teller() );

         /// <summary>
         ///     Object used to synchronize threads.
         /// </summary>
         private static readonly Object SyncRoot = new Object();

         /// <summary>
         ///     Field used to count the number of transactions.
         /// </summary>
         private Int32 _transactionCounter;

         #endregion

         #region Properties

         /// <summary>
         ///     Gets the teller.
         /// </summary>
         /// <value>The teller.</value>
         public static Teller Instance
         {
             get { return Lazy.Value; }
         }

         /// <summary>
         ///     Gets the amount of money that is left in the vault.
         /// </summary>
         /// <value>The amount of money that is left in the vault.</value>
         public Int32 ValueBalance { get; private set; }

         /// <summary>
         ///     Gets a value indicating whether the teller is working or not.
         /// </summary>
         /// <value>A value indicating whether the teller is working or not. </value>
         public Boolean IsWorking
         {
             get { return _transactionCounter < 100; }
         }

         #endregion

         #region Ctor

         /// <summary>
         ///     Creates a new instance of the <see cref="Teller" /> class.
         /// </summary>
         private Teller()
         {
             ValueBalance = 20000;
         }

         #endregion

         #region Public Members

         /// <summary>
         ///     Subtracts amount from the current vault total.
         /// </summary>
         /// <param name="amount">The amount to subtract.</param>
         /// <returns>Returns false if the vault has insufficient funds. </returns>
         public Boolean Withdraw( Int32 amount )
         {
             lock ( SyncRoot )
             {
                 // If amount does not exceed vault, transaction occurs.
                 if ( amount > ValueBalance )
                     return false;
                 ValueBalance -= amount;
                 _transactionCounter++;
                 Console.WriteLine( "Withdraw amount is : {0}", ValueBalance );
                 return true;
             }
         }

         /// <summary>
         ///     Adds the given Deposit amount to the current vault total.
         /// </summary>
         /// <param name="amount">The amount to add.</param>
         public void Deposit( Int32 amount )
         {
             lock ( SyncRoot )
             {
                 ValueBalance += amount;
                 _transactionCounter++;
                 Console.WriteLine( "Deposit amount is : {0}", ValueBalance );
             }
         }

         #endregion
     }

     /// <summary>
     ///     Class representing a customer.
     /// </summary>
     public class Customer
     {
         #region Properties

         /// <summary>
         ///     Gets the balance of the customer.
         /// </summary>
         /// <value>The balance of the customer.</value>
         public Int32 Balance { get; private set; }

         /// <summary>
         ///     Gets or sets the name of the customer.
         /// </summary>
         /// <value>The name of the customer.</value>
         public String Name { get; set; }

         #endregion

         #region Ctor

         /// <summary>
         ///     Creates a new instance of the <see cref="Customer" /> class.
         /// </summary>
         public Customer()
         {
             Balance = 1000;
         }

         #endregion

         #region  Public Members

         /// <summary>
         ///     Start doing payments.
         /// </summary>
         public void MakePayments()
         {
             var deposit = true;
             var amount = RandomHelper.Next( 1, 10 );

             while ( Teller.Instance.IsWorking )
             {
                 if ( deposit )
                 {
                     Deposit( amount );
                     Teller.Instance.Deposit( amount );
                     deposit = false;
                 }
                 else
                 {
                     Withdraw( amount );
                     deposit = true;
                 }
                 Thread.Sleep( RandomHelper.Next( 1, 100 ) );
             }
         }

         #endregion

         #region Private Members

         /// <summary>
         ///     Subtracts the given amount from the current balance.
         /// </summary>
         /// <param name="amount">The amount to subtract.</param>
         /// <returns>Returns false if amount exceeds balance. </returns>
         private Boolean Withdraw( Int32 amount )
         {
             // If amount does not exceed balance or amount in vault, subtract amount from balance. 
             if ( amount > Balance )
                 return false;
             if ( !Teller.Instance.Withdraw( amount ) )
                 return false;
             Balance -= amount;
             return true;
         }

         /// <summary>
         ///     Adds the given amount to the current balance.
         /// </summary>
         /// <param name="amount">The amount to add.</param>
         private void Deposit( Int32 amount )
         {
             Balance += amount;
         }

         private String DepositOrWith( Boolean deposit )
         {
             return deposit ? "Deposit" : "Withdrawal";
         }

         #endregion
     }
 }
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ The advice about not using this is debatable, but a very nice review none the less. \$\endgroup\$ – RubberDuck Apr 3 '15 at 11:34
  • 1
    \$\begingroup\$ I also disagree with "use var whenever possible"; you should only use it when it's clear from the context which type is inferred. \$\endgroup\$ – Snowbody Apr 3 '15 at 13:51
  • 1
    \$\begingroup\$ I disagree with the use of #region (it's generally a code smell indicating the class is too large, and hence doing too much), but overall this is a good review. \$\endgroup\$ – Dan Lyons Apr 3 '15 at 18:17
  • 1
    \$\begingroup\$ Too many people are afraid to change their ReSharper settings IMO... But yes. It can go either way. Consistency is what's important. \$\endgroup\$ – RubberDuck Apr 3 '15 at 20:04
  • 2
    \$\begingroup\$ @DanLyons i don't think the use of regions in and of itself qualifies as a code smell. They're a tool, and like any tool can certainly be abused. It's kind of like saying there's never a valid use of GoTo... \$\endgroup\$ – RubberDuck Apr 3 '15 at 20:06
4
\$\begingroup\$
        // Prevents program from closing so that user may read output.  
        if (transactions == 100)
        {
            Console.WriteLine("The Bank is now closed.  Press any key to exit.");
            Console.ReadKey();
        }

This is the dead code. Your comment is incorrect; the code will likely never be executed.

| improve this answer | |
\$\endgroup\$

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.