Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm trying to implement interfaces in my design but not sure if this is correct or if there is a better way to do it.

What I need to do is

  1. open a text file

  2. keep searching blocks of text until I find a string that matches against one of the classes that defines that string pattern

  3. process the string I found

  4. continue processing file where I left off

This is the code I'm using so far, but not sure if the design is correct. Maybe something about creating a new instance of each stringType class just to check if the string matches with the one defined in the class seems wrong to me. I don't need to create the instance of the object until I actually need to do work on it.

EDIT - Forgot to add the process function, not sure if that changes anything

static void Main(string[] args)
{
    //get types that implement IStringType interface
    List<IStringType> types = TypeFactory.GetTypeList();

    //scan text file and check against the types, store any matches found
    var stringMatches =  from t in types
                        where Regex.IsMatch(sourceString, t.stringPattern)
                        select t.getNewInstance(Regex.Match(sourceString,t.stringPattern).Value);

    if (stringMatches.Any())
    {
        //process string matches
    }
}
public class stringType1 : IStringType
{
    public string stringPattern { get { return @"(?m)^TEST1.+[\r\n]"; } set { this.stringPattern = value; } }
    private string _Message { get; set; }

    public stringType1() { }
    public stringType1(string str)
    {
        _Message = str;
    }

    public string ProcessString(string match)
    {
       //specific code to processes stringType1

    }
    public IStringType getNewInstance(string msg)
    {
        return (IStringType)(new stringType1(msg));
    }
}

public class stringType2 : IStringType
{
    public string stringPattern { get { return @"(?m)^BLAH2.+[\r\n]"; } set { this.stringPattern = value; } }
    private string _Message { get; set; }

    public stringType2() { }
    public stringType2(string str)
    {
        _Message = str;
    }
    public string ProcessString(string match)
    {
       //specific code to processes stringType2

    }
    public IStringType getNewInstance(string msg)
    {
        return (IStringType)(new stringType2(msg));
    }
}       

public interface IStringType
{
    string stringPattern { get; set; }
    string ProcessString(string str);
    IStringType getNewInstance(string str);
}


public static class TypeFactory
{
    public static List<IStringType> GetTypeList()
    {
        List<IStringType> types = new List<IStringType>();
        types.AddRange(from assembly in AppDomain.CurrentDomain.GetAssemblies()
                       from t in assembly.GetTypes()
                       where t.IsClass && t.GetInterfaces().Contains(typeof(IStringType))
                       select Activator.CreateInstance(t) as IStringType);
        return types;
    }
}
share|improve this question

1 Answer 1

It's very easy: you shouldn't. An interface is used to indicate that a subset of classes present common behaviour or should be considered similar in some way.

In your situation there simply is no reason to make this change because all you do is change a text value in a property, which could just as easily be done by taking an argument in your constructor and keeping a private backing fields which should then be used to populate the property's regex.

This will result in several code optimizations:

  • No heavy duplication of code in StringType1 and StringType2 (this should have been a red flag)

  • No reflection necessary to get the different types. Just keep a collection of the different regexes and return a collection of StringType objects, constructed with your regexes.

Additionally, this code will also be obsolete:

public IStringType getNewInstance(string msg)
{
    return (IStringType)(new stringType2(msg));
}

since you should just overwrite that variable with new Stringtype(thisVariable.StringPattern, message).

Ultimately, your code would look something like this:

public class StringType
{
    private IPatternProcessor _processor;
    private string _pattern;
    private string _message { get; set; }

    public StringType(string str, IPatternProcessor processor)
    {
        _message = str;
        _processor = processor;
    }

    public string StringPattern { 
        get { return pattern; } 
        set { pattern = value; } 
    }

    public void Process() {
        _processor.Process(_message);
    }
}    

public static class PatternFactory
{
    private static List<String> _patterns = new List<String>();

    static PatternFactory() {
         _patterns.Add("(?m)^TEST1.+[\r\n]";);
         _patterns.Add("(?m)^BLAH2.+[\r\n]";);
    }    

    public static List<StringType> GetTypeList()
    {
        return _patterns.Select(x => new StringType(x));
    }
}

public interface IPatternProcessor { 
    void Process(string input);
}

public class ProcessMethod1 : IPatternProcessor { }
public class ProcessMethod2 : IPatternProcessor { }

If you want to make it easy for your user, you can hardcode these different methods into properties:

public class PatternProcessor {
    public IPatternProcessor ProcessMethod1 { get { return new ProcessMethod1(); } private set; };
    public IPatternProcessor ProcessMethod2 { get { return new ProcessMethod2(); } private set; };
}

Now you can simply call this with

new StringType(input, PatternProcessor.ProcessMethod1).Process();

If you were really into it, you could just cache the IProcessMethod objects in a collection in your PatternProcessor class (provided each method is stateless), but that might be going a little too far out of the scope of this review.

Style notes

  • Classes are UpperCamelCase (StringType)
  • Private fields are less standardized but they are often written as _field
  • Methods and constructors are always capitalized (StringPattern, StringType), just like the class

Conclusion

This code does not benefit from interfaces at all and as such it is impossible to review it since interfaces are the main problem with the code.

Post-edit

So, with the additional requirement of pattern-specific behaviour the introduction of interfaces and separate types now makes a lot more sense. I see two architecture options off the bat:

  • Separate StringType classes for each situation (if you name them StringType1, StringType2 etc, I'll chase you with a bat)

  • Separate strategies (strategy pattern) for each type of processing with a sauce of factory pattern

I will go with the second option because it would require less rewriting. Someone else can write the less complex first option but the second option is more flexible in terms of allowing two different patterns to use the same processing option.

I will change the earlier block of code with some additions to keep everything in one place.

share|improve this answer
    
static { _patterns.Add("(?m)^TEST1.+[\r\n]";); [...] won't compile, it's not Java ;) it should be static PatternFactory instead. The answer is on the nose anyway –  Morawski Apr 2 at 21:10
    
but I need to process each string different, each stringType class for example has a process() function that processes the string specifically for each type –  erotavlas Apr 2 at 21:11
    
@Morawski: cheers, feel free to edit yourself though! I always switch it up between C# and Java ;) –  Jeroen Vannevel Apr 2 at 21:20
    
@erotavlas: why would you leave out the entire reason why someone would use an interface.. If anything had to be in there, it was that process() method. –  Jeroen Vannevel Apr 2 at 21:21
2  
+1 for if you name them StringType1, StringType2 etc, I'll chase you with a bat ;) –  Mat's Mug Apr 3 at 0:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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