Key value flat file system - single header INI replacement

Question

I'm very happy with the performance, but I always try to optimize my code as much as possible. Since I plan on using the code in production, I feel more save posting it here first. I probably missed something obvious that could break everything.

Let's start with the KVF class. Its job is loading / saving values to the KVC class.

public class KVF :IDisposable
{
    internal string Filename;
    internal KVC Cache;

    internal bool Debugoutput;

    /// <summary>
    /// Initializes KVF values.
    /// Generates KVF file in memory for handling.
    /// </summary>
    /// <param name="filename">The file to load key/value pairs from.</param>
    /// <param name="debugoutput">Specify whether the program is being debugged. <i>default: false</i></param>
    public KVF(string filename,bool debugoutput = false, string kvsplit = "=")
    {
        Debugoutput = debugoutput;
        if (debugoutput)
            Console.WriteLine("[KVF] Concstructor Invoked. New File Handle: {0}", filename);
        Filename = filename;
        Cache = new KVC(this);
        Cache.Generate(kvsplit);
    }

    /// <summary>Writes a value to the specified key (caller).</summary>
    /// <param name="value">The value to write.</param>
    /// <param name="caller">The calling member to write as key.</param>
    public void MagicSave(object value, [CallerMemberName] string caller = null)
    {
        if (caller == null)
            throw new Exception("Caller was null!");

        if (Cache.Contains(caller))
            Cache.KeyValue[caller] = value;
        else
        {
            Cache.LookupTable.Add(caller);
            Cache.KeyValue.Add(caller, value);
        }

        if (Debugoutput)
            Console.WriteLine("[KVF] MagicSave Invoked: {0}={1}", caller, value);
    }

    /// <summary>Writes a value using the specified value to the specified key.</summary>
    /// <param name="key">The key containg the value.</param>
    /// <param name="value">The value to write.</param>
    public void Save(string key, object value)
    {
        if (key == null)
            throw new Exception("Key was null!");

        if (Cache.Contains(key))
            Cache.KeyValue[key] = value;
        else
        {
            Cache.LookupTable.Add(key);
            Cache.KeyValue.Add(key, value);
        }

        if (Debugoutput)
            Console.WriteLine("[KVF] Save Invoked: {0}={1}", key, value);
    }

    public T MagicLoad<T>([CallerMemberName] string key = null)
    {
        if (key == null)
            throw new Exception("Caller was null!");

        if (Debugoutput)
            Console.WriteLine("[KVF] MagicLoad Invoked: {0}", key);

        if (Cache.Contains(key))
            return (T)Convert.ChangeType(Cache.KeyValue[key], typeof(T));

        Cache.KeyValue.Add(key, default(T));
        Cache.LookupTable.Add(key);
        return default(T);
    }
    public T MagicLoad<T>(T defaultval,[CallerMemberName] string key = null)
    {
        if (key == null)
            throw new Exception("Caller was null!");

        if (Debugoutput)
            Console.WriteLine("[KVF] MagicLoad Invoked: {0}", key);

        if (Cache.Contains(key))
            return (T)Convert.ChangeType(Cache.KeyValue[key], typeof(T));

        Cache.KeyValue.Add(key, defaultval);
        Cache.LookupTable.Add(key);
        return defaultval;
    }

    /// <summary>Reads a value from the speified key. Returns as specified data type (T).</summary>
    /// <typeparam name="T">The data type to return as.</typeparam>
    /// <param name="key">The key to read from.</param>
    public T Load<T>(string key)
    {
        if (Debugoutput)
            Console.WriteLine("[KVF] Load Invoked: {0}", key);

        if (Cache.Contains(key))
            return (T)Convert.ChangeType(Cache.KeyValue[key], typeof(T));

        Cache.LookupTable.Add(key);
        Cache.KeyValue.Add(key, default(T));
        return default(T);
    }

    public T Load<T>(string key,T defaultval)
    {
        if (Debugoutput)
            Console.WriteLine("[KVF] Load Invoked: {0}", key);

        if (Cache.Contains(key))
            return (T)Convert.ChangeType(Cache.KeyValue[key], typeof(T));

        Cache.KeyValue.Add(key, default(T));
        Cache.LookupTable.Add(key);
        return defaultval;
    }

    /// <summary>Rebuilds the KVF cache.</summary>
    public void RefreshCache()
    {
        if (Debugoutput)
            Console.WriteLine("[KVF] Rebuilding Cache: " + Filename);
        Cache.KeyValue.Clear();
        Cache.LookupTable.Clear();
        Cache.Generate();
    }

    /// <summary>Commits the cache to the hard drive.</summary>
    public void Commit()
    {
        Cache.Commit();
    }

    /// <summary>Disposes of KVF and KVC resources.</summary>
    public void Dispose()
    {
        if (Debugoutput)
            Console.WriteLine("[KVF] Disposing: " + Filename);
        Cache.Commit();
        Cache.File = null;
        Cache = null;
    }
}

The KVC class (Key Value Cache) keeps everything in RAM, and writes it out on disposal.

internal class KVC
{
    internal KVF File;
    internal string Splitter;
    internal readonly Dictionary<string, object> KeyValue;
    internal readonly HashSet<string> LookupTable;

    /// <summary>
    /// Initializes a new KVF cache to handle the file in memory.
    /// Creates file if non-existent.
    /// </summary>
    /// <param name="file">The KVF file to generate the cache for.</param>
    public KVC(KVF file)
    {
        if (!System.IO.File.Exists(file.Filename))
            System.IO.File.Create(file.Filename).Close();
        File = file;
        KeyValue = new Dictionary<string, object>(StringComparer.Ordinal);
        LookupTable = new HashSet<string>(StringComparer.Ordinal);
        if (File.Debugoutput)
            Console.WriteLine("[KVC] Concstructor Invoked. New Cache Handle: {0}", File.Filename);
    }

    /// <summary>Generates the file for handling in memory.</summary>
    public void Generate(string splitter = "=")
    {
        Splitter = splitter;
        using (var stream = System.IO.File.OpenText(File.Filename))
        {
            while (!stream.EndOfStream)
            {
                var line = stream.ReadLine();
                // ReSharper disable once PossibleNullReferenceException
                var keyvalue = line.Split(new [] { splitter },StringSplitOptions.RemoveEmptyEntries);
                if (keyvalue.Length < 2)
                    continue;

                KeyValue.Add(keyvalue[0], keyvalue[1]);
                LookupTable.Add(keyvalue[0]);
            }
        }
    }

    public bool Contains(string value)
    {
        return Enumerable.Contains(LookupTable, value);
    }

    /// <summary>Flushes the cache to the hard drive.</summary>
    public void Commit()
    {
        using (var stream = System.IO.File.Open(File.Filename, FileMode.Truncate, FileAccess.Write))
        {
            using (var writer = new StreamWriter(stream))
            {
                writer.WriteLine("--- KVF File Format 1.2 by Yuki ---");
                foreach (var kvp in KeyValue)
                {
                    writer.WriteLine("{0}"+Splitter+"{1}", kvp.Key, kvp.Value);
                }
            }
        }
    }
}

I don't mind exceptions on parsing, e.g. no valid splitter. My main concern is speed. I can parse 12.000 of those (http://pastebin.com/amQW5T1Z) in 600ms. That's 2.3x faster than Interop INI (Read/WriteProfileString).

Answer 1

Let's start with your priorities: optimization. You should be primarily optimizing for the most expensive and limited resource, which is almost always developer brain cycles. You optimize for that resource by writing clean, readable, well-organized code with few surprises. Your code can be improved on all of these fronts.

As far as CPU or hard-drive performance goes: do you need to parse 20.000 key-value-pair files per second? If you really need that level of processing of persistent data, I recommend using a more advanced persistence layer such as a database.

Naming

When naming something, your first priority is accuracy, your second is clarity, and your third is convention. Lets look at some of your choices.

public class KVF :IDisposable

When you use abbreviation, C# convention is that you do not capitalize each letter (although it's acceptable to break this convention for 2-letter abbreviations such as IO). Kvf would be a better name for this class, but it's still not great. Your class name is the first thing I look at when I try to understand it. In this case I get no information whatsoever, because all the important info is abbreviated out. Don't use abbreviations unnecessarily. KeyValueFile would be a much better name for this class. I would argue that it still needs improvement, because it's not really representing a file at all.

internal bool Debugoutput;

Always capitalize each word (after the first) in multi-word names. Properties and methods start with a capital letter. I personally also like instance variables to start with a capital letter as well (unless they're wrapped by a property), but I don't know how common this is. A better name for this would be DebugOutput or debugOutput. I also like booleans to be named so that they read well in an if statement. For this reason I think InDebugMode or something similar is preferable. We'll get back to internal.

public void MagicSave

In general "clever" names like this are unhelpful and make you look unprofessional. It needs to be very clear what differentiates Save from MagicSave. It's difficult for me to recommend a better name for this method because I don't think it should exist at all. Its behavior would surprise me, and surprises are bad. I can't imagine wanting your class to make some decision about what keys I want to use on my behalf, especially with a name that is very likely to break after a refactor.

Design

What is the fundamental difference between KVC and KVF? It's not clear to me at all, and there are several red flags that their responsibilities are not very well partitioned, such as Cache = new KVC(this); and the fact that they both make use of each others' instance variables. I can't see any reason why these should be two different classes.

public KVF(string filename,bool debugoutput = false, string kvsplit = "=")

A constructor is an important communication tool: it's a statement of each dependency that is required in order for the class to do its job. The debugoutput parameter is not something KVF needs to do its job. Remove it and learn to use a debugger instead. I'm also not a fan of optional parameters, especially in constructors. Do you ever really use any other splitter than "="? This looks like a violation of YAGNI (you ain't gonna need it).

Don't mark instance variables as internal. Is it really OK if any other class in your assembly changes KVF.Filename? Sets KVF.Cache to null? If you need different visibility for reading and writing, use properties:

public string FileName { get; private set; }

Avoid silently failing on invalid input, like this:

if (keyvalue.Length < 2)
    continue;

If I asked you to deserialize a file in an invalid format, I would expect you to throw an exception and let me know.

public void Save(string key, object value)

This method signature indicates that you can save any object I give you, but that's a lie. In fact you can only save types that Convert.ChangeType can handle. If you want compile safety, create a separate overload for each supported type. If runtime safety is good enough, make Save generic and throw an exception if its generic argument is not something you support.

if (key == null)
    throw new Exception("Caller was null!");

Don't throw exceptions of type Exception. Be more specific. This should be an ArgumentNullException. For more general invalid arguments throw ArgumentException.

Performance

If you're trying to process 20.000 of these things, your bottleneck is definitely hard-drive access. Use a database instead.

writer.WriteLine("{0}"+Splitter+"{1}", kvp.Key, kvp.Value);

String concatenation is generally slow. You're already using string formatting parameters here; just take it one step further:

writer.WriteLine("{0}{1}{2}", kvp.Key, Splitter, kvp.Value);

The last obvious performance issue is all the places where if (debugoutput) is sprinkled. Presumably you will never be debugging your output in production, so you're executing a useless branch. This isn't a huge performance concern (especially with branch prediction) but given that it's also a clarity problem and is bleeding into your class constructor, get rid of it and use a debugger instead.

IDisposable

I have this as its own section because I think it's a very important issue that needs to be fixed in your existing code.

Dispose is for cleaning up unmanaged resources that your class owns, and nothing else. The couple things you might need to worry about disposing are already wrapped in using statements, therefore there's nothing to do in Dispose, so KVF should not implement IDisposable. You should absolutely not do the most important and potentially destructive operation in Dispose. If I want to commit, I will call Commit. I would be very frustrated with you if I was halfway through building my file, ran into some reason to abort, and disposed of the KVF instance without committing, only to find that you had overwritten critical data with my half-formed version.

Reinventing the Wheel

Disclaimer: reinventing the wheel is totally OK for practice or for school assignments. However, you indicated you're planning to use this in production code, so this section very much applies.

A lot of your code seems to exist solely to replace the functionality of Dictionary. Why? What's wrong with just using a Dictionary to store key-value pairs? That will work better with other code (such as LINQ), other developers will not have to learn anything new, and it's much less work for you. It seems like the core functionality of your code is to serialize a Dictionary to a string. I would expect this to be a single class with a constructor resembling:

public DictionarySerializer(IDictionary<string, string> dictionary)

Speaking of serializing and reinventing the wheel: don't invent your own file format. There are great formats for persistence layers already out there, and they have countless serializers and deserializers. Use JSON or XML or a database instead. One really nice thing about using tried-and-true solutions is that they handle edge cases nicely. For example, in your code, what happens if I ask you to save a value that contains an "=" character? What you give me back when I ask you to load it again will not be the same. Add this as a unit test.