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.
Firstname
andSurname
public with a private setter. \$\endgroup\$Surname, Forename
rather thanForename Surname
, or I want to perform sorting on either property, etc. \$\endgroup\$