patterncsharpMinor
Get persons full name from their First and Surname
Viewed 0 times
fullpersonsgetnamefirstandfromsurnametheir
Problem
My
Is there anything that can be done better? Anything that is done poorly?
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;
}
}Solution
Naming
Whenever you have a simple method that starts with
Unnecessary variable
The
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
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:
or using the expression bodied auto property suggested by @Risky Martin you would end up with simply this:
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
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.Code Snippets
public string FullName {
get { return $"Firstname is: {Firstname}, Surname is {Surname}"; }
}get { return $"{Firstname} {Surname}"; }public string FullName => $"{Firstname} {Surname}";Context
StackExchange Code Review Q#131774, answer score: 9
Revisions (0)
No revisions yet.