Simple change tracker for POCOs

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
5
down vote

favorite












Problem



I have a list of POCO model objects (approximatly 1.000 to 10.000) and I want to track changes on them:



  • Check if at least one of the items has changed (to show the user that something changed)

  • Get all new, deleted and modified objects to update the state in the database

For the first version, it is enough to support POCOs with primitive property types. However, probably I'll extend it in future to support complex property types.



Solution



public class ChangeTracker<TEntity> where TEntity : class

private static readonly PropertyInfo thePropertyInfos;

private readonly List<TEntity> myEntities = new List<TEntity>();
private readonly List<TEntity> myDeletedEntites = new List<TEntity>();
private readonly HashSet<TEntity> myNewEntities = new HashSet<TEntity>();
private readonly Dictionary<TEntity, object> myStates = new Dictionary<TEntity, object>();

static ChangeTracker()

thePropertyInfos = typeof(TEntity).GetProperties().ToArray();


public void Add(TEntity entity)

if (myStates.ContainsKey(entity))
throw new InvalidOperationException("It is not possible to add an entity twice.");

myStates.Add(entity, new object[thePropertyInfos.Length]);
myNewEntities.Add(entity);
myEntities.Add(entity);


public void Delete(TEntity entity)

myEntities.Remove(entity);
if (!myNewEntities.Remove(entity))
myDeletedEntites.Add(entity);


public void AcceptChanges()

myNewEntities.Clear();
myDeletedEntites.Clear();
foreach (var entity in myEntities)
myStates[entity] = GetState(entity);


public bool HasChanges()
EntityStates.Modified


private static object GetState(TEntity entity)

return thePropertyInfos
.Select(pi => pi.GetValue(entity))
.ToArray();


private static void SetState(TEntity entity, object state)

for (int i = 0; i < state.Length; i++)
thePropertyInfos[i].SetValue(entity, state[i]);


private bool HasChanges(TEntity entity)

var currentState = GetState(entity);
var previousState = myStates[entity];

for (int i = 0; i < currentState.Length; i++)


return false;


public IEnumerable<TEntity> GetEntities(EntityStates states)

if (states.HasFlag(EntityStates.New))
foreach (var entity in myNewEntities)
yield return entity;

if (states.HasFlag(EntityStates.Deleted))
foreach (var entity in myDeletedEntites)
yield return entity;

if (!states.HasFlag(EntityStates.Modified) &&
!states.HasFlag(EntityStates.Unmodified))
yield break;

foreach (var entity in myEntities.Where(e => !myNewEntities.Contains(e)))

var isModified = HasChanges(entity);
if (states.HasFlag(EntityStates.Modified) && isModified)
yield return entity;
else if (states.HasFlag(EntityStates.Unmodified) && !isModified)
yield return entity;




[Flags]
public enum EntityStates

New = 1,
Deleted = 2,
Modified = 4,
Unmodified = 8,
All = ~0,



Usage:



public class TestItem

public string Property get; set;


var changeTracker = new ChangeTracker<TestItem>();
var item1 = new TestItem();
var item2 = new TestItem();
var item3 = new TestItem();
var item4 = new TestItem();
var item5 = new TestItem();

changeTracker.Add(item1);
changeTracker.Add(item2);
changeTracker.Add(item3);
changeTracker.AcceptChanges();
changeTracker.Add(item4);
changeTracker.Add(item5);
item2.Property = "test";
changeTracker.Delete(item3);
changeTracker.Delete(item5);

Assert.IsTrue(changeTracker.HasChanges());
Assert.AreEqual(4, changeTracker.GetEntities(EntityStates.All).Count());
Assert.AreEqual(item1, changeTracker.GetEntities(EntityStates.Unmodified).Single());
Assert.AreEqual(item2, changeTracker.GetEntities(EntityStates.Modified).Single());
Assert.AreEqual(item3, changeTracker.GetEntities(EntityStates.Deleted).Single());
Assert.AreEqual(item4, changeTracker.GetEntities(EntityStates.New).Single());


Any feedback is welcome. However, I am specially intrested in alternative more performant solutions (without the overhead of boxing/unboxing and reflection).



[Edit]



I also tried binary serialization as "state storing strategy", but it is much slower (~100-200%) and it has the downside that the entity classes have to be marked as serializable.







share|improve this question





















  • thePropertyInfos - a new prefix? ;-D
    – t3chb0t
    Jan 26 at 20:50










  • no, it's the old one for static fields. The prefix for instance fields is still 'my' ;)
    – JanDotNet
    Jan 26 at 21:00










  • One more question, this time a technical one... why is EntityStates a Flags enum? It doesn't make much sense that an entity can have more then one state at the same time.
    – t3chb0t
    Jan 26 at 21:17










  • Becuase it allows to get e.g. all modified, new and deleted entites by combining the enum values: changeTracker.GetEntities(EntityStates.Modified | EntityStates.New | EntityStates.Deleted)
    – JanDotNet
    Jan 26 at 21:20










  • Further more, the enum is not used to associate the entity with a state - just to get entites for special states.
    – JanDotNet
    Jan 26 at 21:21

















up vote
5
down vote

favorite












Problem



I have a list of POCO model objects (approximatly 1.000 to 10.000) and I want to track changes on them:



  • Check if at least one of the items has changed (to show the user that something changed)

  • Get all new, deleted and modified objects to update the state in the database

For the first version, it is enough to support POCOs with primitive property types. However, probably I'll extend it in future to support complex property types.



Solution



public class ChangeTracker<TEntity> where TEntity : class

private static readonly PropertyInfo thePropertyInfos;

private readonly List<TEntity> myEntities = new List<TEntity>();
private readonly List<TEntity> myDeletedEntites = new List<TEntity>();
private readonly HashSet<TEntity> myNewEntities = new HashSet<TEntity>();
private readonly Dictionary<TEntity, object> myStates = new Dictionary<TEntity, object>();

static ChangeTracker()

thePropertyInfos = typeof(TEntity).GetProperties().ToArray();


public void Add(TEntity entity)

if (myStates.ContainsKey(entity))
throw new InvalidOperationException("It is not possible to add an entity twice.");

myStates.Add(entity, new object[thePropertyInfos.Length]);
myNewEntities.Add(entity);
myEntities.Add(entity);


public void Delete(TEntity entity)

myEntities.Remove(entity);
if (!myNewEntities.Remove(entity))
myDeletedEntites.Add(entity);


public void AcceptChanges()

myNewEntities.Clear();
myDeletedEntites.Clear();
foreach (var entity in myEntities)
myStates[entity] = GetState(entity);


public bool HasChanges()
EntityStates.Modified


private static object GetState(TEntity entity)

return thePropertyInfos
.Select(pi => pi.GetValue(entity))
.ToArray();


private static void SetState(TEntity entity, object state)

for (int i = 0; i < state.Length; i++)
thePropertyInfos[i].SetValue(entity, state[i]);


private bool HasChanges(TEntity entity)

var currentState = GetState(entity);
var previousState = myStates[entity];

for (int i = 0; i < currentState.Length; i++)


return false;


public IEnumerable<TEntity> GetEntities(EntityStates states)

if (states.HasFlag(EntityStates.New))
foreach (var entity in myNewEntities)
yield return entity;

if (states.HasFlag(EntityStates.Deleted))
foreach (var entity in myDeletedEntites)
yield return entity;

if (!states.HasFlag(EntityStates.Modified) &&
!states.HasFlag(EntityStates.Unmodified))
yield break;

foreach (var entity in myEntities.Where(e => !myNewEntities.Contains(e)))

var isModified = HasChanges(entity);
if (states.HasFlag(EntityStates.Modified) && isModified)
yield return entity;
else if (states.HasFlag(EntityStates.Unmodified) && !isModified)
yield return entity;




[Flags]
public enum EntityStates

New = 1,
Deleted = 2,
Modified = 4,
Unmodified = 8,
All = ~0,



Usage:



public class TestItem

public string Property get; set;


var changeTracker = new ChangeTracker<TestItem>();
var item1 = new TestItem();
var item2 = new TestItem();
var item3 = new TestItem();
var item4 = new TestItem();
var item5 = new TestItem();

changeTracker.Add(item1);
changeTracker.Add(item2);
changeTracker.Add(item3);
changeTracker.AcceptChanges();
changeTracker.Add(item4);
changeTracker.Add(item5);
item2.Property = "test";
changeTracker.Delete(item3);
changeTracker.Delete(item5);

Assert.IsTrue(changeTracker.HasChanges());
Assert.AreEqual(4, changeTracker.GetEntities(EntityStates.All).Count());
Assert.AreEqual(item1, changeTracker.GetEntities(EntityStates.Unmodified).Single());
Assert.AreEqual(item2, changeTracker.GetEntities(EntityStates.Modified).Single());
Assert.AreEqual(item3, changeTracker.GetEntities(EntityStates.Deleted).Single());
Assert.AreEqual(item4, changeTracker.GetEntities(EntityStates.New).Single());


Any feedback is welcome. However, I am specially intrested in alternative more performant solutions (without the overhead of boxing/unboxing and reflection).



[Edit]



I also tried binary serialization as "state storing strategy", but it is much slower (~100-200%) and it has the downside that the entity classes have to be marked as serializable.







share|improve this question





















  • thePropertyInfos - a new prefix? ;-D
    – t3chb0t
    Jan 26 at 20:50










  • no, it's the old one for static fields. The prefix for instance fields is still 'my' ;)
    – JanDotNet
    Jan 26 at 21:00










  • One more question, this time a technical one... why is EntityStates a Flags enum? It doesn't make much sense that an entity can have more then one state at the same time.
    – t3chb0t
    Jan 26 at 21:17










  • Becuase it allows to get e.g. all modified, new and deleted entites by combining the enum values: changeTracker.GetEntities(EntityStates.Modified | EntityStates.New | EntityStates.Deleted)
    – JanDotNet
    Jan 26 at 21:20










  • Further more, the enum is not used to associate the entity with a state - just to get entites for special states.
    – JanDotNet
    Jan 26 at 21:21













up vote
5
down vote

favorite









up vote
5
down vote

favorite











Problem



I have a list of POCO model objects (approximatly 1.000 to 10.000) and I want to track changes on them:



  • Check if at least one of the items has changed (to show the user that something changed)

  • Get all new, deleted and modified objects to update the state in the database

For the first version, it is enough to support POCOs with primitive property types. However, probably I'll extend it in future to support complex property types.



Solution



public class ChangeTracker<TEntity> where TEntity : class

private static readonly PropertyInfo thePropertyInfos;

private readonly List<TEntity> myEntities = new List<TEntity>();
private readonly List<TEntity> myDeletedEntites = new List<TEntity>();
private readonly HashSet<TEntity> myNewEntities = new HashSet<TEntity>();
private readonly Dictionary<TEntity, object> myStates = new Dictionary<TEntity, object>();

static ChangeTracker()

thePropertyInfos = typeof(TEntity).GetProperties().ToArray();


public void Add(TEntity entity)

if (myStates.ContainsKey(entity))
throw new InvalidOperationException("It is not possible to add an entity twice.");

myStates.Add(entity, new object[thePropertyInfos.Length]);
myNewEntities.Add(entity);
myEntities.Add(entity);


public void Delete(TEntity entity)

myEntities.Remove(entity);
if (!myNewEntities.Remove(entity))
myDeletedEntites.Add(entity);


public void AcceptChanges()

myNewEntities.Clear();
myDeletedEntites.Clear();
foreach (var entity in myEntities)
myStates[entity] = GetState(entity);


public bool HasChanges()
EntityStates.Modified


private static object GetState(TEntity entity)

return thePropertyInfos
.Select(pi => pi.GetValue(entity))
.ToArray();


private static void SetState(TEntity entity, object state)

for (int i = 0; i < state.Length; i++)
thePropertyInfos[i].SetValue(entity, state[i]);


private bool HasChanges(TEntity entity)

var currentState = GetState(entity);
var previousState = myStates[entity];

for (int i = 0; i < currentState.Length; i++)


return false;


public IEnumerable<TEntity> GetEntities(EntityStates states)

if (states.HasFlag(EntityStates.New))
foreach (var entity in myNewEntities)
yield return entity;

if (states.HasFlag(EntityStates.Deleted))
foreach (var entity in myDeletedEntites)
yield return entity;

if (!states.HasFlag(EntityStates.Modified) &&
!states.HasFlag(EntityStates.Unmodified))
yield break;

foreach (var entity in myEntities.Where(e => !myNewEntities.Contains(e)))

var isModified = HasChanges(entity);
if (states.HasFlag(EntityStates.Modified) && isModified)
yield return entity;
else if (states.HasFlag(EntityStates.Unmodified) && !isModified)
yield return entity;




[Flags]
public enum EntityStates

New = 1,
Deleted = 2,
Modified = 4,
Unmodified = 8,
All = ~0,



Usage:



public class TestItem

public string Property get; set;


var changeTracker = new ChangeTracker<TestItem>();
var item1 = new TestItem();
var item2 = new TestItem();
var item3 = new TestItem();
var item4 = new TestItem();
var item5 = new TestItem();

changeTracker.Add(item1);
changeTracker.Add(item2);
changeTracker.Add(item3);
changeTracker.AcceptChanges();
changeTracker.Add(item4);
changeTracker.Add(item5);
item2.Property = "test";
changeTracker.Delete(item3);
changeTracker.Delete(item5);

Assert.IsTrue(changeTracker.HasChanges());
Assert.AreEqual(4, changeTracker.GetEntities(EntityStates.All).Count());
Assert.AreEqual(item1, changeTracker.GetEntities(EntityStates.Unmodified).Single());
Assert.AreEqual(item2, changeTracker.GetEntities(EntityStates.Modified).Single());
Assert.AreEqual(item3, changeTracker.GetEntities(EntityStates.Deleted).Single());
Assert.AreEqual(item4, changeTracker.GetEntities(EntityStates.New).Single());


Any feedback is welcome. However, I am specially intrested in alternative more performant solutions (without the overhead of boxing/unboxing and reflection).



[Edit]



I also tried binary serialization as "state storing strategy", but it is much slower (~100-200%) and it has the downside that the entity classes have to be marked as serializable.







share|improve this question













Problem



I have a list of POCO model objects (approximatly 1.000 to 10.000) and I want to track changes on them:



  • Check if at least one of the items has changed (to show the user that something changed)

  • Get all new, deleted and modified objects to update the state in the database

For the first version, it is enough to support POCOs with primitive property types. However, probably I'll extend it in future to support complex property types.



Solution



public class ChangeTracker<TEntity> where TEntity : class

private static readonly PropertyInfo thePropertyInfos;

private readonly List<TEntity> myEntities = new List<TEntity>();
private readonly List<TEntity> myDeletedEntites = new List<TEntity>();
private readonly HashSet<TEntity> myNewEntities = new HashSet<TEntity>();
private readonly Dictionary<TEntity, object> myStates = new Dictionary<TEntity, object>();

static ChangeTracker()

thePropertyInfos = typeof(TEntity).GetProperties().ToArray();


public void Add(TEntity entity)

if (myStates.ContainsKey(entity))
throw new InvalidOperationException("It is not possible to add an entity twice.");

myStates.Add(entity, new object[thePropertyInfos.Length]);
myNewEntities.Add(entity);
myEntities.Add(entity);


public void Delete(TEntity entity)

myEntities.Remove(entity);
if (!myNewEntities.Remove(entity))
myDeletedEntites.Add(entity);


public void AcceptChanges()

myNewEntities.Clear();
myDeletedEntites.Clear();
foreach (var entity in myEntities)
myStates[entity] = GetState(entity);


public bool HasChanges()
EntityStates.Modified


private static object GetState(TEntity entity)

return thePropertyInfos
.Select(pi => pi.GetValue(entity))
.ToArray();


private static void SetState(TEntity entity, object state)

for (int i = 0; i < state.Length; i++)
thePropertyInfos[i].SetValue(entity, state[i]);


private bool HasChanges(TEntity entity)

var currentState = GetState(entity);
var previousState = myStates[entity];

for (int i = 0; i < currentState.Length; i++)


return false;


public IEnumerable<TEntity> GetEntities(EntityStates states)

if (states.HasFlag(EntityStates.New))
foreach (var entity in myNewEntities)
yield return entity;

if (states.HasFlag(EntityStates.Deleted))
foreach (var entity in myDeletedEntites)
yield return entity;

if (!states.HasFlag(EntityStates.Modified) &&
!states.HasFlag(EntityStates.Unmodified))
yield break;

foreach (var entity in myEntities.Where(e => !myNewEntities.Contains(e)))

var isModified = HasChanges(entity);
if (states.HasFlag(EntityStates.Modified) && isModified)
yield return entity;
else if (states.HasFlag(EntityStates.Unmodified) && !isModified)
yield return entity;




[Flags]
public enum EntityStates

New = 1,
Deleted = 2,
Modified = 4,
Unmodified = 8,
All = ~0,



Usage:



public class TestItem

public string Property get; set;


var changeTracker = new ChangeTracker<TestItem>();
var item1 = new TestItem();
var item2 = new TestItem();
var item3 = new TestItem();
var item4 = new TestItem();
var item5 = new TestItem();

changeTracker.Add(item1);
changeTracker.Add(item2);
changeTracker.Add(item3);
changeTracker.AcceptChanges();
changeTracker.Add(item4);
changeTracker.Add(item5);
item2.Property = "test";
changeTracker.Delete(item3);
changeTracker.Delete(item5);

Assert.IsTrue(changeTracker.HasChanges());
Assert.AreEqual(4, changeTracker.GetEntities(EntityStates.All).Count());
Assert.AreEqual(item1, changeTracker.GetEntities(EntityStates.Unmodified).Single());
Assert.AreEqual(item2, changeTracker.GetEntities(EntityStates.Modified).Single());
Assert.AreEqual(item3, changeTracker.GetEntities(EntityStates.Deleted).Single());
Assert.AreEqual(item4, changeTracker.GetEntities(EntityStates.New).Single());


Any feedback is welcome. However, I am specially intrested in alternative more performant solutions (without the overhead of boxing/unboxing and reflection).



[Edit]



I also tried binary serialization as "state storing strategy", but it is much slower (~100-200%) and it has the downside that the entity classes have to be marked as serializable.









share|improve this question












share|improve this question




share|improve this question








edited Jan 27 at 20:11
























asked Jan 26 at 20:48









JanDotNet

6,4011238




6,4011238











  • thePropertyInfos - a new prefix? ;-D
    – t3chb0t
    Jan 26 at 20:50










  • no, it's the old one for static fields. The prefix for instance fields is still 'my' ;)
    – JanDotNet
    Jan 26 at 21:00










  • One more question, this time a technical one... why is EntityStates a Flags enum? It doesn't make much sense that an entity can have more then one state at the same time.
    – t3chb0t
    Jan 26 at 21:17










  • Becuase it allows to get e.g. all modified, new and deleted entites by combining the enum values: changeTracker.GetEntities(EntityStates.Modified | EntityStates.New | EntityStates.Deleted)
    – JanDotNet
    Jan 26 at 21:20










  • Further more, the enum is not used to associate the entity with a state - just to get entites for special states.
    – JanDotNet
    Jan 26 at 21:21

















  • thePropertyInfos - a new prefix? ;-D
    – t3chb0t
    Jan 26 at 20:50










  • no, it's the old one for static fields. The prefix for instance fields is still 'my' ;)
    – JanDotNet
    Jan 26 at 21:00










  • One more question, this time a technical one... why is EntityStates a Flags enum? It doesn't make much sense that an entity can have more then one state at the same time.
    – t3chb0t
    Jan 26 at 21:17










  • Becuase it allows to get e.g. all modified, new and deleted entites by combining the enum values: changeTracker.GetEntities(EntityStates.Modified | EntityStates.New | EntityStates.Deleted)
    – JanDotNet
    Jan 26 at 21:20










  • Further more, the enum is not used to associate the entity with a state - just to get entites for special states.
    – JanDotNet
    Jan 26 at 21:21
















thePropertyInfos - a new prefix? ;-D
– t3chb0t
Jan 26 at 20:50




thePropertyInfos - a new prefix? ;-D
– t3chb0t
Jan 26 at 20:50












no, it's the old one for static fields. The prefix for instance fields is still 'my' ;)
– JanDotNet
Jan 26 at 21:00




no, it's the old one for static fields. The prefix for instance fields is still 'my' ;)
– JanDotNet
Jan 26 at 21:00












One more question, this time a technical one... why is EntityStates a Flags enum? It doesn't make much sense that an entity can have more then one state at the same time.
– t3chb0t
Jan 26 at 21:17




One more question, this time a technical one... why is EntityStates a Flags enum? It doesn't make much sense that an entity can have more then one state at the same time.
– t3chb0t
Jan 26 at 21:17












Becuase it allows to get e.g. all modified, new and deleted entites by combining the enum values: changeTracker.GetEntities(EntityStates.Modified | EntityStates.New | EntityStates.Deleted)
– JanDotNet
Jan 26 at 21:20




Becuase it allows to get e.g. all modified, new and deleted entites by combining the enum values: changeTracker.GetEntities(EntityStates.Modified | EntityStates.New | EntityStates.Deleted)
– JanDotNet
Jan 26 at 21:20












Further more, the enum is not used to associate the entity with a state - just to get entites for special states.
– JanDotNet
Jan 26 at 21:21





Further more, the enum is not used to associate the entity with a state - just to get entites for special states.
– JanDotNet
Jan 26 at 21:21











1 Answer
1






active

oldest

votes

















up vote
2
down vote



accepted










State arrays



These confuse me rather. I'm going to suggest you don't add an 'empty' array in Add, and instead add null.



myStates.Add(entity, null);


The empty array is a valid state, and as far as I can tell you want this to be treated as a non-set value; null is not, and will cause a violent crash if you try to treat it as such (which is good).



This sort of requires a change to HasChanges (though I don't think it can be called before on a new entity prior to AcceptChanges), to explicitly check if the previousState is null (which could have some performance benefits, but who cares). Of course, you could use some other explicit sentinel type (i.e. do it properly), or just not add anything to the myStates dictionary until such a state exists (has all the benefit of null without any of the fear).



GetEntities



GetEntities is a bit painful to read. I would be inclined to keep new entities in myNewEntities until they are accepted (much cheaper in the case where the changes are rejected), just as deleted entities to not appear in MyEntities. This will simplify the implementation of GetEntities, removing the check for new entities.



You also aren't clearing states stored for deleted entities (should be done in AcceptChanges).



Misc



  • Not a big issue, but I would consider making thePropertyInfos an IReadOnlyList<TEntity> (though I probably wouldn't effect this, since hopefully no-one will ever look at this code).


  • This could do with some inline (\) documentation for the external API, and some indication that it is not thread safe.


  • Is there any particular reason why myEntities is a List and not a HashSet? I'd be somewhat inclined to ditch the list completely and just keep the State dictionary (no redundancy -> less effort to maintain)


  • No RejectChanges? Without this, certain bits can be simplified considerably (e.g. ditch state as soon as you delete something).






share|improve this answer





















  • Thanks for the great feedback, your points are valid and I have changed the implementation as suggested in 'state arrays' and 'GetEntities'. The problem with RejectChange is, that the application holds it's own list of entities. If entites were deleted, reject changes would not restore the entites in the application's list but revert changes of the non-deleted entites. Therefore I decide to not support RejectChanges .
    – JanDotNet
    Jan 27 at 19:40










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%2f186086%2fsimple-change-tracker-for-pocos%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
2
down vote



accepted










State arrays



These confuse me rather. I'm going to suggest you don't add an 'empty' array in Add, and instead add null.



myStates.Add(entity, null);


The empty array is a valid state, and as far as I can tell you want this to be treated as a non-set value; null is not, and will cause a violent crash if you try to treat it as such (which is good).



This sort of requires a change to HasChanges (though I don't think it can be called before on a new entity prior to AcceptChanges), to explicitly check if the previousState is null (which could have some performance benefits, but who cares). Of course, you could use some other explicit sentinel type (i.e. do it properly), or just not add anything to the myStates dictionary until such a state exists (has all the benefit of null without any of the fear).



GetEntities



GetEntities is a bit painful to read. I would be inclined to keep new entities in myNewEntities until they are accepted (much cheaper in the case where the changes are rejected), just as deleted entities to not appear in MyEntities. This will simplify the implementation of GetEntities, removing the check for new entities.



You also aren't clearing states stored for deleted entities (should be done in AcceptChanges).



Misc



  • Not a big issue, but I would consider making thePropertyInfos an IReadOnlyList<TEntity> (though I probably wouldn't effect this, since hopefully no-one will ever look at this code).


  • This could do with some inline (\) documentation for the external API, and some indication that it is not thread safe.


  • Is there any particular reason why myEntities is a List and not a HashSet? I'd be somewhat inclined to ditch the list completely and just keep the State dictionary (no redundancy -> less effort to maintain)


  • No RejectChanges? Without this, certain bits can be simplified considerably (e.g. ditch state as soon as you delete something).






share|improve this answer





















  • Thanks for the great feedback, your points are valid and I have changed the implementation as suggested in 'state arrays' and 'GetEntities'. The problem with RejectChange is, that the application holds it's own list of entities. If entites were deleted, reject changes would not restore the entites in the application's list but revert changes of the non-deleted entites. Therefore I decide to not support RejectChanges .
    – JanDotNet
    Jan 27 at 19:40














up vote
2
down vote



accepted










State arrays



These confuse me rather. I'm going to suggest you don't add an 'empty' array in Add, and instead add null.



myStates.Add(entity, null);


The empty array is a valid state, and as far as I can tell you want this to be treated as a non-set value; null is not, and will cause a violent crash if you try to treat it as such (which is good).



This sort of requires a change to HasChanges (though I don't think it can be called before on a new entity prior to AcceptChanges), to explicitly check if the previousState is null (which could have some performance benefits, but who cares). Of course, you could use some other explicit sentinel type (i.e. do it properly), or just not add anything to the myStates dictionary until such a state exists (has all the benefit of null without any of the fear).



GetEntities



GetEntities is a bit painful to read. I would be inclined to keep new entities in myNewEntities until they are accepted (much cheaper in the case where the changes are rejected), just as deleted entities to not appear in MyEntities. This will simplify the implementation of GetEntities, removing the check for new entities.



You also aren't clearing states stored for deleted entities (should be done in AcceptChanges).



Misc



  • Not a big issue, but I would consider making thePropertyInfos an IReadOnlyList<TEntity> (though I probably wouldn't effect this, since hopefully no-one will ever look at this code).


  • This could do with some inline (\) documentation for the external API, and some indication that it is not thread safe.


  • Is there any particular reason why myEntities is a List and not a HashSet? I'd be somewhat inclined to ditch the list completely and just keep the State dictionary (no redundancy -> less effort to maintain)


  • No RejectChanges? Without this, certain bits can be simplified considerably (e.g. ditch state as soon as you delete something).






share|improve this answer





















  • Thanks for the great feedback, your points are valid and I have changed the implementation as suggested in 'state arrays' and 'GetEntities'. The problem with RejectChange is, that the application holds it's own list of entities. If entites were deleted, reject changes would not restore the entites in the application's list but revert changes of the non-deleted entites. Therefore I decide to not support RejectChanges .
    – JanDotNet
    Jan 27 at 19:40












up vote
2
down vote



accepted







up vote
2
down vote



accepted






State arrays



These confuse me rather. I'm going to suggest you don't add an 'empty' array in Add, and instead add null.



myStates.Add(entity, null);


The empty array is a valid state, and as far as I can tell you want this to be treated as a non-set value; null is not, and will cause a violent crash if you try to treat it as such (which is good).



This sort of requires a change to HasChanges (though I don't think it can be called before on a new entity prior to AcceptChanges), to explicitly check if the previousState is null (which could have some performance benefits, but who cares). Of course, you could use some other explicit sentinel type (i.e. do it properly), or just not add anything to the myStates dictionary until such a state exists (has all the benefit of null without any of the fear).



GetEntities



GetEntities is a bit painful to read. I would be inclined to keep new entities in myNewEntities until they are accepted (much cheaper in the case where the changes are rejected), just as deleted entities to not appear in MyEntities. This will simplify the implementation of GetEntities, removing the check for new entities.



You also aren't clearing states stored for deleted entities (should be done in AcceptChanges).



Misc



  • Not a big issue, but I would consider making thePropertyInfos an IReadOnlyList<TEntity> (though I probably wouldn't effect this, since hopefully no-one will ever look at this code).


  • This could do with some inline (\) documentation for the external API, and some indication that it is not thread safe.


  • Is there any particular reason why myEntities is a List and not a HashSet? I'd be somewhat inclined to ditch the list completely and just keep the State dictionary (no redundancy -> less effort to maintain)


  • No RejectChanges? Without this, certain bits can be simplified considerably (e.g. ditch state as soon as you delete something).






share|improve this answer













State arrays



These confuse me rather. I'm going to suggest you don't add an 'empty' array in Add, and instead add null.



myStates.Add(entity, null);


The empty array is a valid state, and as far as I can tell you want this to be treated as a non-set value; null is not, and will cause a violent crash if you try to treat it as such (which is good).



This sort of requires a change to HasChanges (though I don't think it can be called before on a new entity prior to AcceptChanges), to explicitly check if the previousState is null (which could have some performance benefits, but who cares). Of course, you could use some other explicit sentinel type (i.e. do it properly), or just not add anything to the myStates dictionary until such a state exists (has all the benefit of null without any of the fear).



GetEntities



GetEntities is a bit painful to read. I would be inclined to keep new entities in myNewEntities until they are accepted (much cheaper in the case where the changes are rejected), just as deleted entities to not appear in MyEntities. This will simplify the implementation of GetEntities, removing the check for new entities.



You also aren't clearing states stored for deleted entities (should be done in AcceptChanges).



Misc



  • Not a big issue, but I would consider making thePropertyInfos an IReadOnlyList<TEntity> (though I probably wouldn't effect this, since hopefully no-one will ever look at this code).


  • This could do with some inline (\) documentation for the external API, and some indication that it is not thread safe.


  • Is there any particular reason why myEntities is a List and not a HashSet? I'd be somewhat inclined to ditch the list completely and just keep the State dictionary (no redundancy -> less effort to maintain)


  • No RejectChanges? Without this, certain bits can be simplified considerably (e.g. ditch state as soon as you delete something).







share|improve this answer













share|improve this answer



share|improve this answer











answered Jan 27 at 11:11









VisualMelon

2,280716




2,280716











  • Thanks for the great feedback, your points are valid and I have changed the implementation as suggested in 'state arrays' and 'GetEntities'. The problem with RejectChange is, that the application holds it's own list of entities. If entites were deleted, reject changes would not restore the entites in the application's list but revert changes of the non-deleted entites. Therefore I decide to not support RejectChanges .
    – JanDotNet
    Jan 27 at 19:40
















  • Thanks for the great feedback, your points are valid and I have changed the implementation as suggested in 'state arrays' and 'GetEntities'. The problem with RejectChange is, that the application holds it's own list of entities. If entites were deleted, reject changes would not restore the entites in the application's list but revert changes of the non-deleted entites. Therefore I decide to not support RejectChanges .
    – JanDotNet
    Jan 27 at 19:40















Thanks for the great feedback, your points are valid and I have changed the implementation as suggested in 'state arrays' and 'GetEntities'. The problem with RejectChange is, that the application holds it's own list of entities. If entites were deleted, reject changes would not restore the entites in the application's list but revert changes of the non-deleted entites. Therefore I decide to not support RejectChanges .
– JanDotNet
Jan 27 at 19:40




Thanks for the great feedback, your points are valid and I have changed the implementation as suggested in 'state arrays' and 'GetEntities'. The problem with RejectChange is, that the application holds it's own list of entities. If entites were deleted, reject changes would not restore the entites in the application's list but revert changes of the non-deleted entites. Therefore I decide to not support RejectChanges .
– JanDotNet
Jan 27 at 19:40












 

draft saved


draft discarded


























 


draft saved


draft discarded














StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f186086%2fsimple-change-tracker-for-pocos%23new-answer', 'question_page');

);

Post as a guest













































































Popular posts from this blog

Chat program with C++ and SFML

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

Will my employers contract hold up in court?