Cfrtjryk

I am learning C# from a book, and there is an exercise as follows:

Write a program that has a person enter full name, age, and phone
number. Display this information with formatting. Also display the
person's initials.

I wrote some code and I feel that it could be improved, so I ask anyone to comment on it.

using System;

using System.Collections.Generic;



internal class Person

{

    public string fname;

    public string mname;

    public string lname;

    public int age;

    public string phnumber;



    public Person()

    {

        fname = KeyInput.keyIn("nnPlease enter your first name");

        if (fname.ToLower() == "exit") return;



        mname = KeyInput.keyIn("nPlease enter your middle name");

        lname = KeyInput.keyIn("nPlease enter your last name");



        while (true)

        {

            try

            {

                age = Convert.ToInt32(KeyInput.keyIn("nPlease enter your age"));

                break;

            }



            catch (ArgumentException)

            {

                Console.WriteLine("No value was entered");

            }



            catch (FormatException)

            {

                Console.WriteLine("You didn't entered a valid number");

            }



            catch (Exception e)

            {

                Console.WriteLine("Something went wrong with the conversion.");

            }

        }



        phnumber = KeyInput.keyIn("nPlease enter your  phone number");

    }



    public void personDataDisplay()

    {

        Console.WriteLine("nYour name is: " + fname + " " + mname[0] + "." + " " + lname);

        Console.WriteLine("Your initials are: " + fname[0] + mname[0] + lname[0]);

        Console.WriteLine("Your age is: " + age);

        Console.WriteLine("Your phone number is: " + phnumber + "n");

    }

}



internal class KeyInput

{

    public static string keyIn(string scrtext)

    {

        Console.WriteLine(scrtext);

        string buffer = Console.ReadLine();

        return buffer;

    }

}



internal class MyApp

{

    public static void Main()

    {

        Console.WriteLine("Please enter data for person(s). Enter EXIT to end.");



        List<Person> list_persons = new List<Person>();



        while (true)

        {

            Person person = new Person();

            list_persons.Add(person);



            if (person.fname.ToLower() == "exit")

                break;

        }



        foreach (Person person in list_persons)

        {

            if (person.fname.ToLower() == "exit")

                break;



            person.personDataDisplay();

        }

    }

}

In designing classes we should try and confirm to the Single Responsibility Principle. Each class should only do one thing. Sometimes defining what makes a single responsibility is hard, but your Person class clearly does two things: it represents a person, and it questions the user via the console. This makes it difficult to reuse your Person class: what if you wanted to create a Person from a database, or a file or a web request? You would be better splitting this into a PersonQuestionnaire class which is responsible for creating a Person from a console input.

You should never have any significant logic in your constructor like you have here. Design your constructors to simply save any parameters and set up any default values. Any logic involving external classes, such as the Console, should definitely be placed in another method that is called once the object is constructed.

You should consider using the int.TryParse() method to read the age. Catching exceptions is slow (you will notice you ever write applications that are doing a lot of this type of thing), verbose, and sometimes unpredictable. You should definitely avoid catching the top level Exception type other than where you can do something meaningful: doing a lot of this will make your applications very difficult to debug.

In addition to the other good advice in the other answers, I would add:

• Do not catch any exception that you could have prevented. Instead, prevent the exception.

There is never any reason to catch ArgumentException because it should never be thrown in the first place. You are responsible for ensuring that when you pass arguments to a method, that those arguments meet the requirements of the method you're calling. You know if the string you're going to pass is null or empty; if it is, don't pass it.

Read this for more information:

http://blogs.msdn.com/b/ericlippert/archive/2008/09/10/vexing-exceptions.aspx

What happens when someone does not have a middle name? I think you'll find that if you test that, nothing good happens.

Person

Split user-input from your class definition. It's not a good practice to mix the input in the definition. Try to decouple as much as possible. I also redefined the properties in the class:

public string FirstName { get; set; }

public string MiddleName { get; set; }

public string LastName { get; set; }

public int Age { get; set; }

public string Phone { get; set; }

Same goes for displaying the information about the person. It's not a good practice to write it to screen from within this class. Just override the ToString()-method and display this on screen. The FirstChar()-method is there to check if the name exists to take the first character of that name, otherwise return a spacer.

public override string ToString()

{

    StringBuilder sb = new StringBuilder();

    sb.AppendLine(String.Format("Your name is: {0} {1}. {2}", FirstName, FirstChar(MiddleName), LastName));

    sb.AppendLine(String.Format("Your initials are: {0}{1}{2}", FirstChar(FirstName), FirstChar(MiddleName), FirstChar(LastName)));

    sb.AppendLine(String.Format("Your age is: {0}", Age));

    sb.AppendLine(String.Format("Your phone number is: {0}", Phone));



    return sb.ToString();

}



private char FirstChar(string input)

{

    return String.IsNullOrEmpty(input) ? ' ' : input[0];

}

Input

Since the input is not mixed in the Person-class we have to declare it somewhere else. In this example I put the input-code in the KeyInput class. The method returns an instance of the Person-class and if the user enters "exit", null is returned. This way we can later on catch if the user wants to stop.

public static Person NewPerson()

{

    string first = KeyInput.keyIn("Please enter your first name: ");

    if (first.ToLower().Equals("exit"))

        return null;



    string middle = KeyInput.keyIn("Please enter your middle name: ");

    string last = KeyInput.keyIn("Please enter your last name: ");

    int age = 0;



    while (true)

    {

        try

        {

            age = Convert.ToInt32(KeyInput.keyIn("Please enter your age"));

            break;

        }

        catch (ArgumentException)

        {

            Console.WriteLine("No value was entered");

        }

        catch (FormatException)

        {

            Console.WriteLine("You didn't entered a valid number");

        }

        catch (Exception)

        {

            Console.WriteLine("Something went wrong with the conversion.");

        }

    }



    string phone = KeyInput.keyIn("Please enter your  phone numbern");



    return new Person { FirstName = first, MiddleName = middle, LastName = last, Age = age, Phone = phone };

}

Main

Since all previous code has changed, the logic of your Main will change too.

public static void Main()

{

    Console.WriteLine("Please enter data for person(s). Enter EXIT to end.");



    List<Person> people = new List<Person>();



    while (true)

    {

        var person = KeyInput.NewPerson();



        if(person == null)

            break;



        people.Add(person);

    }



    foreach (var person in people)

    {

        Console.WriteLine(person.ToString());

    }

}

Summary

I'm not saying that my code is perfect in any way. Probably my code could be revised and corrected as well. I only want to show you the way how you decouple user-input from class-definitions, use proper variable names, bring a proper structure and logic in your code. Please feel free if to comment if anything's wrong with my code.

Edit:

Thanks to the valid comments I received, I edited the code. The posters of the comments were absolutely right. I've also rewritten the Initial()-method to a more correct logic. Here's the result in it's entirety:

internal class Person

{

    public string FirstName { get; set; }

    public string MiddleName { get; set; }

    public string LastName { get; set; }

    public int Age { get; set; }

    public string Phone { get; set; }



    public override string ToString()

    {

        var sb = new StringBuilder();



        sb.AppendFormat("Your name is: {0} {1} {2}", FirstName, Initial(MiddleName, true), LastName).AppendLine();

        sb.AppendFormat("Your initials are: {0}{1}{2}", Initial(FirstName, false), Initial(MiddleName, false), Initial(LastName, false)).AppendLine();

        sb.AppendFormat("Your age is: {0}", Age).AppendLine();

        sb.AppendFormat("Your phone number is: {0}", Phone).AppendLine();



        return sb.ToString();

    }



    private static string Initial(string input, bool dot)

    {

        if (String.IsNullOrEmpty(input))

            return input;



        if(input.Contains(" "))

            return input.Split(' ').Aggregate("", (current, s) => current + s[0]);



        return input[0] + (dot ? "." : "");

    }

}



internal class KeyInput

{

    public static string KeyIn(string scrtext)

    {

        Console.Write(scrtext);

        var buffer = Console.ReadLine();

        return buffer;

    }

}



internal class Program

{

    public static void Main(string args)

    {

        Console.WriteLine("Please enter data for one or more person(s)." + Environment.NewLine);



        var people = new List<Person>();

        var newInput = true;



        while (newInput)

        {

            var person = GetNewPerson();

            people.Add(person);

            newInput = KeyInput.KeyIn("Add another person? (Y/N): ").ToLower().Equals("y");

        }



        foreach (var person in people)

            Console.WriteLine(person.ToString() + Environment.NewLine);



        Console.ReadKey();

    }



    public static Person GetNewPerson()

    {

        var first = KeyInput.KeyIn("Please enter your first name: ");

        var middle = KeyInput.KeyIn("Please enter your middle name: ");

        var last = KeyInput.KeyIn("Please enter your last name: ");

        int age;



        while (!int.TryParse(KeyInput.KeyIn("Please enter your age: "), out age))

            Console.WriteLine("Invalid input, try again...");



        var phone = KeyInput.KeyIn("Please enter your  phone number: ");



        return new Person { FirstName = first, MiddleName = middle, LastName = last, Age = age, Phone = phone };

    }

}

Still feel free to comment, I'm learning myself! ;)

I have a question?

In my project I wanted to generate a ID automatically by students first name character last name character and his birthday combination of this I want to generate I'd help please