Project Euler problem solver in windows forms

Question

I've programmed a few Project Euler problem but they are all separated in different solutions so I decided to group them all together in a nice Windows forms application. Here's how it looks like :

You can select any of the available problems from the list check the answer and the execution time. I'm looking primarly for code style review or maybe a better way of separating classes and the overall architecture.

Main form class

public partial class Form1 : Form
{
    private readonly Dictionary<string, IProblem> problems = new Dictionary<string, IProblem>();
    private readonly Dictionary<int, ProblemOutput> solvedProblems = new Dictionary<int, ProblemOutput>();

    private IProblem currentProblem;

    public Form1()
    {
        InitializeComponent();
        LoadProblems();
    }

    private void LoadProblems()
    {
        Type[] allProblems = GetDerivedTypesFor(typeof(IProblem)).ToArray();
        foreach (Type problem in allProblems)
        {
            ProblemSelector.Items.Add(FormatName(problem.Name));
            IProblem problemInstance = CreateInstanceByName<IProblem>(problem.Name);
            problems.Add(problem.Name, problemInstance);
        }
    }

    private string FormatName(string inputString)
    {
        StringBuilder result = new StringBuilder();
        for (int i = 0; i < inputString.Length; i++)
        {
            if (result.Length > 0)
            {
                if (char.IsUpper(inputString[i]))
                {
                    result.Append(' ');
                }
                else if (i - 1 >= 0 && char.IsNumber(inputString[i]) && char.IsLetter(inputString[i - 1]))
                {
                    result.Append(' ');
                } 
            }
            result.Append(inputString[i]);
        }
        return result.ToString();
    }

    private static T CreateInstanceByName<T>(string className)
    {
        Assembly assembly = Assembly.GetExecutingAssembly();
        Type type = assembly.GetTypes().First(t => t.Name == className);
        return (T)Activator.CreateInstance(type);
    }

    private static IEnumerable<Type> GetDerivedTypesFor(Type baseType)
    {
        Assembly assembly = Assembly.GetExecutingAssembly();
        return assembly.GetTypes().Where(baseType.IsAssignableFrom).Where(t => baseType != t);
    }

    private void button1_Click(object sender, EventArgs e)
    {
        if (currentProblem != null)
        {
            ProblemOutput problemOutput;
            if (!solvedProblems.ContainsKey(currentProblem.ID))
            {
                problemOutput = currentProblem.Solve();
                solvedProblems.Add(currentProblem.ID, problemOutput);
            }
            else
            {
                problemOutput = solvedProblems[currentProblem.ID];
            }
            ProblemAnswer.Text = problemOutput.OutputText.ToString();
            ProblemExecutionTime.Text = problemOutput.ExecutionTime/1000d + @" seconds";
        }
    }

    private void ProblemSelector_SelectedIndexChanged(object sender, EventArgs e)
    {
        IProblem selectedProblem = problems[Regex.Replace(((Control)sender).Text, @"\s+", "")];
        if (currentProblem == null || selectedProblem.ID != currentProblem.ID)
        {
            ProblemExecutionTime.Text = string.Empty;
            ProblemAnswer.Text = string.Empty;
            currentProblem = selectedProblem;
            ProblemText.Text = currentProblem.Condition;
        }
    }
}

Base interface for all problems

public interface IProblem
{
    int ID { get; }
    string Condition { get; }
    ProblemOutput Solve();
}

Where ProblemOutput is primarly a wrapper/helper class

public sealed class ProblemOutput
{
    private readonly double executionTime;
    public double ExecutionTime => executionTime;

    private readonly object outputText;
    public object OutputText => outputText;

    public ProblemOutput(double executionTime, object outputText)
    {
        this.executionTime = executionTime;
        this.outputText = outputText;
    }
}

Here's and example of problem class

public sealed class Problem1 : IProblem
{
    public int ID => 1;
    public string Condition => ProblemsConditions.ProblemConditions[ID];

    public ProblemOutput Solve()
    {
        int sum = 0;
        Stopwatch sw = Stopwatch.StartNew();
        for (int i = 1; i < 1000; i++)
        {
            if (i % 3 == 0 || i % 5 == 0)
            {
                sum += i;
            }
        }
        sw.Stop();
        return new ProblemOutput(sw.ElapsedMilliseconds, sum);
    }
}

Any methods shared between at least 2 problems are grouped in the ProblemHelper class

public static class ProblemHelper
{
    private static int GetFactorial(int n)
    {
        int factorial = 1;
        for (int i = 2; i <= n; i++)
        {
            factorial *= i;
        }
        return factorial;
    }

    // some other functions
}

Answer 1

[screenshot]

Since each problem has an ID, order them by ID before you add them, rather than by name.

public partial class Form1 : Form
...
private void button1_Click

Doesn't matter much here as this is a small program, but I've worked on existing Winforms projects where there were about 100 controls of various sorts all over, and half of them were named "button1", "label7", things like that. It's ridiculously difficult to know what control you're looking at when you don't rename controls from their defaults. As I said, doesn't matter much for your tiny project, but still a good habit to take on imo.

private void LoadProblems()
    {
        Type[] allProblems = GetDerivedTypesFor(typeof(IProblem)).ToArray();
        foreach (Type problem in allProblems)
        {
            ProblemSelector.Items.Add(FormatName(problem.Name));
            IProblem problemInstance = CreateInstanceByName<IProblem>(problem.Name);

Style issue, you should go on to define the functions you use under the functions that call them. When I see this method call GetDerivedTypesFor method, I look under the current method to find GetDerivedTypesFor. After all, it's "first" used by LoadProblems, and the first method it calls. Instead I see FormatName right under this function - why? Then CreateInstanceByName, and finally GetDerivedTypesFor. The order should be:

1. LoadProblems
2. GetDerivedTypesFor
3. FormatName
4. CreateInstanceByName

Though if for example FormatName calls some function and especially if it's only used by FormatName, then that function should be right after FormatName and before CreateInstanceByName. Basically for the most part (it's not a hard and fast rule but should be the case usually) a function/method should be defined right after its first use.

private static T CreateInstanceByName<T>(string className)
{
    Assembly assembly = Assembly.GetExecutingAssembly();
    Type type = assembly.GetTypes().First(t => t.Name == className);
    return (T)Activator.CreateInstance(type);
}

I don't really do much reflection, but this seems unnecessary - for every name, you're going to re-get the types and reiterate through them. You already got all the types you wanted in GetDerivedTypesFor, so can't you just create an instance from those types directly?

ProblemExecutionTime.Text = problemOutput.ExecutionTime/1000d + @" seconds";

You don't need the @ here.

private readonly double executionTime;
public double ExecutionTime => executionTime;

private readonly object outputText;
public object OutputText => outputText;

I mostly just know .NET 4.0, but I believe your code above uses C# 6.0, so (I believe) you could implement the above like so:

public double ExecutionTime { get; }
public object OutputText { get; }

And then just assign to the property in the constructor.

Also why is OutputText an object rather than a string?

Answer 2

I'd like to put in my two cents too.

Storing types and creating names

Your current code is based on strings e.g. you look for types by names that you both store in a dictionary and in the list. I find this duplication is not a good idea. Try to use strong types whereever possible and I think it is here.

What if you have two Problem1 but in different namespaces? The application wouldn't work anymore.

You also search for the problem-types multiple times. First when you create the list then again when you want to instantiate a particular problem.

Solution: we'll store everything in the ListBox but first you need to create a new type to store everything you need. The listbox can keep more then just a string.

class ProblemInfo
{ 
    public ProblemInfo(Type problemType)
    {
        ProblemId = ((Problem)Activator.CreateInstance(problemType)).Id;
        ProblemType = problemType;
    }

    public int ProblemId { get; }

    public Type ProblemType { get; }

    public string FullName => $"Problem {ProblemId,0:00}";
}

With this new type we encapsulate the view-logic. It is now responsible for creating a string for the Problem.Id and additionaly it'll store the type of the problem. No more strings.

---

Next we want to improve the problems themselves.

Instead of an interface I suggest using an abstract class that can retrieve the Id which we now store as an attribute. The Id shouldn't part of the implementation.

abstract class Problem
{
    public int Id  => GetType().GetCustomAttribute<IdAttribute>();

    public abstract ProblemOutput Solve();
}

And this is the attribute:

[AttributeUsage(AttributeTargets.Class)]
class IdAttribute : Attribute
{
    private readonly int _value;
    public IdAttribute(int value) { _value = value; }
    public static implicit operator int(IdAttribute idAttribute) => idAttribute._value;
}

You use it to decorate each problem and you can give them more meaningful names.

[Id(1)]
class SomeProblem : Problem { }

[Id(10)]
class AnotherProblem : Problem { }

[Id(2)]
class DifferentProblem : Problem { }

You can use the same approach for the descriptions. Just create another attribute and put the description there. Or the name of the resource etc. Use the ProblemInfo class to retrieve it and display it e.g. in a textbox upon selection.

---

Creating the ListBox

But how do we create the ListBox now? Like this. We set the MultiColumn = false and tell it to use the FullName for the display.

var listBox = new MyListbox();
listBox.MultiColumn = false;
listBox.DisplayMember = nameof(ProblemInfo.FullName);

listBox.BeginUpdate();
listBox.Items.Add(new ProblemInfo(typeof(DifferentProblem)));
listBox.Items.Add(new ProblemInfo(typeof(SomeProblem)));
listBox.Items.Add(new ProblemInfo(typeof(AnotherProblem)));    
listBox.EndUpdate();

listBox.Sorted = true;

I used hard-coded types but you of course will have a loop there to add the items.

You probably noticed that I have some kind of a MyListBox there. Yes, I created this to implement the natural-sort logic. This is no longer based on names and strings but on the Id.

class MyListbox : ListBox
{
    protected override void Sort()
    {
        if (Items.Count == 0) { return; }
        var swapped = false;
        do
        {
            var counter = Items.Count - 1;
            swapped = false;
            while (counter > 0)
            {
                var isLessThen = ((ProblemInfo)Items[counter]).ProblemId < ((ProblemInfo)Items[counter - 1]).ProblemId;
                if (isLessThen) 
                {
                    var tmp = Items[counter];
                    Items[counter] = Items[counter - 1];
                    Items[counter - 1] = tmp;
                    swapped = true;
                }
                counter--;
            }

        } while (swapped);
    }
}

The custom MyListBox overrides the Sort method and works with the Id so that you can format the string however you like and it'll still work.

---

Selecting problems

With all those improvements you can now modify also the SelectedIndexChange event handler to do this:

private void ProblemSelector_SelectedIndexChanged(object sender, EventArgs e)
{
    var selectedProblem = ((MyListbox)sender).SelectedItem as ProblemInfo;

}

Now you can store it and have a direct access to the Type which you can use to instantiate without regex and strings.

---

Benchmarking

The Problems shouldn't measure thier execution time. What if you want to run it more then once? You cannot measure the entire execution time.

Instead create a ProblemSolver.

class ProblemSolver
{ 
    public ProblemResult Solve(Problem problem)
    {
        // measure time etc...
    }    
}

This means that the ProblemOutput looses it's execution time and you move it into the new type ProblemResult which should contain both values.

Should you want to create other benchmarks then you can extend the ProblemSolver with other algorithms but no longer need to modify the problems themselves. They should just solve the problem and don't do anything else.

Answer 3

A few things I noticed:

You're formatting the title of the problem based on the name of the problem. However you're also storing the number of the problem and each one starts with Problem It would make more sense to just use the string concatenator $"Problem {Id.ToString("00")}" This allows you to format the number to 2 digits so that 02 will come after 01, instead of 2 coming after 19. If you use that in overriding the ToString() method, you can actually add instances of the problem class directly to the Listbox. Then running the specific problem is just a matter of casting the SelectedItem to the problem class and running the Solve method.

You're using 2 dictionaries to store the problem and the results. I'm wondering if using the ProblemOutput from the problem class would simplify that and only require 1 dictionary.

I think you'll find that many of the Euler problems will execute in less than 1 millisecond. It might better to base the time on perhaps 1000 or even 10000 iterations.