Class design decision

[__PPS__]

Hello all,
I was working on someone else's code to implement some classes. The
task seemed to be simple and I was confident that I won't spent alot of
time on that task... But! I've already spent more time to integrade
old working code with new design than it would take me to write it all
from scratch!

so... after having tons of headaches I desided to figure out what's
wrong with the design (or me) and why it's all so complicated.

Here's an example of Rtp Player class. (rtp/rtcp real time data
trnasmition...)
In short RtpPlayer is something that receives data from network, so, it
probably will need to have some kind of connection to the remote end.
Data is encoded, so before playing my rtp player will have to decode
that data. Then to play it (if it's audio) it will have to open audio
device and write data to it.
simplest design is something like:
class rtp_player {
rtp_connection rtp;
data_decoder decoder;
audio_device audio;
public:
...
};
====

Now the way it's badly designed in my case...
rtp_player's has a constructor that takes pointers to interface classes
like this (for sending data):
class data_senderI {
virtual void sendData(const char* data, unsigned int size) = 0;
}
similar shit for decoder, audio device etc.... Not to mention that all
these classes have lot's of other similar shit classes.

So... there's a top level class Media... it has sockets (for
rtp/rtcp)... this class implements data_senderI, then it does new
rtp_player(this, this, this); (it implements other interfaces etc);
I don't know where this kind of garbage design comes from (my guess is
that it's from java world) but it just drives me crazy, I'm getting
lost... There are tens of almost empty classes that do nothing, all of
them have or implement two or three interfaces and it goes on and on.
Result - this construction is unmanagable at all!

Most of the code is constructed in this way... My goal is to convince
managers that a major fix or refactoring is required, how do I convince
them that's not the way to go with all that interfaces user classes and
userInterface classes that came nobody knows from where and designed
nobody knows by who... All other ppl that work on that project are not
good in c++ and they have tons of troubles tracing function call flows
etc...

Any ideas on that? thank you

 

[eriwik]

Most of the code is constructed in this way... My goal is to convince
managers that a major fix or refactoring is required, how do I convince
them that's not the way to go with all that interfaces user classes and
userInterface classes that came nobody knows from where and designed
nobody knows by who... All other ppl that work on that project are not
good in c++ and they have tons of troubles tracing function call flows
etc...

Find an application that can create UML-diagrams from code, create some
diagrams of the existing code, start with class diagrams and if
possible make some iteraction-diagrams or similar of selected parts.
Then sitt down and do a new design the way you want it. Then create
UML-diagrams of your design, one correspondig (if there exist on) for
each of the ones of the old code.

With some luck your design will have fewer classes, shorter call-paths
and fewer dependencies, that would probably result in cleaner diagrams
as well. Show these diagrams to your manages, let them compare your
design and the existing one, explain why your's will be easier to
maintain, extend and so on.

 

[sototh]

Now the way it's badly designed in my case...

rtp_player's has a constructor that takes pointers to interface classes
like this (for sending data):
class data_senderI {
virtual void sendData(const char* data, unsigned int size) = 0;
}
similar shit for decoder, audio device etc.... Not to mention that all
these classes have lot's of other similar shit classes.

IMO there's no reason to call this design a shit. It gives you set of
ABCs (abstract base class) that allow you to easily implement different
i.e. data senders and use them all through simple data_senderI class
interface (polymorphism is a great thing). It is normal and proper
technique used with C++.

So... there's a top level class Media... it has sockets (for
rtp/rtcp)... this class implements data_senderI, then it does new
rtp_player(this, this, this); (it implements other interfaces etc);

If I understood correctly, you created class which inherites from many
of library ABCs. It's a bad practice to make such monolith. You should
rather create set of implementations, small and simple classes that
every one of them do just their, strictly constrained, job.

Result - this construction is unmanagable at all!

That's a result of misunderstood of modern C++ usage. Are you skilled
in C++ programming, not C, but C++? I suggest you to get in touch with
C++ FAQ (http://www.parashift.com/c++-faq-lite/) and "C++ Coding
Standards: 101 Rules, Guidelines, and Best Practices" of Herb Sutter
and Andrei Alexandrescu
(http://www.amazon.com/exec/obidos/ASIN/0321113586/modecdesi-20), if
you aren't familiar with them already.

Most of the code is constructed in this way... My goal is to convince
managers that a major fix or refactoring is required, how do I convince
them that's not the way to go with all that interfaces user classes and
userInterface classes that came nobody knows from where and designed
nobody knows by who...

Do you like your job? If yes, than you should not even try to do this!
Just sit, learn and improve your C++ skills. It is you this time, who
have to adapt to the situation.

The same advice goes to other people working on this projects that "are
not good in c++".

I wish you good luck!

 

[Tony]

I just installed Doxygen on my machine this week along with GraphViz
which Doxygen uses to make lots of pretty graphs of your code. Highly
recommended and free!

Tony

Most of the code is constructed in this way... My goal is to convince
managers that a major fix or refactoring is required, how do I convince
them that's not the way to go with all that interfaces user classes and
userInterface classes that came nobody knows from where and designed
nobody knows by who... All other ppl that work on that project are not
good in c++ and they have tons of troubles tracing function call flows
etc...

Find an application that can create UML-diagrams from code, create some
diagrams of the existing code, start with class diagrams and if
possible make some iteraction-diagrams or similar of selected parts.
Then sitt down and do a new design the way you want it. Then create
UML-diagrams of your design, one correspondig (if there exist on) for
each of the ones of the old code.

With some luck your design will have fewer classes, shorter call-paths
and fewer dependencies, that would probably result in cleaner diagrams
as well. Show these diagrams to your manages, let them compare your
design and the existing one, explain why your's will be easier to
maintain, extend and so on.

 

[__PPS__]

Ok, data_senderI is a good choice for abstract interface, but not a
good example for my case. There are many classes that are used ony in
one place: abstract RtpConnection class is used only in one place like
this:
someclass {RtpConnection *rtp; }
then somewhere: rtp = new RtpConeectionImpl();
then in every function where rtp used there's this line of code:
((RtpConeectionImpl*)rtp) -> do_some_shit();
Nice!

That's a result of misunderstood of modern C++ usage. Are you skilled
in C++ programming, not C, but C++? I suggest you to get in touch with
C++ FAQ (http://www.parashift.com/c++-faq-lite/) and "C++ Coding
Standards: 101 Rules, Guidelines, and Best Practices" of Herb Sutter
and Andrei Alexandrescu
(http://www.amazon.com/exec/obidos/ASIN/0321113586/modecdesi-20), if
you aren't familiar with them already.

Never knew C. Started from c++. I've read these books a few years ago
(not completely, though). Sometimes I read Sutter's gotw.ca, I love c++
faq.

Do you like your job? If yes, than you should not even try to do this!
Just sit, learn and improve your C++ skills. It is you this time, who
have to adapt to the situation.

I personally beleive that I'm good enough in c++. Some ppl at my work
say so (very good)
Well, I said to the managers that somebody wrote interfaces so that I
need to implement all of them. I said that I can as well design code
myself and I don't need anybody do useless work (designing empty
interfaces). They agreed with me, now I do it all on my own.

The same advice goes to other people working on this projects that "are
not good in c++".

These ppl aren't really working with me. They are direct emplyees, I'm
a consultant. I help them find/fix errors, teaching them c++ is not my
responsibility (I don't want to loose my job if they become good
enough, just joking). Anyways, they don't code anything - I end up
doing everything anyways.

I just installed Doxygen on my machine this week along with GraphViz
which Doxygen uses to make lots of pretty graphs of your code. Highly
recommended and free!

Yes!!! That's true... I wanted to do it this way. In big projects I do
it this way... BUT dyoxygen choked on this code. Some geniuses designed
new way of inheritance:
..h: ===
namespace voip{
class Service {
#include "../serice_common.h"
...
}

..cpp ===
using namespace voip;
#include "../serice_common.cpp"
....

So, there are tens of different services and all of them are written
this way, so that at the end dyoxygen cannot resolve this cpp files and
cannot draw me anything...

 

[janerad]

__PPS__ napisal(a):

Ok, data_senderI is a good choice for abstract interface, but not a
good example for my case. There are many classes that are used ony in
one place: abstract RtpConnection class is used only in one place like
this:
someclass {RtpConnection *rtp; }
then somewhere: rtp = new RtpConeectionImpl();
then in every function where rtp used there's this line of code:
((RtpConeectionImpl*)rtp) -> do_some_shit();
Nice!

Wrong!

Assume RtpConnection is abstract base class:


class RtpConnection
{
public:
virtual bool do_something() = 0;
};


//You have implementation like this:


class RtpConnectionImpl : public RtpConnection
{
public:
RtpConnectionImpl(some params);

bool do_something();
};


//And class when you use the impl.:


class RtpPlayer
{
public:
RtpPlayer(RtpConnection* rtpcon) : mRtpCon(rtpcon) {}

void play();
private:
RtpConnection* mRtpCon;
};

void RtpPlayer:lay()
{
mRtpCon->do_something(); // no indirection needed
}


/*
The RtpPlayer class doesn't know anything about implementation. It uses
only ABC defined interface, so it is independent of any further
RtpConnection implementations. This is VERY important in case of code
reusability. Remember, that somewhen in future someone else may be
using RtpPlayer but with another connection implementation. In that
case he doesn't have to change anything - just create another
RtpConnection implementation. That's why use of ABCs is a good approach
in OOP. I.e.:
*/


class MyNewRtpConnectionImpl : public RtpConnection
{
public:
MyNewRtpConnectionImpl(some params);

bool do_something(); // probably something else then before
};


//And somewhere in the code:


RtpConnection* rtpcon = 0;

if (use_old_type_connection()) {
rtpcon = new RtpConnectionImpl(some params);
} else {
rtpcon = new MyNewRtpConnectionImpl(another params);
}

RtpPlayer* player = new RtpPlayer(rtpcon);
player->play();

Never knew C. Started from c++. I've read these books a few years ago
(not completely, though). Sometimes I read Sutter's gotw.ca, I love c++
faq.

Strange love, if you doesn't use advices they provide... i.e.
http://www.parashift.com/c++-faq-lite/abcs.html, rule no 36 in "C++
Coding Standards" - "Prefer providing abstract interfaces".

I personally beleive that I'm good enough in c++. Some ppl at my work
say so (very good)

Are they better than you? If they aren't, there is no reason to be
happy... Really...

Well, I said to the managers that somebody wrote interfaces so that I
need to implement all of them. I said that I can as well design code
myself and I don't need anybody do useless work (designing empty
interfaces). They agreed with me, now I do it all on my own.

Are your managers skilled in C++ or even OO programming? Can they
knowingly decide if you are right?


I have a strong feelling, that you graduated recently (maybe some
university), because your arrogant treatment of somebody's job (a good
job), shit-filled comments, blaming of good programming practices is so
relevant. Calm down, get a little more modesty, and try to find out
what was those solutions made for.

greetz