HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Key value flat file system - single header INI replacement

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
headerfilesystemvalueflatsingleinikeyreplacement

Problem

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;

///
/// Initializes KVF values.
/// Generates KVF file in memory for handling.
///
/// The file to load key/value pairs from.
/// Specify whether the program is being debugged. default: false
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);
}

/// Writes a value to the specified key (caller).
/// The value to write.
/// The calling member to write as key.
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);
}

/// Writes a value using the specified value to the specified key.
/// The key containg the value.
/// The value to write.
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
{
Ca

Solution

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

Code Snippets

public class KVF :IDisposable
internal bool Debugoutput;
public void MagicSave
public KVF(string filename,bool debugoutput = false, string kvsplit = "=")
public string FileName { get; private set; }

Context

StackExchange Code Review Q#92418, answer score: 10

Revisions (0)

No revisions yet.