55502f40dc8b7c769880b10874abc9d0

Hello guys,
I working in a project and i have in my hand a refactoring project, but the only problem is I don't have any idea how start. My project is about a theatre. I have 5 C# files Customer.cs, happening.cs, receipt.cs, row.cs, theatre.cs and a testrun.cs where I'm doing some test.

using System;
using System.Collections.Generic;
using System.Text;

namespace Assignment1MH_Base
{
    class Customer
    {
        public const int NonMember = 0;
        public const int Bronze = 1;
        public const int Silver = 2;
        public const int Gold = 3;
        private int memberType;
        private String lastName;
        private String firstName;
        private String creditNumber;
        private String creditType;
        private String expiry;

        public Customer(int memberType, String lastName, String firstName, String creditNumber, String creditType, String expiry)
        {
            this.memberType = memberType;
            this.lastName = lastName;
            this.firstName = firstName;
            this.creditNumber = creditNumber;
            this.creditType = creditType;
            this.expiry = expiry;
        }

        public Customer(String lastName, String firstName, String creditNumber, String creditType, String expiry)
        {
            this.lastName = lastName;
            this.firstName = firstName;
            this.creditNumber = creditNumber;
            this.creditType = creditType;
            this.expiry = expiry;
        }

        public String getCreditNumber()
        {
            return creditNumber;
        }

        public String getLastName()
        {
            return lastName;
        }

        public String getFirstName()
        {
            return firstName;
        }

        public String getCreditType()
        {
            return creditType;
        }

        public String getExpiry()
        {
            return expiry;
        }

        public int getMemberType()
        {
            return memberType;
        }

        public void setCreditNumber(String creditNumber)
        {
            this.creditNumber = creditNumber;
        }

        public void setCreditType(String creditType)
        {
            this.creditType = creditType;
        }

        public void setLastName(String lastName)
        {
            this.lastName = lastName;
        }

        public void setFirstName(String firstName)
        {
            this.firstName = firstName;
        }

        public void setExpiry(String expiry)
        {
            this.expiry = expiry;
        }

        public void setMemberType(int memberType)
        {
            this.memberType = memberType;
        }
    }
}
------------------------------------------
using System;
using System.Collections.Generic;
using System.Collections;
using System.Text;

namespace Assignment1MH_Base
{
    class Happening
    {
        private Hashtable custBooks = new Hashtable();
        private const double StallCharge = 37.25;
        private const double DressCircleCharge = 48.80;
        private const double BalconyCharge = 89.00;
        private const double BronzeDiscount = 0.90;
        private const double SilverDiscount = 0.80;
        private const double GoldDiscount = 0.75;
        private Theatre theTheatre;

        public Happening(Theatre theTheatre)
        {
            this.theTheatre = theTheatre;
        }

        public Boolean bookSeats(int priceCode, int number, Customer theCust)
        {
            if (priceCode == Row.Stalls)
            {
                ArrayList theStalls = theTheatre.getStalls();
                IEnumerator theEnum = theStalls.GetEnumerator();
                while (theEnum.MoveNext())
                {
                    Row theRow = (Row)theEnum.Current;
                    if (theRow.bookSeats(number))
                    {
                        Receipt newBooking = new Receipt(priceCode, theCust, theStalls.IndexOf(theRow) + 1, theRow.getLastBooked(), number);
                        custBooks.Add(theCust, newBooking);
                        return true;
                    }
                }
                return false;
            }
            else if (priceCode == Row.DressCircle)
            {
                ArrayList theDC = theTheatre.getDressCircle();
                IEnumerator theEnum = theDC.GetEnumerator();
                while (theEnum.MoveNext())
                {
                    Row theRow = (Row)theEnum.Current;
                    if (theRow.bookSeats(number))
                    {
                        Receipt newBooking = new Receipt(priceCode, theCust, theDC.IndexOf(theRow) + 1, theRow.getLastBooked(), number);
                        custBooks.Add(theCust, newBooking);
                        return true;
                    }
                }
                return false;
            }
            else if (priceCode == Row.Balcony)
            {
                ArrayList theBalcony = theTheatre.getBalcony();
                IEnumerator theEnum = theBalcony.GetEnumerator();
                while (theEnum.MoveNext())
                {
                    Row theRow = (Row)theEnum.Current;
                    if (theRow.bookSeats(number))
                    {
                        Receipt newBooking = new Receipt(priceCode, theCust, theBalcony.IndexOf(theRow) + 1, theRow.getLastBooked(), number);
                        custBooks.Add(theCust, newBooking);
                        return true;
                    }
                }
                return false;
            }
            return false;
        }

        public double getCharge(Customer theCust)
        {
            Receipt theBooking = (Receipt)custBooks[theCust];
            double theCharge = theTheatre.getDiscount(theCust.getMemberType()) * theTheatre.getCharge(theBooking.getPriceCode()) * theBooking.getNumberOfSeats();
            return theCharge;
        }

        public Receipt getCustomerBooking(Customer cust)
        {
            return (Receipt)custBooks[cust];
        }
    }
}
------------------------------------------------------------------------
using System;
using System.Collections.Generic;
using System.Text;

namespace Assignment1MH_Base
{
    class Receipt
    {
        private int priceCode;
        private int numberOfSeats;
        private Customer theCust;
        private int rowNum;
        private int startSeatNum;

        public String toString () 
        {
            return "Price Code =" + priceCode + ", seats booked =" + numberOfSeats + ", sitting at row " + rowNum;
        }

        public Receipt(int priceCode, Customer theCust, int rowNum, int startSeatNum, int seatsBooked)
        {
            this.priceCode = priceCode;
            this.theCust = theCust;
            this.rowNum = rowNum;
            this.startSeatNum = startSeatNum;
            this.numberOfSeats = seatsBooked;
        }

        public int getNumberOfSeats()
        {
            return numberOfSeats;
        }

        public int getPriceCode()
        {
            return priceCode;
        }

        public Customer getTheCust()
        {
            return theCust;
        }

        public int getStartSeatNum()
        {
            return startSeatNum;
        }

        public int getRowNum()
        {
            return rowNum;
        }

        public void setNumberOfSeats(int numberOfSeats)
        {
            this.numberOfSeats = numberOfSeats;
        }

        public void setPriceCode(int priceCode)
        {
            this.priceCode = priceCode;
        }

        public void setTheCust(Customer theCust)
        {
            this.theCust = theCust;
        }

        public void setRowNum(int rowNum)
        {
            this.rowNum = rowNum;
        }

        public void setStartSeatNum(int startSeatNum)
        {
            this.startSeatNum = startSeatNum;
        }
    }
}
-----------------------------------------------------------------------
using System;
using System.Collections.Generic;
using System.Text;

namespace Assignment1MH_Base
{
    class Row
    {
        public const int Stalls = 0;
        public const int DressCircle = 1;
        public const int Balcony = 2;
        private const double StallCharge = 37.25;
        private const double DressCircleCharge = 48.80;
        private const double BalconyCharge = 89.00;
        private const double BronzeDiscount = 0.90;
        private const double SilverDiscount = 0.80;
        private const double GoldDiscount = 0.75;

        private int priceCode;
        private int numberAvailable;
        private int currentSeat;
        private int lastBooked;

        public Row(int numAvail, int code)
        {
            this.priceCode = code;
            this.numberAvailable = numAvail;
            currentSeat = 1;
            lastBooked = 1;
        }

        public Boolean bookSeats(int num)
        {
            if (numberAvailable >= num)
            {
                lastBooked = currentSeat;
                currentSeat += num;
                numberAvailable -= num;
                return true;
            }
            return false;
        }

        public int getPriceCode()
        {
            return priceCode;
        }

        public int getLastBooked ()
        {
            return lastBooked;
        }

        public int getCurrentSeat()
        {
            return currentSeat;
        }
    }
}
----------------------------------------------------------
using System;
using System.Collections.Generic;
using System.Collections;
using System.Text;

namespace Assignment1MH_Base
{
    class Theatre
    {
        private const double StallCharge = 37.25;
        private const double DressCircleCharge = 48.80;
        private const double BalconyCharge = 89.00;
        private const double BronzeDiscount = 0.90;
        private const double SilverDiscount = 0.80;
        private const double GoldDiscount = 0.75;

        private ArrayList stalls = new ArrayList();
        private ArrayList dressCircle = new ArrayList();
        private ArrayList balcony = new ArrayList();

        public double getDiscount(int memberCode)
        {
            if (memberCode == Customer.Bronze)
                return BronzeDiscount;
            if (memberCode == Customer.Silver)
                return SilverDiscount;
            if (memberCode == Customer.Gold)
                return GoldDiscount;
            return 1;
        }

        public double getCharge(int priceCode)
        {
            if (priceCode == Row.Stalls)
                return StallCharge;
            if (priceCode == Row.DressCircle)
                return DressCircleCharge;
            if (priceCode == Row.Balcony)
                return BalconyCharge;
            return -1;
        }

        public ArrayList getBalcony()
        {
            return balcony;
        }

        public ArrayList getDressCircle()
        {
            return dressCircle;
        }

        public ArrayList getStalls()
        {
            return stalls;
        }

        public void setStalls(ArrayList stalls)
        {
            this.stalls = stalls;
        }

        public void setBalcony(ArrayList balcony)
        {
            this.balcony = balcony;
        }

        public void setDressCircle(ArrayList dressCircle)
        {
            this.dressCircle = dressCircle;
        }
    }
}

Refactorings

No refactoring yet !

F9a9ba6663645458aa8630157ed5e71e

Ants

May 3, 2010, May 03, 2010 10:00, permalink

1 rating. Login to rate!

I highly recommend reading the first couple of chapter's of Martin Fowler's book: Refactoring. It should give you a good idea of how to tackle refactoring.

A couple of questions about the code above:
1) Are you coming from a C++ background? The setter and getter methods you have can be replaced with C# property declarations.
2) Are you coming from a STL or Java background? C#'s recommended style guide recommends methods be Pascal-cased, rather than Camel-cased.
3) What version of C# and the .NET framework are you targetting? With C# 3.0 and .NET 3.5, you can be more type safe and avoid casts by using Dictionary<Customer, Receipt> instead of Hashtable, and List<Row> instead of ArrayList.
4) So customer can't buy seats on two different rows, like a row for himself and his girlfriend and a different row for his siblings? The use of a Hashtable looks like it makes this use case impossible.

Anyway, one of the first things I would look at refactoring is folding the duplicated code in Happening.bookSeats().

F9a9ba6663645458aa8630157ed5e71e

Ants

May 3, 2010, May 03, 2010 10:16, permalink

1 rating. Login to rate!

This is my first pass refactoring of the code above targetting C# 3.0/ .NET 3.5. I converted some methods to become properties, and then later moved the properties to live with the classes that I felt was more responsible for them.

(Without seeing the test code from the original poster, I may have broken the Law of Demeter in the process of doing the refactoring.)

using System;
using System.Collections.Generic;

namespace Assignment1MH_Base
{
    public enum MemberType
    {
        NonMember,
        Bronze,
        Silver,
        Gold,
    }

    public class Customer
    {
        static Dictionary<MemberType, decimal> Discounts = new Dictionary<MemberType, Decimal>();

        static Customer()
        {
            Discounts[MemberType.NonMember] = 1.00m;
            Discounts[MemberType.Bronze]    = 0.90m;
            Discounts[MemberType.Silver]    = 0.80m;
            Discounts[MemberType.Gold]      = 0.75m;
        }

        public MemberType   MemberType      { get; set; }
        public String       LastName        { get; set; }
        public String       FirstName       { get; set; }
        public String       CreditNumber    { get; set; }
        public String       CreditType      { get; set; }
        public String       Expiry          { get; set; }

        public Decimal Discount
        {
            get { return Discounts[MemberType]; }
        }
    }
}
using System;
using System.Collections.Generic;

namespace Assignment1MH_Base
{
    public class Happening
    {
        Dictionary<Customer, Receipt> bookings = new Dictionary<Customer, Receipt>();
        Theatre theatre;

        public Happening(Theatre theatre)
        {
            this.theatre = theatre;
        }

        public bool BookSeats(RowType rowType, int number, Customer customer)
        {
            Receipt newBooking = null;
            int startSeatNum = 0;

            foreach (Row row in theatre.Rows[rowType])
            {
                if (row.BookSeats(number, out startSeatNum))
                {
                    newBooking = new Receipt(row, customer, startSeatNum, number);
                    bookings.Add(customer, newBooking);
                    break;
                }
            }
            return newBooking != null;
        }

        public Receipt GetCustomerBooking(Customer cust)
        {
            return bookings[cust];
        }
    }
}
using System;

namespace Assignment1MH_Base
{
    public class Receipt
    {
        public Row      Row             { get; set; }
        public int      NumberOfSeats   { get; set; }
        public Customer Customer        { get; set; }
        public int      StartSeatNum    { get; set; }

        public decimal Charge
        {
            get { return Customer.Discount * Row.Charge * NumberOfSeats; }
        }

        public Receipt(Row row, Customer theCust, int startSeatNum, int seatsBooked)
        {
            this.Row = row;
            this.Customer = theCust;
            this.StartSeatNum = startSeatNum;
            this.NumberOfSeats = seatsBooked;
        }

        public override string ToString()
        {
            return String.Format("Price Code = {0}, seats booked = {1}, sitting at row {2}",
                                 Row.RowType,
                                 NumberOfSeats,
                                 Row.Number);
        }
    }
}
using System;
using System.Collections.Generic;

namespace Assignment1MH_Base
{
    public enum RowType
    {
        Stalls,
        DressCircle,
        Balcony,
    }

    public class Row
    {
        static Dictionary<RowType, decimal> Charges = new Dictionary<RowType, decimal>();

        static Row()
        {
            Charges[RowType.Stalls]         = 37.25m;
            Charges[RowType.DressCircle]    = 48.80m;
            Charges[RowType.Balcony]        = 89.00m;
        }

        public int      Number          { get; private set; }
        public int      SeatsAvailable  { get; private set; }
        public int      CurrentSeat     { get; private set; }
        public RowType  RowType         { get; private set; }

        public decimal Charge  
        {
            get { return Charges[RowType]; } 
        }

        protected Row(int rowNum, RowType rowType, int numAvail)
        {
            RowType = rowType;
            Number = rowNum;
            SeatsAvailable = numAvail;
            CurrentSeat = 1;
        }

        public bool BookSeats(int num, out int startSeatNum)
        {
            if (SeatsAvailable >= num)
            {
                startSeatNum = CurrentSeat;
                CurrentSeat += num;
                SeatsAvailable -= num;
                return true;
            }
            startSeatNum = 0;
            return false;
        }
    }
}
using System;
using System.Collections.Generic;

namespace Assignment1MH_Base
{
    public class Theatre
    {
        public Dictionary<RowType, IList<Row>> Rows { get; private set; }

        public Theatre()
        {
            Rows = new Dictionary<RowType,IList<Row>>();
            Rows[RowType.Stalls]        = new List<Row>();
            Rows[RowType.DressCircle]   = new List<Row>();
            Rows[RowType.Balcony]       = new List<Row>();
        }
    }
}
9e9d8a30f0e3eb1f79953fd11eccd1db

Guarren

May 4, 2010, May 04, 2010 03:02, permalink

1 rating. Login to rate!

As Ants said, you can use auto-properties for your getters/setters. A lot of your constructors can be replaced in favor of properties entirely.

You also probably want to stay away from public const variables in favor of readonly (run-time constant instead of compile time) as it could cause problems if you ever pull this assembly into another one and defined const value changes.

Look at examples on the MSDN for a more idiomatic C# style - yours is decidedly Java.

Get rid of the Camel-Case. toString should be ToString (and maybe get rid of all the concatenation).

Lastly, your tying yourself to the ArrayList implementation - favor a more generic interface such as IList or better yet, IEnumerable.

This is a small enough project that I'd hesitate to refer to the above changes as refactorings, more like common practice. C# and Java, while very similar, both have pretty distinct patterns and idioms that you'll want to pick up. Read a book: "Effective C#" by Bill Wagner.

55502f40dc8b7c769880b10874abc9d0

navajo00.myopenid.com

May 4, 2010, May 04, 2010 13:33, permalink

No rating. Login to rate!

Thank you for the help guys. I have begun to read the book that Ants suggest and is very good. It has helped me to have a better idea of what is refactoring.
Now, answering Ants's question:
My background is Java, this is my first time using C# so that is why I reading C# and now Martin Fowler's book all the time.
I'm using .Net 3.5 and Customers can currently only make a single booking for each event.
Ants, Can you tell me? which "code smell" you use to refactor the code above. I really apreciate your help.
Thanks again.

F9a9ba6663645458aa8630157ed5e71e

Ants

May 4, 2010, May 04, 2010 20:53, permalink

1 rating. Login to rate!

The code smell I started off with the duplicated code in Happening.bookSeats. Originally I move the duplicated code out to a helper method that took an ArrayList as the input.

With that out of the way, I then tackled the parallel code smells of the multiple 'if' statements for different types of memberships and row types. It was easy to convert those to switch statements, but that still left a code smell. My next approach was to apply the Strategy pattern to encapsulate the logic, but I saw that bloat the code, so I ended up backing off and going to using Dictionary<> to just store the needed values/containers.

With that done, I figured out that I didn't need the helper function anymore, so contents of that function moved back in bookSeats().

And then the last code smell I tackled was trying to ensure a clear separation of concerns. Hence the migration of methods/properties from their original locations, to where I felt they made more sense.

All other changes were mostly cosmetic, or style changes to take advantage of language and CLR features.

As a side note, I feel that static Charges dictionary that I setup in Rows should actually live within Happening and be dynamically configured, because I doubt that an amateur production of "The Sound of Music" would charge the same rate as professional production of "Les Miserables".

55502f40dc8b7c769880b10874abc9d0

navajo00.myopenid.com

May 5, 2010, May 05, 2010 01:47, permalink

No rating. Login to rate!

Thanks again Ants. Sorry but I have one more question. I was reading the book and it says that "I need to build a solid set of tests .The tests are essential because even though I follow refactorings structured to avoid most of the opportunities for introducing bugs, I'm still human and still make mistakes. Thus I need solid tests." The author used JUNIT to make those test, but he is using java. I was lookink on internet and I found NUnit 2.5.4. Is a good one? or there others better than that? and what I should test in this project?
Thanks

F9a9ba6663645458aa8630157ed5e71e

Ants

May 5, 2010, May 05, 2010 05:39, permalink

No rating. Login to rate!

I'm a fan of xUnit: http://xunit.codeplex.com I feel that it is conceptually cleaner than NUnit, and additionally takes advantage of a lot of .NET CLR features like attributes. Before xUnit, I was using NUnit.

In the end, what matters the most is having unit tests and using them. (For my private projects, I hand tweak my MSBuild files to always run unit tests after EVERY build.)

If you are using Visual Studio, let me recommend getting TestDriven.NET's so you can just right click and Run a test. TestDriven.NET works with various different unit test packages.

Also check out ReSharper: http://www.jetbrains.com/resharper/index.html It's a great refactoring tool.

7ec5a6876bdba08edf723ea14dc9646c

last minute holidays

July 17, 2010, July 17, 2010 05:04, permalink

No rating. Login to rate!

can you do thi for me, http://www.wearediabetic.org/mlbtraderumorss/bio Discount mlb trade rumors, >:]],

can you do thi for me, http://www.wearediabetic.org/mlbtraderumorss/bio Discount mlb trade rumors,  >:]],
57e65eb3edfc6811910484f7e5393693

sample pay stubs price

July 19, 2010, July 19, 2010 20:49, permalink

No rating. Login to rate!

I like your work!, http://www.wearediabetic.org/samplepaystubss/bio First sample pay stubs, :OOO,

I like your work!, http://www.wearediabetic.org/samplepaystubss/bio First sample pay stubs,  :OOO,
4256482ca637172864783b200f9c24c2

Cheapest latina ass

July 20, 2010, July 20, 2010 00:10, permalink

No rating. Login to rate!

Best Wishes!, http://www.wearediabetic.org/latinaasss/bio latina ass for you, 753,

Best Wishes!, http://www.wearediabetic.org/latinaasss/bio latina ass for you,  753,
7ec5a6876bdba08edf723ea14dc9646c

nicole bobek drug ring

July 20, 2010, July 20, 2010 07:34, permalink

No rating. Login to rate!

Best Wishes, http://www.wearediabetic.org/nicolebobekdrugrins/bio First nicole bobek drug ring, =]],

Best Wishes, http://www.wearediabetic.org/nicolebobekdrugrins/bio First nicole bobek drug ring,  =]],
100fe9a189f243d770195b7ed6ed3184

jonas dating aiken for you

July 20, 2010, July 20, 2010 07:49, permalink

No rating. Login to rate!

I want to say thanks!, http://www.wearediabetic.org/jonasdatingaikens/bio jonas dating aiken for you, ycazbb,

I want to say thanks!, http://www.wearediabetic.org/jonasdatingaikens/bio jonas dating aiken for you,  ycazbb,
1268289b2aa6469a63a523c1aa6a038b

adult party games

July 20, 2010, July 20, 2010 13:29, permalink

No rating. Login to rate!

best for you, http://www.wearediabetic.org/adultpartygamess/bio adult party games for you, 415,

best for you, http://www.wearediabetic.org/adultpartygamess/bio adult party games for you,  415,
7ec5a6876bdba08edf723ea14dc9646c

bikini wallpapers

July 20, 2010, July 20, 2010 20:04, permalink

No rating. Login to rate!

Best Wishes, http://www.wearediabetic.org/bikiniwallpaperss/bio bikini wallpapers, iobp,

Best Wishes, http://www.wearediabetic.org/bikiniwallpaperss/bio bikini wallpapers,  iobp,
7f0c7363aecc04d13e50d3cfe40b939c

chanel sunglasses information

July 20, 2010, July 20, 2010 20:46, permalink

No rating. Login to rate!

Best Wishes!, http://www.wearediabetic.org/chanelsunglassess/bio chanel sunglasses information, hyk,

Best Wishes!, http://www.wearediabetic.org/chanelsunglassess/bio chanel sunglasses information,  hyk,
C5efe848afb7ce8f197b9adc54864c1a

lesbian dildo information

July 21, 2010, July 21, 2010 04:04, permalink

No rating. Login to rate!

Thank you, http://www.wearediabetic.org/lesbiandildos/bio lesbian dildo, 894464,

Thank you, http://www.wearediabetic.org/lesbiandildos/bio lesbian dildo,  894464,
100fe9a189f243d770195b7ed6ed3184

Best darvocet

August 9, 2010, August 09, 2010 14:25, permalink

No rating. Login to rate!

Great site. Keep doing., http://www.formspring.me/darvocets Best darvocet, 7131,

Great site. Keep doing., http://www.formspring.me/darvocets Best darvocet,  7131,
Efbe21a8c704547ed09f2f89797f74db

diovan

August 9, 2010, August 09, 2010 15:02, permalink

No rating. Login to rate!

I have the same., http://www.formspring.me/diovanhere diovan for you, ctoj,

I have the same., http://www.formspring.me/diovanhere diovan for you,  ctoj,
8d2ba2b3ffe6305e7a8961cf8ba32729

diflucan

August 9, 2010, August 09, 2010 15:21, permalink

No rating. Login to rate!

Nice, http://www.formspring.me/diflucan diflucan, nzqdp,

Nice, http://www.formspring.me/diflucan diflucan,  nzqdp,

Your refactoring





Format Copy from initial code

or Cancel