Edit
A more detailed code review and addressing comments and answers in this thread.
Car.AddFeature()
I really like that the Car client does not need to know any internal structure, only telling the car (asking?) to add a feature.
Big thumbs up for the ToString override. I almost never see anyone take advantage of that.
You know "BL" for a namespace is horrible.
private float price;
Use Decimal for money. MSDN documentation explains why.
public readonly CarTypes Model; public readonly CarManufactures Manufacture;
Just "Type", "Manufacturer". And not plural. myCar.Type is more natural.
I'm on the fence about the extension methods. This smells like we have non-cohesive classes. Why are GetRating and GetBaseRating not in Car.
As I read over all the code I seeing a need for a car compare class. This class will consolidate code and make the idea of comparing cars cohesive.
main does the hard detail work of comparing.- and so does
CompareCars extension
- Why are these different? What's the context?
Feature.CompareTo - is exposed directly in what looks like a complex ToString method inside of main. The client, main, must know everything in that method. Stuff like CompareTo() == 1 should be encapsulated with a method name that tells us what ==1 means.
Make Useful Collections
Given the current code I think this is the most significant change that can be made. It has broad implications. And it's easier to implement at this point than the meta data idea, and the meta data idea needs these collections anyway.
internal List<Feature> _features;
//should be a domain specific named list/collection. Then write meaningful methods.
So in Car, not this...
internal void AddFeature(Feature feature) {
this._features.Add(feature);
this.price += feature.Install();
}
... but this:
internal void AddFeature(Feature feature) {
this._features.AddFeature(feature);
// the price calc should go too, see below
}
This goes to single responsibility, DSL, and reuse. The features collection should know how to add a feature to itself. I should not have to instantiate a car just to do that.
Calculating price should be dynamic. I guarantee that it gets buggy otherwise. And the various collections should do their part:
public Decimal Price() {
return this.BasePrice + Features.TotalPrice() + Options.TotalPrice();
}
So What?
Solves the "I can call whatever method I want and screw things up" issue raised by @Paparazzi comment
Appropriate responsibilities makes a class re-usable.
OO principles are fractal. As we go down the call stack, it all reads abstractly (in DSL terms!) until we get to the class(es) with the source data.
Decoupled classes. I don't need a car object to calculate the cost of my features list.
Domain Data
I really cannot over emphasize how important it is to define basic, foundational data. It prevents problems and has a very strong tendency to really simplify code. It will prevent some problems as shown and discussed on this thread.
Here is my first draft of that idea. Don't get hung up on access modifiers. enums vs string vs whatever. That's implementation details to tweak later:
public class CarConfiguration {
public Manufacturer WhoMakesMe { get; protected set; }
public Model model { get; protected set; }
public FeatureCollection Features { get; protected set; }
public OptionCollection Options { get; protected set; }
}
enum Manufacturer { undefined, Ford, Chevrolet, BMW }
enum Model {F150, Corvette, i330 }
public Dictionary<Manufacturer, ModelCollection> MakerModels;
public Dictionary<Model, FeatureCollection> ModelFeatures;
public Dictionary<Model, OptionCollection> ModelOptions;
This is a hurried sketch. "Rating" and "Type" has to be in here somewhere. I may be redefining your domain inappropriately when I talk about "model", for example.
A factory will a "data set" of this stuff. More structure/heirarchy may be desired. We'll see how the coding goes.
Totally re-usable. I can see instances of this data bound to a UI. Maybe also to carry the data for unique car specifications - for comparing or purchasing.
The Dictionaries and enums are declared and populated beforehand. Hard code the dictionaries for expediency. We can rig up a database connection later, but guess what. This configuration API hides that.
Sounding really messy? That's what factories are for, hiding the messy building details. But all that messiness is HERE in the factory construction. The client class code will be a pleasure to write, I promise.
So What?
Solves the controlling features issue raised by @Paparazzi comment
We won't hard code constructor parameters
class FocusCombi : Combi{
public Focus() : base("Ford") {}
}
Prevents an explosion of car subclasses with the concomitant tortured class names that is sure to happen. We don't need a subclass for every combination of Manufacturer, Model, (Car)Type, whatever.
Subclassing Car is a design decision. It's not wrong but weigh the pros and cons. In any case hard coding constructor parameters is a code smell. (A smell means it might be bad, but I say it is).
Ditto for Feature sub-classing.
Console App
Everything after the car list instantiation should be in a class.
All the involved classes should override ToString so the output is just a bunch of ToString calls. And most of that is implicit. Console.WriteLine know to call ToString.
public class CarComparer {
// TODO: create a compare function. It will output in a form
// that we can use directly in the format string as seen in ToString.
// comparing inside ToString is a single responsibility principle violation
// but for now at least we got the building of comparison report
// out of main. main should read very high level. Grunt work belongs
// in appropriate classes.
// TODO: make a "comparison results" structure that is easy to use,
// particularly using the string formatting in ToString.
private CarCollection selectedCars;
public CarComparer(CarCollection cars) {
selectedCars = cars;
}
public overide string ToString() {
// Leverage the custom collections!
FeaturesCollection matchingFeatures;
Car targetCar = myCars.RemoveCar(0);
StringBuilder me = new StringBuilder ();
me.AppendLine("\n\n---------------------");
me.AppendLine("/**** Compare cars: *****/");
me.AppendLine("---------------------");
me.AppendLine("/**** Specific feature vs car - {0}: *****/", myCars[0]);
me.AppendLine("---------------------");
foreach (Car compareCar in myCars) {
// FeatureCollection.MatchingFeatures() is called in here
matchingFeatures = targetCar.MatchingFeatures(compareCar.Features);
foreach(Feature feature in matchingFeatures) {
me.AppendLine("Comparing feature - {0}: ",feature);
("Feature exist in {1} and is better: {0} [{2} vs {3}]",
targetCar.GetFeature(feature).CompareTo(compareCar.GetFeature(feature))==1,
compareCar,
targetCar.GetFeature(feature).Rating, compareCar.GetFeature(feature).Rating);
}
}
return me.ToString();
}
}
end Edit
