views:

884

answers:

2

OK so im trying to create a settings class to store certain strings that i need to access throughout the system. I have created a globalSettings.cs with the code below:

public class GlobalSettings
{
    private readonly Hashtable myHT;

    public GlobalSettings()
    {
        //Hashtable used to store global strings
        myHT = new Hashtable();
        myHT.Add("logCategory","TMBC"); //event log category
        myHT.Add("logSource", "MVC"); //event log source


        //setup required options

        //Create log source if required
        if (!EventLog.SourceExists(myHT["logSource"].ToString()))
        {
            EventLog.CreateEventSource(myHT["logSource"].ToString(), myHT["logCategory"].ToString());
        }

    }

    public string getSetting(string key)
    {
        return myHT.ContainsKey(key) ? myHT[key].ToString() : null;
    }
}

At the moment i have initialised this class in each one of my controllers with the following:

protected GlobalSettings globalSettings = new GlobalSettings();

**Should i set the constructor to private and implement the singleton pattern as it is afterall a settings class and only need one instance?

Would i be better off extending the controller class with the setting information in it?

**

+2  A: 

Personally, I'd rather compartmentalize those things. For example, why do all your controllers need to know about writing event logs? I'd have a single LogWriter class and ILogWriter interface, and use dependency injection (see MVCContrib for samples) - i.e.

class FooController : Controller {
    private readonly ILogWriter logWriter;
    public FooController(ILogWriter logWriter) {
        this.logWriter = logWriter; // <==== edited for clarity
    }
}

(and using a DI-based controller-factory)

This allows you to unit test the log-writing by mocking the log-writer. Then the settings would fit reasonably well as constants (or fetched from config) inside the LogWriter class.


Re the specific question; if all the values are constant, use constants (or maybe static properties):

public static GlobalSettings
{
    public static const string LogCategory = "TMBC"; //event log category
    public static const string LogSource = "MVC"; //event log source
}

A dictionary would be useful if they are fetched from configuration; if they are truly global, a static dictionary should suffice - instances would only be useful if it changes between impressions. A singleton would serve no purpose here; use static members instead.

Marc Gravell
so pass the log writer and globalsettings through with the constructer as and when i need them yes? class FooController : Controller { public FooController(ILogWriter logWriter,GlobalSettings mySettings) {/* store it */}}
Andi
In the log-writer scenario, I'm saying you don't **need** a global settings object; leave that logic local to the log-writer.
Marc Gravell
ok, so would you still initialise the global settings class like i did but just make sure not to include log code in their (and use constants not a hash table)?
Andi
I would only include the GlobalSettings class if it did something useful... in the "encapsulate this in a single class" approach (LogWriter), there is no need to repeat it in the GlobalSettings. The GlobalSettings with constants was an *alterative* approach; not part of the LogWriter approach.
Marc Gravell
I'm a bit lost whenever i call code cimilar to below and have a class passed with the constructor but when i try and access logWriter.myMethod() it says logWriter doesnt exist? class FooController : Controller { public FooController(ILogWriter logWriter) {/* store it */}}
Andi
Did you add a field to "store it" in...?
Marc Gravell
posted below my code so far, thanks for your time!!
Andi
A: 

@Marc Gravell

So far i have my main controller:

    public class TasksController : Controller
{
    private tasklistDataContext db = new tasklistDataContext();

    public TasksController(ILogWriter myWriter)
    {
        /* constructor */
    }


    //displays list of tasks
    public ActionResult Index()
    {
        ViewData["Message"] = "Task List";



        IOrderedQueryable<task> tasks = from t in db.tasks orderby t.entryDate descending select t;

        return View(tasks.ToList());
    }


}

The ILogWriter class so far is below:

    public class ILogWriter
{
    public static string logCategory;
    public static string logSource;

    public ILogWriter()
    {

        logCategory = "TMBC";
        logSource = "MVC";

        //Create log source if required
        if (!EventLog.SourceExists(logSource))
        {
            EventLog.CreateEventSource(logSource, logCategory);
        }

    }

    public void writeLog(string eventMsg)
    {
        EventLog.WriteEntry(logSource, eventMsg, EventLogEntryType.Error);
    }

}

i know this should be simple but i'm so unfamiliar with .NET i'm finding it a bit challenging at the moment :)

Andi