19
\$\begingroup\$

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)?

\$\endgroup\$
  • \$\begingroup\$ @200_success - I'm not convinced that edit was necessary. 'user' doesn't really provide any relevant context to this review. It is a User class, but it's with regard to any immutable class at all. \$\endgroup\$ – JᴀʏMᴇᴇ Apr 26 '17 at 14:56
  • 8
    \$\begingroup\$ Think about the long term health of the site. I'm trying to preempt title collisions for the next decade. \$\endgroup\$ – 200_success Apr 26 '17 at 16:15
  • \$\begingroup\$ By the way, in C♯ 8, your entire class may eventually end up looking like this: class User(string FirstName, string Surname, string Email). \$\endgroup\$ – Jörg W Mittag Apr 26 '17 at 20:45
  • \$\begingroup\$ @200_success - surely then my title was not specific enough? Either that or future questions are duplicates? I'm just not convinced the reference to the 'user' context is the right way to make this specific enough. I'm willing to concede, you're more experienced in the management of content than I, just wanted to provide some feedback that's all. \$\endgroup\$ – JᴀʏMᴇᴇ Apr 28 '17 at 8:29
  • \$\begingroup\$ @JörgWMittag - wow! I'm curious to know more about that. \$\endgroup\$ – JᴀʏMᴇᴇ Apr 28 '17 at 8:29
38
\$\begingroup\$

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.

\$\endgroup\$
  • 6
    \$\begingroup\$ Note that in C# 7.0 you can simplify the properties validation even further like this. FirstName = firstName ?? throw new ArgumentNullException(nameof(firstName));. Same applies for the other properties as well of course. \$\endgroup\$ – pgerchev Apr 26 '17 at 11:25
  • 3
    \$\begingroup\$ This really is an excellently concise and thorough answer. I didn't think I'd get such informative feedback on a code review this small and simple. Well played. \$\endgroup\$ – JᴀʏMᴇᴇ Apr 26 '17 at 11:46
  • 5
    \$\begingroup\$ Perfect post, but don't agree at all on the Debug.Assert. Debug.Assert works (by default) only in debug builds, and it's scope is to help during debug. When your code is encountering an unexpected situation the standard and expected behaviour is to raise an exception, no matters if it's an internal, public, or private class, because you need to manage the condition. Your use of asserts instead seems to imply "I will never meet this condition in release because I've already tested every possible situation during debug", that means that the code will crash at the first chance when in production. \$\endgroup\$ – motoDrizzt Apr 26 '17 at 12:56
  • 1
    \$\begingroup\$ @AdrianoRepetti: when it comes to coding everybody has its own style, so if this is ok for you it's ok for me too. I was just pointing out that what you are doing here is the exact reason for bugs to appear when it comes to released software: "I tested everything in debug so the software will never crash" -> next crash in three, two, one... I'm not a huge fan myself of exceptions, by the way (and about recovering at all), but what you are doing here is let loose a code in production that will end up allowing bad data to be gathered. Now all that you need is an overconfident DBA, et voila'... \$\endgroup\$ – motoDrizzt Apr 26 '17 at 13:17
  • 1
    \$\begingroup\$ @IgorSoloydenko That 'fail fast and don't recover' mentality is one of the many reasons that a plethora of users (including other developers, like myself) get extremely aggravated at current software: I lose hours of work in the Microsoft Office products regularly, because they have the 'fail fast and don't recover' mentality. There are other applications I use that are even worse. You can 'fail fast', but you should always try to recover program state. Not doing so is an affront to your users and other developers, regardless of what the program touches. \$\endgroup\$ – Der Kommissar May 1 '17 at 11:54
3
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Technically, you shouldn't throw an ArgumentNullException for an empty string, only a null string. It's a fairly unambiguous nomenclature. However, a subclass, perhaps called StringArgumentNullOrEmptyException would be caught by anything that catches ArgumentNullException as well. \$\endgroup\$ – Jesse C. Slicer May 1 '17 at 18:49
  • \$\begingroup\$ @JesseC.Slicer an empty string is usally as usefull as a null string so I usally treat them as one. There is rarely a case where you can continue with an empty string and still get valid results. \$\endgroup\$ – t3chb0t May 1 '17 at 18:55
3
\$\begingroup\$

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.

\$\endgroup\$
  • 9
    \$\begingroup\$ Except these are now fields and it is generally discouraged to have public fields. \$\endgroup\$ – TheLethalCoder Apr 26 '17 at 13:05
  • \$\begingroup\$ Final public fields are pretty much exactly the same as using getter-only properties (Although many programmers still react negatively because they have been taught a pattern and haven't reasoned it through themselves--also sometimes it's just easier and safer to blindly follow a pattern). However, this code has the advantage of making the fact that the fields are intended to be immutable obvious (if they were not read only, one might not think to treat them as immutable when modifying the User class). The ideal solution would be to add readonly to Adriano Repetti's solution. \$\endgroup\$ – Bill K Apr 26 '17 at 19:29
  • 2
    \$\begingroup\$ @BillK They are not and this solution has a very important downside namely you won't be able to write an interface for it because interfaces can only have properties and not fields. \$\endgroup\$ – t3chb0t Apr 26 '17 at 19:35
  • \$\begingroup\$ @t3chb0t As I said, the best solution would be to add readonly to the other solution, and your point is valid, but not a situation I've ever encountered. If I found myself writing an interface for properties I'd have to seriously question my design for a bit. \$\endgroup\$ – Bill K Apr 26 '17 at 19:41
  • 2
    \$\begingroup\$ Bill, a property with only a getter IS (has) a readonly field as backing store. You can't reassign them outside ctor. Hmmm don't you ever had an interface with one property?! Well, from an academic point of view I agree they don't offer any better incapsulation than fields but I prefer consistency with when I need them to validate (for example) the input. \$\endgroup\$ – Adriano Repetti Apr 26 '17 at 19:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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