29

Suppose I have a segment of code to connect to internet and show connection results like it:

HttpRequest* httpRequest=new HttpRequest();
httpRequest->setUrl("(some domain .com)");
httpRequest->setRequestType(HttpRequest::Type::POST);
httpRequest->setRequestData("(something like name=?&age=30&...)");
httpRequest->setResponseCallback([=](HttpClient* client, HttpResponse* response){
    string responseString=response->getResponseDataString();
        if(response->getErrorCode()!=200){
            if(response->getErrorCode()==404){
                Alert* alert=new Alert();
                alert->setFontSize(30);
                alert->setFontColor(255,255,255);
                alert->setPosition(Screen.MIDDLE);
                alert->show("Connection Error","Not Found");
            }else if((some other different cases)){
                (some other alert)
            }else
                Alert* alert=new Alert();
                alert->setFontSize(30);
                alert->setPosition(Screen.MIDDLE);
                alert->setFontColor(255,255,255);
                alert->show("Connection Error","unknown error");
            }
        }else{
            (other handle methods depend on different URL)
        }
}

the code is long, and it is commonly used, but the code above does not require any extra things such as custom function and class (HttpRequest and Alert are both provided by framework by default), and although the code segment is long, it is straightforward and not complex (it is long just because there are bundles of settings such as url, font size...), and the code segment has little variations among class (e.g.: url,request data, error code handle cases, normal handle cases...)

My question is, is it acceptable to copy and paste long but straightforward code instead of wrapping them in a function to reduce the dependency of code?

14
  • 89
    Imagine you have a bug in that code, such as not freeing objects you allocate. (Does your framework free the Alert objects?) Now imagine you have to find every copied instance of this code to fix the bug. Now imagine it's not you who has to do it, but a crazy axe murderer who knows you were the one who created all these copies in the first place. Commented Jan 25, 2016 at 10:31
  • 8
    And BTW, mixing networking and error display in one place is already a big no-no, IMHO.
    – sleske
    Commented Jan 25, 2016 at 11:33
  • 11
    No. Never. Entirely unacceptable. If you were on my project, you would no longer be on my project and you'd be on a training program or PIP.
    – nhgrif
    Commented Jan 25, 2016 at 13:30
  • 10
    And along comes the supervisor that says "This alert box in the middle of my screen is an absolute terror. I am watching cat jifs and the pop-up is blocking my view every time it shows up. Please move it to the upper-right." 3 weeks later "What the heck did you do?! I can no longer close my cat jifs because YOUR pop-up is covering the X in the upper-right, fix it."
    – MonkeyZeus
    Commented Jan 25, 2016 at 14:38
  • 11
    I think everyone here seems to think this is a bad idea. But to turn the question around, why would you NOT put this code in a separate class or function? Commented Jan 25, 2016 at 16:12

8 Answers 8

87

You need to consider the cost of change. What if you wanted to change how connections are made? How easy would it be? If you have a lot of duplicated code, then finding all the places that need changing could be quite time consuming and error prone.

You also need to consider clarity. Most likely, having to look at 30 lines of code isn't going to be as easy to understand as a single call to a "connectToInternet" function. How much time is going to be lost trying to understand the code when new functionality needs to be added?

There are certain rare cases where duplication isn't a problem. For example, if you are doing an experiment and the code is going to be thrown away at the end of the day. But in general, the cost of duplication outweighs the small time savings of not having to pull the code out into a separate function.

See also https://softwareengineering.stackexchange.com/a/103235/63172

2
  • 19
    ...and who knows if these 30 lines are really the same as those other ones that you looked at previously or if somebody switched to a different port or IP address in his/her copy for whatever reason. The implicit assumption that "any bunch of somewhat 30 lines starting with the HttpRequest are all the same" is an easy to make fallacy.
    – null
    Commented Jan 25, 2016 at 21:28
  • @null Excellent point. I once worked on code where there were database connection hookups copied and pasted all over, but some had subtle differences in the setup. I had no idea if these were important, deliberate changes, or just random differences
    – user949300
    Commented Jan 26, 2016 at 21:57
54

No.

In fact, even your "simple" code should be split into smaller parts. At least two.

One to make the connection and handle the normal 200 response. For example, what if you change from a POST to a PUT in some cases? What if you are making zillions of these connections and need some multi-threading or connection-pooling? Having the code in one single place, with an argument for the method, will make this much easier

Similarly, another to handle errors. For example, if you change the color or font size of the alert. Or you are having issues with intermittent connections and want to log the errors.

3
18

is it acceptable to copy and paste ...

No.

For me, the deciding argument is this one:

... it is commonly used ...

If you use a piece of code in more than one place then, when it changes, you have to change it in more than one place or you start to get inconsistencies - "odd things" start to happen (i.e. you introduce Bugs).

it is straightforward and not complex ...

And so should be all the easier to refactor into a function.

... there are bundles of settings such as url, font size ...

And what do users love to change? Fonts, font sizes, colours, etc., etc.

Now; in how many places will you have to change that same piece of code to get them all the same colour/font/size again? (Recommended answer: just one).

... the code segment has little variations among class (e.g.: url,request data, error code handle cases, normal handle cases...)

Variation => function parameter(s).

1
  • "And what do users love to change? Fonts, font sizes, colours, etc" this is so deal on. Do you really want to change that in dozens of locations?
    – Dan
    Commented Jan 26, 2016 at 17:05
8

This doesn't really have anything to do with copy and paste. If you take code from elsewhere, at the second you take the code it's your code and your responsibility, so whether it's copied or written completely by yourself doesn't make a difference.

In your alerts you make some design decisions. Most likely similar design decisions should be made for all alerts. So it's likely that you should have a method somewhere "ShowAlertInAStyleSuitableForMyApplication" or maybe a bit shorter, and that should be called.

You will have lots of http requests with similar error handling. You should likely not duplicate the error handling again and again and again but extract common error handling. Especially if your error handling gets a bit more elaborate (what about timeout errors, 401, and so on).

6

Duplication is OK in some circumstances. But not in this one. That method is too complex. There is a lower limit, when duplication is easier than "factoring out" a method.

For example:

def add(a, b)
    return a + b
end

is stupid, just do a + b.

But when you get just a little, tiny bit more complex, then you're usually way over the line.

foo.a + foo.b

should become

foo.total
def foo
    ...
    def total
        return self.a + self.b
    end
end

In your case, I see four "methods". Probably in different classes. One to make the request, one to get the response, one to display errors, and some kind of call back to be called after the response returns to process the response. I personally would likely add a "wrapper" of sorts on top of that as well to make the calls easier.

In the end, to make a web request I would want a call to look something like:

Web.Post(URI, Params, ResponseHandler);

That line is what I would have all over my code. Then when I needed to make changes to "how I get stuff" I could quickly do so, with much less effort.

This also keeps the code DRY and helps with SRP.

0

In a project of any size/complexity, I want to be able to find code when I need it for the following purposes:

  1. Fix it when it's broke
  2. Change the functionality
  3. Reuse it.

Wouldn't it be lovely, to either join a project in progress or continue to work on a project for several years and when a new request to "connect to internet and show connection results" was in some easy to find location due to having a good design instead of relying on doing a search throughout the code for an httprequest? It's probably easier to find with Google anyway.

Don't worry, I'm the new guy and I'll refactor this block of code because I'm so upset for joining this clueless team with the awful code base or I'm now under a lot of pressure like the rest of you and will just copy and paste it. At least it will keep the boss off my back. Then when the project is truly identified as a disaster, I'll be the first to recommend we rewrite it by copying and pasting the version in the latest and greatest framework that none of us understand.

0

A class and/or a function is better, at least in my opinion. For once, it makes the file smaller which is a very heavy gain if you deal with web applications or applications for devices with little storage (IoT, older phones, etc.)

And obviously the best point is that if you have something to change due to new protocols, etc. you just change the content of the function and not countless of times you put this function somewhere which might even be in different files making them even harder to find and change.

I wrote a whole SQL interpreter so I could switch better from MySQL to MySQLi in PHP, because I just have to change my interpreter and everyting is working, although that is an a bit of an extreme example.

-1

To decide whether a code should be duplicated or moved to a function which is called twice, try to determine which is more probable:

  1. It will be necessary to change both uses of the code in the same way.

  2. It will be necessary to change at least one use of the code so that they differ.

In the first case, it will likely be better to have one function handle both usages; in the latter case, it will be likely be better to have separate code for the two usages.

In deciding whether a piece of code which will be used once should be written in-line or pulled out into another function, figure out how one would fully describe the function's required behavior. If a complete and accurate description of the function's required behavior would be as long or longer than the code itself, moving the code into a separate function may make things harder to understand rather than easier. It may still be worth doing if there's a high likelihood that a second caller will need to use the same function, and any future changes to the function will need to affect both callers, but in the absence of such consideration, readability would favor splitting things to the level where the code and a description of its required behavior would be about equally long.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.