Settings class for web app

Question

I needed to write a settings class for my web app; ideally this class should read its settings from a file (and be able to save them back to the file), and have only 1 instance per application - all threads read the same object, so that the settings only need to be loaded from the file when the app runs for the first time.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.ComponentModel;
using System.Security.Cryptography;
using System.Configuration;
using System.Xml.Serialization;
using System.Web.Caching;
using System.IO;

namespace Secure_Password_Repository.Settings
{
    [Serializable]
    public sealed partial class AppSettings
    {
    private static readonly XmlSerializer serial = new XmlSerializer(typeof(AppSettings));
    private static AppSettings instance;
    private static Object thisLock = new Object();

    private static void serializeToDisk(string key, CacheItemUpdateReason reason, out Object expensiveObject, out CacheDependency dependency, out DateTime absoluteExpiration, out TimeSpan slidingExpiration)
    {

        string filename = HttpContext.Current.Server.MapPath("settings.xml");

        expensiveObject = instance;
        dependency = new CacheDependency(filename);
        absoluteExpiration = Cache.NoAbsoluteExpiration;
        slidingExpiration = Cache.NoSlidingExpiration;

        using (StreamWriter sw = new StreamWriter(filename))
        {
        serial.Serialize(sw, instance);
        }
    }

    public static void Save()
    {
        string filename = HttpContext.Current.Server.MapPath("settings.xml");
        using (StreamWriter sw = new StreamWriter(filename))
        {
        serial.Serialize(sw, instance);
        }
    }

    public static AppSettings Default
    {
        get
        {

        Cache cache = HttpRuntime.Cache;
        string filename = HttpContext.Current.Server.MapPath("settings.xml");

        if (cache[filename] != null) {
            return (AppSettings)cache[filename];
        }

        lock (thisLock)
        {

        if (cache[filename] != null) {
            return (AppSettings)cache[filename];
        }

            using (StreamReader sr = new StreamReader(filename))
            {
            instance = (AppSettings)serial.Deserialize(sr);
            cache.Insert(filename, instance, null, Cache.NoAbsoluteExpiration, Cache.NoSlidingExpiration);

            if (instance.EncryptionKeySalt == "")
            {
                //random number generator
                RNGCryptoServiceProvider rngCsp = new RNGCryptoServiceProvider();

                //create 20 random bytes for the salt
                byte[] CryptoBytes = new byte[40];
                rngCsp.GetBytes(CryptoBytes);

                rngCsp.Dispose();

                instance.EncryptionKeySalt = Convert.ToBase64String(CryptoBytes);
            }

            return (AppSettings)cache[filename];
            }
        }

        }
    }

    public string LogoImage{ get; set; }

    public string DefaultAccountRole{ get; set; }

    public string SMTPServerAddress{ get; set; }

    public string SMTPServerUsername{ get; set; }

    public string SMTPServerPassword{ get; set; }

    public string EncryptionKeySalt{ get; set; }

    }
}

I'm just curious: is this "good code"? Are there any potential pitfalls I'm not seeing? is there anything I could be doing better?

Welcome to Code Review! Here, receive an up-vote as a thank you for apparently understanding what we are about! — Simon Forsberg
– Simon Forsberg, Commented Jun 11, 2014 at 11:17
Many thanks... You'd think "code review" in the domain would make it obvious ;) — binks
– binks, Commented Jun 11, 2014 at 11:53

Nick Udell · Accepted Answer · 2014-12-02 16:12:12Z

1

Small issues:

Personally I'd indent things in between by curly braces
I'd use string.Empty instead of ""
I'd probably put my default public properties before my custom ones

As a higher level question, do you need to use an XML settings file with an explicit lock? Could you not use a custom section in .config file of some kind?

edited Dec 2, 2014 at 16:12

Nick Udell

5,2471 gold badge29 silver badges68 bronze badges

answered Jun 11, 2014 at 8:01

dougajmcdonald

4642 silver badges5 bronze badges

Add a comment |

Nick Udell · Accepted Answer · 2014-12-03 10:37:31Z

Var

Prefer the var keyword when declaring local variables if the right hand side of the declaration makes the variable's type obvious.

StreamWriter sw = new StreamWriter(filename)

Should be:

var sw = new StreamWriter(filename)

Doing this makes it easier to change variable type later, and makes it more concise and easy to read for others.

Naming

Avoid using acronyms or shortened forms of words in variable, type and method names. By using the longer form, it is much clearer and easier for a maintenance programmer to understand.

StreamWriter sw = new StreamWriter(filename)

Should be:

StreamWriter writer = new StreamWriter(filename)

Braces

Opening curly braces should be on their own line and, like dougajmcdonald suggests, the contents of the braces should be indented.

Structure

This method is a mess, it has four out parameters, and no return type!

private static void serializeToDisk(string key, CacheItemUpdateReason reason, out Object expensiveObject, out CacheDependency dependency, out DateTime absoluteExpiration, out TimeSpan slidingExpiration)
{

    string filename = HttpContext.Current.Server.MapPath("settings.xml");

    expensiveObject = instance;
    dependency = new CacheDependency(filename);
    absoluteExpiration = Cache.NoAbsoluteExpiration;
    slidingExpiration = Cache.NoSlidingExpiration;

    using (StreamWriter sw = new StreamWriter(filename))
    {
    serial.Serialize(sw, instance);
    }
}

An out parameter is (unless there's a very good reason to have it) a strong code smell. Refactor to have a single type returned (most likely a bespoke class containing your out parameters), or split the method into four.

Secondly, in that function the parameter key appears to be doing nothing at all.

Validation

You don't validate any of your property setters. You should check, at the bare minimum, for null values, and anything else that could throw an exception and report it at the source.

This method (serializeToDisk) can be removed, as it isn't used anywhere. — Heslacher
– Heslacher, Commented Dec 3, 2014 at 10:45
I assumed they intended to use it later, but it is definitely worth mentioning. — Nick Udell
– Nick Udell, Commented Dec 3, 2014 at 10:49

Heslacher · Accepted Answer · 2014-12-03 11:09:53Z

remove dead (unused) code. Here serializeToDisk() method can be removed as it isn't used anywhere.

inside the property Default this

if (instance.EncryptionKeySalt == "")
{
    //random number generator
    RNGCryptoServiceProvider rngCsp = new RNGCryptoServiceProvider();

    //create 20 random bytes for the salt
    byte[] CryptoBytes = new byte[40];
    rngCsp.GetBytes(CryptoBytes);

    rngCsp.Dispose();

    instance.EncryptionKeySalt = Convert.ToBase64String(CryptoBytes);
}

is bad style. Instead of comparing to "" you should compare to String.Empty. Also extracting this to a separate method would be much cleaner like

private void AssignDefaultKeySalt() { if (!String.IsNullOrWhiteSpace(EncryptionKeySalt)) { return; }

using (RNGCryptoServiceProvider rngCsp = new RNGCryptoServiceProvider())
{
    byte[] CryptoBytes = new byte[40];
    rngCsp.GetBytes(CryptoBytes);
    instance.EncryptionKeySalt = Convert.ToBase64String(CryptoBytes);
}

}

using the using statement for objects which implements IDisposable is recommended.

comments should comment on why something is done, the what is done should be done by the code itself using meaningful names for classes, methods and parameters. Also comments are needed to be maintained also. Wrong comments are useless like
```
 //create 20 random bytes for the salt
byte[] CryptoBytes = new byte[40];
```

Jesse C. Slicer · Accepted Answer · 2014-12-03 16:23:05Z

0

Just a couple things about this line:

private static Object thisLock = new Object();

First, prefer the C# alias object over the framework Object:

private static object thisLock = new object();

Second, since you or your code maintainers shouldn't be modifying that member after construction, mark it readonly:

private static readonly object thisLock = new object();

answered Dec 3, 2014 at 16:23

Jesse C. Slicer

14.6k1 gold badge40 silver badges54 bronze badges

Add a comment |

Stack Exchange Network

Settings class for web app

4 Answers 4

You must log in to answer this question.

Hot Network Questions

Settings class for web app

4 Answers 4

You must log in to answer this question.

Related

Hot Network Questions