4
\$\begingroup\$

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?

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

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?

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ This method (serializeToDisk) can be removed, as it isn't used anywhere. \$\endgroup\$ – Heslacher Dec 3 '14 at 10:45
  • \$\begingroup\$ I assumed they intended to use it later, but it is definitely worth mentioning. \$\endgroup\$ – Nick Udell Dec 3 '14 at 10:49
1
\$\begingroup\$
  • 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];
    
\$\endgroup\$
0
\$\begingroup\$

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();
\$\endgroup\$

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.