1

Here is my situation. I have implemented the accepted .NET 4.0 answer in this question, which is working.

However, my codebase is quite large and as responsible for this code I've started getting inquiries regarding this solution. I've been asked if there are no easier ways of accomplishing the same thing, and if there is any way to shorten the usage.

In almost all cases, this class is used to get or set a single property only. This favors other solutions, since the usage ends up like this: new UserSessionData().CustomerNumber = 123;

The closest thing I have is a class (static or not) with static members and methods, like this:

public static class UserSessionData
{
    public static Dictionary<string, string> AsDictionary()
    {
        return typeof(UserSessionData).GetProperties()
                   .ToDictionary(p => p.Name, p => p.GetValue(null, null) as string);
    }
    private static string Get(Expression<Func<string>> e)
    {
        var me = e.Body as MemberExpression;
        return ExternalInterfaceClass.Get(me.Member.Name);
    }
    private static void Set(Expression<Func<string>> e, string value)
    {
        var me = e.Body as MemberExpression;
        ExternalInterfaceClass.Set(me.Member.Name, value);
    }

    public static string CustomerNumber {
        get { return Get(() => CustomerNumber); }
        set { Set(() => CustomerNumber, value); }
    }

    // ... and hundreds more properties similar to "CustomerNumber"
}

This feels like a quite good solution:

  • It's strongly typed (i.e. I can easily refactor CustomerNumber throughout)
  • Usage is great (UserSessionData.CustomerNumber = 123;)
  • The boilerplate is not that ugly - I think it's acceptable

However, there is one issue that I can't wrap my head around. Sometimes we need to be able to use this a little differently, A) as a return type and B) bulk-editing the structure before saving. As for A, I understand that a static class cannot be used as a return type, however I don't know what my alternatives would be in this specific situation?

As for B, I guess that I could delay the actual saving by providing a stated subclass and requiring explicit save, but the usage would be worsened for the normal use case (setting or getting a single property) since every one-liner would have to be followed by a call to the save method.

I thought about creating an interface for the class but interfaces can't define static properties. I'm feeling a little muddled here, help is appreciated. Please provide a minimal code example when presenting your thoughts, if possible - it clarifies the point a lot.

migrated from codereview.stackexchange.com Jan 29 '14 at 15:04

This question came from our site for peer programmer code reviews.

  • 2
    Have you considered the Singleton Pattern for (A)? – amon Jan 29 '14 at 11:08
  • I'd use abstract instance methods or an interface together with dependency injection. Accessing global state through static members is generally a bad idea. – CodesInChaos Jan 29 '14 at 15:20
2

It's not exactly an answer to your question but have you looked into T4 templates?

I get the impression you are manually coding those boilerplate properties. If they do indeed follow an almost identical pattern and you would like strong typing try writing code that generates code...

A T4 allows you to iterate through your items and essentially build up a code file, which will be evaluated as you compile, this would mean as you add/remove items to/from your session data it will add/delete those accessors and keep your code up to date and allow you to only set your design standard in one place and have it auto propagate in a loop across all future properties.

A common example of why/where you would use this would be a roll-your-own ORM systems to map a table to a database table without a huge library if you don't need all the CRUD operations...

1

First off: It seems weird that the class is abstract. An abstract class is meant to be a base class which cannot be instantiated and contains some abstract methods which need to implemented by derived classes. If all your methods and properties on the class are static and you don't want the class to be able to be instantiated then you can simply make the class static too:

public static class UserSessionData
{
    ... // all static methods and properties and fields

Secondly: Don't do it like this. Seriously. Don't.

Your example code shows that your UserSessionData class also accesses an ExternalInterfaceClass which also seems to be a static class. This is getting real nasty here - global state galore. Unit testing code like this is hugely painful (been there done that).

I don't see the reason why the class should be static. There is nothing wrong with having an instance of it which being passed around. Going even further the class should get the external interface injected as a dependency so you can mock it out. Incidentally this could provide you wit ha solution to your bulk editing problem.

Let's try something:

public class UserSessionData : IUserSessionData
{
    private IExternalInterface _ExternalInterface;

    public UserSessionData(IExternalInterface externalInterface)
    {
        _ExternalInterface = externalInterface;
    }

    protected virtual string Get(Expression<Func<string>> e)
    {
        var me = e.Body as MemberExpression;
        return _ExternalInterface.Get(me.Member.Name);
    }

    protected virtual void Set(Expression<Func<string>> e, string value)
    {
        var me = e.Body as MemberExpression;
        _ExternalInterface.Set(me.Member.Name, value);
    }

    public Dictionary<string, string> AsDictionary()
    {
        return typeof(UserSessionData).GetProperties()
                   .ToDictionary(p => p.Name, p => p.GetValue(this) as string);
    }

    public IBulkEditableUserSessionData AsBulkEditable()
    {
        return new BulkEditableUserSessionData(_ExternalInterface);
    }

    public string CustomerNumber {
        get { return Get(() => CustomerNumber); }
        set { Set(() => CustomerNumber, value); }
    }

    // ... hundreds of properties

    private class BulkEditableUserSessionData : UserSessionData, IBulkEditableUserSessionData 
    {
        private Dictionary<string, string> _EditCache = new Dictionary<string, string>();

        public BulkEditableUserSessionData(IExternalInterface externalInterface)
            : base(externalInterface)
        {
        }

        protected override string Get(Expression<Func<string>> e)
        {
            var fieldName = ((MemberExpression)e.Body).Member.Name;
            // return modified value from cache if present
            string value;
            if (_EditCache.TryGet(fieldName, out value))
            {
                return value;
            }
            return _ExternalInterface.Get(fieldName);
        }

        protected virtual void Set(Expression<Func<string>> e, string value)
        {
            var fieldName = ((MemberExpression)e.Body).Member.Name;
            _EditCache[fieldName] = value;
        }

        public void Save()
        {
            foreach (var kvp in _EditCache)
            {
                _ExternalInterface.Set(kvp.Key, kvp.Value);
            }
        }
    }
}


public interface IExternalInterface
{
    string Get(string fieldName);
    void Set(string fieldName, string value);
}

public interface IUserSessionData
{
    Dictionary<string, string> AsDictionary();

    string CustomerNumber { get; set; }

    // .. all the other hundreds of properties
}

public interface IBulkEditableUserSessionData : IUserSessionData
{
    void Save();
}

Intended usage:

At one point in the application life cycle (presumably somewhere in the initialization phase) a UserDataSession instance gets created and the external interface injected.

Any code which needs access to the user session data gets given that instance (e.g. passed through as constructor parameter) and can use it like this:

public class BusinessClass
{
    private IUserSessionData _UserSessionData;

    public BusinessClass(..., IUserSessionData userSessionData, ...)
    {
        _UserSessionData = userSessionData;
    }

    public void UpdateCustomerNumber(string newCustomerNumber)
    {
        _UserSessionData.CustomerNumber = newCustomerNumber;
    }

    public void UpdateManyProperties()
    {
        var bulkEdit = _UserSessionData.AsBulkEditable();

        bulkEdit.CustomerNumber = ...
        bulkEdit.CustomerPhone = ...
        bulkEdit.CustomerWhatever = ...

        bulkEdit.Save();
    }
}

The design can be refined and probably needs adjustment based on your use cases but should illustrate the idea.

Slight downside is that you have to add new properties to the class and the interface but that should not be too much of a problem. If you forget to add it to the interface your users will be complaining that it's not available. And if you forget to add it to the implementation the compiler will complain.

  • Thanks for your suggestion. I've changed the class to static in my question - the abstract keyword was actually a leftover from experimenting with a solution similar to the one you are suggesting. The boilerplate gets quite hefty though, which is why I abandoned this path: I'm thinking other devs will whine. But of course - if there is no better solution which covers the usage I'm after, then they won't be able to come up with a better solution either. – Simeon Feb 11 '14 at 9:01
  • @Simeon Hm, I don't know. The boilerplate code doesn't look that bad to me. From experience I'd say the static class approach will cause you much more long term pain and suffering. I've seen these situations and almost always at some point the decision was made to embark on a painful refactoring way to get rid of it. Better to not start it in the first place. – ChrisWue Feb 11 '14 at 23:57

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.