Best way of updating a list of unique items

Question

I am wondering what the best practice for updating a list that only accepts unique items. Right now, I have a button that loads data from a database into a list. I can press this button numerous times to check if there is any new data in it. Here is the method I created:

private void loadNewDataFromDatabase()
 {
 DataClasses1DataContext con = new DataClasses1DataContext(LOCALDB_CONN_STRING);
 var unfinishedorder = from x in con.Orders
 where x.Status == "NEW"
 select new
 {
 x.NO,
 x.OrderNumber,
 x.TypeNumber,
 x.Color,
 x.Width,
 x.Height,
 x.Status,
 x.StatusChanged
 };
 foreach (var item in unfinishedorder.ToList())
 {
 bool alreadyExists = lstMainData.Any(x => x.intNO == item.NO);
 if (alreadyExists == false)
 {
 Action dispatchAction = () => lstMainData.Add(new clsMainData(item.NO, item.OrderNumber, item.TypeNumber, item.Color, item.Width, item.Height, item.Status, item.StatusChanged));
 this.Dispatcher.BeginInvoke(dispatchAction);
 }
 }
 }

Is this piece of code ideal for what I am trying to achieve? Basically I am checking if the table's current field in the NO column already exists in the list called lstMainData. If it does not, add it to the list. I read something about HashSets as well but knowing the index of an element in my list is important for some other part of my application and as far as I understood, HashSets do not have ordering of elements.

So, is this code looking fine or are there any immediate beauty changes I could make?

I am using DevExpress controls by the way.

lstMainData is an ObservableCollection consisting of objects in a basic class called clsMainData. The class looks like this:

public class clsMainData
{
 public int intNO { get; set; }
 public string strOrderNumber { get; set; }
 public string strTypeNumber { get; set; }
 public int intColor { get; set; }
 public int intWidth { get; set; }
 public int intHeight { get; set; }
 public string strStatus { get; set; }
 public DateTime? dtStatusChanged { get; set; }
 public clsMainData (int NO, string OrderNumber, string TypeNumber, int Color, int Width, int Height, string Status, DateTime? StatusChanged)
 {
 intNO = NO;
 strOrderNumber = OrderNumber;
 strTypeNumber = TypeNumber;
 intColor = Color;
 intWidth = Width;
 intHeight = Height;
 strStatus = Status;
 dtStatusChanged = StatusChanged;
 }
}

That ObservableCollection will be filled with data from a table in a database the application creates locally on the machine. That database has a table called Orders that has several columns.

I set the ItemsSource of my grid to lstMainData in the constructor of my MainWindow.

The user can highlight a row in the grid and press a button that is called ACCEPT. This button runs the following method that takes a string as parameter. I have other buttons that runs this method as well. In the case of the ACCEPT button, the parameter would be "COMPLETED":

private void updateDatabaseStatus(string status)
{
 DataClasses1DataContext con = new DataClasses1DataContext(LOCALDB_CONN_STRING);
 int rowHandle = tbviewMain.FocusedRowHandle;
 var rowValue = dxgMainGrid.GetRow(rowHandle);
 if (rowValue != null)
 {
 string ordernumber = (rowValue as clsMainData).strOrderNumber;
 var update = from x in con.Orders
 where x.OrderNumber == ordernumber
 select x;
 foreach (var item in update)
 {
 item.Status = status;
 item.StatusChanged = DateTime.Now;
 con.SubmitChanges();
 }
 int index = lstMainData.IndexOf(lstMainData.Where(x => ordernumber == x.strOrderNumber).FirstOrDefault());
 lstMainData.RemoveAt(index);
 dxgMainGrid.RefreshData();
 }
}

The column status will then change to COMPLETED in the table where the row's OrderNumber value is equal to the one in the table (which is the primary key) but will be removed completely from the lstMainData and therefore from the grid as well. The highlighted row is then moved to the next one etc. You can sort the rows with the column Color for example to run through rows with a certain color code first.

The thought behind this is that the database will keep track of how many orders are completed and when they were completed, but the user does not care about this and only wants it removed from the grid.

The reason I need to check if an item already exists in the list is that I do not want duplicates in the grid if the user clicks the button several times. I cannot just disable the button, as clicking the button again would find new rows if any were added to the database table in the mean time.

• itsbruce: Why not use SortedSet? If you want to preserve the order of insertion, just create an extension of SortedSet which wraps each object in a container which has an "order" property, incrementing it by 1 as each item is added and sorting on that property.
• Edward: Welcome to Code Review. The question looks just fine, but it might be useful if you could provide additional context for the function.
• itsbruce: It would certainly help to know why the index within the list is important. Not least because you are discarding duplicate items, so the order does not exactly match what is in the database.
• St0ffer: I updated my question to include more information to the thoughts behind the process. Now that I think about it, it might not be necessary at all to find the index to remove the right item from the list in updateDatabaseStatus()
• RobH: Good update - I assume your "NO" is a unique field? I.e. every order has a unique "NO"?

Answer 1

Naming:

General advice:

Classnames, properties (not fields) and methods use PascalCase, private members and parameters use camelCase.

Also, you prepend the type to every name you use. cls for a class, str for a string, etc... Don't do this.

Property-names like intNO and strOrderNumber should be No and OrderNumber. The name of the class becomes MainData, and the method updateDatabaseStatus should be UpdateDatabaseStatus. The names of the parameters in the constructor of the MainData class should start lower case: string TypeNumber will become string typeNumber.

Variables should make clear what they stand for. A name like con doesn't say much, make it dbContext instead.

The var keyword:

From MSDN:

Use implicit typing for local variables when the type of the variable is obvious from the right side of the assignment, or when the precise type is not important.

Do not use var when the type is not apparent from the right side of the assignment.

So this line:

bool alreadyExists = lstMainData.Any(x => x.intNO == item.NO);

will become:

var alreadyExists = lstMainData.Any(x => x.intNO == item.NO);

MainData:

public class MainData
{
 public int No { get; set; }
 public string OrderNumber { get; set; }
 public string TypeNumber { get; set; }
 public int Color { get; set; }
 public int Width { get; set; }
 public int Height { get; set; }
 public string Status { get; set; }
 public DateTime? StatusChanged { get; set; }
 public MainData (int no, string orderNumber, string typeNumber, int color, int width, int height, string status, DateTime? statusChanged)
 {
 No = no;
 OrderNumber = orderNumber;
 TypeNumber = typeNumber;
 Color = color;
 Width = width;
 Height = height;
 Status = status;
 StatusChanged = statusChanged;
 }
}

I only don't understand why you set the properties through the constructor and then don't validate them. Your code now only enforces that all properties will be set, although unvalidated.

Also, if you want the properties to be set through the constructor only, make the set private. Example:

public string OrderNumber { get; private set; }

If this is not the case, ignore this tip.

LoadNewDataFromDatabase():

Generally this is not bad, but it can be improved. Naming tips will already be applied without further explanation, this is done in the previous part.

Since you already know the loaded data in the method, you should not get all the items from the database and then check which ones you want to add to the main data list. Fetch only the items that are not already in the list. This makes more sense but more important: your SQL-task will be less heavy.

private void LoadNewDataFromDatabase()
{
 var dbContext= new DataClasses1DataContext(LOCALDB_CONN_STRING);
 var existingNrs = mainDataItems.Select(x => x.No);
 var unfinishedOrders = dbContext.Orders.Where(x => x.Status == "NEW" && !existingNrs.Contains(x.No));
 foreach (var item in unfinishedOrders)
 {
 Action dispatchAction = () => mainDataItems.Add(new MainData(item.No, item.OrderNumber, item.TypeNumber, item.Color, item.Width, item.Height, item.Status, item.StatusChanged));
 this.Dispatcher.BeginInvoke(dispatchAction);
 }
}

UpdateDatabaseStatus():

You should only instantiate the DataClasses1DataContext when rowValue is not null. And since you don't use the rowHandle variable, get the rowValue in one line. I also applied the naming tips already in this method.

private void updateDatabaseStatus(string status)
{
 var rowValue = dxgMainGrid.GetRow(tbviewMain.FocusedRowHandle);
 if (rowValue != null)
 {
 var dbContext = new DataClasses1DataContext(LOCALDB_CONN_STRING);
 var orderNumber = (rowValue as MainData).OrderNumber;
 var ordersToUpdate = dbContext.Orders.Where(x => x.OrderNumber == orderNumber);
 foreach (var order in ordersToUpdate)
 {
 order.Status = status;
 order.StatusChanged = DateTime.Now;
 dbContext.SubmitChanges();
 }
 var index = mainDataItems.IndexOf(mainDataItems.Where(x => (rowValue as MainData).OrderNumber == x.OrderNumber).FirstOrDefault());
 mainDataItems.RemoveAt(index);
 dxgMainGrid.RefreshData();
 }
}

• RubberDuck: I keep seeing you tell people to be consistent with var. I don't feel that's good advice. You should use var only when the type is obvious, otherwise, it's best to explicitly declare the type. This will always lead to mixing and matching var with explicit typing. Otherwise, it's a nice answer.
• St0ffer: Wow that was a very thorough response. Thanks a lot, I can definitely see the improvements here. Also a good explanation of the common practices, which should be pretty clear is not something I have considered that much.
• Abbas: @RubberDuck The OP has following in his code: bool alreadyExists = lstMainData.Any(x => x.intNO == item.NO); . I think var is in it's place here. Sorry to ask, but did you downvote?
• St0ffer: I noticed that only fetching the items not already in the list removes the need for doing the alreadyExists check, which is pretty great. I had to add .ToList() before to avoid the "Collection was modified" exception; now I do not have to.
• RubberDuck: Abbas, if you want to talk about this, we can take it to chat. Fact of the matter is, I did leave a comment explaining my downvote.

Answer 2

Quick notes:

• loadNewDataFromDatabase() and updateDatabaseStatus() do not follow the convention that method names should be in PascalCase.

• unfinishedorder doesn't follow camelCase ("order" should have a capital) and moreover it should be plural.

• Do not use Hungarian notation, e;g. clsMainData, strOrderNumber, etc.

• I don't see the point of having a multi-argument constructor for clsMainData when you don't do anything with the provided arguments (e.g. check their validity).

• Moreover, why convert the result from your query on Orders to an anonymous type? Why not work with the resulting Order objects directly and then convert that object to a clsMainData if/when necessary?

• DataClasses1DataContext is IMHO a bad name.

• You instantiate a DataClasses1DataContext in updateDatabaseStatus() while you might not even need it. Only do so when you need one, i.e. if (rowValue != null).

• Why have rowHandle when you only use it once? Just have var rowValue = dxgMainGrid.GetRow(tbviewMain.FocusedRowHandle);.

• update is a bad name for that variable. Also, why do you do select when you only should retrieve one item?

• You should really separate the DB-related code into a distinct class. Separate your code into layers. It might sound like overkill for a "simple application", but applications usually don't remain simple. Moreover, it is better to learn such good practices early on and apply them as much as possible.

• St0ffer: Thanks for the input, I will definitely consider these points for better readability!

Answer 3

You can use Enumerable.Except (or Queryable.Except) to get the missing items. I'm not very comfortable with Query syntax, but you can use w/e you like.

var missingItems = con.Orders
 .Where(x => x.Status == "NEW")
 .Select(x => new clsMainData(x.NO, ....))
//you will have to either implement IEquatable<T> or pass IEqualityComparer<T> as parameter
 .Except(lstMainData)
 .ToArray();
Dispatcher.BeginInvoke(new Action(() => lstMainData.AddRange(missingItems)));