Accessing fields in LINQ query for customers with overdue balances

Question

Practicing my autodidactism using a free Kindle Unlimited book to get myself acquainted with the basics of C# in hopes of getting enough of a foundation to get into automation and I still feel a little wibbly on the best practices, etc and I just feel like I could've been a bit more elegant than what I enacted with the get; set; but I don't know what I don't know.

"C#: Learn C# in One Day and Learn it Well" in this instance. Which, I've exceeded a day by far... anyways, I've been challenging myself to use what I have learnt in prior chapters to unravel and actively enact theoretical applications in the book without any actual 'code-alongside' snippets when they rear their head.

I'm current learning about var and their usage in using LINQ queries which is fairly easy as I've prior experience in SQL but I feel like I've inelegantly accessed/set the values for what cust.Balance and cust.Name are pulling in the code with the get; set; in the snippet below and feel like there's some way to have accessed them without actively creating another set of public strings.

Is there a better way to have executed this?

 using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Text;
 using System.Threading.Tasks;
 namespace LinqTut2
 {
 class Program
 {
 static void Main(string[] args)
 {
 List<Customer> customers = new List<Customer>();
 customers.Add(new Customer("Alan", "80911291", "ABC Street", 25.60m));
 customers.Add(new Customer("Bill", "19872131", "DEF Street", -32.10m));
 customers.Add(new Customer("Carl", "29812371", "GHI Street", -12.2m));
 customers.Add(new Customer("David", "78612312", "JKL Street", 12.6m));
 var overdue =
 from cust in customers
 where cust.Balance < 0
 orderby cust.Balance ascending
 select new { cust.Name, cust.Balance };
 foreach (var cust in overdue)
 Console.WriteLine("Name = {0}, Balance = {1}", cust.Name, cust.Balance);
 Console.ReadLine();
 }
 }
 class Customer
 {
 private string CustName;
 private string CustPhone;
 private string CustAddress;
 private decimal CustBalance;
 public decimal Balance
 {
 get
 {
 return CustBalance;
 }
 set
 {
 CustBalance = value;
 }
 }
 public string Name
 {
 get
 {
 return CustName;
 }
 set
 {
 CustName = value;
 }
 }
 public Customer(string name, string phn, string addr, decimal bal)
 {
 CustName = name;
 CustPhone = phn;
 CustAddress = addr;
 CustBalance = bal;
 }
 }
 }

• user33306: It seems to me that, a more practical approach would be creating a data table with the data you want and use the SQL query to filter the data. This would have more practical uses in production code.

Answer 1

This is a pretty good attempt for someone just starting out. However, there is always a lot to learn and the books never seem to take advantage of all the latest language features.

The first thing I'm going to tell you to do is stop abbreviating everything. You don't pay extra for full naming.

I'll choose you constructor to highlight this:

public Customer(string name, string phn, string addr, decimal bal)
{
 CustName = name;
 CustPhone = phn;
 CustAddress = addr;
 CustBalance = bal;
}

First, Customer is a great name, name is also good.

Now, string phn I'd guess at phoneNumber but it could also be the name of the Primary Health Network if this was an Australian healthcare customer. string addr - is address really that much harder to type? decimal is a great choice for the balance but bal is a terrible name and there's some extra whitespace which should be removed.

Also, your fields shouldn't start with Cust. They should aslo be camelCase. The old version of the guidelines stated this explicitly but the newer C# standards don't specify a convention for non public fields. Most people use camelCase in my experience. As a commenter has mentioned, you can use auto properties:

public string Name { get; set; }
public decimal Balance { get;set; }

You can set them directly in your constructor.

---

I'm assuming you're using the latest and greatest C# for the next few points. I can't remember when collection initializers were introduced but it's a while ago now.

This is a very common pattern:

List<Customer> customers = new List<Customer>();
customers.Add(new Customer("Alan", "80911291", "ABC Street", 25.60m));
customers.Add(new Customer("Bill", "19872131", "DEF Street", -32.10m));
customers.Add(new Customer("Carl", "29812371", "GHI Street", -12.2m));
customers.Add(new Customer("David", "78612312", "JKL Street", 12.6m));

It's so common, we have a construct to make it easier:

var customers = new List<Customer>
{
 new Customer("Alan", "80911291", "ABC Street", 25.60m),
 new Customer("Bill", "19872131", "DEF Street", -32.10m),
 new Customer("Carl", "29812371", "GHI Street", -12.2m),
 new Customer("David", "78612312", "JKL Street", 12.6m)
};

That's a lot less typing and a bit easier to scan IMO.

We also now have string interpolation (this one is much newer):

Console.WriteLine("Name = {0}, Balance = {1}", cust.Name, cust.Balance);

Can become:

Console.WriteLine($"Name = {cust.Name}, Balance = {cust.Balance}");

Which is a lot less scanning backwards and forwards to see what goes where.

---

For the sake of completeness, let's look at your naming here:

var overdue =
 from cust in customers
 where cust.Balance < 0
 orderby cust.Balance ascending
 select new { cust.Name, cust.Balance };

overdue is better as overdueCustomers. from customer in customers is equally better than using cust which isn't a word.

You could then finally do a foreach (var overdueCustomer in overdueCustomers) to make it obvious everywhere in the code that the reason you're interested in this particular customer is that they are overdue.

Edit

As noted in a comment from Peter Taylor, overdrawnCustomers is an even better name. It doesn't have the date connotation that overdue does.

• Peter Taylor: Good critiques in general, but overdueCustomers doesn't fix the biggest problem with the name: overdue implies that there's a date involved somewhere. overdrawnCustomers would be descriptive.

Answer 2

I totally agree with @RobH answer but I would like to mention some extra things.

You have 2 unused fields that I guess are there for testing reasons but they are good to illustrate the concept of readonly fields/properties as you only set them in the constructor.

Your Customer class could look like this.

class Customer
{
 private readonly string CustPhone; // read only field
 private string Address { get; } // read only property
 public decimal Balance { get; set; }
 public string Name { get; set; }
 public Customer(string name, string phn, string addr, decimal bal)
 {
 Name = name;
 CustPhone = phn;
 Address = addr;
 Balance = bal;
 }
}

For the query you are doing, my personal choice would be to use fluent syntax

 overdrawnCustomers = customers.Select(c => new {c.Name, c.Balance}) .
 Where(c => c.Balance < 0).OrderBy(c => c.Balance);

And finally, I am against this

 foreach (var cust in overdrawnCustomers)
 Console.WriteLine("Name = {0}, Balance = {1}", cust.Name, cust.Balance);

And I would rewrite it like

 foreach (var cust in overdrawnCustomers)
 {
 Console.WriteLine("Name = {0}, Balance = {1}", cust.Name, cust.Balance);
 }

Keep on going learning c#, my feeling is that you are doing pretty well :)