This is difficult for me to put into words, but say I have some function:
a = a + b;
printf("HI THERE");
b = a + c;
(pretend this is 30 lines instead of 3 lines). Now say I want to do the exact same thing again later on, except instead of printing "HI THERE" I print "HO THERE". What I currently do is copy paste all the lines of this function just to change HI THERE to HO THERE. This isn't very elegant for large code blocks.
I know one solution might be like:
adder_function(1);
adder_function(2);
void adder_fuction(int input) {
a = a + b;
printer_function(input);
b = a + c;
}
void printer_function(int input) {
if (input == 1) printf("HI THERE");
if (input == 2) printf("HO THERE");
}
But this seems to be inelegant as well for more complex blocks of code. Any ideas for better solutions?
EDIT: Just to show what it is I'm doing, here's the code in question (you can see that almost nothing changes between the blocks except .input and .output and the printf statements):
found=line.find("INPUTS");
if (found == 0) {
inputfound = true;
found = line.find_first_of(":");
if (found == string::npos) {
printf("BAD NETLIST INPUT DECLARATION\n\r");
exit(1);
}
found = line.find_first_not_of("\n\t ",found+1);
if (found == string::npos) {
printf("BAD NETLIST INPUT DECLARATION\n\r");
exit(1);
}
else {
temp_node_name += line[found];
for (i = found+1; i < line.size(); i++) {
if ( isalnum(line[i]) || isspace(line[i]) ) {
if ( isalnum(line[i]) )
temp_node_name += line[i];
if ( isspace(line[i]) || i == line.size() - 1 ) {
if (!temp_node_name.empty()) {
if (determine_uniqueness(temp_node_name)) {
nodes.push_back(dummy_node);
nodes.at(id_counter).name_in_netlist = temp_node_name;
nodes.at(id_counter).input = true;
temp_node_name.erase();
id_counter++;
}
}
}
}
else {
printf("BAD NETLIST INPUT DECLARATION\n\r");
exit(1);
}
}
}
printf("NETLIST INPUT DECLARATION OK\n\r");
continue;
}
SEPARATE CODE BLOCK THAT IS COPY PASTED
found=line.find("OUTPUTS");
if (found == 0){
outputfound = true;
found = line.find_first_of(":");
if (found == string::npos) {
printf("BAD NETLIST OUTPUT DECLARATION\n\r");
exit(1);
}
found = line.find_first_not_of("\n\t ",found+1);
if (found == string::npos) {
printf("BAD NETLIST OUTPUT DECLARATION\n\r");
exit(1);
}
e开发者_如何学Clse {
temp_node_name += line[found];
for (i = found+1; i < line.size(); i++) {
if ( isalnum(line[i]) || isspace(line[i]) ) {
if ( isalnum(line[i]) )
temp_node_name += line[i];
if ( isspace(line[i]) || i == line.size() - 1 ) {
if (!temp_node_name.empty()) {
if (determine_uniqueness(temp_node_name)) {
nodes.push_back(dummy_node);
nodes.at(id_counter).name_in_netlist = temp_node_name;
**nodes.at(id_counter).output = true;**
temp_node_name.erase();
id_counter++;
}
}
}
}
else {
printf("BAD NETLIST OUTPUT DECLARATION\n\r");
exit(1);
}
}
}
printf("NETLIST OUTPUT DECLARATION OK\n\r");
continue;
}
Making functions is, of course, a great way to reuse code. If you have the same code repeated in multiple places, you can make a function out of it. If you find that you have a lot of functions doing similar things, maybe you can combine them into one function with a few extra parameters. In the example you gave, you could do something like this:
void adder_function(const char * message) {
a = a + b;
printf("%s", message);
b = a + c;
}
int main() {
adder_function("HI THERE");
adder_function("HO THERE");
}
I recommend that you try to avoid adding magic numbers (e.g. 1 and 2) to your source code. It is better to use #defines or avoid that situation altogether, as I have done above.
Following the Single responsibility principle, you should break up your Code in smaller parts. In your example you mix up UI Code (printf) with calculation logic. Avoid this.
Since you talk in plain C Code (no C++) it is hard to give you an example of a more flexible design using common OOP patterns. But a very simple first solution for your problem is to seperate the output stuff from the calcualtion:
void ShowMessage(const char* msg);
int Calculate(int a, int b);
int DoTheHiThereCalculation()
{
int c = Calculate(1, 2);
ShowMessage("Hi THERE");
return c;
}
I'm late to the party so I'll only be addressing your updated code.
First off, I see you printing error messages to stdout
(via printf
) and then calling exit
. Why? You should print error (and perhaps debugging) messages to stderr
(with fprintf(stderr, ...)
or with perror
, though perror
is better if a library routine fails and sets errno
). Also, since this is C++ (not plain C), you might want to use iostream
s instead of *printf
. It's more C++-ish (though, as predominantly a C programmer, I prefer *printf
myself).
Since the only difference between your two blocks of code is the string containing INPUT
in the first and OUTPUT
in the second, I recommend stuffing all of this into a function that, among other necessary parameters (whatever line
is is probably one of them), takes a parameter called bool is_input
. The first time you call this, is_input
should be true
, and the second time, it should be false
.
Then, in both of your blocks of code, you can change the printf
lines to:
fprintf(stderr, "BAD NETLIST %s DECLARATION\n", is_input ? "INPUT" : "OUTPUT");
or
fprintf(stderr, "NETLIST %s DECLARATION OK\n", is_input ? "INPUT" : "OUTPUT");
And then for the member modification write:
(is_input ? nodes[id_counter].input : nodes[id_counter]) = true
(Note that there's no need to use .at
twice in a row with the same index - if it threw the exception once, it won't reach the second call, and if it didn't throw the first time it won't throw the second. It probably won't be a huge speedup, but it's the thought that counts?)
Last, if you need the external variables inputfound
and outputfound
to be set, add a bool &found
parameter to your function, and set it to be true
inside the function. The first time you call it, pass inputfound
, and the second, outputfound
.
Now your two code blocks are identical, and can be put together into one function, and called twice (once with "INPUT"
, and once with "OUTPUT"
). Easy.
In the future, whenever you find yourself writing a block of code that's strikingly similar to another block of code, stop rewriting (or copy-and-pasting). Copy the block of code into a new function all by itself, and replace the original block with a call to the new function. Now you can reuse that block of code as many times as you want.
Since it seems that the various simple solutions presented here did no good, I will propose a sledgehammer (WARNING: use with caution)
There are two design patterns in OO languages to deal with pattern repetition:
- Template Method (not to be mistaken with C++ templates)
- Strategy
In your case, I will recommend trying out the Strategy
pattern, in a Dependency Injection way.
struct Strategy {
virtual void setFoo(Foo& foo, int i) = 0;
virtual void setBar(Bar& bar, int i) = 0;
};
struct A: Strategy {
virtual void setFoo(Foo& foo, int i) { foo.a = i; }
virtual void setBar(Bar& bar, int i) { bar.a = i; }
};
struct B: Strategy {
virtual void setFoo(Foo& foo, int i) { foo.b = i; }
virtual void setBar(Bar& bar, int i) { bar.b = i; }
};
void doSomething(Foo& foo, Bar& bar, Strategy& strategy) {
for (size_t i = 0; i < 10; ++i) {
//
strategy.setFoo(foo, i);
//
for (size_t j = 0; j < 10; ++j) {
//
strategy.setBar(bar, j);
//
}
}
}
You may now invoke doSomething
with either a A
or a B
strategy. Example:
int main(int argc, char* argv[]) {
if (argc == 1) { std::cerr << "usage: %prog STRAT\n"; return 1; }
Foo foo;
Bar bar;
char const strat = argv[1][0];
switch(strat) {
case 'a': case 'A': {
A a;
doSomething(foo, bar, a);
return 0;
}
case 'b': case 'B': {
B b;
doSomething(foo, bar, b);
return 0;
}
default:
std::cerr << "Unknown Strategy: " << strat << ", pick A or B\n";
return 2;
}
}
const char* STR_TABLE[N] =
{
"HI THERE",
"HO THERE",
...
};
printf(STR_TABLE[n]);
According to topic starter comment
. If I want to do something like node_struct.XXXXX = 2; where XXXXXX can be an arbitrary string, how would I go about doing this?
As far as you are using c/c++ you could use macros. (look here)
#define NODE_MEMBER(x) node_struct.##x
......
NODE_MEMBER(a)
NODE_MEMBER(b)
Try passing the things that change (in the 2 functions) as a parameter to a function (this is the one and only function that accepts those different parameters). That way, you can do both the things in a single function.
The easiest case: If it's just parameters of the intermediate operation that change, just pass them into the function as parameters (plain C suffices):
void adder_fuction(const char *text) {
a = a + b;
puts(text);
b = a + c;
}
int main() {
adder_function("HI THERE");
adder_function("HO THERE");
}
Now if it's not, you'll want to pass a function. There are many ways. The first is to pass a function pointer (plain C still suffices):
void adder_function(void (*callback)()) {
a = a + b;
callback();
b = a + c;
}
void callback1() {
puts("HI THERE");
}
void callback2() {
puts("HO THERE");
}
int main() {
adder_function(&callback1);
adder_function(&callback2);
}
You can of course have callback with arguments and return value. E.g.:
void adder_function(int (*callback)(const char *)) {
a = a + b;
c = c + callback("HI THERE");
b = a + c;
}
int callback1(const char *) ...
Now the last option that plain C has is using the preprocessor. It should be considered an ugly hack, though it does have it's place if you are doing something very generic in plain C:
#define define_adder_function(name, code) \
name() { \
a = a + b; \
code; \
b = a + c; \
}
Since C macros are not hygienic, it allows things like:
define_adder_function(adder_function1, c = 22) // NO semicolon here. Using preprocessor does have it's quirks
int main() {
adder_function1();
}
Now C++ allows some extra options:
struct Callback {
virtual void operator()(const char *) = 0;
};
void adder_function(Callback &cb) {
a = a + b;
cb("foo");
b = a + c;
}
int main() {
struct Cb : Callback {
void operator()(const char *arg) { printf("Foo %s\n", arg); }
} cb;
adder_function(cb);
}
If you have C++11 std::function
, either from C++0x/C++11 support in compiler, from C++ TR1 (as std::tr1::function
) or from Boost (as boost::function
), you can avoid declaring the virtual base class manually.
#include <functional>
void adder_function(std::function<void (const char *)> cb) {
a = a + b;
cb("foo");
b = a + c;
}
void callback1(const char *arg) { puts(arg); }
int main() {
struct { // You can have local structures, even with methods, but not local functions
void operator()(const char *arg) { printf("Foo %s\n", arg); }
} callback2;
adder_function(&callback1); // std::function should accept either function or functor
adder_function(callback2);
}
Other way to avoid virtual base class and this time also virtual dispatch (at the cost of the code being compiled twice) is (works in C++98):
template <typename Callback>
void adder_function(Callback cb) {
a = a + b;
cb("foo");
b = a + c;
}
void callback1(const char *arg) { puts(arg); }
int main() {
struct Callback2 { // Anonymous types can't be used in templates before C++11.
void operator()(const char *arg) { printf("Foo %s\n", arg); }
} callback2;
adder_function(&callback1); // template also accepts either function pointer or functor
adder_function(callback2);
}
Last if you have C++11 support in compiler (no way to do this in library, it's new syntax), you can combine the last two with the new lambda syntax:
int main() {
adder_function([](const char *arg) { puts(arg) });
}
If you don't mind, I would refactor it to break the responsibility. I would separate the parsing from the program flow that decides what to do when the input is parsed. In the enclosing function you will just call the helper, and based on the return (or exception) determine the error to show, if an error is encountered, or the variables to set. This will remove duplication from the parsing of the netlist (parsing is exactly the same for input and output, the only change is what the error message to be shown is and whether to set the input/output flag. Setting of the flag can be done externally, if your parser returns a sequence of names, then the outer code can construct the nodes and set the flag according to the stage of the process in which you are.
精彩评论