3

I have just started working at a company where I have inherited a C# codebase from a previous developer. I know programming well, have an engineering degree + an (unfinished, several year long) PhD education in computer science and > 10 years of previous experience of "practical" programming, mainly in C++ and Java but also in other languages.

My problem is that the code that I have inherited is, in my opinion, absurdly over-engineered. I have enormous problems following the program flow and finding any concrete implementations of anything. The amount of abstractions are totally mind-boggling and there is no documentation whatsoever. Furthermore, it's difficult to debug the application since it's a server application which is split up into a "GUI-client" server and a "Control-Server" (also a relatively pointless abstraction since they always run on the same machine and there is a 1:1 relationship between them) which interfaces with industrial hardware which is not available.

To give an illustratory example, one of the classes have a field like this:

IClientFactory<IClient<ICommunicationService>> communicationFactory;

I have a hard time wrapping my head around it since the previous programmer also wrote this code (i.e. not using a bit-shift, and this is one of the few things that is documented):

public static bool IsBitHigh(int value, int bitIndex) {
    int valueAbs = Math.Abs(value);
    if (bitIndex < 0)
        throw new ArgumentException("Bit index must be above or equal to 0");
    int result = valueAbs & Convert.ToInt32(Math.Pow(Convert.ToDouble(2.0), Convert.ToDouble(bitIndex - 1)));

        return (result > 0);
    }

There are also indications that the previous programmer was surprised by the fact that casting floats to ints "rounds them down", and that's after several years in the industry.

So given this I have two questions:

  • Is this how code typically looks like in the industry? (I have an engineering/research background so I was under the illusion that people were still using for-loops.)

And more importantly:

  • Is there any easy way to reduce the complexity of the code, i.e. replace the interfaces with their concrete implementations? (To reverse-refactor the code in a sense.)
user1794399
  • 39
  • 2
  • 1
    possible duplicate of [I've inherited 200K lines of spaghetti code — what now?](http://programmers.stackexchange.com/questions/155488/ive-inherited-200k-lines-of-spaghetti-code-what-now) – gnat Feb 26 '15 at 16:02
  • see also: [What is the point of having every service class have an interface?](http://programmers.stackexchange.com/questions/150045/what-is-the-point-of-having-every-service-class-have-an-interface) – gnat Feb 26 '15 at 16:04
  • 1
    Pretty sure he meant bitIndex to be greater than 0, since he subtracts 1 from it. Also, he could have done `int result = valueAbs & (1 <<( bitIndex - 1))` and achieved the same result. The previous programmer doesn't strike me as very bright. – Neil Feb 26 '15 at 16:05
  • 1
    See [this Programmers post](http://programmers.stackexchange.com/questions/274459/when-not-to-apply-dependency-inversion) for more information why code ends up like this. – gbjbaanb Feb 26 '15 at 16:22
  • Someone will link you to this, might as well be me... Michael Feathers, Working Effectively with Legacy Code. Does not matter if this code was written yesterday - it is legacy code - make this book your bible. – mattnz Feb 26 '15 at 20:35
  • I would advice on judging something over-engineered. As you already said you just started working at this company so it is a little (actually very) presumptuous from your side to consider yourself able to make such an evaluation. From my personal experience, any real application program can start very robust, simple, and easy to read but also incomplete and unsatisfactory. As you add features to cover every minor nuance of the real process the software can be kept robust but certainly not simple and probably not easy to read if don't know the nuances yourself. – Mandrill Sep 18 '15 at 18:29
  • Beauty and elegance can probably be improved if a complete rewrite is performed by the same people that did the original program, because only after the job is finished is that they are capable of seeing the whole thing and all details and would probably have done it diferently. But that certainly not worth doing so in profitable company, maybe in academic environment. – Mandrill Sep 18 '15 at 18:33

1 Answers1

5

My problem is that the code that I have inherited is, in my opinion, absurdly over-engineered. I have enormous problems following the program flow and finding any concrete implementations of anything. The amount of abstractions are totally mind-boggling and there is no documentation whatsoever.

Its not you, I find that a lot of C# and Java (and other "developer productivity languages" that also have tools for refactoring and 'twiddling' with code) have a preponderance of this kind of codebase, possibly because if its easy to write then there is plenty of time left over for tinkering. One company I worked for showed me a reference implementation of a webservice that had 1 method that took 1 parameter and returned a string... and was implemented using 32 C# class files in 6 assemblies. And to top it off there was a comment in one of them saying he would have used dependency injection but didn't want to over-complicate it!!! (A C equivalent would have been 32 lines of code)

So... I think the best thing to do is to work with it until you understand its layers, as I'm sure the previous developer managed to hold the overall structure in his head so he could work with it, with time you could get the same mental model of where all the bits are in order to maintain them.

An alternative is to understand the parts that fit together and consolidate them to remove much of the interfacing. eg your client-GUI and Control server can be merged but I feel this isn't the best place to attack - it might be a lot more work to add a control to the GUI (as you have to cascade the changes through both components) but that's common practice for n-tier development anyway.

Even with the morass of interfaces, they're probably OK overall. Maybe you need to consolidate some of them - not necessarily to their concrete classes as this won't give you much, but to remove much of the abstract plumbing that the interfaces were put in to support. ICommunicationService sounds like a good interface, a IClient<ICommunicationService> sounds unnecessary fluff, and getting rid of that might see IClientFactory going too as collateral damage.

All in all, you do what you can. But unfortunately it is somewhat common in the industry. Nobody understands KISS anymore :-(

gbjbaanb
  • 48,354
  • 6
  • 102
  • 172
  • 1
    It's my experience that programmers enjoy programming. I imagine that it is how builders feel when they build something wondrous. However unlike builders, it's all still abstract, and adding improvements where none are needed is like adding windows in the basement of a building. – Neil Feb 26 '15 at 16:36
  • 1
    @Neil True, and that's the problem - if only they enjoyed building the end-result, rather than building as an end in itself. Then maybe a few projects would get finished :-) – gbjbaanb Feb 26 '15 at 16:38
  • We all need to find joy in the elegance of how the program is coded, not just what it does. ;) – Neil Feb 26 '15 at 16:42
  • 1
    @Neil and the most elegantly designed programs are coded as if they were baroque cathedrals! – gbjbaanb Feb 26 '15 at 16:51
  • 1
    Well by elegant I don't mean sophisticated. I mean robust, simple, and easy to read. – Neil Feb 26 '15 at 16:54