String modification application

Below is working code of a semi complete program. Its purpose is to take an input string of any type and modify it based on rules defined for each type. So in this example I pass it a string in CSV format and modify some of the fields.

My questions are

-
Can the use of interfaces be improved?

-
How can I handle different string modification patterns (I currently make it work by using IStringModificationPattern - but I want to extend this to other types like HTML)?

-
What design changes/ additions can I make to integrate the string modification pattern into a configuration class (i.e. using an XML file based settings)?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace StringModificationTest
{
    class Program
    {
        static void Main(string[] args)
        {
            //usage example
            //
            string inputText = "CSV,field1,field2,field3,field4,field5,field6,field7,,,,";

            StringModifier SM = new StringModifier();

            string outputText = SM.ModifyString(inputText);

        }
    }

StringModifier class

```
public class StringModifier
{
private List RegisteredTypes = new List();
private IStringModificationPattern ModificationPattern;

public StringModifier()
{
RegisterTypes();
}

private void RegisterTypes()
{
RegisteredTypes = TypeFactory.GetTypeList();
}

public string ModifyString(string str)
{
///sets the message and sets the Modification pattern
IString detectedStr = DetectStringType(str);

///returns the modified string
return detectedStr.Modify(ModificationPattern);
}

private IString DetectStringType(string str)
{
IString msg = new NullString(str);

for (int i = 0; i < RegisteredTypes.Count; i++)
{

The first word that comes to my mind, is over-engineering. This looks like a lot of trouble just to get to work with a CSV string, you turn a 10-liner into a party, happiness ensues and neighbors come too, and their friends also, and before you know it you've got 100 lines of code written, to accomplish the exact same thing.

This answer/review does not care about that.


StringModifier

The RegisteredTypes private field can be made readonly, and its naming doesn't match naming conventions for private types. I'd declare it like this:

private readonly List _registeredTypes;

And initialize it directly in the constructor, and remove the private RegisterTypes method altogether:

public StringModifier()
    {
        _registeredTypes = TypeFactory.GetTypeList();
    }

While str is a well-understood short for string and I prefer str over @string, I think a better name for that parameter could be, simply, value - also I'd remove String from the method's name, it's in a class called StringModifier, it takes a string parameter and returns a string value. If Modify isn't clear enough, I don't know what is!

public string Modify(string value)
    {
        var detectedString = DetectStringType(value);
        return detectedString.Modify(ModificationPattern);
    }

I don't think the comments add anything significant in the ModifyString method. You have descriptive method names, already doing the job of describing what the code does (and that's good!).

The DetectStringType method has opportunities for improvements - the RegisteredTypes needs only to be accessed once, and msg just seems like an arbitrary name - result makes more sense I find:

private IString DetectStringType(string value)
    {
        IString result = new NullString(value);

        for (var i = 0; i < _registeredTypes.Count; i++)
        {
            var type = _registeredTypes[i];
            if (value.StartsWith(type.start))
            {
                result = type.GetNewInstance(value);
                ModificationPattern = type.MODPattern;
            }

        }

        return result;
    }

TypeFactory

The name is ambiguous, if not overly broad. StringTypeFactory would be a better name. This class is clearly infrastructure code. I don't see a need for it to be static though: by using it in StringModifier the way you are, you have tightly coupled the two classes, essentially making StringModifier responsible for finding all IStringType implementations in the current app domain... which seems like a lot to do for a StringModifier.

I'd move this infrastructure code to infrastructure level, and inject an IEnumerable into StringModifier's constructor:

private readonly IEnumerable _registeredTypes;
    public StringModifier(IEnumerable types)
    {
        _registeredTypes = types;
    }

By infrastructure level, I mean this:

var inputText = "CSV,field1,field2,field3,field4,field5,field6,field7,,,,";

    var factory = new StringTypeFactory();
    var modifier = new StringModifier(factory.GetTypeList());

    var outputText = modifier.Modify(inputText);

..and I know you're not going to like it. Because you probably want to be able to new up a StringModifier whenever you need to use one - my recommendation is to avoid newing up anything. With that many abstractions/interfaces, call me crazy but I'll say that you're missing an IStringModifier abstraction:

public interface IStringModifier
{
    string Modify(string value);
}

Now whenever you have a class that wants to use a StringModifier, you'll inject an IStringModifier through that class' constructor, assign a private readonly field, and use that field. This is called Dependency Injection, and it greatly helps decoupling code and making it more unit-testable. Creating objects is a concern on its own, that belongs to infrastructure code.

This hypothetical SomeClass consumes an IStringModifier and shows how constructor injection works:

public class SomeClass
{
    private readonly IStringModifier _modifier;

    public SomeClass(IStringModifier modifier)
    {
        _modifier = modifier;
    }

    public void Foo(string bar)
    {
        var modified = _modifier.Modify(bar);
        // ...
    }
}

But back to your code...


IStringType

The interface members don't follow casing conventions. Should look more like this:

public interface IStringType
{
    string Start { get; }
    IString GetNewInstance(string value);
    IStringModificationPattern Pattern { get; }
}

IString

The interface members don't follow casing conventions either:

public interface IString
{
    string Value { get; }
    string Modify(IStringModificationPattern pattern);
}

IStringType Implementations

The IStringModificationPattern property smells. All there is to it, is a getter that returns a new instance of a tightly coupled implementation

StackExchange Code Review Q#48452, answer score: 2