473,327 Members | 1,967 Online
Bytes | Software Development & Data Engineering Community
Post Job

Home Posts Topics Members FAQ

Join Bytes to post your question to a community of 473,327 software developers and data experts.

design criticism



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

void
BigClass::_FindLayoutToSet(CApplicationContextMana ger::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_funcp_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(CApplicationContextManag er::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<CSCDocImplmp_impl;
};

class DefaultContextToLayoutPred:public IContextToLayoutPred
{
public:

DefaultContextToLayoutPred():m_layout_type(MLayout Manager::LAYOUT_UNKNOWN)
{};

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

bool Compare(CSCDoc::p_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<IContextToLayoutPredmp_layout_to _contex_pred;

MLayoutManager::ELayoutType
GetLayoutType(CApplicationContextManager::EApplica tionContext
i_context) const;
bool
RemoveLayoutFuncToContext(CApplicationContextManag er::EApplicationContext
i_context, const std::string_t & i_name_origin);
};
//-----------------------------------------------------------------------------
MLayoutManager::ELayoutType
CSCDoc::CSCDocImpl::GetLayoutType(CApplicationCont extManager::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(CApp licationContextManager::EApplicationContext
i_context, const std::string_t & i_name_origin)
{
bool ret =
m_context_to_layout.RemoveLayoutFuncToContext(i_co ntext,
i_name_origin);
return ret;
}

//------------------------------------------------------------------------------------
void
CSCDoc::SetLayoutFuncToContext(CApplicationContext Manager::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_cont ext,
i_name_origin, ip_get_layout_type_from_context);
}

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

//------------------------------------------------------------------------------------
CSCDoc::p_get_layout_func
CSCDoc::GetLayoutFuncToContext(CApplicationContext Manager::EApplicationContext
i_context,
const std::string_t & i_name_origin) const
{
return mp_impl-
>m_context_to_layout.GetLayoutFuncToContext(i_cont ext, i_name_origin);
}


//------------------------------------------------------------------------------------
void
CSCDoc::SetLayoutFuncToContextPredicate(boost::sha red_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::p_get_layout_func i_func) const
{ return false;};

bool operator()(const CSCDoc::p_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_funcp_get_layout_func ;

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

public:
MLayoutManager::ELayoutType
GetLayoutType(CApplicationContextManager::EApplica tionContext
i_context,
boost::shared_ptr<IContextToLayoutPredip_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(CApplicationContextManag er::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(CApplic ationContextManager::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::p_get_layout_func
ContextToLayout::GetLayoutFuncToContext(CApplicati onContextManager::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(CApplicationContext Manager::EApplicationContext
i_context,
boost::shared_ptr<IContextToLayoutPredip_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::seco nd, _1)));

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

//-----------------------------------------------------------------------------
So as a result we have :

void
BigClass::_FindLayoutToSet(CApplicationContextMana ger::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.

Nov 30 '07 #1
10 1393
Learn about code smells and you should be able to evaluate it yourself.
Nov 30 '07 #2
On Nov 30, 6:13 pm, Noah Roberts <u...@example.netwrote:
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
Nov 30 '07 #3
Alf P. Steinbach wrote:
* yurec:
>On Nov 30, 6:13 pm, Noah Roberts <u...@example.netwrote:
>>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

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.
Nov 30 '07 #4
On 1 Dec, 15:09, "Alf P. Steinbach" <al...@start.nowrote:

<snip>
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>

--
Nick Keighley
Dec 3 '07 #5
On Nov 30, 10:18 am, "Alf P. Steinbach" <al...@start.nowrote:
* yurec:
On Nov 30, 6:13 pm, Noah Roberts <u...@example.netwrote:
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

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?
Dec 3 '07 #6
On Dec 3, 2:17 pm, "Alf P. Steinbach" <al...@start.nowrote:
* mike3:


On Nov 30, 10:18 am, "Alf P. Steinbach" <al...@start.nowrote:
* yurec:
>On Nov 30, 6:13 pm, Noah Roberts <u...@example.netwrote:
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
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?

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:
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...
Dec 4 '07 #7
On Dec 3, 2:17 pm, "Alf P. Steinbach" <al...@start.nowrote:
* mike3:


On Nov 30, 10:18 am, "Alf P. Steinbach" <al...@start.nowrote:
* yurec:
>On Nov 30, 6:13 pm, Noah Roberts <u...@example.netwrote:
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
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?

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.
Dec 4 '07 #8
On 3 Dec, 15:40, "Alf P. Steinbach" <al...@start.nowrote:
*Nick Keighley:
On 1 Dec, 15:09, "Alf P. Steinbach" <al...@start.nowrote:
<snip>
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?

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?
--
Nick Keighley

Dec 4 '07 #9
On 4 Dec, 15:11, "Alf P. Steinbach" <al...@start.nowrote:
*Nick Keighley:
On 3 Dec, 15:40, "Alf P. Steinbach" <al...@start.nowrote:
*Nick Keighley:
On 1 Dec, 15:09, "Alf P. Steinbach" <al...@start.nowrote:
>>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:
context is everything...
>> * "protected:" is used for internal functions that can't possibly be
meant to be called by derived classes.
>?
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.
I'm sure I'm about look an even bigger idiot than I already do but...
but in general, being prepared to make a fool of oneself often pays
off!

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:".
ah, so I read what you wrote- just ignored the context in which it
was
written.

When protected is wise to use is still something I'm getting
to grips with. My current rules of thumb: make data private, make
"interface
methods" public non-virtual functions, make over-ridable
"implementation
methods" virtual protected functions.

As with all rules of thumb they get broken (I commonly have public
virtual
functions).

Cheers, & hth.,
yes, ta

--
Nick Keighley
Dec 4 '07 #10
On Dec 4, 8:13 am, "Alf P. Steinbach" <al...@start.nowrote:
* mike3:


On Dec 3, 2:17 pm, "Alf P. Steinbach" <al...@start.nowrote:
* mike3:
>On Nov 30, 10:18 am, "Alf P. Steinbach" <al...@start.nowrote:
* yurec:
On Nov 30, 6:13 pm, Noah Roberts <u...@example.netwrote:
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
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?
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.

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.
Eww. You mean like putting the whole darned program into one
giant, bloated object?

Dec 4 '07 #11

This thread has been closed and replies have been disabled. Please start a new discussion.

Similar topics

30
by: Christian Seberino | last post by:
How does Ruby compare to Python?? How good is DESIGN of Ruby compared to Python? Python's design is godly. I'm wondering if Ruby's is godly too. I've heard it has solid OOP design but then...
6
by: Sean Dettrick | last post by:
Hi, I have a class which I want everything to have access to, and which I only want to construct once. I have a hierarchy of classes: class ScalarField{ vector<double> f; // constructors etc...
3
by: Greg Adourian | last post by:
Hi, Still in the design process of a Windows 2003 web server with a SQL backend. Expecting to have about 2000 visitors a day accessing lists and search queries on a 200-300 MB db. This server...
98
by: Pamel | last post by:
I know this must have been asked elsewhere, but I cannot find it. There is a piece of text on my web page that I don't want browsers to resize. IE won't resize it if I specify the size in px, but...
29
by: MP | last post by:
Greets, context: vb6/ado/.mdb/jet 4.0 (no access)/sql beginning learner, first database, planning stages (I think the underlying question here is whether to normalize or not to normalize this...
62
by: Xah Lee | last post by:
Criticism versus Constructive Criticism Xah Lee, 2003-01 A lot intelligent people are rather confused about criticism, especially in our “free-speech” free-for-all internet age. When they...
17
by: roN | last post by:
Hi, I'm creating a Website with divs and i do have some troubles, to make it looking the same way in Firefox and IE (tested with IE7). I checked it with the e3c validator and it says: " This...
10
by: lawrence k | last post by:
I work mostly in PHP, but at the web design firm where I work we are thinking of switching to Ruby on Rails. Our lead designer recently installed Typo on a client's site and he said to us, with...
84
by: aarklon | last post by:
Hi all, I found an interesting article here:- http://en.wikipedia.org/wiki/Criticism_of_the_C_programming_language well what do you guys think of this article....??? Is it constructive...
0
by: DolphinDB | last post by:
Tired of spending countless mintues downsampling your data? Look no further! In this article, youll learn how to efficiently downsample 6.48 billion high-frequency records to 61 million...
0
by: ryjfgjl | last post by:
ExcelToDatabase: batch import excel into database automatically...
0
isladogs
by: isladogs | last post by:
The next Access Europe meeting will be on Wednesday 6 Mar 2024 starting at 18:00 UK time (6PM UTC) and finishing at about 19:15 (7.15PM). In this month's session, we are pleased to welcome back...
1
isladogs
by: isladogs | last post by:
The next Access Europe meeting will be on Wednesday 6 Mar 2024 starting at 18:00 UK time (6PM UTC) and finishing at about 19:15 (7.15PM). In this month's session, we are pleased to welcome back...
0
by: jfyes | last post by:
As a hardware engineer, after seeing that CEIWEI recently released a new tool for Modbus RTU Over TCP/UDP filtering and monitoring, I actively went to its official website to take a look. It turned...
1
by: PapaRatzi | last post by:
Hello, I am teaching myself MS Access forms design and Visual Basic. I've created a table to capture a list of Top 30 singles and forms to capture new entries. The final step is a form (unbound)...
1
by: Defcon1945 | last post by:
I'm trying to learn Python using Pycharm but import shutil doesn't work
1
by: Shllpp 09 | last post by:
If u are using a keypad phone, how do u turn on JavaScript, to access features like WhatsApp, Facebook, Instagram....
0
isladogs
by: isladogs | last post by:
The next Access Europe User Group meeting will be on Wednesday 3 Apr 2024 starting at 18:00 UK time (6PM UTC+1) and finishing by 19:30 (7.30PM). In this session, we are pleased to welcome former...

By using Bytes.com and it's services, you agree to our Privacy Policy and Terms of Use.

To disable or enable advertisements and analytics tracking please visit the manage ads & tracking page.