Is this a correct class implementation and initialization?

Question

I've found my coding practices to be slipping lately - and noticed myself falling into some bad habits - usually due to lack of motivation on my part (probably caused but the inane nature of the tasks set for me) - So to give myself a kick up the behind I decided to write myself which should be a very simple, basic class. Here it is:

public class Customer
{       
    public string CUSTOMERNAME;
    public List<Site> sites = new List<Site>();

    public Customer()
    {

    }

    public void AddSite(Site location)
    {
        sites.Add(location);
    }

}

public class Site
{
        public string SITENAME;
        public Address SITEADDRESSDETAILS;
        public string SITEPHONENUMBER;

        public Site(string sitename, Address siteaddress, string tel)
        {
            SITENAME = sitename;
            SITEADDRESSDETAILS = siteaddress;
            SITEPHONENUMBER = tel;
        }

}

public class Address 
{
    public List<string> address = new List<string>();

    public Address() {

    }

    public void AddAddressDetail(string line)
    {
        address.Add(line);
    }

}

Now everything seems to work but I just can't help feeling that things could be done better. I've tested it with the following code:

static void Main(string[] args)
{
    Customer customer = new Customer();
    customer.CUSTOMERNAME = "Max Hammer Ltd";

    Address addy = new Address();
    addy.AddAddressDetail("1 Edgerail Close");
    addy.AddAddressDetail("Greenbushes");
    addy.AddAddressDetail("Bluehill");
    addy.AddAddressDetail("Surrey");
    addy.AddAddressDetail("RH0 6LD");

    Site surreyOffice = new Site("Surrey Office", addy, "01737 000000");

    addy = new Address();
    addy.AddAddressDetail("6 Electric Avenue");
    addy.AddAddressDetail("Brixton");
    addy.AddAddressDetail("London");
    addy.AddAddressDetail("SW4 1BX");

    Site brixtonOffice = new Site("Brixton Office", addy, "020 7101 3333");

    customer.AddSite(surreyOffice);
    customer.AddSite(brixtonOffice);

    Console.WriteLine(customer.CUSTOMERNAME);
    int numberOfSutes = customer.sites.Count;
    for (int i = 0; i < numberOfSutes; i++)
    {
        Console.WriteLine(customer.sites[i].SITENAME);
        foreach (string line in customer.sites[i].SITEADDRESSDETAILS.address)
        {
            Console.WriteLine(line);
        }
        Console.WriteLine(customer.sites[i].SITEPHONENUMBER);
    }
    Console.ReadKey();

}

I'm not happy with my Main class and I'm not sure why - even though it does what I want. Any tips, pointers?

for a start I would suggest not to use ALL UPPERCASE VARIABLE NAMES — thumbmunkeys, Oct 14 '13 at 12:57
`public string CUSTOMERNAME;` you should have stopped there. — , Oct 14 '13 at 12:59
For starters, please use some common programming practices like small case for private fields. You can read about them here http://msdn.microsoft.com/en-us/library/vstudio/ff926074.aspx — Nikhil Agrawal, Oct 14 '13 at 12:59
Instead of using public fields, you should go for auto-implemented properties: `public string SiteName { get; set; }` This way you can maintain it more easy. Like serializing/validation or raising changed events from the set. — Jeroen van Langen, Oct 14 '13 at 12:59
Use properties as public members (not fields!). Use capitalized names in the properties. Why the `Add*` methods when the lists are public? You can use `customer.Sites.Add(...)` — Ahmed KRAIEM, Oct 14 '13 at 12:59
StackOverflow isn't really the place to ask such opinion-based questions. For example, I'd get rid of the pointless Customer constructor, make both Site and Customer structs, etc, but they are all just opinions. — David Arno, Oct 14 '13 at 13:00
Also, your design for how you store addresses is strange. A `Site` has an `Address`, but one `Address` has a *list* of strings also called `address`? Huh? Also, @NikhilAgrawal, care to elaborate? — tnw, Oct 14 '13 at 13:01
Oh and IMO take no notice of @JeroenvanLangen's suggestion to add noise to your code by replacing public fileds with auto properties. They are effectively identical. — David Arno, Oct 14 '13 at 13:01
First of all, please try to understand **what problem** you want to solve by the application. — Sergey Vyacheslavovich Brunov, Oct 14 '13 at 13:03
@nuke That's unhelpful and nonconstructive. If you lack the motivation to provide actual usable feedback, StackOverflow probably isn't the site for you. — tnw, Oct 14 '13 at 13:03
@DavidArno: _You should expose properties instead of public fields from your components, because properties can be versioned, they allow data hiding, and the accessor methods can execute additional logic. Generally, because of just-in-time optimizations, properties are no more expensive than fields._ **source**: Properties Overview http://msdn.microsoft.com/en-us/library/65zdfbdt.aspx Thats why I suggested it. It's only IMO. — Jeroen van Langen, Oct 14 '13 at 13:05
@tnw I was not trying to be harsh, I think this is a valid standpoint. — Lukas Maurer, Oct 14 '13 at 13:06
@nuke "due to lack of motivation on my part (probably caused but the inane nature of the tasks set for me)" - If you've ever been put in a position where you are asked to do uncreative, boring projects over and over again you would know where I'm coming from. - Thanks for the helpful tips though. It's the slap I need! — tripbrock, Oct 14 '13 at 13:10
@nuke That's totally irrelevant. How does berating him for lacking motivation (which he's already admitted) provide anything remotely helpful? — tnw, Oct 14 '13 at 13:11
@tripbrock: Fair enough. tnw provided me with the slap I probably needed :) — Lukas Maurer, Oct 14 '13 at 13:13
@JeroenvanLangen, auto properties don't offer information hiding and don't execute additional logic. So unless one is hiding data or execting additional logic, use fields rather auto properties. Also just my opinion... — David Arno, Oct 14 '13 at 13:17

score 1 · Accepted Answer · edited Oct 14 '13 at 13:15

Here is how I see it:

Use properties with appropriate capitalization.
Why list of lines in Address?! [Removed].
Direct access to lists.

public class Customer
{
    public string Name {get; set;}
    public List<Site> Sites { get; set; }

    public Customer()
    {
        Sites = new List<Site>();
    }
}

public class Site
{
    public string Name { get; set; }
    public string Address { get; set; }
    public string PhoneNumber { get; set; }

    public Site(string sitename, string siteaddress, string tel)
    {
        Name = sitename;
        Adress = siteaddress;
        PhoneNumber = tel;
    }

}

public class Program
{
    static void Main(string[] args)
    {

        Customer customer = new Customer();
        customer.Name = "Max Hammer Ltd";

        string address = string.Join(Environment.NewLine, new []{"1 Edgerail Close", "Greenbushes", "Bluehill"
             , "Surrey", "RH0 6LD"});

        Site surreyOffice = new Site("Surrey Office", address, "01737 000000");

        address = string.Join(Environment.NewLine, new[]{"1 Edgerail Close", "Greenbushes", "Bluehill"
                , "Surrey", "RH0 6LD"});

        Site brixtonOffice = new Site("Brixton Office", address, "020 7101 3333");

        customer.Sites.Add(surreyOffice);
        customer.Sites.Add(brixtonOffice);

        Console.WriteLine(customer.Name);
        foreach (Site site in customer.Sites)
        {
            Console.WriteLine(site.Name);
            Console.WriteLine(site.Adress);
            Console.WriteLine(site.PhoneNumber);
            Console.WriteLine();
        }
        Console.ReadKey();

    }
}

What if an address line has a comma in? I always use list data structures for storing lists rather than parsing some sort of delimitted string back and forth. — Ben Robinson, Oct 14 '13 at 14:09
Thank you Ahmed, your example highlights to me exactly where I've let the bad practices set in - and set me straight. Again, many thanks. — tripbrock, Oct 14 '13 at 14:18

Is this a correct class implementation and initialization?

1 Answers1