5
\$\begingroup\$

I'm not sure whether my distaste for reflection is justified or not, but it feels ucky that I'm using reflection here.

I'm trying to make a very simple class which has a number of List<T> where T : Model with a load and save method.

The load method should:

  1. For every list,
  2. Open or create "Data/(<T>)/"
  3. Sequentially load each file in the folder into memory by creating a new instance of T and calling Deserialize(Text)

The save method should:

  1. For every list,
  2. For every instance,
  3. Call Serialize(), and dump the result into "/Data/(<T>)/(ID)"

Originally, I began writing code to loop through my Member list and my Game list and then realised I was going to be writing duplicate code for every list of data and switched to having a List<IEnumerable<Model>> and looping that and ended up having to use reflection. I was wondering if anyone can think of a way of achieving my goals without using reflection or having duplicate code for every type of model stored.

I'll attach my current completed class below. I don't mind doing string modelClassName = models.GetType().GenericTypeArguments.First().Name; so much but it's the code:

                    Type modelType = models.GetType().GenericTypeArguments.First();

                    dynamic list = (this.GetType().GetRuntimeProperty(modelClassName + "s").GetValue(this));
                    dynamic loaded = Activator.CreateInstance(modelType);
                    loaded.Deserialize("");
                    list.Add(loaded);

That's really bothering me. It just feels wrong and suboptimal and I'm sure I'm not thinking this through properly.

Full code below:

using System;
using System.Collections.Generic;
using System.Text;
using System.Reflection;

using System.Linq;
using Chess_Club.Models;
using Windows.Storage;
using System.Diagnostics;

namespace Chess_Club.DAL {
    class ChessClubContext {
        public List<IEnumerable<Model>> Models = new List<IEnumerable<Model>>();
        public List<Game> Games = new List<Game>();
        public List<Member> Members = new List<Member>();

        private StorageFolder RootFolder;
        private StorageFolder RootDataFolder;

        public ChessClubContext() {
            RootFolder = Windows.Storage.ApplicationData.Current.RoamingFolder;

            Models.Add(Games);
            Models.Add(Members);
        }

        public async void LoadData() {
            RootDataFolder = await RootFolder.CreateFolderAsync("Data", CreationCollisionOption.OpenIfExists);

            foreach (IEnumerable<Model> models in Models) {
                string modelClassName = models.GetType().GenericTypeArguments.First().Name;
                StorageFolder modelFolder = await RootDataFolder.CreateFolderAsync(modelClassName, CreationCollisionOption.OpenIfExists);

                IReadOnlyList<StorageFile> files = await modelFolder.GetFilesAsync();

                foreach (StorageFile file in files) {
                    // Some quick reflection to access the property... Saves duplicating this code n times for n as number of lists
                    Type modelType = models.GetType().GenericTypeArguments.First();

                    dynamic list = (this.GetType().GetRuntimeProperty(modelClassName + "s").GetValue(this));
                    dynamic loaded = Activator.CreateInstance(modelType);
                    loaded.Deserialize(""); // Pretend I'm actually loading the text around here, for now an empty string is fine.
                    list.Add(loaded);
                }
            }
        }

        public async void SaveData() {
            foreach (IEnumerable<Model> models in Models) {
                string modelClassName = models.GetType().GenericTypeArguments.First().Name;
                StorageFolder modelFolder = await RootDataFolder.CreateFolderAsync(modelClassName, CreationCollisionOption.OpenIfExists);

                foreach (Model model in models) {
                    StorageFile file = await modelFolder.CreateFileAsync(model.ID.ToString(), CreationCollisionOption.ReplaceExisting);
                    await FileIO.WriteTextAsync(file, model.Serialize());
                }
            }
        }
    }
}
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Reflection performance penalty would be negligible comparing to the serialization and IO cost, but you can surely work without reflection here.

In this code example, i'm demonstrating NET generics approach, which allows you to get specific type information statically (typeof(T)) and access members of a client class (ModelContext) with delegates, in place of reflection access to class members in your code.

You can take it a step further, replacing typeof(T).Name with an explicit collection folder name, this would require changing CreateRepository method signature slightly:

modelRepositories.Add(CreateRepository(() => Games, "Games"));

You can see the fix for async pattern usage. The correct signature is async Task for a void method, so you could use await.

Usage of public properties with setters for collections is dangerous, even if it works in provided example, because delegate accesses the newest assigned collection each time it's called.

This code example is not for a Windows Store app, but the approach still applies.

public class ModelContext
{
    private readonly List<IModelRepository> modelRepositories = new List<IModelRepository>();
    public List<Game> Games = new List<Game>();
    public List<Member> Members = new List<Member>();

    public ModelContext()
    {
        modelRepositories.Add(CreateRepository(() => Games));
        modelRepositories.Add(CreateRepository(() => Members));
    }

    public async Task LoadData()
    {
        var rootFolderName = GetRootFolder();
        foreach (var modelRepository in modelRepositories)
        {
            await modelRepository.Load(rootFolderName);
        }
    }

    public async Task SaveData()
    {
        var rootFolderName = GetRootFolder();
        foreach (var modelRepository in modelRepositories)
        {
            await modelRepository.Save(rootFolderName);
        }
    }

    private static string GetRootFolder()
    {
        return Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.MyDocuments), "TestRepository");
    }

    private static IModelRepository CreateRepository<T>(Func<List<T>> modelProperty) where T : Model, new()
    {
        return new ModelRepository<T>(modelProperty);
    }
}

public interface IModelRepository
{
    Task Save(string rootFolderName);
    Task Load(string rootFolderName);
}

public class ModelRepository<T> : IModelRepository where T: Model, new()
{
    private readonly Func<List<T>> modelProperty;

    private readonly static string ModelName = typeof(T).Name;

    public ModelRepository(Func<List<T>> modelProperty)
    {
        this.modelProperty = modelProperty;
    }

    public async Task Save(string rootFolderName)
    {
        var modelFolderName = GetFolderName(rootFolderName);
        if (!Directory.Exists(modelFolderName))
        {
            Directory.CreateDirectory(modelFolderName);
        }
        foreach (var model in modelProperty())
        {
            var modelFilePath = Path.Combine(modelFolderName, model.Id.ToString(CultureInfo.InvariantCulture));
            var model1 = model;
            await Task.Run(() => File.WriteAllText(modelFilePath, model1.Serialize()));
        }
    }

    public async Task Load(string rootFolderName)
    {
        var modelFolderName = GetFolderName(rootFolderName);
        foreach (var fileName in Directory.GetFiles(modelFolderName))
        {
            var fileName1 = fileName;
            var readAllText = await Task.Run(()=>File.ReadAllText(fileName1));
            LoadItem(readAllText);
        }
    }

    private static string GetFolderName(string rootFolderName)
    {
        return Path.Combine(rootFolderName, ModelName + "s");
    }

    protected void LoadItem(string s)
    {
        var loaded = new T();
        loaded.Deserialize(s);
        modelProperty().Add(loaded);
    }
}
\$\endgroup\$
1
\$\begingroup\$

First of all a remark: I am not entirely sure about the requirements but in order to make your app Windows Store qualified you have to adhere to certain rules. One of them being you're not fiddling with .NET stuff in a WinRT component.

I would look up whether or not reflection is ported from .NET to WinRT. Just because it works on your machine locally doesn't mean Microsoft will allow it.


You are right in that you want to avoid reflection: it is extremely expensive. I don't know the exact performance impact of attributes you could create a custom attribute to specify the name instead of having to fiddle around with the type arguments.

internal class ModelTypeAttribute : System.Attribute
{
    public Type Type { get; set; }

    public ModelTypeAttribute(Type type)
    {
        Type = type;
    }
}

You can use this (or more) attributes to store all the metadata that you want. It will be more work that seems redundant but makes some things more loosely coupled and displays the intent better.


Private instance variables are written using _lowerCase.


Don't use public instance variables; make them properties.


Never have async void unless you have either of these situations:

  • An asynchronous event handler
  • It's a fire-and-forget task

If you make it async void you can never await the call because there is no Task to wait for. Instead, make it return async Task (which is essentially a void method, but then in an asynchronous context).

\$\endgroup\$
5
  • \$\begingroup\$ Thanks for the improvement ideas! I wasn't aware that using properties over public instance variables was considered good practice so thanks for that -- Is there any chance you could elaborate on why that is though? I was under the impression that I could easily change it into a property at any point in time without affecting anything if I needed to so I was always under the assumption that just using a public variable is easiest way to keep it simple unless I need the complexity. \$\endgroup\$ Sep 15, 2014 at 18:45
  • 1
    \$\begingroup\$ You're right, you could just change it into a property and no functionality would be lost. But it will, for one, require you to make that change when you might as well use the property from the get-go. Secondly it breaks the (theory of) encapsulation but granted: if you can just change it when needed, it's not a real problem. And lastly: if you have meta-programming sections (reflection, code-generation) that use field-specific actions that aren't applicable for properties, you might get into trouble there because you're restricted from making the change. \$\endgroup\$ Sep 15, 2014 at 19:25
  • \$\begingroup\$ @JeroenVannevel: Actually, changing a field to a property is a breaking change, because a property is a method underneath, which is part of the reason to use them rather than public fields. Also, underscore prefixes are just, like, your opinion. \$\endgroup\$
    – Magus
    Sep 15, 2014 at 22:56
  • \$\begingroup\$ @Magus: Can you show me a situation where changing public List<Game> Games = new List<Game>(); to public List<Game> Games { get; set; } with initialization in the constructor is a breaking change? \$\endgroup\$ Sep 15, 2014 at 23:02
  • \$\begingroup\$ @JeroenVannevel: Any time the assembly is distributed compiled. If it it's in source format, you'll never notice because it'll be recompiled anyway, but you won't be able to just drop a single new DLL in and have it continue working. \$\endgroup\$
    – Magus
    Sep 16, 2014 at 2:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.