Duplicate code
c.pro...@googlemail.com
Considering the code :
//
====================================================================//
#include <stdlib.h>
#include <stdio.h>
#define TRUE 1
#define FALSE 0
#define BL unsigned
#define U32 unsigned
#define I32 int
struct ST {
I32 i1;
I32 i2;
};
struct S {
U32 u;
struct ST a ;
struct ST b ;
};
void dup1 ( struct S * ) ;
void dup2 ( struct S *d) ;
void func ( BL , struct S * );
void main ( void ) {
struct S data ;
func(TRUE, &data);
func(FALSE, &data);
return ;
}
void func ( BL select , struct S *data ) {
if ( select == TRUE ) {
dup1( data );
} else if ( select == FALSE ) {
dup2( data );
} else {
printf("Oops ; don't you like boolean selection ? :)");
printf("\n");
}
return ;
}
void dup1 ( struct S *data ) {
data->u = FALSE;
data->a.i1 = TRUE;
data->a.i2 = FALSE;
return;
}
void dup2 ( struct S *data ) {
data->u = FALSE;
data->b.i1 = TRUE;
data->b.i2 = FALSE;
return;
}
//
====================================================================//
How can I avoid such (almost) duplicated functions like dup1 and dup2
which only differ at using "a" or "b" ?
This is because these functions grow on parallel and so seem really
odd plus maintaining them may becomes harder , should a bug jump in.
Regards.
James Kuyper
> Hi ,
> Considering the code :
>
> //
> ====================================================================//
> #include <stdlib.h>
> #include <stdio.h>
>
> #define TRUE 1
> #define FALSE 0
> #define BL unsigned
>
> #define U32 unsigned
> #define I32 int
It will generally be better to use typedefs for this purpose, rather
than macros; this is precisely what typedefs are for.
Are you aware that neither int nor unsigned are required to be 32 bits?
You'd be better off using one of the size-named types from <stdint.h>,
at least if you can afford to restrict the portability of your code to
those compilers which support this feature of C99.
> struct ST {
> I32 i1;
> I32 i2;
> };
> struct S {
> U32 u;
> struct ST a ;
> struct ST b ;
> };
>
> void dup1 ( struct S * ) ;
> void dup2 ( struct S *d) ;
> void func ( BL , struct S * );
>
> void main ( void ) {
void main() is not-portable. Implementations are not required to allow
it, and are not required to inform you if you use it and they don't
support it.
One alternative:
struct S {
U32 u;
struct ST ab[2];
};
void dup1 ( struct S *data, int item ) {
data->u = FALSE;
data->ab[item].i1 = TRUE;
data->ab[item].i2 = FALSE;
return;
}
Why do you want initialize either only a or only b? That seems odd to
me, though I'm sure there could be a good reason for it.
maverik
I32 i1;
I32 i2;};
struct S {
U32 u;
struct ST a ;
struct ST b ;
};
void no_dup ( U32 , struct ST * ) ;
void func ( BL , struct S * );
int
main ( void ) {
struct S data ;
func(TRUE, &data);
func(FALSE, &data);
return 0;
}
void func ( BL select , struct S *data ) {
if ( select == TRUE ) {
no_dup( data.u, &(data->a) );
} else if ( select == FALSE ) {
no_dup( data.u, &(data->b) );
} else {
printf("Oops ; don't you like boolean
selection ? :)");
printf("\n");
}
}
void
no_dup ( U32 u, struct ST *data ) {
u = FALSE;
data->i1 = TRUE;
data->i2 = FALSE;
}
The function no_dup should know about "struct S" as small as possible.
So it's better to pass not the whole structure S but just a part, such
as only fields you need.
Bartc
<c.pro...@googlemail.com> wrote in message
news:1766171b-c0f0-4d21...@d32g2000yqe.googlegroups.com...
> void dup1 ( struct S *data ) {
> data->u = FALSE;
> data->a.i1 = TRUE;
> data->a.i2 = FALSE;
> return;
> }
>
> void dup2 ( struct S *data ) {
> data->u = FALSE;
> data->b.i1 = TRUE;
> data->b.i2 = FALSE;
> return;
> }
> How can I avoid such (almost) duplicated functions like dup1 and dup2
> which only differ at using "a" or "b" ?
How about a macro:
#define DUPBODY(aorb) \
data->u = FALSE; \
data->aorb.i1 = TRUE; \
data->aorb.i2 = FALSE; \
return;
void dup1 ( struct S *data ) {
DUPBODY(a)
}
void dup2 ( struct S *data ) {
DUPBODY(b)
}
Otherwise just make the a or b selection a parameter to a single dup()
function.
--
Bartc
nick_keigh...@hotmail.com
<snip>
> How can I avoid such (almost) duplicated functions like dup1 and dup2
> which only differ at using "a" or "b" ?
> This is because these functions grow on parallel and so seem really
> odd plus maintaining them may becomes harder , should a bug jump in.
void init_st (struct ST *st)
{
st->i1 = TRUE;
st->i2 = FALSE;
}
void func (BL select, struct S *data)
{
if ( select == TRUE )
dup1 (data);
else if (select == FALSE)
dup2 (data);
else
printf("Oops ; don't you like boolean selection ? :)\n");
}
void dup1 (struct S *data)
{
data->u = FALSE;
init_st (&data->a);
}
void dup2 (struct S *data)
{
data->u = FALSE;
init_st (&data->b);
}
/*********************/
or get rid of those dup functions all together
void init_st (struct ST *st)
{
st->i1 = TRUE;
st->i2 = FALSE;
}
void func (BL select, struct S *data)
{
data->u = select;
if (select)
init_st (&data->a);
else
init_st (&data->b);
}
--
Nick Keighley
c.pro...@googlemail.com
> It will generally be better to use typedefs for this purpose, rather
> than macros; this is precisely what typedefs are for.
Yes using typedef sounds a plausible approach though I don't know what
is used in the actual code.
> Are you aware that neither int nor unsigned are required to be 32 bits?
Yes !
> You'd be better off using one of the size-named types from <stdint.h>,
> at least if you can afford to restrict the portability of your code to
> those compilers which support this feature of C99.
Note that this is a sample program ; in practice I use compiler
specific types which is none sense here.
> void main() is not-portable. Implementations are not required to allow
> it, and are not required to inform you if you use it and they don't
> support it.
Hmm ; again the work of an unfortunate typer who wants to avoid
typing ... arguments.
> One alternative:
>
> struct S {
> U32 u;
> struct ST ab[2];
> };
>
> void dup1 ( struct S *data, int item ) {
> data->u = FALSE;
> data->ab[item].i1 = TRUE;
> data->ab[item].i2 = FALSE;
> return;
> }
Yes but the struct S and some older functions are not particularly
happy about this ...
> Why do you want initialize either only a or only b? That seems odd to
> me, though I'm sure there could be a good reason for it.
Again since this is a sample code so a and b are not only initialized
but used in various dup clones.
c.pro...@googlemail.com
Thanks ; I should try this.
ps : by "data->u = select" I assume you mean "data->u = FALSE"
c.pro...@googlemail.com
> The function no_dup should know about "struct S" as small as possible.
> So it's better to pass not the whole structure S but just a part, such
> as only fields you need.
This is the main point I think.
c.pro...@googlemail.com
Bartc wrote:
> How about a macro:
>
> #define DUPBODY(aorb) \
> data->u = FALSE; \
> data->aorb.i1 = TRUE; \
> data->aorb.i2 = FALSE; \
> return;
>
> void dup1 ( struct S *data ) {
> DUPBODY(a)
> }
>
> void dup2 ( struct S *data ) {
> DUPBODY(b)
> }
Should work also but on lengthy functions this doesn't seem to be the
best.
jameskuyper
c.prog....@googlemail.com wrote:
> James Kuyper wrote:
...
> > void main() is not-portable. Implementations are not required to allow
> > it, and are not required to inform you if you use it and they don't
> > support it.
>
> Hmm ; again the work of an unfortunate typer who wants to avoid
> typing ... arguments.
That reason doesn't hold up very well. Specifying a return type of
'int' requires typing one fewer character than 'void'; while I
wouldn't recommend taking advantage of the fact, inserting a
corresponding 'return 0;' at the end of main() is optional (at least
in C99).
> > One alternative:
> >
> > struct S {
> > U32 u;
> > struct ST ab[2];
> > };
> >
> > void dup1 ( struct S *data, int item ) {
> > data->u = FALSE;
> > data->ab[item].i1 = TRUE;
> > data->ab[item].i2 = FALSE;
> > return;
> > }
>
> Yes but the struct S and some older functions are not particularly
> happy about this ...
If the struct S is fixed, that will limit your options for avoiding
duplicate code.
> > Why do you want initialize either only a or only b? That seems odd to
> > me, though I'm sure there could be a good reason for it.
>
> Again since this is a sample code so a and b are not only initialized
> but used in various dup clones.
That doesn't really address my question. You have one initialization
routine that leaves 'a' uninitialized, which make sense only if 'a'
either won't be used, or will be initialized separately. If 'a' won't
be used, why is it there? If it will be initialized seperately, why
not write a single function for initializing both members? The answers
to those questions will have a lot to do with choosing the best way to
avoid the duplication you're worried about.
nick_keigh...@hotmail.com
> nick_keighley_nos...@hotmail.com wrote:
> > On 8 Dec, 12:12, c.prog....@googlemail.com wrote:
> > or get rid of those dup functions all together
>
> > void init_st (struct ST *st)
> > {
> > st->i1 = TRUE;
> > st->i2 = FALSE;
> > }
>
> > void func (BL select, struct S *data)
> > {
> > data->u = select;
>
> > if (select)
> > init_st (&data->a);
> > else
> > init_st (&data->b);
> > }
>
> > --
> > Nick Keighley
you shouldn't normally quote sigs. The bit after "-- "
> Thanks ; I should try this.
>
> ps : by "data->u = select" I assume you mean "data->u = FALSE
ah, yes. I assumed dup1() and dup2() set data->u to something
different. In fact if they don't do that I don't see how other
parts of your program can tell which of a or b are set correctly.
Have you looked at typedef and union? They might be applicable
to you program
--
Nick Keighley
c.pro...@googlemail.com
jameskuyper wrote:
> If the struct S is fixed, that will limit your options for avoiding
> duplicate code.
Well ; have you seen others posts here ? Some have suggested several
methods already which I implemented one.
> That doesn't really address my question. You have one initialization
> routine that leaves 'a' uninitialized, which make sense only if 'a'
> either won't be used, or will be initialized separately. If 'a' won't
> be used, why is it there?
Once again : This is a sample program and of course "a" is there for a
reason.
> If it will be initialized seperately, why
> not write a single function for initializing both members?
AFAIK this means duplicated code in one function.Isn't it ?
Regards.
c.pro...@googlemail.com
nick_keighley_nos...@hotmail.com wrote:
> In fact if they don't do that I don't see how other
> parts of your program can tell which of a or b are set correctly.
Note : this a sample program so actual code differs from it say that
is more complex. Also I don't get what you mean here ; Can you explain
please ?
> Have you looked at typedef and union? They might be applicable
> to you program
Sure typedef was used in original code.Union ? I thought I am the only
one here who over simplifies.
Regards.
jameskuyper
c.prog....@googlemail.com wrote:
> jameskuyper wrote:
>
> > If the struct S is fixed, that will limit your options for avoiding
> > duplicate code.
>
> Well ; have you seen others posts here ? Some have suggested several
> methods already which I implemented one.
Yes, but from what you've said you can't implement any suggestion that
involves changing the definition of S, and a couple of such
suggestions have been made.
> > That doesn't really address my question. You have one initialization
> > routine that leaves 'a' uninitialized, which make sense only if 'a'
> > either won't be used, or will be initialized separately. If 'a' won't
> > be used, why is it there?
>
> Once again : This is a sample program and of course "a" is there for a
> reason.
Once again: why? Note: I'm not picking on 'a' specifically, the actual
question is why there is both an 'a' and a 'b'. The fact that they are
both there for a reason doesn't mean they are necessarily there for a
good reason. The need for this duplicated code suggests otherwise,
though it doesn't actually prove that this is poor design. Since you
can't change the definition of S, it's a moot point, but knowing
whether or not it was badly designed, and why, could help with
choosing the best way to deal with the problem.
> > If it will be initialized seperately, why
> > not write a single function for initializing both members?
>
> AFAIK this means duplicated code in one function.Isn't it ?
Not necessarily, if the two sub-structures are initialized
identically, a single subroutine containing the code for that
initialization could be executed twice within the single function that
initializes the entire structure. From what you've said, i suspect
that the sub-structure initialization is much more complicated in the
real code than in your simplified example, which could make such an
approach very attractive.
jameskuyper
> nick_keighley_nos...@hotmail.com wrote:
>
> > In fact if they don't do that I don't see how other
> > parts of your program can tell which of a or b are set correctly.
>
> Note : this a sample program so actual code differs from it say that
> is more complex.
We understand that you've simplified the code, and we're quite
grateful for the fact. However, the point is that your simplification
has gone a little bit too far - it fails to capture some of the
characteristics of the actual code that are needed in order to decide
the best way to deal with this issue.
> ... Also I don't get what you mean here ; Can you explain
> please ?
How do other parts of the program know whether they need to look at
a.i1 or b.i1? Do they ever look at both?
> > Have you looked at typedef and union? They might be applicable
> > to you program
>
> Sure typedef was used in original code.Union ? I thought I am the only
> one here who over simplifies.
Depending upon how 'a' and 'b' are used (which is why we're asking
about how they are used), a union might be appropriate if 'a' and 'b'
had different types. Since you've presented them as having the same
type, that's not a useful suggestion. However, if the fact that they
have the same type is an artifact of your simplification, and is not
the case in the actual code, now would be a good time to tell us so.
It's very relevant to deciding upon the best way to deal with this
issue.
c.pro...@googlemail.com
jameskuyper wrote:
> Once again: why? Note: I'm not picking on 'a' specifically, the actual
> question is why there is both an 'a' and a 'b'. The fact that they are
> both there for a reason doesn't mean they are necessarily there for a
> good reason.
I have no more to say on this.
>The need for this duplicated code suggests otherwise,
> though it doesn't actually prove that this is poor design. Since you
> can't change the definition of S, it's a moot point, but knowing
> whether or not it was badly designed, and why, could help with
> choosing the best way to deal with the problem.
Who said there is any need for duplicated code?
Did you notice that the problem is solved (at least as far as I am
concerned )?
> Not necessarily, if the two sub-structures are initialized
> identically, a single subroutine containing the code for that
> initialization could be executed twice within the single function that
> initializes the entire structure. From what you've said, i suspect
> that the sub-structure initialization is much more complicated in the
> real code than in your simplified example, which could make such an
> approach very attractive.
When I choose a method among several alternatives I have my reasons
including : access to original code , code design , ease of use ,
coding methodology ( modularization in my case ) , trying to avoid
bugs , readability and so on. Now when I say I would try something It
is not that others give worse suggestions ; It is MY ( possibly
imperfect yet viable ) selection and that is all !
James Kuyper
...
> Who said there is any need for duplicated code?
You did, in the subject line for this thread, and also in the first
message you posted to start this thread.
> Did you notice that the problem is solved (at least as far as I am
> concerned )?
I noticed one suggestion that you responded to by saying "Thanks; I
should try this". If you're satisfied with that solution, there's
nothing more to talk about. It's a perfectly fine solution for some
circumstances that could be described in the same way that you've
described your situation.
However, that solution might not have been the best one for your
situation. Because of the questions we've asked that you've chosen not
to answer, the best solution for you might not have even been mentioned yet.
c.pro...@googlemail.com
James Kuyper wrote:
> You did, in the subject line for this thread, and also in the first
> message you posted to start this thread.
I assume that I have not , and I would hold my position unless someone
(not limited to you) can 'prove'[1] that I stated the "need" for
duplicated code.
[1] Logic ...
> However, that solution might not have been the best one for your
> situation. Because of the questions we've asked that you've chosen not
> to answer, the best solution for you might not have even been mentioned yet.
Define "best solution" please ; Again anyone is invited.
Regards.
nick_keigh...@hotmail.com
> > In fact if they don't do that I don't see how other
> > parts of your program can tell which of a or b are set correctly.
>
> Note : this a sample program so actual code differs from it say that
> is more complex. Also I don't get what you mean here ; Can you explain
> please ?
no not really. If you explain what you are trying to do I might
be able to suggest a sensible design (or someone else might).
The slightly snotty attitude doesn't help you either.
It seems really odd to me that you initialise only one
of two fields. You also cannot tell which one is initialised
without examining something external to the structure. For
instance your 'select' variable may be global
> > Have you looked at typedef and union? They might be applicable
> > to you program
>
> Sure typedef was used in original code.Union ? I thought I am the only
> one here who over simplifies.
as I said, slightly snotty
--
Nick Keighley
c.pro...@googlemail.com
Hi ,
Some notes :
1.As I already said a model is just a model not matching completely
with the original phenomenon. In the particular case structures have
been there for some time and that means that some functions use them
the way they are. Do they look odd ? yes maybe ; but even now I prefer
them like they are , though this may change. Also for the model there
is initialization but in practice there are many various functions
working with them. The ones who work with sub-structures usually act
as duals on both. So the act of updating is simultaneous in most cases
at least it is what I (suppose to) know now. Maybe the names I chose
were not the best also the functions. And interesting fact is that
some suggestions seemed to be ok since they prevent code duplication
(at least this is what my eyes see). Even it was implemented and
worked and it doesn't seem to contain bugs and appears ( to me ) that
it does not do extra work either. Maybe I can work with that too
(personal preference). Of course there are better ways to be found all
the time.
2.I did not mean to act as one who knows more ; In fact I think
everyone here knows more about programming in general and c
programming in particular. That's what the ID stands for "a c prog
(ramming) fan" not a "c programmer" or anything alike. I was just
trying to say that a specific method seems to solve the problems I
currently recognize in a particular project with someone who has
little knowledge. Note that if I post the actual code and everyone
make suggestions ( there would be so many I am sure ) then I would not
be learning because my knowledge, IQ, experience etc is limited and I
can step not run.
3.Please excuse me everyone because it seems that some got sad with my
sayings here ; I may not post on this group anymore.
Thanks for your patience.
nick_keigh...@hotmail.com
> nick_keighley_nos...@hotmail.com wrote:
> > On 8 Dec, 22:57, c.prog....@googlemail.com wrote:
> > > nick_keighley_nos...@hotmail.com wrote:
>
> > > > In fact if they don't do that I don't see how other
> > > > parts of your program can tell which of a or b are set correctly.
>
> > > Note : this a sample program so actual code differs from it say that
> > > is more complex. Also I don't get what you mean here ; Can you explain
> > > please ?
>
> > no not really. If you explain what you are trying to do I might
> > be able to suggest a sensible design (or someone else might).
> > The slightly snotty attitude doesn't help you either.
I think I was misinterprating what you were trying to.
I thought maybe you wanted to use either structure A or
structure B and using u so you could tell which one.
Hence i thought u should take a different value and
hence my sugestion to consider a union (personally
I hardly ever find a use for union). But you seem
to want to use both a and b even though you initialise
them in different places. As I remarked this seems "odd"
to me. But it's your program and only you can judge
was it right for your problem.
> > It seems really odd to me that you initialise only one
> > of two fields. You also cannot tell which one is initialised
> > without examining something external to the structure. For
> > instance your 'select' variable may be global
>
> > > > Have you looked at typedef and union? They might be applicable
> > > > to you program
>
> > > Sure typedef was used in original code.Union ? I thought I am the only
> > > one here who over simplifies.
>
> > as I said, slightly snotty
>
> > --
> > Nick Keighley
don't quote the sig
> Some notes :
>
> 1.As I already said a model is just a model not matching completely
> with the original phenomenon. In the particular case structures have
> been there for some time and that means that some functions use them
> the way they are. Do they look odd ? yes maybe ; but even now I prefer
> them like they are , though this may change. Also for the model there
> is initialization but in practice there are many various functions
> working with them. The ones who work with sub-structures usually act
> as duals on both.
? they act on both sub-structures?
> So the act of updating is simultaneous in most cases
that seems *less* odd
> at least it is what I (suppose to) know now. Maybe the names I chose
> were not the best also the functions. And interesting fact is that
> some suggestions seemed to be ok since they prevent code duplication
> (at least this is what my eyes see). Even it was implemented and
> worked and it doesn't seem to contain bugs and appears ( to me ) that
> it does not do extra work either. Maybe I can work with that too
> (personal preference). Of course there are better ways to be found all
> the time.
>
> 2.I did not mean to act as one who knows more ;
its just a couple of us were pressing you for more
details about your actual code. This was because the
code you presented looked "odd" and we were not sure if this
reflected
the real code or just your "model".
When I said "a little snotty" I was thinking of things like
"can you prove (ie. logic) that I said that"? (paraphrasing).
> In fact I think
> everyone here knows more about programming in general and c
> programming in particular. That's what the ID stands for "a c prog
> (ramming) fan" not a "c programmer" or anything alike. I was just
> trying to say that a specific method seems to solve the problems I
> currently recognize in a particular project with someone who has
> little knowledge. Note that if I post the actual code and everyone
> make suggestions ( there would be so many I am sure ) then I would not
> be learning because my knowledge, IQ, experience etc is limited and I
> can step not run.
>
> 3.Please excuse me everyone because it seems that some got sad with my
> sayings here ; I may not post on this group anymore.
I didn't mean to be that harsh.
--
Nick Keighley
James Kuyper
> James Kuyper wrote:
>
>> You did, in the subject line for this thread, and also in the first
>> message you posted to start this thread.
>
> I assume that I have not , and I would hold my position unless someone
> (not limited to you) can 'prove'[1] that I stated the "need" for
> duplicated code.
It was not clear from your previous message that it was the "need" which
you were questioning, rather than the "duplicated code". I agree that
you did not assert that it was needed, that you did in fact ask how to
avoid it, which implies that you thought it was not needed.
When I said that the "The need for this duplicated code suggests [that
struct S might have been poorly designed]", I should actually have
spoken in terms of "the apparent need". What I was pointing out was the
fact that you even wanted to ask the question with which you started
this thread is an indication that S might not be well designed - not
proof, just an indication.
>> However, that solution might not have been the best one for your
>> situation. Because of the questions we've asked that you've chosen not
>> to answer, the best solution for you might not have even been mentioned yet.
>
> Define "best solution" please ; Again anyone is invited.
That's context dependent. You know what your context is, we don't, which
makes it your responsibility to define what "best solution" means in
this context. Had you given us enough details about what the situation
is, it's quite possible that someone would have been able to make a
suggestion that you would have been considered better than the solution
you ended up accepting. Without the information you're so coy about,
it's not possible to know what suggestion to offer.
There's nothing odd about having two different structure members of the
same type, nor about wanting to write a routine to initialize
structures. What's pretty peculiar is wanting to initialize either one
or the other of the two members, but not both, in different
circumstances. I can imagine several different bad reasons why someone
might want to do that, but I haven't come up with any good reasons for
doing so. That's not to say that there can't be a good reason, but I
haven't been able to guess what it might be. I shouldn't have to guess -
it should be sufficient to ask you for the reason; but that has proven
to be an ineffective strategy, so far.
Please note that this is not a request for additional information, just
some observations for your consideration.
nick_keigh...@hotmail.com
<snip>
> There's nothing odd about having two different structure members of the
> same type, nor about wanting to write a routine to initialize
> structures. What's pretty peculiar is wanting to initialize either one
> or the other of the two members, but not both, in different
> circumstances. I can imagine several different bad reasons why someone
> might want to do that, but I haven't come up with any good reasons for
> doing so. That's not to say that there can't be a good reason, but I
> haven't been able to guess what it might be. I shouldn't have to guess -
> it should be sufficient to ask you for the reason; but that has proven
> to be an ineffective strategy, so far.
and those of us exposed to Constantine and Yourdon at an
impressionable age tend to hear the whispers of long dead
programmers "...but his span of control doesn't match his
scope of control..." when code like this sees the light of day:
void func ( BL select , struct S *data )
{
if ( select == TRUE ) {
dup1( data );
} else if ( select == FALSE ) {
dup2( data );
} else {
printf("Oops ; don't you like boolean
selection ? :)");
printf("\n");
}
return ;
}
which translated into english means "having a function do
two different things depending on a control flag seems like a
bad idea".
int main (void)
{
struct S data ;
init (&data.a);
init (&data.b);
return 0;
}
getting rid of func() altogether
--
Nick Keighley
"Implementation, maintenance, and modification [costs]
generally will be minimized when each piece of the system
corresponds to exactly one small, well-defined piece of
the problem, and each relationship between a system's
pieces corresponds only to a relationship between pieces
of the problem."
Constantine and Yourdon "Structured Design"