1
\$\begingroup\$

I have created a Synchronizer, the purpose of which is to read data from an XML source file and store the result in a DB.

I have different source types, for example Student.XML, School.XML, etc. These files are copied in my source directory, C:\Source.

My Synchronizer watches the Source folder, and every time there is a new file, it should read it, map it to my domain class and write the domain class into the DB.

If C:\Source folder contains Student.XML, it should read the content of the XML file and copy it in Student Table in the DB. If C:\Source contains School.XML, it should read the file and copy the content to School Table.

To solve this, I have defined an ISyncer interface:

public interface ISyncer
{
    // Read input and synchronize destination, return error message if any
    string Sync();
}

Now all my Syncer types (e.g. StudentSyncer, SchoolSyncer, etc) should implement this interface:

public class StudentSyncer : ISyncer
{
    // simple XML reader, reads XML file into a list of List<XMLStudent>
    private XMLStudentReader _reader;

    public StudentSyncer(string file)
    {
        _reader = new XMLStudentReader(file);
    }

    public string Sync()
    {
        // read the input
        List<XMLStudent> source = _reader.ReadAll();

        // using automapper map to domain object
        List<Student> dbStudent = Mapper.Map<List<XMLStudent>, List<Student>>(source);
        dbStudent.ForEach(s => s.IsCurrent = true);                // set IsCurrent

        // write records to Student table, using Unit of Work patterns
        using (var context = new DbContext())
        {
            UnitOfWork uow = new UnitOfWork(context);

            // set the existing records to not current
            var curSet = uow.Student.FindByTrackingChanges(s => s.IsCurrent == true).ToList();
            curSet.ForEach(s => s.IsCurrent = false);
            uow.Student.AddRange(dbStudent);
            uow.SaveChanges();
        }

        return "";  // no error
    }
}

I have a Windows service, which calls the DoSync() method every 30 seconds:

public void DoSync()
{
    foreach (string file in Directory.EnumerateFiles(@"C:\Source", "*.xml"))
    {
        // use syncer factory to initialize the correct instance of syncer based on input file name
        ISyncer syncer = _syncerFactory.CreateInstance(file);

        // do the synchronization task
        syncer.Sync();

        // Move file to processed folder
        MoveFile(@"C:\Destination");
    }
}

The Factory is a simple factory. It reads the source file name, e.g. Student.XML, School.XML and based on the file name. It initializes the correct Syncer, e.g. StudentSyncer, SchoolSyncer.

I am happy with the code, but it is bothering me because I feel it is not following the Single Responsibility Principle. My Syncer class is not doing 1 task, but it is doing 3 tasks: read input, map to domain class, write to DB.

\$\endgroup\$
  • 1
    \$\begingroup\$ Synchronizer should watch the Source folder - where is the code that does that? Have you removed anything? \$\endgroup\$ – t3chb0t Jun 22 '18 at 9:42
  • \$\begingroup\$ @t3chb0t, thanks. It's a Windows event handler which calls DoSync () every 30 seconds. I did not include that part of the code in the review as I thought it's straight forward ... can add it, if it is necessary. \$\endgroup\$ – Hooman Bahreini Jun 22 '18 at 9:50
2
\$\begingroup\$

I know this is a little late, but instead of the timed call to DoSync(), you could consider to use FileSystemWatcher and then subscribe to the relevant events.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. Yes, that's an alternative and @t3chb0t gave the same comment... my main question is about Single Responsibility Principle... at what point do we say that a class has too many responsibilities and we should refactor it? The best article that I have seen is this one by Nikola Malovic... I think Nikola's 2nd law of Ioc answers my question above: "Any class having more then 3 dependencies should be questioned for SRP violation" \$\endgroup\$ – Hooman Bahreini Mar 22 at 10:51
  • 1
    \$\begingroup\$ @HoomanBahreini: OK, I haven't seen that comment. About SRP: This can be obtained on all levels of a project (modules, classes, methods) and depends on the abstraction level IMO: On one level of abstraction your Syncer does only one thing (syncronize a db from a xml file) but on another it does three: read, map, save - and one an even lower level the reading is about reading each byte from a stream etc.... In each project you have to find a reasonable ratio between SRP and fragmentation. I agree that you question may be more about IoC than SRP. \$\endgroup\$ – Henrik Hansen Mar 22 at 11:15
0
\$\begingroup\$

How about extracting tasks common to all source types into an (abstract) base class that, in turn, implements your ISyncer interface?

I would also change the error reporting by using exceptions, rather than have the method return a string in case of error.

Edit, code example (C#-ish pseudo code)

abstract class SyncerBase : ISyncer
{
    sealed List<T> Map(List source, List target)
    {
        // Mapping magic here
    }

    sealed SaveToDB(List dataToSave, params ...)
    {
        // Your database access here
    }
}

Put utility methods and common stuff for all your synchronizers in that base class, and inherit it.

class StundentSyncer : SyncerBase
{
    // Constructor as in your code
    void Sync(string file)
    {
        var XMLstudents = _reader.ReadAll();
        var students = Map(XMLstudents); // calling base class here!
        // Manipulate student list here as needed
       SaveToDB (students, database params here) // base class here, too
    }
}
\$\endgroup\$
  • \$\begingroup\$ thanks. Exception is very good advice. I am not sure what you mean about abstracting the common functinality of all types? \$\endgroup\$ – Hooman Bahreini Jun 22 '18 at 10:11
  • \$\begingroup\$ @Hooman you mention that you have StudentSyncer, SchoolSyncer, etc - it's very likely that they repeat the same code but it's hard to say because we don't see the other implemenations. \$\endgroup\$ – t3chb0t Jun 22 '18 at 10:17
  • \$\begingroup\$ What is type T? And how to initialize Syncer / SyncerBase using a factory pattern? The reason I did not include Reader, Writer and Mapper inside ISyncer, was that I wanted to easily initialize the Syncers in a Factory. \$\endgroup\$ – Hooman Bahreini Jun 23 '18 at 0:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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