One of the major projects at my place of work came into my sphere of influence recently, and importantly it came to me with no restrictions on what I could do with it. Unfortunately, so far, all I've been doing is sorting out horrible implementation and code weirdness. Now, I can't go into specifics, my employer would not thank me, however, I can and intend to document one of the major problems.
This is a problem born out of the project developer - the guy who sat down and thought it up then set his fingers to the keyboard - being out of date, you see this chap has never had a formal introduction to object orientated programming. He has a semi-idea of the uses of OOP, but he's never really put it into decent practice.
I always knew this, speaking to him at times I with an OOP eye would see things one very clear way, a clarion thought of how the structure of the class hierarchy would, and perhaps should, layout. However, I would get a dull eyes stare back, something like a dog being shown a card trick. I've used that statement on this blog before so I won't continue, what I will do is given an example of what I found and removed from one of the most major critical systems.
The system forms a set of classes, each have to contain the same flags and values as the others so they're mutable, but each is a different class. So, say you have a "Person" and then you have "Staff", "Drivers" and "Servants" derived from them.. You could easily see these as objects.
What I was not expecting was a very strange use of interfaces... So, enough talk, some code, and this is C# folks... I'm sorry!
public interface Person
{
string Name
{
get;
set;
}
int Age
{
get;
set;
}
}
Given this, you can now implement this interface in other classes, and those classes will be forced to use define those elements... Right. So, this code:
public class Driver : Person
{
}
Fails, with the messages: "Error 1 'Driver' does not implement interface member 'Person.Name'" and "Error 2 'Driver' does not implement interface member 'Person.Age'"... And I can see what the chap who wrote this was thinking... It went down something like this... "GREAT, Now everyone trying to use my Person interface is forced to define Name and Age"... And that is what Interfaces are for, but that's not what these classes are used like, what they need is to encapsulate a name and age which are already defined.
In the interface above each and every class has to define the function, but that means there's a copy of the same code being written over and over and over, that's useful if you need those overrides to define different behaviour, but this person class, just like the class in the software I was working on, are used for the same thing each and every time.... This makes repeating the code over and over wasteful, and hard to maintain.
What should have been done is encapsulation, the base class should provide the functionality and then the derived classes just have it, they inherit it, the base class encapsulates the common functionality and then the derived classes don't have to worry, you've written one set of code for the equivalent of this interface and instead of rewriting it you just put the inherit into your derived class...
class Person
{
private string m_Name;
public string Name { get; set; }
private int m_Age;
public int Age { get; set; }
}
class Driver : Person
{
}
So, I showed this the chap in question, and you know what he said... He got quite shirty in fact... "Well I can't stop people just creating instances of the "Person" class, that makes my derived classes pointless!"
Oh yeah buddy, let me show you the way...
class Person
{
private string m_Name;
public string Name { get; set; }
private int m_Age;
public int Age { get; set; }
private Person () { }
}
class Driver : Person
{
}
With a private constructor now no-one can create the Person class itself - create private copy & move constructors too for good measure!
The silence which ensued was palpable, about twenty minutes later, after staring and staring the guy came back with this:
class A
{
private string m_Value;
public string Value { get; set; }
private A () { }
}
class B : A
{
}
B bInstance = new B();
"Right, now I got that, but Value is undefined in B, I need it to have a default value"... I swear to you now, I stared past his left shoulder and just shivered as I did this for him:
class A
{
private string m_Value = string.Empty;
public string Value { get; set; }
private A () { }
}
class B : A
{
}
B bInstance = new B();
"No no" he screamed with glee, giggling, "I mean a value so I know its a B!" or whatever, so I did this:
class A
{
private string m_Value;
public string Value { get; set; }
protected A (string p_Value)
{
m_Value = p_Value;
}
}
class B : A
{
private static readonly string c_Name = "BValue";
public B ()
:
base(c_Name)
{
}
}
The silence was all encompassing by now, and I left him to it. I had by the end of my programming session - before I went for my afternoon break - created around 12 versions of his derived class from the new base and totally removed his interfaces which were so ugly. I don't think he'll forgive me, but the code was a lot smaller and easier to read, and in my opinion more correct.
The only reason the chap did not get to the same conclusion as me was a lack of understanding of the object model & hierarchy, a professional programmer foxed by this, and a more senior programmer than me... Really makes me wonder why I'm earning so little!
