1
\$\begingroup\$

This is purely testing my skills; I am not actually coding for a bank. This is part of a bank account class.

public void withDraw(double amount)
    {
        if(balance - amount >= odLimit)
        {
            balance -= amount;
        }
        else
        {
            Console.WriteLine("Error.  Withdrawing will take you below overdraft limit.  Enter a new amount: ");
            double amount2;
            while (!double.TryParse(Console.ReadLine(), out amount2) || (balance - amount2 < odLimit))
            {
                Console.WriteLine("Enter a valid number: ");
            }

            balance -= amount2;
        }
    }

odLimit is the overdraft limit. The code checks if the balance - amount is greater than or equal to the odLimit (say -900), and if so, it simply takes that money from the account. If that's not the case then it keeps asking the user to enter an amount they wish to withdraw, and when they enter a value that won't take the account below the overdraft limit and is a double.

Just checking that this is the most efficient way to do this. I prefer minimum lines of code.

Also in the main program class. Would you find it more efficient to create a menu function and call the withDraw function from the menu using a switch statement within the menu, or call a method in the program class from the switch statement that asks the user for their value, and then calls the withDraw function in the bank account class?

\$\endgroup\$
  • 4
    \$\begingroup\$ Usually when one deals with money or currency, they use the Decimal instead of double. \$\endgroup\$ – Rick Davin Nov 23 '15 at 20:25
  • \$\begingroup\$ @TiernanWatson Is this part of a larger class? Perhaps you should post the entire class instead of just this snippet. \$\endgroup\$ – Der Kommissar Nov 23 '15 at 21:01
  • \$\begingroup\$ The verb "withdraw" isn't a compound word, so the "d" should not be capitalized. Moreover, methods should be PascalCase, so your method should be named Withdraw. \$\endgroup\$ – BCdotWEB Nov 24 '15 at 10:34
1
\$\begingroup\$

For proper encapsulation, you want the interface logic to be completely separate from your atomic behaviours like withdrawing funds.

Something to consider: Where are the funds going? In a traditional banking system all operations are done in pairs as an atom. You withdraw from one account and deposit to another account, or into the cash withdrawal ledger, et al.

Your Withdraw method should return a result containing enough information that the caller can determine how to act. This would include whether the operation was successful (and if not, why not). For a simple case, you could probably do this with an enum. The method should take a decimal in (always use decimal for money, since you're concerned with precision and double is an approximate). Your calling routine should handle the validation and conversion, and if necessary the prompt back to the user to enter a valid number.

public enum OpResult = { Complete, Failed_Insufficient_Funds, Failed_Account_Frozen, Failed_Invalid_Amount };

    ...

public OpResult Withdraw(decimal amount)
{
   if (amount <= 0)
   {
       return OpResult.Failed_Invalid_Amount;
   }
   else if (balance - amount < odLimit)
   {
       return OpResult.Failed_Insufficient_Funds;
   }
   ...
   else 
   {
       balance -= amount;
       return OpResult.Complete;
   }
}
\$\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.