Really simple immutable class

Question

I have a User entity. Each instance of the User class should have a first name, surname and email, and these shall never be assigned values aside from during their initialisation, since they should never change (in the context of my code).

public class User
{
    private string _firstName, _surname, _email;

    public string FirstName => _firstName;
    public string Surname => _surname;
    public string Email => _email;

    public User(string firstName, string surname, string email)
    {
        _firstName = firstName;
        _surname = surname;
        _email = email;
    }
}

As simple as it is, I'd like a quick review of the above since it's how I've started coding up all of my classes in favour of immutability (unless a particular class' property is subject to change).

For example, do you feel I should be using readonly instead? Or some other access modifier?

Also, are there any opinions on how I've declared multiple variables on one line (the privates)?

Answer 1

Make it simpler, you can use read-only properties:

public class User
{
    public string FirstName { get; }
    public string Surname { get; }
    public string Email { get; }

    public User(string firstName, string surname, string email)
    {
        FirstName = firstName;
        Surname = surname;
        Email = email;
    }
}

Now let's consider to make it closed, I do not see any extension point then if you don't have any other requirement against this you should also mark it as not inheritable:

public sealed class User

What next? Make it safer. Your class and its ctor are public then you should validate parameters:

public User(string firstName, string surname, string email)
{
    if (firstName == null)
        throw new ArgumentNullException(nameof(firstName));

    if (surname == null)
        throw new ArgumentNullException(nameof(surname));

    if (email == null)
        throw new ArgumentNullException(nameof(email));

    FirstName = firstName;
    Surname = surname;
    Email = email;
}

If you're using C#7 then consider to use throw exceptions (see pgerchev's comment and t3schb0t's answer).

Class has not to be public? Remove public from its declaration. Constructor is not intended to be called outside your own assembly but class has to be public? Mark ctor as internal and assert about its arguments.

Note about Debug.Assert: obviously this does not mean you won't need to validate somewhere these arguments (see also comments) when building in release mode, just that here you assume they're right. See also Exception Vs Assertion, consider pre/post-conditions not an alternative to validation but - sort of - run-time unit testing:

internal User(string firstName, string surname, string email)
{
    Debug.Assert(firstName != null);
    Debug.Assert(surname != null);
    Debug.Assert(email != null);

    FirstName = firstName;
    Surname = surname;
    Email = email;
}

Do you need any other validation (email has not to be empty and it has to be a valid e-mail address, for example)? Add them all there. Be careful with people's name validation (see David's linked post).

Anything else? Make it easier to debug:

[DebuggerDisplay("{FirstName} {Surname} ({Email})")]
public sealed class User

Last note: yes, there is an [ImmutableObject] attribute you may use to decorate your class but AFAIK it's used only by PropertyGrid control. You may add it as reminder (for you or for future readers) that class has to be immutable but nothing else.

Answer 2

I would like only to show other possibility how "lock" a variable, but Adriano Repetti shows a really nice work in the other post. So, you also might consider using readonly keyword which allows setting to the variable only in the constructor. So this will basically become (plus consider all the other stuff said by Adriano)

public class User
{
    public readonly string FirstName;
    public readonly string Surname;
    public readonly string Email;

    public User(string firstName, string surname, string email)
    {
        FirstName = firstName;
        Surname = surname;
        Email = email;
    }
}

---

EDIT: For clarification, I was addressing only the part about immutability. Without any further considerations about interfaces, reflections etc. as that was not asked for. And given "simplicity" of question it seemed a bit like overthinking.

Answer 3

The new throw expressions in C# 7 are very nice but unfortunatelly they cannot be used when string must not be null or empty. You can workaround this with a helper extension:

public static string NullIfNullOrEmpty(this string str) => string.IsNullOrEmpty(str) ? null : str;

so that you may validate the input with:

FirstName = firstName.NullIfNullOrEmpty() ?? throw new ArgumentNullException(nameof(firstName));

and you don't need an additional if.