Synchronizer for importing XML files into a database when folder content changes

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
3
down vote

favorite












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 (S of SOLID). My Syncer class is not doing 1 task, but it is doing 3 tasks: read input, map to domain class, write to DB.







share|improve this question





















  • Synchronizer should watch the Source folder - where is the code that does that? Have you removed anything?
    – t3chb0t
    Jun 22 at 9:42










  • @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.
    – Hooman
    Jun 22 at 9:50
















up vote
3
down vote

favorite












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 (S of SOLID). My Syncer class is not doing 1 task, but it is doing 3 tasks: read input, map to domain class, write to DB.







share|improve this question





















  • Synchronizer should watch the Source folder - where is the code that does that? Have you removed anything?
    – t3chb0t
    Jun 22 at 9:42










  • @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.
    – Hooman
    Jun 22 at 9:50












up vote
3
down vote

favorite









up vote
3
down vote

favorite











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 (S of SOLID). My Syncer class is not doing 1 task, but it is doing 3 tasks: read input, map to domain class, write to DB.







share|improve this question













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 (S of SOLID). My Syncer class is not doing 1 task, but it is doing 3 tasks: read input, map to domain class, write to DB.









share|improve this question












share|improve this question




share|improve this question








edited Jul 5 at 2:31
























asked Jun 22 at 3:50









Hooman

1387




1387











  • Synchronizer should watch the Source folder - where is the code that does that? Have you removed anything?
    – t3chb0t
    Jun 22 at 9:42










  • @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.
    – Hooman
    Jun 22 at 9:50
















  • Synchronizer should watch the Source folder - where is the code that does that? Have you removed anything?
    – t3chb0t
    Jun 22 at 9:42










  • @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.
    – Hooman
    Jun 22 at 9:50















Synchronizer should watch the Source folder - where is the code that does that? Have you removed anything?
– t3chb0t
Jun 22 at 9:42




Synchronizer should watch the Source folder - where is the code that does that? Have you removed anything?
– t3chb0t
Jun 22 at 9:42












@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.
– Hooman
Jun 22 at 9:50




@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.
– Hooman
Jun 22 at 9:50










1 Answer
1






active

oldest

votes

















up vote
1
down vote













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







share|improve this answer























  • thanks. Exception is very good advice. I am not sure what you mean about abstracting the common functinality of all types?
    – Hooman
    Jun 22 at 10:11










  • @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.
    – t3chb0t
    Jun 22 at 10:17










  • 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.
    – Hooman
    Jun 23 at 0:44











Your Answer




StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);








 

draft saved


draft discarded


















StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f197024%2fsynchronizer-for-importing-xml-files-into-a-database-when-folder-content-changes%23new-answer', 'question_page');

);

Post as a guest






























1 Answer
1






active

oldest

votes








1 Answer
1






active

oldest

votes









active

oldest

votes






active

oldest

votes








up vote
1
down vote













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







share|improve this answer























  • thanks. Exception is very good advice. I am not sure what you mean about abstracting the common functinality of all types?
    – Hooman
    Jun 22 at 10:11










  • @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.
    – t3chb0t
    Jun 22 at 10:17










  • 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.
    – Hooman
    Jun 23 at 0:44















up vote
1
down vote













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







share|improve this answer























  • thanks. Exception is very good advice. I am not sure what you mean about abstracting the common functinality of all types?
    – Hooman
    Jun 22 at 10:11










  • @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.
    – t3chb0t
    Jun 22 at 10:17










  • 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.
    – Hooman
    Jun 23 at 0:44













up vote
1
down vote










up vote
1
down vote









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







share|improve this answer















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








share|improve this answer















share|improve this answer



share|improve this answer








edited Jun 22 at 10:56


























answered Jun 22 at 9:37









TomG

34115




34115











  • thanks. Exception is very good advice. I am not sure what you mean about abstracting the common functinality of all types?
    – Hooman
    Jun 22 at 10:11










  • @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.
    – t3chb0t
    Jun 22 at 10:17










  • 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.
    – Hooman
    Jun 23 at 0:44

















  • thanks. Exception is very good advice. I am not sure what you mean about abstracting the common functinality of all types?
    – Hooman
    Jun 22 at 10:11










  • @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.
    – t3chb0t
    Jun 22 at 10:17










  • 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.
    – Hooman
    Jun 23 at 0:44
















thanks. Exception is very good advice. I am not sure what you mean about abstracting the common functinality of all types?
– Hooman
Jun 22 at 10:11




thanks. Exception is very good advice. I am not sure what you mean about abstracting the common functinality of all types?
– Hooman
Jun 22 at 10:11












@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.
– t3chb0t
Jun 22 at 10:17




@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.
– t3chb0t
Jun 22 at 10:17












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.
– Hooman
Jun 23 at 0:44





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.
– Hooman
Jun 23 at 0:44













 

draft saved


draft discarded


























 


draft saved


draft discarded














StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f197024%2fsynchronizer-for-importing-xml-files-into-a-database-when-folder-content-changes%23new-answer', 'question_page');

);

Post as a guest













































































Popular posts from this blog

Greedy Best First Search implementation in Rust

Function to Return a JSON Like Objects Using VBA Collections and Arrays

C++11 CLH Lock Implementation