6
\$\begingroup\$

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("\n\nPlease 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();
        }
    }
}
\$\endgroup\$
12
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
12
\$\begingroup\$

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

| improve this answer | |
\$\endgroup\$
6
\$\begingroup\$

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

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ You are right, I should better check if it is null or empty, however this is misunderstanding due to language/region barrier, as in my country everyone has a middle name as it is considered to be one of parents name (usually father's name). \$\endgroup\$ – Nenad Bulatovic Mar 13 '13 at 18:37
  • 2
    \$\begingroup\$ @nenad: I would hazard a guess that most of the people in your country have a middle name, not everyone. One of the things you learn writing software to be used by people is that assuming that what most people do is the same as what everyone does just leads to pain later. \$\endgroup\$ – Eric Lippert Mar 13 '13 at 21:00
4
\$\begingroup\$

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 number\n");

    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! ;)

| improve this answer | |
\$\endgroup\$
  • 5
    \$\begingroup\$ NewPerson() feels wrong in the KeyInput class. They are two completely separate operations. \$\endgroup\$ – Jeff Vanzella Mar 13 '13 at 15:55
  • 2
    \$\begingroup\$ I don't like seeing while(true) - it makes me have to figure out why you're looping - I'd change it to while(!int.TryParse(KeyInput.keyIn("Please enter your age"), out age)) or use a well named variable. I'd also use sb.AppendFormat(...).AppendLine() but that's personal preference. \$\endgroup\$ – RobH Mar 13 '13 at 16:02
  • \$\begingroup\$ I edited my answer based on your useful comments :) \$\endgroup\$ – Abbas Mar 13 '13 at 22:46
  • \$\begingroup\$ Thank you for your effort to provide such a detailed insight into my code. \$\endgroup\$ – Nenad Bulatovic Mar 14 '13 at 9:41
  • \$\begingroup\$ No problem, I hope it helped you further! \$\endgroup\$ – Abbas Mar 14 '13 at 10:27
2
\$\begingroup\$

Working our way out of this mess

You have been given some good tips about separating concerns and coding to prevent exceptions. The application itself should be a very simple loop. Parsing user input and formatting output takes up most of the code.

Ideally, we would like to end up with this loop:

public static void Run()
{
    UserPane.AppInfo();
    while (true) // until 'EXIT' is typed by user
    {
        UserPane.PersonInfo(UserPane.ReadPerson());
    }
}

To get there, we require some refactoring. Also note, that unlike your perpetual checks for early exit, I would opt to let a low level handler (shown later on) exit the process on one place (DRY). This enables us to avoid redundant code in the likes of..

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

Let's start with cleaning up Person. As suggested by others, you should adhere to Separation of Concerrns. We will have to take out the Console printing. Furthermore, we need proper naming and member declaration conventions. Do not abbreviate member names. Prefer properties over public fields.

public string fname;
public string mname;
public string lname;
public int age;
public string phnumber;

We end up with this POCO:

class Person
{
    public string FirstName { get; set; }
    public string MiddleName { get; set; }
    public string LastName { get; set; }
    public int Age { get; set; }
    public string PhoneNumber { get; set; }
}

It's time to focus on user input. You already started a class KeyInput, which is a good idea to offset Console interop to. But you could have gone much further with this design to allow for clean application code.

internal class KeyInput
{
    public static string keyIn(string scrtext)
    {
        // .. code omitted for brevity
    }
}

This should be avoided in application code:

while (true)
{
    try
    {
        age = Convert.ToInt32(KeyInput.keyIn("\nPlease enter your age"));
        break;
    }

    catch (ArgumentException)
    {
        Console.WriteLine("No value was entered");
    }

    // .. code omitted for brevity
}

Let's make a class UserPane, which will handle both user input and formatted output. The code method for reading input is Read<T>. It provides a generic way to prompt the user for input, validate the input, allowing for retries, exiting the application on demand and projecting input to any other meaningful type.

static class UserPane
{
    const string Exit = "exit";

    static T Read<T>(string prompt, Func<string, bool> validator, Func<string, T> projector,
        string validationMessage = null, Action exitHandler = null)
    {
        string input = default;
        var validated = false;
        Console.WriteLine($"\r\n{prompt}");

        do
        {
            input = Console.ReadLine().Trim();

            if (input.ToLowerInvariant().Trim().Equals(Exit))
            {
                if (exitHandler == null)
                {
                    exitHandler = () => {
                        Environment.ExitCode = 1;
                        Environment.Exit(Environment.ExitCode);
                    };
                }
                exitHandler();
                return default;
            }

            validated = validator(input);

            if (!validated)
            {
                if (validationMessage == null)
                {
                    validationMessage = "Please try again.";
                }
                Console.WriteLine(validationMessage);
            }

        } while (!validated);


        return projector(input);
    }
}

This allows us to make convenience methods for asking user input. The ones we need for our application are..

static string ReadNonEmptyString(string prompt)
{
    return Read(prompt, input => !string.IsNullOrEmpty(input), input => input);
}

static string ReadString(string prompt)
{
    return Read(prompt, input => true, input => input);
}

static int ReadInt32(string prompt)
{
    return Read(prompt, input => int.TryParse(input, out var dummy), input => int.Parse(input));
}

With all these methods in place, we can now read a Person from user input. Like suggested by others, the middle name is optional. The phone number is parsed as any string. As an exercise, you could extend the parser to allow for a ReadPhoneNumber method.

internal static Person ReadPerson()
{
    return new Person
    {
        FirstName = ReadNonEmptyString("Please enter your first name:"),
        MiddleName = ReadString("Please enter your middle name (optional):"),
        LastName = ReadNonEmptyString("Please enter your last name:"),
        Age = ReadInt32("Please enter your age:"),
        PhoneNumber = ReadNonEmptyString("Please enter your phone number:")
    };
}

The last step is allowing for custom formatting and outputting to the console.

internal static void PersonInfo(Person person)
{
    var hasMiddleName = !string.IsNullOrEmpty(person.MiddleName);

    if (hasMiddleName)
    {
        Console.WriteLine($"Your name is: {person.FirstName} {person.MiddleName}. {person.LastName}");
        Console.WriteLine($"Your initials are: {person.FirstName[0]}{person.MiddleName[0]}{person.LastName[0]}");
    }
    else
    {
        Console.WriteLine($"Your name is: {person.FirstName} {person.LastName}");
        Console.WriteLine($"Your initials are: {person.FirstName[0]}{person.LastName[0]}");
    }

    Console.WriteLine($"Your age is: {person.Age}");
    Console.WriteLine($"Your phone number is: {person.PhoneNumber}");
}

And some app info..

internal static void AppInfo()
{
    Console.WriteLine("This program asks a person to enter full name, age, and phone number.");
    Console.WriteLine("The person's information is then displayed together with the initials.");
    Console.WriteLine("At any time the program could be terminated by typing: EXIT.");
}

This brings us to the application loop as shown at the start..

public static void Run()
{
    UserPane.AppInfo();
    while (true)
    {
        UserPane.PersonInfo(UserPane.ReadPerson());
    }
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ This is too cool for this simple app :-P how about decorating properties with some kind of [Console(prompt:"Text")] attributes and reflecting that with another tool? ;-] \$\endgroup\$ – t3chb0t Jul 28 '19 at 8:21

Not the answer you're looking for? Browse other questions tagged or ask your own question.