design criticism

[yurec]

class BigClass
{
....
protected:
void
_FindLayoutToSet(CApplicationContextManager::EApplicationContext/
*resolve_state_param*/ i_param );
void _SetLayout(MLayoutManager::ELayoutType/*state*/ i_param );
};

void
BigClass::_FindLayoutToSet(CApplicationContextManager::EApplicationContext
i_param )
{
MLayoutManager::ELayoutType state_to_set;
switch(i_param)
{
case 0:
{
A a;
a.DoSmth();
bool a_ret = a.GetSmth();
B b;
bool b_ret = b.DoSmth();
if (a && b)
{
state_to_set = some_state;
}
else
{
state_to_set = another_state;
}
break;
}
case 1:
... //the same complex logic
break;
default:
break;
}
_SetLayout(state_to_set);
}

I decided to redesign it into smth like this :


class BigClass //more 15000 lines in cpp
{
....
public:
typedef boost::function<MLayoutManager::ELayoutType (void)>
get_layout_func;
typedef boost::shared_ptr<get_layout_func> p_get_layout_func;

void
SetLayoutFuncToContextPredicate(boost::shared_ptr<IContextToLayoutPred>
i_pred);
boost::shared_ptr<IContextToLayoutPred>
GetLayoutFuncToContextPredicate() const;

void
SetLayoutFuncToContext(CApplicationContextManager::EApplicationContext
i_context,
const std::string_t & i_name_origin,
p_get_layout_func ip_get_layout_type_from_context);

bool
RemoveLayoutFuncToContext(CApplicationContextManager::EApplicationContext
i_context,
const std::string_t & i_name_origin);

p_get_layout_func
GetLayoutFuncToContext(CApplicationContextManager::EApplicationContext
i_context,
const std::string_t & i_name_origin) const;

protected:
void _FindApplicationStateToSet(int i_param );
void _SetApplicationState(state i_param );

private:
struct CSCDocImpl;
std::auto_ptr<CSCDocImpl> mp_impl;
};



class DefaultContextToLayoutPredublic IContextToLayoutPred
{
public:

DefaultContextToLayoutPred():m_layout_type(MLayoutManager::LAYOUT_UNKNOWN)
{};

virtual MLayoutManager::ELayoutType GetLayoutType() const
{
return m_layout_type;
}

bool Compare(CSCDoc:_get_layout_func i_func) const // bad
method name. means get result of function ,
// change
internal predicate state and return,
// if got
enough information
{
const MLayoutManager::ELayoutType layout_type = (*i_func)();
if (layout_type != m_layout_type)
{
m_layout_type = layout_type;
return true;
}
return false;
}

virtual void Reset()
{
m_layout_type = MLayoutManager::LAYOUT_UNKNOWN;
}

private:
mutable MLayoutManager::ELayoutType m_layout_type;
};

//-----------------------------------------------------------------------------
struct CSCDoc::CSCDocImpl
{
ContextToLayout m_context_to_layout;
boost::shared_ptr<IContextToLayoutPred> mp_layout_to_contex_pred;

MLayoutManager::ELayoutType
GetLayoutType(CApplicationContextManager::EApplicationContext
i_context) const;
bool
RemoveLayoutFuncToContext(CApplicationContextManager::EApplicationContext
i_context, const std::string_t & i_name_origin);
};


//-----------------------------------------------------------------------------
MLayoutManager::ELayoutType
CSCDoc::CSCDocImpl::GetLayoutType(CApplicationContextManager::EApplicationContext
i_context) const
{
if (false == mp_layout_to_contex_pred)
{
return MLayoutManager::LAYOUT_UNKNOWN;
}

return m_context_to_layout.GetLayoutType(i_context,
mp_layout_to_contex_pred);
}

//-----------------------------------------------------------------------------
bool
CSCDoc::CSCDocImpl::RemoveLayoutFuncToContext(CApplicationContextManager::EApplicationContext
i_context, const std::string_t & i_name_origin)
{
bool ret =
m_context_to_layout.RemoveLayoutFuncToContext(i_context,
i_name_origin);
return ret;
}



//------------------------------------------------------------------------------------
void
CSCDoc::SetLayoutFuncToContext(CApplicationContextManager::EApplicationContext
i_context,
const std::string_t & i_name_origin,
p_get_layout_func ip_get_layout_type_from_context)
{
mp_impl->m_context_to_layout.SetLayoutFuncToContext(i_context,
i_name_origin, ip_get_layout_type_from_context);
}

//------------------------------------------------------------------------------------
bool
CSCDoc::RemoveLayoutFuncToContext(CApplicationContextManager::EApplicationContext
i_context,
const std::string_t & i_name_origin)
{
return mp_impl->RemoveLayoutFuncToContext(i_context,
i_name_origin);
}

//------------------------------------------------------------------------------------
CSCDoc:_get_layout_func
CSCDoc::GetLayoutFuncToContext(CApplicationContextManager::EApplicationContext
i_context,
const std::string_t & i_name_origin) const
{
return mp_impl-

m_context_to_layout.GetLayoutFuncToContext(i_context, i_name_origin);

}




//------------------------------------------------------------------------------------
void
CSCDoc::SetLayoutFuncToContextPredicate(boost::shared_ptr<IContextToLayoutPred>
i_pred)
{
mp_impl->mp_layout_to_contex_pred = i_pred;
}

//------------------------------------------------------------------------------------
boost::shared_ptr<IContextToLayoutPred>
CSCDoc::GetLayoutFuncToContextPredicate() const
{
return mp_impl->mp_layout_to_contex_pred;
}


/*-------------------ContextToLayout + IContextToLayoutPred
---------------------------------------*/
#pragma once

#include <boost/function.hpp>
#include "MLayoutManager.h"

class IContextToLayoutPred
{
public:
virtual ~IContextToLayoutPred(){};
virtual void Reset(){};
virtual MLayoutManager::ELayoutType GetLayoutType() const
{return MLayoutManager::LAYOUT_UNKNOWN;};
virtual bool Compare(CSCDoc:_get_layout_func i_func) const
{ return false;};

bool operator()(const CSCDoc:_get_layout_func & i_func)
{
return Compare(i_func);
}
};

//-----------------------------------------------------------------------------
class ContextToLayout
{
public:
typedef boost::function<MLayoutManager::ELayoutType (void)>
get_layout_func;
typedef boost::shared_ptr<get_layout_func> p_get_layout_func;

private:
typedef std::map<std::string_t, p_get_layout_func >
get_layout_func_map;
typedef
std::map<CApplicationContextManager::EApplicationContext,
get_layout_func_map > context_to_layout;

public:
MLayoutManager::ELayoutType
GetLayoutType(CApplicationContextManager::EApplicationContext
i_context,
boost::shared_ptr<IContextToLayoutPred> ip_pred) const;

p_get_layout_func
GetLayoutFuncToContext(CApplicationContextManager::EApplicationContext
i_context,
const std::string_t & i_name_origin) const;

void
SetLayoutFuncToContext(CApplicationContextManager::EApplicationContext
i_context,
const std::string_t & i_name_origin,
p_get_layout_func i_get_layout_type_from_context);

bool
RemoveLayoutFuncToContext(CApplicationContextManager::EApplicationContext
i_context,
const std::string_t & i_name_origin);

private:
context_to_layout m_context_to_layout;
};

//-----------------------------------------------------------------------------
#include "stdafx.h"
#include "ContextToLayout.h"

#include "ApplicationContextManager.h"

#include <boost/bind.hpp>

//-----------------------------------------------------------------------------
void
ContextToLayout::SetLayoutFuncToContext( CApplicationContextManager::EApplicationContext
i_context,
const std::string_t & i_name_origin,
p_get_layout_func ip_get_layout_type_from_context)
{
m_context_to_layout[i_context][i_name_origin] =
ip_get_layout_type_from_context;
}

//-----------------------------------------------------------------------------
bool
ContextToLayout::RemoveLayoutFuncToContext(CApplicationContextManager::EApplicationContext
i_context,
const std::string_t & i_name_origin)
{
context_to_layout::iterator iter_context =
std::find_if( m_context_to_layout.begin(),

m_context_to_layout.end(),

boost::bind(&context_to_layout::value_type::first,_1) == i_context);

get_layout_func_map & named_func_map = iter_context->second;
context_to_layout::mapped_type::iterator iter_context_name =
named_func_map.find(i_name_origin);
if (named_func_map.end() != iter_context_name)
{
named_func_map.erase(iter_context_name);
return true;
}
return false;
}

//-----------------------------------------------------------------------------
ContextToLayout:_get_layout_func
ContextToLayout::GetLayoutFuncToContext(CApplicationContextManager::EApplicationContext
i_context,
const std::string_t & i_name_origin) const
{
context_to_layout::const_iterator iter_context =
std::find_if( m_context_to_layout.begin(),

m_context_to_layout.end(),

boost::bind(&context_to_layout::value_type::first,_1) == i_context);
const get_layout_func_map & named_func_map = iter_context-

second;

context_to_layout::mapped_type::const_iterator iter_context_name
= named_func_map.find(i_name_origin);
return iter_context_name != named_func_map.end() ?
(iter_context_name->second) : p_get_layout_func();
}

//-----------------------------------------------------------------------------
MLayoutManager::ELayoutType
ContextToLayout::GetLayoutType(CApplicationContextManager::EApplicationContext
i_context,
boost::shared_ptr<IContextToLayoutPred> ip_pred) const
{
MLayoutManager::ELayoutType layout_type =
MLayoutManager::LAYOUT_UNKNOWN;

context_to_layout::const_iterator iter_context =
m_context_to_layout.find(i_context);
if (m_context_to_layout.end() == iter_context)
{
layout_type = ip_pred->GetLayoutType();
ip_pred->Reset();
return layout_type;
}

const get_layout_func_map & named_func_map = iter_context-

second;

get_layout_func_map::const_iterator iter_func =
std::find_if(named_func_map.begin(), named_func_map.end(),

boost::bind(&IContextToLayoutPred::Compare, ip_pred,

boost::bind(&get_layout_func_map::value_type::second, _1)));

layout_type = ip_pred->GetLayoutType();
ip_pred->Reset();
return layout_type;
}

//-----------------------------------------------------------------------------


So as a result we have :

void
BigClass::_FindLayoutToSet(CApplicationContextManager::EApplicationContext
i_param )
{
MLayoutManager::ELayoutType state_to_set = mp_impl-

GetLayoutType(i_context);

_SetLayout(state_to_set);
}

So there are no dependecies between this class and
and classes wich really knows what layout we must
have based on their internal state.

Thank you very much if you have read all above.
Looking forward to your comments.

 

[Noah Roberts]

Learn about code smells and you should be able to evaluate it yourself.

 

[yurec]

Learn about code smells and you should be able to evaluate it yourself.

Sorry for my poor english, but I can understand what do you mean

 

[Alf P. Steinbach]

* yurec:

Sorry for my poor english, but I can understand what do you mean

He means that

class BigClass //more 15000 lines in cpp

stinks.

It's so rotten that you should not only think about refactoring, but
really think about re-implementing the entire program from scratch, and
perhaps also, at a higher level, whether that program or something like
it is really a good solution to the information processing needs.

Cheers, & hth.,

- Alf

 

[Noah Roberts]

* yurec:

He means that

class BigClass //more 15000 lines in cpp

stinks.

It's so rotten that you should not only think about refactoring, but
really think about re-implementing the entire program from scratch, and
perhaps also, at a higher level, whether that program or something like
it is really a good solution to the information processing needs.

Sigh...that is not at all what I mean.

Go look up "code smell" and read about the various different kinds and
the design problems they exhibit. Then you'll have some tools with
which to evaluate design.

 

[Alf P. Steinbach]

* Noah Roberts:

Sigh...that is not at all what I mean.

Oh, sorry.

That is what you /should/ have meant.

Go look up "code smell" and read about the various different kinds and
the design problems they exhibit. Then you'll have some tools with
which to evaluate design.

If you don't smell the above, then your nasal cavities are probably
clogged by demons.

Cheers,

- Alf

 

[yurec]

* Noah Roberts:







Oh, sorry.

That is what you /should/ have meant.


If you don't smell the above, then your nasal cavities are probably
clogged by demons.

Cheers,

- Alf

--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?- Hide quoted text -

- Show quoted text -

Wikipedia gives nice definition of "code smell" )
However this class is a part of framework,
maybe everyone knows that it needs refactoring (re-implementing),
but there is no time for this given by management.
So my question covers just a piece of possible refactoring and

i wonder if my design decision is good ( correct, useful)

 

[Alf P. Steinbach]

* yurec:

Wikipedia gives nice definition of "code smell" )

Look up "God class".

However this class is a part of framework,

Then don't touch it.

maybe everyone knows that it needs refactoring (re-implementing),
but there is no time for this given by management.
So my question covers just a piece of possible refactoring and
i wonder if my design decision is good ( correct, useful)

It seems you have introduced additional complexity in order to remove a
little file dependency.

That's probably not good, and wrapping a functor in a shared_ptr, well,
it absolutely doesn't smell right, and with the quality of original code
shown it's probably absolutely NOT a good idea to use newfangled things
such as Boost functions and template algorithms: think maintainance.

But I don't have time to delve into the particulars of your solution,
especially since you fail to explain what the heck it is meant to be a
solution of, but regarding the original code, here are some comments:

* "protected:" is used for internal functions that can't possibly be
meant to be called by derived classes.

* "_FindLayoutToSet", names starting with underscore followed by
uppercase are reserved to the implementation. UB right here.

* "_FindLayoutToSet" calling "_SetLayout", misleading name for the
first, it doesn't find a layout, it sets the layout, plus,
procedural coding.

* Big switch and if/else statement, failure to use OO or any
abstraction, look for bugs here.

* Magic numbers (0, 1 and so forth).

But all this means is that the code is a total mess and probably bugsy
as hell, and we /knew/ that already from the God class property.

If I were to do anything with that code (I'd never!), I'd start by
/naming/ things, those magic numbers. In order to bring clarity. So as
to remove the zillion bugs that are certain to lurk in there.

Because you can't refactor something that's full of bugs unless you have
a detailed specification of what it should do, which given the little
original code you presented it is very improbable that you have.

Cheers, & hth.,

- Alf

 

[yurec]

* yurec:







Look up "God class".


Then don't touch it.


It seems you have introduced additional complexity in order to remove a
little file dependency.

That's probably not good, and wrapping a functor in a shared_ptr, well,
it absolutely doesn't smell right, and with the quality of original code
shown it's probably absolutely NOT a good idea to use newfangled things
such as Boost functions and template algorithms: think maintainance.

But I don't have time to delve into the particulars of your solution,
especially since you fail to explain what the heck it is meant to be a
solution of, but regarding the original code, here are some comments:

* "protected:" is used for internal functions that can't possibly be
meant to be called by derived classes.

* "_FindLayoutToSet", names starting with underscore followed by
uppercase are reserved to the implementation. UB right here.

* "_FindLayoutToSet" calling "_SetLayout", misleading name for the
first, it doesn't find a layout, it sets the layout, plus,
procedural coding.

* Big switch and if/else statement, failure to use OO or any
abstraction, look for bugs here.

* Magic numbers (0, 1 and so forth).

But all this means is that the code is a total mess and probably bugsy
as hell, and we /knew/ that already from the God class property.

If I were to do anything with that code (I'd never!), I'd start by
/naming/ things, those magic numbers. In order to bring clarity. So as
to remove the zillion bugs that are certain to lurk in there.

Because you can't refactor something that's full of bugs unless you have
a detailed specification of what it should do, which given the little
original code you presented it is very improbable that you have.

Cheers, & hth.,

- Alf

--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?- Hide quoted text -

- Show quoted text -

The piece of code i want to refactot does :
it takes some enumed value as input parameter, then
it examines application state using the case corresponding to the
value, then
it sets application view state using some value, calculated in one of
switch statement code.

1)There are no magics numbers ( I just didn't want to write our emuns
as they didn't important hear)

2)Underscore before protected methods names is a part of our coding
conventions

3)The target of my design is to remove switch statement and dependency
incapsulating case logic into
smth like Command.
( If it requires not a few classes and using switch it will definetly
require more and more,
so I don't think, that it's really little dependency).

4)I didn't want to store those functors ( commands ) as far as they
could be state functors and include some objects.
So using boost::shared_ptr seemed to be simple and obvious solution.
(as auto_ptr must not be used with containers)

 

[Nick Keighley]

But I don't have time to delve into the particulars of your solution,
especially since you fail to explain what the heck it is meant to be a
solution of, but regarding the original code, here are some comments:

* "protected:" is used for internal functions that can't possibly be
meant to be called by derived classes.

?

so why not make them private?

<snip>

 

[Alf P. Steinbach]

* yurec:

2)Underscore [followed by uppercase] before protected methods
names is a part of our coding conventions

Usually coding conventions are made by the best and most experienced
people in the firm.

What you're saying is that in your firm, those people don't even know
the basic rules of the language.

Good luck to you!


Cheers,

- Alf

 

[Alf P. Steinbach]

* Nick Keighley:

?

so why not make them private?

I'm sure you didn't read what you replied to.

It's often a good idea to read what one replies to.

Cheers,

- Alf

 

[mike3]

* yurec:



He means that

class BigClass //more 15000 lines in cpp

stinks.

It's so rotten that you should not only think about refactoring, but
really think about re-implementing the entire program from scratch, and
perhaps also, at a higher level, whether that program or something like
it is really a good solution to the information processing needs.

Does this mean it would actually be less work for him to rewrite
entirely
from scratch than to try and repair it?

 

[Alf P. Steinbach]

* mike3:

Does this mean it would actually be less work for him to rewrite
entirely
from scratch than to try and repair it?

No.

It means trying to repair it and then trying to repair it and then and
so on, is in total a lot more work, with the same sorry mess as end
result technically. Meantime, clients may get more and more frustrated
(of course, this doesn't apply in defense industry where you can sell
'em tanks made of cardboard or, as in US, e.g. bullet proof vests
guaranteed to not stop bullets). So it's a matter of perspective, what
context you think within: thinking within only the smallest possible
context a goto is a good solution to any execution control problem, but
then later, in a little wider context, those gotos become problematic.

"Higher level" means management at the proper level needs to take a
closer look, but for that, they need good information -- and that's
starting to get off-topic here...

Cheers, & hth.,

- Alf

 

[yurec]

* Nick Keighley:


I'm sure you didn't read what you replied to.

It's often a good idea to read what one replies to.

Cheers,

- Alf

--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

17.4.3.1.2 Global Names
1 Certain sets of names and functions signature are always reserved
to
the implementation:
-- Each name that contains a double underscore (_ _) or begins with
an
underscore followed by an uppercase letter (2.11) is reserved to the
implementation for any use.
-- Each name that begins with an underscore is reserved to the
implementation for use as a name in the global namespace.

Does this mean that SomeClass::Method or SomeClass::member must not
begin from underscore followed by uppercase letter?
I thought that it make sense only for functions in global
namespace.But in this case
what "-- Each name that begins with an underscore is reserved to the
implementation for use as a name in the global namespace." stands for?
Ok, it seems you are right.
So i'm ashamed.

 

[mike3]

* mike3:







No.

No? Down here it sounds like you're suggesting yes, it would
be less work to rewrite from scratch -- you say "trying to
repair it again and again and again is in total a lot _more_
work", not less:

 

[mike3]

* mike3:







No.

It means trying to repair it and then trying to repair it and then and
so on, is in total a lot more work, with the same sorry mess as end
result technically. Meantime, clients may get more and more frustrated
(of course, this doesn't apply in defense industry where you can sell
'em tanks made of cardboard or, as in US, e.g. bullet proof vests
guaranteed to not stop bullets). So it's a matter of perspective, what
context you think within: thinking within only the smallest possible
context a goto is a good solution to any execution control problem, but
then later, in a little wider context, those gotos become problematic.

"Higher level" means management at the proper level needs to take a
closer look, but for that, they need good information -- and that's
starting to get off-topic here...

Also, is the very concept of his "class BigClass" thingy that takes
15000+ lines of C++ code a poor idea? It seems to be a giant
catchall of some sort, a grab bag.

 

[Nick Keighley]

*Nick Keighley:


I'm sure you didn't read what you replied to.

It's often a good idea to read what one replies to.

I'm sure I'm about look an even bigger idiot than I already do but...

you wrote:

""protected:" is used for internal functions that can't possibly be
meant to be called by derived classes."

which I read as:

""protected:" is used for functions that cannot be called by
derived
classes."

but protected function *can* be called by derived classes...

so is my paraphrase wrong?

 

[Alf P. Steinbach]

* Nick Keighley:

I'm sure I'm about look an even bigger idiot than I already do but...

you wrote:

""protected:" is used for internal functions that can't possibly be
meant to be called by derived classes."

which I read as:

""protected:" is used for functions that cannot be called by
derived
classes."

but protected function *can* be called by derived classes...

so is my paraphrase wrong?

Just a misunderstanding.

The code presented uses "protected:" for functions that really should be
"private:".

Cheers, & hth.,

- Alf

 

[Alf P. Steinbach]

* mike3:

Also, is the very concept of his "class BigClass" thingy that takes
15000+ lines of C++ code a poor idea? It seems to be a giant
catchall of some sort, a grab bag.

Yes, it's known as "God" object (or class).

Such large classes are symptomatic of lack of design.

A class that has evolved to take on more and more responsibilities.

Cheers, & hth.,

- Alf