The last three weeks I have visit several companies and talked about writing good code. It's amazing to see how different developer thinks about writing good code. Here are some comments when I asked if it's important to write good code:
- I don't care as long as it works it's fine.
- I don't have the time to write good code.
- The customer don't see the code, so as long as the application works, I'm satisfied.
- Most customers want to pay as little as possible for as much as possible, and to deliver it in time, I make sure the code only works.
- I know that we can use refactoring to make the code more readable, but we don't have the time to do it.
When I asked about reading other peoples code and also maintain it, most people answer:
- It's not easy all the time
- It's horrible and hard to understand what the code does.
That are some of the results when developers only write code to make stuff works and write code for them self and don't care to write code for other humans to understand.
Here is an example which I use during my presentation, it's a method which will calculate the price for renting a movie, the price differs between different type of customers and also different types of movies.
public class MovieRenter
{
public double CalculatePrice(string customerType, string movieType)
{
if (customerType == "VIC" && movieType == "Transfer")
return 20;
else if (customerType == "Regular" && movieType == "Transfer")
return 30;
else if (customerType == "VIC" && movieType == "Normal")
return 10;
else if (customerType == "Regular" && movieType == "Normal")
return 20;
else
return 50;
}
}
MovieRenter movieRenter = new MovieRenter();
double price = movieRenter.CalculatePrice("VIC", "Transfer");
I asked the attendance if the method looks ok, I got surprised when the answer was "Yes". Some one told me, well I should have used enumeration and switch statement instead of strings and if statements. I did some refactoring and use an enumeration and switch statement instead with the help of the attendance, the result was the following code:
public class MovieRenter
{
public double CalculatePrice(CustomerType customerType, MovieType movieType)
{
switch (customerType)
{
case CustomerType.VIC:
switch (movieType)
{
case MovieType.Transfer:
return 20;
case MovieType.Normal:
return 10;
default:
return 20;
}
break;
case CustomerType.Regular:
switch (movieType)
{
case MovieType.Transfer:
return 30;
case MovieType.Normal:
return 20;
default:
return 30;
}
break;
default:
return 30;
}
}
public enum MovieType
{
Transfer,
Normal
}
public enum CustomerType
{
VIC,
Regular
}MovieRenter movieRenter = new MovieRenter();
double price = movieRenter.CalculatePrice(CustomerType.VIC, MovieType.Transfer);
When I asked if we can do it even better, someone wanted to split the switch statement inside of the case CustomerType.VIC into a separate method, the same with the switch statement in the case CustomerType.Regular. I did that changes to the code and everyone was happy. The problem with conditional statement in code, is that they can be hard to maintain, if I need to add a new kind of movie and customer, I need to add more conditions to my code. Sooner or later it will be difficult to maintain. So one thing we can do, is to replace a conditional statement with polymorphism.
If we look at the code, we can see that we have a customer and a movie to which a customer wants to rent. So we create two entities, one Customer entity and one Movie entity. We can then add a Rebate property to the Customer class and a Price property to the Move class. A movie have the price, and Customer have a rebate. Now when we have a Customer and a Movie, we need to make sure we also have the different type of Customers and Movie represented as classes, we don't want to add a CustomerType or MovieType property to the Customer and Movie class, if we do so, we will en up with a new conditional statement. So we create a new class called CustomerVIC which will inherit from Customer, and we create a MovieTransfer class which inherits from the Movie class. We let the Customer class represents a regular customer and the Movie class as normal movie. Because a VIC Customer should have it's own Rebate and a Transfer Movie should have it's own price, we make sure the base classes properties are virtual so we can override them in our sub classes.
public class Customer
{
public virtual double Rebate
{
get { return 0; }
}
}
public class CustomerVIC : Customer
{
public override double Rebate
{
get { return 10; }
}
}
public class Movie
{
public virtual double Price
{
get { return 20; }
}
}
public class MovieTransfer : Movie
{
public virtual double Price
{
get { return 30; }
}
}If we compare a normal customer with a VIC, we can see that a VIC will have 10 in rebate. So we make sure the CustomerVIC class Rebate property returns the price a VIC Customer should have in Rebate over a regular customer. The price of a Transfer movie is 30 and the price of a normal movie is 20.
Now when we have created our Customers and Movies, we can change the MovieRenter's CalculatePrice method, which in this case should take a Customer as an argument and a Movie.
public class MovieRenter
{
public double CalculatePrice(Customer customer, Movie movie)
{
return movie.Price - customer.Rebate;
}
}CustomerVIC vicCustomer = new CustomerVIC();
MovieTransfer transferMovie = new MovieTransfer();
MovieRenter movieRenter = new MovieRenter();
double price = movieRenter.CalculatePrice(vicCustomer, transferMovie);
So by using polymorphism and more Object Oriented Programming over a normal functional programming, we can remove conditional statements. If I need a new type of Customer or Movie, I just create a new class and inherits from its base class and override the Rebate and Price property. Anyone have another suggestion of making the first code example better, and do you think it's important to make it better even if it works?
What do you think is most important when it comes to writing code?
What is the most common thing you think a developers are doing wrong when they write code?