HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Best way of updating a list of unique items

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
uniqueupdatingwayitemslistbest

Problem

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 DevExpr

Solution

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

Code Snippets

bool alreadyExists = lstMainData.Any(x => x.intNO == item.NO);
var alreadyExists = lstMainData.Any(x => x.intNO == item.NO);
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;
    }
}
public string OrderNumber { get; private set; }
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);
    }
}

Context

StackExchange Code Review Q#73443, answer score: 7

Revisions (0)

No revisions yet.