3
\$\begingroup\$

I have an ordering system that uses SharePoint. Below I've included a small snapshot of the system that is used in a WPF form when the user clicks a button to refresh the order listing. The outcome is a populated WPF datagrid. Note that this code is using C# 6.0 as well.

Everything works well, and it's quite quick, but I feel there's lots of room in my skill-set for improvements - happy for all guidance that can be offered!

Refresh Orders code

private void RefreshOrders(string loadStatus)
{
    Orders.LoadStatus = loadStatus;

    var collListItem = Orders.LatestOrders;

    if (!collListItem.Any())
    {
        MessageBox.Show(
            "No orders are currently within the queue.",
            "Information Center",
            MessageBoxButton.OK,
            MessageBoxImage.Information);

        return;
    }

    if (collListItem == null)
    {
        MessageBox.Show(
            "Order processor was unable to reach the SharePoint server to download orders.",
            "SharePoint Currently Unavailable",
            MessageBoxButton.OK,
            MessageBoxImage.Asterisk);

        return;
    }

    var customers = new ObservableCollection<Customer>();

    foreach (var customer in
        collListItem.Select(
            item =>
            new
            {
                Persona = item["Persona"].ToString(),
                CustomerName = item["Title"].ToString(),
                Email = item["Customer_x0020_E_x002d_mail"].ToString(),
                Organization = item["Organization"].ToString(),
                PhoneNumber = item["Customer_x0020_Phone_x0020_Numbe"].ToString(),
                Street = item["Customer_x0020_Street"].ToString(),
                Suburb = item["Customer_x0020_Suburb"].ToString(),
                Postcode = item["Customer_x0020_Postcode"].ToString(),
                OrderDate = Convert.ToDateTime(item["Created"], new CultureInfo("hr-HR")).Date ,
                DispatchedDate = Convert.ToDateTime(item["Modified"], new CultureInfo("hr-HR")).Date,
                Status = item["Status"].ToString()
            })
            .Distinct()
            .Select(
                r =>
                new Customer
                {
                    Persona = r.Persona,
                    CustomerName = r.CustomerName,
                    Email = r.Email,
                    Organization = r.Organization,
                    PhoneNumber = r.PhoneNumber,
                    Street = r.Street,
                    Suburb = r.Suburb,
                    Postcode = r.Postcode,
                    OrderDate = r.OrderDate.Date,
                    DispatchedDate = r.DispatchedDate,
                    Status = r.Status
                }))
    {
        customers.Add(customer);
    }

    dataGridOutstandingOrders.ItemsSource = customers;
}

My customer class

public class Customer
{
    /// <summary>
    /// Gets or sets the status.
    /// </summary>
    public string Status { get; set; }

    /// <summary>
    /// The order date.
    /// </summary>
    private DateTime orderDate;

    /// <summary>
    /// The dispatched date.
    /// </summary>
    private DateTime dispatchedDate;

    /// <summary>
    /// The id.
    /// </summary>
    public int Id;

    /// <summary>
    /// Gets or sets the persona.
    /// </summary>
    public string Persona { get; set; }

    /// <summary>
    /// Gets or sets the customer name.
    /// </summary>
    [DisplayName(@"Customer Name")]
    public string CustomerName { get; set; }

    /// <summary>
    /// Gets or sets the resource name.
    /// </summary>
    public string ResourceName { get; set; }

    /// <summary>
    /// Gets or sets the quantity.
    /// </summary>
    public int Quantity { get; set; }

    /// <summary>
    /// Gets or sets the company.
    /// </summary>
    public string Organization { get; set; }

    /// <summary>
    /// Gets or sets the email.
    /// </summary>
    public string Email { get; set; }

    /// <summary>
    /// Gets or sets the phone number.
    /// </summary>
    public string PhoneNumber { get; set; }

    /// <summary>
    /// Gets or sets the street.
    /// </summary>
    public string Street { get; set; }

    /// <summary>
    /// Gets or sets the suburb.
    /// </summary>
    public string Suburb { get; set; }

    /// <summary>
    /// Gets or sets the postcode.
    /// </summary>
    public string Postcode { get; set; }

    /// <summary>
    /// Gets or sets a value indicating whether is checked.
    /// </summary>
    public bool IsChecked { get; set; }

    /// <summary>
    /// Gets or sets the partially dispatched.
    /// </summary>
    public int PartiallyDispatched { get; set; }

    /// <summary>
    /// Gets or sets the order date.
    /// </summary>
    public DateTime OrderDate
    {
        get
        {
            return Convert.ToDateTime(this.orderDate, new CultureInfo("hr-HR")).Date;
        }
        set
        {
            this.orderDate = value.Date;
        }
    }

    public DateTime DispatchedDate
    {
        get
        {
            return Convert.ToDateTime(this.dispatchedDate, new CultureInfo("hr-HR")).Date;
        }
        set
        {
            this.dispatchedDate = value.Date;
        }
    }
}

My orders class

public static class Orders
{
    public static string LoadStatus = "Approved";
    /// <summary>
    /// Backing field for storing the latest orders that have been retrieved from SharePoint
    /// </summary>
    private static ListItemCollection ordersCache;

    /// <summary>
    /// Property for retrieving cached orders, will fetch and set if none are currently present.
    /// </summary>
    public static ListItemCollection CachedOrders
        => ordersCache ?? (ordersCache = LoadListItemCollection(LoadStatus));

    /// <summary>
    /// Property to retrieve the latest orders and cache for future reference.
    /// </summary>
    public static ListItemCollection LatestOrders => ordersCache = LoadListItemCollection(LoadStatus);

    /// <summary>
    /// Loads orders from SharePoint using CAML
    /// </summary>
    /// <returns>
    /// The <see cref="ListItemCollection"/>.
    /// </returns>
    private static ListItemCollection LoadListItemCollection(string status)
    {
        using (var ctx = new ClientContext("http://sharepoint/resources"))
        {
            var list = ctx.Web.Lists.GetByTitle("Resource Orders");


            var query = new CamlQuery
                            {
                ViewXml = @"<View><Query><Where><Eq><FieldRef Name=""Status""></FieldRef><Value Type=""Text"">Approved</Value></Eq></Where></Query></View>"
            };

            switch (status)
            {

                case "Ordered":
                    query.ViewXml =
                        @"<View><Query><Where><Eq><FieldRef Name=""Status""></FieldRef><Value Type=""Text"">Ordered</Value></Eq></Where></Query></View>";
                    break;

                case "History":
                    query.ViewXml =
                        @"<View><Query><Where><In><FieldRef Name=""Status""/><Values><Value Type=""Text"" >Rejected</Value><Value Type=""Text"">Dispatched</Value><Value Type=""Text"">Ordered</Value><Value Type=""Text"">Approved</Value></Values></In></Where></Query></View>";
                    break;

                case "Approved":
                default:
                    query.ViewXml =
                        @"<View><Query><Where><Eq><FieldRef Name=""Status""></FieldRef><Value Type=""Text"">Approved</Value></Eq></Where></Query></View>";
                    break;
            }

            var collListItem = list.GetItems(query);

            ctx.Load(
                collListItem,
                items =>
                items.Include(
                    item => item.Id,
                    item => item.DisplayName,
                    item => item.HasUniqueRoleAssignments,
                    item => item["Status"] ,
                    item => item["Persona"],
                    item => item["Quantity_x0020_Ordered"],
                    item => item["Resource_x0020_Name"],
                    item => item["Title"],
                    item => item["Customer_x0020_E_x002d_mail"],
                    item => item["Customer_x0020_Phone_x0020_Numbe"],
                    item => item["Customer_x0020_Street"],
                    item => item["Customer_x0020_Suburb"],
                    item => item["Customer_x0020_Postcode"],
                    item => item["Organization"],
                    item => item["Modified"],
                    item => item["Created"]));

            ctx.ExecuteQuery();

            return collListItem;
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

Have you tested this? You've used the words 'quick' and 'SharePoint' in the same question which, in my experience, is almost always a lie :)

You have a problem here:

private void RefreshOrders(string loadStatus)
{
    Orders.LoadStatus = loadStatus;

    var collListItem = Orders.LatestOrders;

    if (!collListItem.Any())
    {
        // omitted.
    }

    if (collListItem == null)
    {

That second if statement can never be hit! If collListItem is null, then the call to Any() will throw a null reference exception.

A few quick notes on your code:

  • LoadStatus should probably be an enum
  • Put the column names in named constants
  • A property shouldn't do non trivial work - CachedItems should be retrieved through a method
  • You could remove the foreach loop to add to customers observable collection and use the constructor which takes an IEnumerable

Update

var customerQuery = collListItem.Select(
        item =>
        new
        {
            Persona = item["Persona"].ToString(),
            CustomerName = item["Title"].ToString(),
            Email = item["Customer_x0020_E_x002d_mail"].ToString(),
            Organization = item["Organization"].ToString(),
            PhoneNumber = item["Customer_x0020_Phone_x0020_Numbe"].ToString(),
            Street = item["Customer_x0020_Street"].ToString(),
            Suburb = item["Customer_x0020_Suburb"].ToString(),
            Postcode = item["Customer_x0020_Postcode"].ToString(),
            OrderDate = Convert.ToDateTime(item["Created"], new CultureInfo("hr-HR")).Date ,
            DispatchedDate = Convert.ToDateTime(item["Modified"], new CultureInfo("hr-HR")).Date,
            Status = item["Status"].ToString()
        })
        .Distinct()
        .Select(
            r =>
            new Customer
            {
                Persona = r.Persona,
                CustomerName = r.CustomerName,
                Email = r.Email,
                Organization = r.Organization,
                PhoneNumber = r.PhoneNumber,
                Street = r.Street,
                Suburb = r.Suburb,
                Postcode = r.Postcode,
                OrderDate = r.OrderDate.Date,
                DispatchedDate = r.DispatchedDate,
                Status = r.Status
            }));

var customers = new ObservableCollection<Customer>(customerQuery);
\$\endgroup\$
  • \$\begingroup\$ I'm a bit lost how I could add customers using IEnumerable, I don't suppose you could provide a loose example or reference at all? \$\endgroup\$ – Michael A Aug 26 '15 at 23:15
  • \$\begingroup\$ @Codingo - added that for you. \$\endgroup\$ – RobH Aug 27 '15 at 7:47
2
\$\begingroup\$

Considering there are calls to MessageBox I suspect you're not using the MVVM pattern. I would really advise you to do this. That way you can also avoid things like dataGridOutstandingOrders.ItemsSource and instead bind via XAML.


collListItem is an incomprehensible variable name, especially considering that it isn't a single item but apparently a list of items. Why do you even create it? Why don't you simply use Orders.LatestOrders?


There are repeated uses of new CultureInfo("hr-HR"). What if you need to change the CultureInfo? Store this in a single place and reuse it.


In one and the same LINQ method you first convert SharePoint ListItems to an anonymous type, and then convert those to Customers. This is something that really should be happening in a separate class. Moreover, it should not be a two-step operation: convert those ListItems to Customers directly.


The column names you use in RefreshOrders are also appearing in Orders. These are "magic strings" and should be constants, stored in a static class.


IMHO class names rarely should be plural. But then most of what Orders does shouldn't be there anyway, it belongs in a service. This goes back to my first point: you should restructure your project and separate the various layers. In your current code you're talking to SharePoint from what is basically the code-behind of a UI object.

As it happens I'm currently working on a project where I'm extracting data from SharePoint. Here's my current structure:

  • Retriever class that connects to SharePoint and returns ListItems.
  • Converter class that converts ListItems to custom classes.
  • Storer class that saves these entities to the database.
  • Both Retriever and Converter use a Config class that contains the names of the various SharePoint fields and the name of the SharePoint list.

Each of these classes is small and easily maintainable. That's what you should strive for: make it easy for the next person to look at your code to figure out what happens where. Having an Orders class where you to the retrieval from SharePoint and return a ListItemCollection which gets converted to Customers in the code-behind of a UI class is a maintenance nightmare.

\$\endgroup\$
  • 1
    \$\begingroup\$ Just to add to your fourth point: You have to make sure you get GetHashCode and Equals correct as Distinct is being used. (+1 though) \$\endgroup\$ – RobH Aug 26 '15 at 11:43
  • \$\begingroup\$ 'collListItem' is pretty common in SharePoint land, its origins are in the MSDN documentation. Going through the rest of the feedback now and looking at what changes I can make, thank-you very much! \$\endgroup\$ – Michael A Aug 26 '15 at 23:17
  • \$\begingroup\$ @Codingo MSDN often has code that I wouldn't consider best practices. It's not as bad as the official Microsoft books about classic ASP (which offered advice like "store your db connection in a session variable"), but that doesn't mean you should regard it as exemplary. \$\endgroup\$ – BCdotWEB Aug 27 '15 at 7:53
  • \$\begingroup\$ @BCdotWEB Noted and appreciated, thank-you \$\endgroup\$ – Michael A Aug 27 '15 at 9:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.