4
\$\begingroup\$

I have a private field called individualProfile (camelCase) and I've created a public property called IndividualProfile (PascalCase) on it:

private Profile individualProfile;
public Profile IndividualProfile
{
    get
    {
        if (individualProfile == null)
        {
            individualProfile = ProfileFacade.Instance.GetMainProfileByType
                                (
                                    PortalContext.CurrentUserId,
                                    ProfileType.IrnicIndividual
                                );
            if (individualProfile == null)
            {
                individualProfile = new Profile()
                {
                    FirstName = string.Empty,
                    Family = string.Empty,
                    Details = new IrnicIndividual()
                    {
                        NationalCode = string.Empty
                    }
                };
            }
        }
        return individualProfile;
    }
}

I have to use this public property in almost 10 places, so I first check the private member to see if it's null and load it only once, so that I load it only once and I cache it and use it 10 times. Also, since GetMainProfileByType can return null value, I check another time to see if the private member is null or not. If it's null, I simply set it to an empty object, so that I won't bother checking null values when I bind this object to my view.

Is this code efficient and maintainable? Is there anything I've overlooked?

\$\endgroup\$

3 Answers 3

6
\$\begingroup\$

Good thinking about the caching and efficiency. I have two minor suggestions

1 - Enforce SRP (single responsibility principal), which basically states (according to Robert C. Martin) that in the world of "Clean Code" classes must have one reason to change for better maintainability, which is one of the concerns you had in your question. So to apply this to your case, the class that contains the Profile property need not be changed if you decide to change the signature of the ProfileFacade.Instance.GetMainProfileByType method, or maybe choose another method to populate the Profile. So maybe refactoring that code out of the class would be a good idea. Here is what I mean by that:

private Profile _individualProfile;
public Profile IndividualProfile
{
    get
    {
        if (_individualProfile != null) return _individualProfile;

        return (_individualProfile = ProfileFacade.Instance.GetIrnicProfile() ?? Profile.NullProfile);
    }
}

As you can see in the code snippet I used the short hand null check using the ?? null-coalescing operator.

2 - NULL object pattern. Basically define the neutral state (or NULL state) of your object inside the object itself, benefiting from not changing the calling code if ou decide to redefine what the NULL means for your object. In my code snippet I added Profile.NullProfile which is defined as such:

public class Profile
{
    public string FirstName { get; set; }
    // Other properties here
    public static Profile NullProfile
    {
        get
        {
            return new Profile
                        {
                            FirstName = String.Empty,
                            // etc...
                        };
        }
    }
}

Other than that... Good work!

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Your first code block is convoluted and hard to skim, thus bad from a maintainability point of view. This ruins your answer, IMO. I would suggest to keep the original if null assign; afterwards return approach. \$\endgroup\$
    – ANeves
    Oct 25, 2011 at 15:07
  • \$\begingroup\$ The succinct style I use is orthogonal to the answer, I do not believe it takes away from the validity of the answer. I had similar reaction when I first realized how versatile and multi-paradigm C# can be, after all there's a lot of personal/individual style involved in coding. \$\endgroup\$ Oct 25, 2011 at 18:15
  • \$\begingroup\$ This being codereview style is not orthogonal to answers :) , and the question also asks whether the code is maintainable. But other than that, I still think your answer is very good. \$\endgroup\$
    – ANeves
    Oct 26, 2011 at 9:38
2
\$\begingroup\$

Looks perfect to me.

The only comment I have is that, assuming an empty Profile makes sense, the String.Empty default on the properties could be set by the constructor - rather than you setting them explicitly.

\$\endgroup\$
2
\$\begingroup\$

Do you have .NET 4 at your disposal? If so, you can use the Lazy<T> as such:

    private readonly Lazy<Profile> individualProfile =
        new Lazy<Profile>(() =>
        {
            var newProfile = ProfileFacade.Instance.GetMainProfileByType(
                PortalContext.CurrentUserId,
                ProfileType.IrnicIndividual);

            return newProfile ?? new Profile
            {
                FirstName = string.Empty,
                Family = string.Empty,
                Details = new IrnicIndividual
                {
                    NationalCode = string.Empty
                }
            };
        });

    [DebuggerBrowsable(DebuggerBrowsableState.Never)]
    public Profile IndividualProfile
    {
        get
        {
            return individualProfile.Value;
        }
    }
\$\endgroup\$

Your Answer

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

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