18
\$\begingroup\$

I am a new .NET/C# developer and one issue bugs me ever since I've wrote code the first time. How can I improve this code?

Program.cs:

class Program
    {
        static void Main(string[] args)
        {
            App();
        }

        public static void App()
        {
            BankDetails bankdet = new BankDetails();

            ShowMenu();

            while (true)
            {
                Console.WriteLine("");

                string userInput = Console.ReadLine();
                switch (userInput)
                {
                    case "a":
                        Console.WriteLine("'Citire din fisier' selected");
                        break;
                    case "b":
                        Console.WriteLine("'Creare cont' selected");
                        Console.WriteLine("");
                        bankdet.CreateAccount("Name 1");
                        bankdet.CreateAccount("Name 2");
                        bankdet.CreateAccount("");
                        bankdet.CreateAccount("Name 4");
                        bankdet.CreateAccount("Name 5");
                        break;
                    case "c":
                        Console.WriteLine("'Depunere bancara' selected");
                        bankdet.Deposit();
                        break;
                    case "d":
                        Console.WriteLine("'Retragere bancara' selected");
                        bankdet.Withdraw();
                        break;
                    case "e":
                        Console.WriteLine("'Afisare sold' selected");
                        bankdet.Balance();
                        break;
                    case "f":
                        Environment.Exit(0);
                        break;
                    default:
                        Console.WriteLine("Please select a valid option");
                        break;
                }
            }
        }

        public static void ShowMenu()
        {
            Console.WriteLine("a. Citire din fisier");
            Console.WriteLine("b. Creare cont");
            Console.WriteLine("c. Depunere bancara");
            Console.WriteLine("d. Retragere bancara");
            Console.WriteLine("e. Afisare sold");
            Console.WriteLine("f. Iesire");
        }

    }

BankDetails.cs:

class BankDetails : IBankDetails
{
    ReturnedVal rv = new ReturnedVal();
    List<BankAccount> _accounts;

    public BankDetails()
    {
        _accounts = new List<BankAccount>();
    }

    public List<BankAccount> Account
    {
        get { return _accounts; }
    }


    public void CreateAccount(string name)
    {
        BankAccount account = new BankAccount();
        CalculateIBAN calculateIban = new CalculateIBAN();

        try
        {
            if (!String.IsNullOrEmpty(name))
            {
                account.Name = name;
                account.IBAN = calculateIban.IBAN();
                _accounts.Add(account);
                Console.WriteLine("Account created - Name: {0}, IBAN: {1}", account.Name, account.IBAN);
            }
            else
            {
                Console.WriteLine("Account name is null or empty.");
            }
        }
        catch (NullReferenceException ne)
        {
            Console.WriteLine(ne.StackTrace);
        }
    }

    public float Deposit()
    {
        string iban = rv.EnterIban();
        BankAccount account = rv.GetAccountByName(iban, _accounts);

        while (account == null)
        {
            Console.WriteLine("Account doesn't exist");
            iban = rv.EnterIban();
            account = rv.GetAccountByName(iban, _accounts);
        }

        float sum = rv.AmountToDeposit();
        while (sum <= 0)
        {
            Console.WriteLine("Amount cannot be less or equal than 0.");
            sum = 0;
            sum = rv.AmountToDeposit();
        }

        account.Sum += sum;
        Console.WriteLine("Added {0} to account {1}", sum, iban);

        return account.Sum;
    }

    public float Withdraw()
    {
        BankDetails details = new BankDetails();
        BankAccount.Comision c = Comission.Comision;

        string iban = rv.EnterIban();
        BankAccount account = rv.GetAccountByName(iban, _accounts);

        while (account == null)
        {
            Console.WriteLine("Account doesn't exist");
            iban = rv.EnterIban();
            account = rv.GetAccountByName(iban, _accounts);
        }

        float sum = rv.AmountToDeposit();
        while (sum <= 0)
        {
            Console.WriteLine("Amount cannot be less or equal than 0.");
            sum = 0;
            sum = rv.AmountToDeposit();
        }

        account.Sum -= sum;

        Console.Write("Withdrawn {0} from account {1}.", sum, iban);
        Console.WriteLine("Comision {0}", Math.Round(c(account.Sum), 2));
        account.Sum -= c(account.Sum);
        Console.WriteLine("Remaining: {0}", Math.Round(account.Sum, 2));

        return account.Sum;
    }

    public float Balance()
    {
        string iban = rv.EnterIban();
        BankAccount account = rv.GetAccountByName(iban, _accounts);

        Console.WriteLine("IBAN: {0} has {1} left", iban, account.Sum);

        return account.Sum;
    }
}

IBankDetails.cs:

interface IBankDetails
    {
        float Balance();
        void CreateAccount(string name);
        float Deposit();
        float Withdraw();
    }

ReturnedVal.cs:

class ReturnedVal
    {
        internal string EnterIban()
        {
            Console.WriteLine("IBAN:");
            string iban = Console.ReadLine();

            return iban;
        }

        internal float AmountToDeposit()
        {
            Console.WriteLine("Type in the amount ");
            float sum = float.Parse(Console.ReadLine());

            return sum;
        }

        internal BankAccount GetAccountByName(string iban, List<BankAccount> _conturi)
        {
            BankAccount ba = _conturi.Find(c => c.IBAN == iban);

            if (ba != null)
            {
                return ba;
            }
            else
            {
                return null;
            }
        }
    }

CalculateIBAN.cs:

public class CalculateIBAN
    {
        static Random generator = new Random();
        string accountNumber = generator.Next(100, 999).ToString();

        public string IBAN()
        {
            return accountNumber;
        }
    }

BankAccount.cs:

public class BankAccount
    {
        public string Name { get; set; }
        public string IBAN { get; set; }
        public float Sum { get; set; }

        public delegate float Comision(float comission);

    }

This is only a class from my application, but it's currently the one with the most lines.

\$\endgroup\$
  • \$\begingroup\$ To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question. \$\endgroup\$ – SuperBiasedMan Oct 5 '15 at 15:19
  • \$\begingroup\$ @Heslacher It is a delegate that calculates the remaining amount - a 5% comission. \$\endgroup\$ – Alexandru Mitu Oct 5 '15 at 15:27
  • \$\begingroup\$ Never edit code in question. After improving the code, wait a day and post a follow-up. \$\endgroup\$ – Caridorc Oct 5 '15 at 16:10
  • 1
    \$\begingroup\$ @Caridorc Sorry, I didn't know that, I won't edit code in question. \$\endgroup\$ – Alexandru Mitu Oct 5 '15 at 16:13
  • 1
    \$\begingroup\$ I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Oct 5 '15 at 16:14
14
\$\begingroup\$

Account Structure

Since it looks like you are trying to model the bank account slightly more realistically than a typical getting started tutorial, I'd like to address the conceptual structure of your BankAccount class.

In real life, an account is much more likely to be composed of a collection of transactions, rather than an updated balance. Getting the balance would be accomplished by a method which sums up all the transactions.

So instead of (simplified):

class BankAccount //balance is data, transactions are methods.
{
    decimal balance;
    void Deposit(decimal amount) { balance += amount; }
    void Withdraw(decimal amount) { balance -= amount; }
}

You would have something like:

class Transaction
{
    decimal amount;
    //other details
}

class BankAccount //transactions are data, balance is method.
{
    List<Transaction> transactions;
    void AddTransaction(Transaction t) { transactions.Add(t); }
    decimal GetBalance() { /* return sum of all transactions */ }
}

Not only does this allow you to show your customer the details of the transactions made on their account, but it allows you to retroactively cancel an erroneous transaction and have the balance update automatically.

Floating Point

Another important issue is your choice of float as a representation of currency. This is inherently flawed, as floating point numbers are not meant to represent base-10 values with accuracy.

For this exact reason we have the System.Decimal type in c#.

Redundancy (one example)

if (ba != null)
{
   return ba;
}
else
{
   return null;
}

Can be replaced with

return ba;
\$\endgroup\$
  • \$\begingroup\$ The redundancy in the null, I was writing it when the page updated and I saw it in your answer. The faster wins, well done. \$\endgroup\$ – Caridorc Oct 5 '15 at 15:41
10
\$\begingroup\$

Don't catch NullReferenceException, this is a programming exception, it shouldn't happen when to code is running if you're doing your job well. If something throws a NullReferenceException, find why and fix it!

When manipulating money, always use decimal, it's the best data type for this.

ReturnedVal isn't a good class name, what/when/why does it return what? If I understand correctly, this class is used for some operations that belong in other classes. Usually, (here's a tip) if you can't explain with one sentence what your class does as a whole (properties/methods etc), this class isn't well designed. An explanation for this class would be "Methods to returns stuff, with stuff". These 3 methods belong somewhere else!

Your class is too console dependant. You'd want to apply separation of concerns. The class BankDetail's methods Withdraw should receive a parameter (which is the money to withdraw) and Deposit should receive a paramter (the money to deposit, you get that!). This way, your class can be reusable anywhere else, without depending on the console. (Imagine I want to use your class in my Winforms application, right now it'be impossible).

About your naming, methods usually represents actions, meaning Balance should be GetBalance etc.

\$\endgroup\$
10
\$\begingroup\$

You have some noise in your code:

        string iban = Console.ReadLine();

        return iban;

and

        float sum = float.Parse(Console.ReadLine());

        return sum;

The variable is assigned to be immediately returned, rendering it effectively useless.

Just use:

return Console.ReadLine();

and

return float.Parse(Console.ReadLine());

The same thing can be seen in one more passage at:

        BankAccount ba = _conturi.Find(c => c.IBAN == iban);

        if (ba != null)
        {
            return ba;
        }
        else
        {
            return null;
        }

following @Rotem suggestion:

BankAccount ba = _conturi.Find(c => c.IBAN == iban);
return ba;

and we can apply the same semplification:

return _conturi.Find(c => c.IBAN == iban);

The Deposit function has 3 responsibilities, while a good function should have only one:

  1. It changes the inside of the class:

    account.Sum += sum;
    
  2. It communicates with the outside world:

    Console.WriteLine("Added {0} to account {1}", sum, iban);
    
  3. It returns a value:

    return account.Sum;
    

You should split it in (at least) three functions.


And you should write all of your code in English. It is an international convention that makes code equally readable to anyone in the world. Of course you may keep the message for interacting with the user in French, but I suggest swapping that you write that in English too, for consistency.

\$\endgroup\$
  • \$\begingroup\$ Thank you, I have done that to all methods (I think). Should I add the methods in another file? \$\endgroup\$ – Alexandru Mitu Oct 5 '15 at 16:02
  • \$\begingroup\$ @AlexandruMitu I think that returning the value could be removed altogether as there is a setter for it, printing can be done for all the Account info at once with one toString method that produces a readable string representation of the object. This way your functions have single responsability and you avoid code bloat. \$\endgroup\$ – Caridorc Oct 5 '15 at 16:04
  • \$\begingroup\$ as there is a getter for it (typo) \$\endgroup\$ – Caridorc Oct 5 '15 at 16:09
8
\$\begingroup\$
  • You don't need to do Console.WriteLine("") - Console.WriteLine() will do the trick.

  • Your use of float for money amounts makes me shudder. float is not designed for really strict accuracy, as is needed when working with money amounts - use decimal instead.

  • Don't output in your processing methods. Deposit() should not be outputting - if the account does not exist, throw a custom exception instead, for example.

  • CalculateIBAN is not a good class name. Classes are like objects - they are something. Methods do something, so this would be a better method than class.

  • Separate your concerns. Deposit() should only be concerned with depositing values, not with checking if accounts exist. I would pass the account and amount into the method.

  • Use better names: account.Sum -= c(account.Sum); - c doesn't state what it does. rv isn't the best name either.

  • float float.Parse(string) will throw an exception if an invalid string is entered. You can avoid this by using bool float.TryParse(string, out float) which will just return false for invalid entries.


if (ba != null)
{
    return ba;
}
else
{
    return null;
}

You can combine that into return ba;

\$\endgroup\$
  • \$\begingroup\$ The only new point is You don't need to do Console.WriteLine("") - Console.WriteLine() will do the trick. the other improvements have already been said. \$\endgroup\$ – Caridorc Oct 5 '15 at 16:07
  • \$\begingroup\$ Thank you for your answer, I will change that Console.WriteLine("") immediately, working on the decimal thingy. \$\endgroup\$ – Alexandru Mitu Oct 5 '15 at 16:12
  • 1
    \$\begingroup\$ @Caridorc I didn't read the other answers until I was almost done, and the Console.WriteLine() thingy wasn't enough for an answer. I figured mentioning a point a couple times won't hurt. \$\endgroup\$ – Hosch250 Oct 5 '15 at 16:13

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.