Suggestion on refactoring existing code

[shuisheng]

Dear All,

I have a code developed by former employees. I extract some part of it
as below:

// definition of class CWNPrimitiveFace, it represent a face
class CWNPrimitiveFace : public CWN3DObjBase
{
friend ofstream& operator<<( ofstream& f, CWNPrimitiveFace& obj );
friend ifstream& operator>>( ifstream& f, CWNPrimitiveFace& obj );
public:
CWNPrimitiveFace();
CWNPrimitiveFace( wxString name );
CWNPrimitiveFace( unsigned int nid );
CWNPrimitiveFace( const CWNPrimitiveFace& face );
virtual ~CWNPrimitiveFace();

CWNPrimitiveFace& operator=( const CWNPrimitiveFace& obj );

// override
virtual void Scale( double k );
//
bool IsPlane();
bool IsReferenceFace();
bool IsRect();
bool IsEllipse();

void SetRefFace( void* pAcisFace );
void* GetRefFace();
void SetRect( double w, double h );
bool GetRect( double& w, double& h );
void SetEllp( double r0, double r1 );
bool GetEllp( double& r0, double& r1 );

protected:
int m_nFaceType; // 0 - unknown, 1 - referent to other face,
// 2 - rect, 3 - ellp, 4 - ....
union{
struct {
void *m_pOwner;
} ref_face;

struct{
double width;
double height;
} rect;

struct{
double r0;
double r1;
} ellp;
} m_Para;

private:
void InitData();
void CopyData( const CWNPrimitiveFace& obj );
void RemoveRefFace();
};

void CWNPrimitiveFace::CopyData( const CWNPrimitiveFace& obj )
{
// remove old face
RemoveRefFace();

//
m_nFaceType = obj.m_nFaceType;

if( m_nFaceType == 1 )
{
m_Para.ref_face.m_pOwner =
g_Acis.CopyEntity( obj.m_Para.ref_face.m_pOwner );
}
else if( m_nFaceType == 2 )
{
m_Para.rect.width = obj.m_Para.rect.width;
m_Para.rect.height = obj.m_Para.rect.height;
}
else if( m_nFaceType == 3 )
{
m_Para.ellp.r0 = obj.m_Para.ellp.r0;
m_Para.ellp.r1 = obj.m_Para.ellp.r1;
}
}

In the code, most classes have the similar structure: prefer to union
other than polymorphism. Some even have nested switch-cases. The code
are not fully tested. It has been only used to run some cases and
several crash bugs were found. The code is of 70K line. The code is
wrritten by a guy with 10 years of c++ experiences in 8 moths. I am
wondering is the code worth refactoring?

Thanks,

Shuisheng

[Victor Bazarov]

shuisheng wrote:

Dear All,

I have a code developed by former employees. I extract some part of it
as below:

[..]

In the code, most classes have the similar structure: prefer to union
other than polymorphism. Some even have nested switch-cases. The code
are not fully tested. It has been only used to run some cases and
several crash bugs were found. The code is of 70K line. The code is
wrritten by a guy with 10 years of c++ experiences in 8 moths. I am
wondering is the code worth refactoring?

Unless the code is totally not working, it's always worth refactoring.
But don't refactor for the sake of refactoring, though. Deterimine
what you're going to get out of it. It's not improbable that even
code like what you posted can be made to work with minimal changes.

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask

[JLS]

On Mar 13, 5:25 pm, "shuisheng" <[e-mail]:

Dear All,

I have a code developed by former employees. I extract some part of it
as below:

// definition of class CWNPrimitiveFace, it represent a face
class CWNPrimitiveFace : public CWN3DObjBase
{
friend ofstream& operator<<( ofstream& f, CWNPrimitiveFace& obj );
friend ifstream& operator>>( ifstream& f, CWNPrimitiveFace& obj );
public:
CWNPrimitiveFace();
CWNPrimitiveFace( wxString name );
CWNPrimitiveFace( unsigned int nid );
CWNPrimitiveFace( const CWNPrimitiveFace& face );
virtual ~CWNPrimitiveFace();

CWNPrimitiveFace& operator=( const CWNPrimitiveFace& obj );

// override
virtual void Scale( double k );
//
bool IsPlane();
bool IsReferenceFace();
bool IsRect();
bool IsEllipse();

void SetRefFace( void* pAcisFace );
void* GetRefFace();
void SetRect( double w, double h );
bool GetRect( double& w, double& h );
void SetEllp( double r0, double r1 );
bool GetEllp( double& r0, double& r1 );

protected:
int m_nFaceType; // 0 - unknown, 1 - referent to other face,
// 2 - rect, 3 - ellp, 4 - ....
union{
struct {
void *m_pOwner;
} ref_face;

struct{
double width;
double height;
} rect;

struct{
double r0;
double r1;
} ellp;
} m_Para;

private:
void InitData();
void CopyData( const CWNPrimitiveFace& obj );
void RemoveRefFace();

};

void CWNPrimitiveFace::CopyData( const CWNPrimitiveFace& obj )
{
// remove old face
RemoveRefFace();

//
m_nFaceType = obj.m_nFaceType;

if( m_nFaceType == 1 )
{
m_Para.ref_face.m_pOwner =
g_Acis.CopyEntity( obj.m_Para.ref_face.m_pOwner );
}
else if( m_nFaceType == 2 )
{
m_Para.rect.width = obj.m_Para.rect.width;
m_Para.rect.height = obj.m_Para.rect.height;
}
else if( m_nFaceType == 3 )
{
m_Para.ellp.r0 = obj.m_Para.ellp.r0;
m_Para.ellp.r1 = obj.m_Para.ellp.r1;
}

}

In the code, most classes have the similar structure: prefer to union
other than polymorphism. Some even have nested switch-cases. The code
are not fully tested. It has been only used to run some cases and
several crash bugs were found. The code is of 70K line. The code is
wrritten by a guy with 10 years of c++ experiences in 8 moths. I am
wondering is the code worth refactoring?

Thanks,

Shuisheng

I think it depends on circumstances. If the code is going to be
modified and enhanced and is part of active development, then it is
probably worth refactoring; The code will be easier to understand and
maintain. If the code is not going to be maintained then the costs and
risks involved in refactoring may outway the benefits.

[Puppet_Sock]

On Mar 13, 5:25 pm, "shuisheng" <[e-mail]:
[code sample snipped]

In the code, most classes have the similar structure: prefer to union
other than polymorphism. Some even have nested switch-cases. The code
are not fully tested. It has been only used to run some cases and
several crash bugs were found. The code is of 70K line. The code is
wrritten by a guy with 10 years of c++ experiences in 8 moths. I am
wondering is the code worth refactoring?

This is kind of peripheral to language issues. This is
getting much more into software engineering. Oh well.

It is not really possible to tell you what to do from what
you have provided. But there are some suggestions I can make.

A union is not automatically good or bad. There are situations
in which a union makes sense. That's part of why it has been
kept in the language. An example might be, as you mention, a
form of allowing a class to handle different situations in
the same memory. There are, of course, some rules you need
to follow to make unions sensible. Such as, do not use them
to try to convert one type to another.

Nested switch cases are, again, not automaticlaly good or bad.
It indicates a certain complexity of coding, and that isn't
necessarily a good sign. Usually there are ways to get around
such a level of complexity. But they are not always a gain
overall, as they sometimes involve "tricky" coding methods.
And they often involve either more functions or more classes.
It's not automatic that there is a net gain.

Generally speaking, don't modify code unless there is some
business reason to do so. Making code "pretty" or "elegant"
is not a good enough reason to open it to possible bugs
due to coders. Remember that every bug in the code was
put there by a coder. If it works, don't change it.

But when there are already bugs, and you are touching
the code anyway, it may be that there are things that
are worth doing while you are there. Again, from the
snippet you provided, it isn't possible to decide.

Consider how much effort it will take to make the
changes you are contemplating. Consider what the
benefit will be from having the modified code.
Try to make an estimation of whether the changes
will in fact remove all important bugs, or whether
the code is too complex for a reasonable coder to
be able to maintain it.

Also give some thought to the level of testing you
are applying, the quality of the documentation,
what specifications for performance you have, etc.
For a code with 70K lines, and 8 months of development
effort, there should be some significant effort in
these directions. Making these efforts easier may be
a good reason to make changes to the code. But again,
make sure it makes sense to spend the time on it
before you start paying some software guy(s) to spend
a long time working over it.
Socks

[Jim Langston]

"shuisheng" <sh*********@yahoo.comwrote in message
news:11*********************@l77g2000hsb.googlegro ups.com...

Dear All,

I have a code developed by former employees. I extract some part of it
as below:

// definition of class CWNPrimitiveFace, it represent a face
class CWNPrimitiveFace : public CWN3DObjBase
{
friend ofstream& operator<<( ofstream& f, CWNPrimitiveFace& obj );
friend ifstream& operator>>( ifstream& f, CWNPrimitiveFace& obj );
public:
CWNPrimitiveFace();
CWNPrimitiveFace( wxString name );
CWNPrimitiveFace( unsigned int nid );
CWNPrimitiveFace( const CWNPrimitiveFace& face );
virtual ~CWNPrimitiveFace();

CWNPrimitiveFace& operator=( const CWNPrimitiveFace& obj );

// override
virtual void Scale( double k );
//
bool IsPlane();
bool IsReferenceFace();
bool IsRect();
bool IsEllipse();

void SetRefFace( void* pAcisFace );
void* GetRefFace();
void SetRect( double w, double h );
bool GetRect( double& w, double& h );
void SetEllp( double r0, double r1 );
bool GetEllp( double& r0, double& r1 );

protected:
int m_nFaceType; // 0 - unknown, 1 - referent to other face,
// 2 - rect, 3 - ellp, 4 - ....
union{
struct {
void *m_pOwner;
} ref_face;

struct{
double width;
double height;
} rect;

struct{
double r0;
double r1;
} ellp;
} m_Para;

private:
void InitData();
void CopyData( const CWNPrimitiveFace& obj );
void RemoveRefFace();
};

void CWNPrimitiveFace::CopyData( const CWNPrimitiveFace& obj )
{
// remove old face
RemoveRefFace();

//
m_nFaceType = obj.m_nFaceType;

if( m_nFaceType == 1 )
{
m_Para.ref_face.m_pOwner =
g_Acis.CopyEntity( obj.m_Para.ref_face.m_pOwner );
}
else if( m_nFaceType == 2 )
{
m_Para.rect.width = obj.m_Para.rect.width;
m_Para.rect.height = obj.m_Para.rect.height;
}
else if( m_nFaceType == 3 )
{
m_Para.ellp.r0 = obj.m_Para.ellp.r0;
m_Para.ellp.r1 = obj.m_Para.ellp.r1;
}
}

In the code, most classes have the similar structure: prefer to union
other than polymorphism. Some even have nested switch-cases. The code
are not fully tested. It has been only used to run some cases and
several crash bugs were found. The code is of 70K line. The code is
wrritten by a guy with 10 years of c++ experiences in 8 moths. I am
wondering is the code worth refactoring?

As opposed to what? Most likely, yes. Are you asking is the code worth
refactoring, or should we start from scratch? It's hard to say without
looking at all the code, but I've found in most cases refacting is better as
long as the original design wasn't way off course.

I've started refactoring some badly written code and by the time I was done
nothing in the program was the same, I had rewritten all of it. The code
here doesn't look that bad, and if I actually spent some time looking at it
I could probably fix any errors relatively click. But this is just at a
quick glance.

70k is not that big. I just looked at the source for one of my projects,
the client .cpp is 170k The server .cpp is 155k. Client launcher is 49k.
Plus various other .cpps generally less that 10k each. 70k is probably
small and shouldn't be that bad.

I would say, if the program crashes, fix the existing program instead of
rewriting it.