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.

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();
    }
}

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.

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)));