4
\$\begingroup\$

My Person class has two properties; name and surname, and a function that returns the first name and the surname together.

Is there anything that can be done better? Anything that is done poorly?

public class Person
{
    private string Firstname { get; set; }
    private string Surname { get; set; }

    public Person(string firstname, string surname)
    {
        this.Firstname = firstname;
        this.Surname = surname;

    }
    public string GetFullName()
    {
        string fullname = String.Format("Firstname is: {0}, Surname is {1}", Firstname, Surname);
        return fullname;
    }
}
\$\endgroup\$
2
  • 4
    \$\begingroup\$ You might consider making Firstname and Surname public with a private setter. \$\endgroup\$
    – Rick Davin
    Commented Jun 12, 2016 at 12:13
  • \$\begingroup\$ I don't think exposing a "Full Name", whether by method or property, is useful. I'd much rather have both the first name and surname exposed, and I as a client could then use and format that information as I see fit. Maybe I want to display it as Surname, Forename rather than Forename Surname, or I want to perform sorting on either property, etc. \$\endgroup\$
    – 404
    Commented Jun 13, 2016 at 16:47

2 Answers 2

9
\$\begingroup\$

Naming

Whenever you have a simple method that starts with Get, it's an indication that should should consider making it a property (in this case a read only one).

Unnecessary variable The fullname variable is unnecessary, you might was well return simply the result of the String.Format.

Alternate for higher .Net versions

Depending on your .Net version, you might also want to use string interpolation instead. If you combine these three things then your GetFullName becomes:

public string FullName { 
                  get { return $"Firstname is: {Firstname}, Surname is {Surname}"; } 
              }

As an aside, usually you would simply expect the combined full name, rather than it being explicitly stated which is which, so you would have:

get { return $"{Firstname} {Surname}"; } 

or using the expression bodied auto property suggested by @Risky Martin you would end up with simply this:

public string FullName => $"{Firstname} {Surname}";

Parameter Validation

It may seem trivial for such a small class, but depending on the use case you should consider if you need to add validation to your constructor. Is it really valid to construct a Person with a null or empty string for example? As has been said by @Rick Davin, do you really want/need the setter for FirstName and Surname to be public, does it make sense for a Person object to change its name? If it does make sense, then again, you should consider if rather than using the auto property you should be performing validation in your set methods.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ C# 6.0 has syntax for even terser property getters: public string FullName => $"{Firstname} {Surname}"; \$\endgroup\$ Commented Jun 12, 2016 at 20:09
  • \$\begingroup\$ @RiskyMartin I hadn't noticed that before, it's quite nice for simple methods like this, thanks. \$\endgroup\$
    – forsvarir
    Commented Jun 12, 2016 at 22:35
0
\$\begingroup\$
public class Person
{
    public Person(string firstname, string surname)
    {
        FullName = $"Firstname is: {firstname}, Surname is {surname}";
    }
    public string FullName { get; private set; }
}

Seeing you class I found you don't need to generate FullName each time calling a method and you don't need any private properties also. Keep it simple.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ private set does not convey immutability the way readonly does. There is no need for FullName to be modified independently from the other properties; a get-only property with a backing field is semantically much closer to the intent. C# 6 introduced immutable get-only properties; this answer could be improved with a C# 6 example IMO. \$\endgroup\$ Commented Jun 13, 2016 at 12:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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