8 ms·
I am firmly in the DRY camp with a very important caveats: 1- Do no generalize so you can factor out like code too early. The first time just write it. The sec
by jnordwick 10y ago
I am firmly in the DRY camp with a very important caveats:
1- Do no generalize so you can factor out like code too early. The first time just write it. The second time write it again even if only minimally different. The third time is a judgement call. By the fourth time you see the proper abstraction, something that might have been much different if you were to have refactored early on.
Corollary to #1- Don't run ahead of yourself making all these placeholder functions, interfaces, and abstract classes with the expectation that you would have had to refactor this way later. Unless you done versions of your project a dozen times (even then, sometimes I don't even do it), you are most likely going to be wrong.
2- Don't refactor out single lines unless there a lot going on on that line or you have the experience to know (don't lie to yourself either) that this will be an important and growing line.
3- Sometimes performance can trump. And sometimes you do know this ahead of time, especially if performance is correctness as it is on some projects with very tight constraints.
Besides that. I freaking hate seeing similar code everywhere, blowing up my instruction cache, making changes difficult. Please stop it.
- InclinedPlane 10y agoI disagree with your point 2 very strongly. A lot of people have a mental block when it comes to 1-line functions, and they really shouldn't, they can often be very helpful for a lot of reasons. Generally, the most useful tool in refactoring is to use extract method relentlessly, even into 1-line functions. More often than not, a well named function with well named parameters is one of the absolute best and most reliable ways to make your code as close to "self-documenting" as possible. But there are additional advantages as well. Spinning things off into function calls often makes refactoring easier. Because it discourages bad habits like local variable reuse (using variables as scratch pads) and excess mutable state. It also encourages a more functional style, which has a lot of benefits especially in multi-threading but also in the ability to reason about and determine the correctness of code. It can also make it a lot easier to see when you need to spin off functionality into other classes/modules. For example, if you have a member field and you have a ton of little one line snippets everywhere that anything happens with that field, that usually means you need to extract it out into a class and have those one-line snippets be various methods on that class. When all that code is inline, this can be harder to see. Additionally, there are lots of mental blocks that happen when you have inline "one liners". There's a bias to avoid "cluttering them up", even when it's necessary. For example, when you need to add error checking and error handling. When you've spun out the code into a method call, you tend not to have that block, you just add the error handling code because it's trivial, and going from a 1-line method to a 5-line method isn't a big deal. But bloating up another function by taking a sweet one-liner and replacing it with 5-lines of code can seem like clutter, so often times you just have an aversion to doing so. Additionally, sometimes those "sweet one-liners" can be more readable when they're broken up into multiple lines, and this is again something that is trivial and straightforward when it's spun out into its own method but will see resistance when the code is inline. This is true even if the code remains a single statement broken up into multiple lines to enhance readability. A lot of the time these patterns aren't obvious. Because the end result of spinning off one-liners is often not one-line method definitions, it's short methods that are a couple lines with several lines of extra error detection/handling. Or it's separate utility classes to encapsulate handling of special member fields. The idea of ensuring that you always have "meaty" functions is unhelpful, what's important is thinking about things from a perspective of abstraction, modularity, readability, and correctness.
- jnordwick 10y agoIf you have a one-line function that is used one place (or even a few places) and it doesn't mean it is easier to understand just because you made it look like English. More likely you just obfuscated it: update_string_with_value(string,val) is NOT easier to undestand than string.append(val). I really and truly hate code where people think they need to take the programming out of programming. This goes all the way back to when they learned it was a good idea to comment their code "next = ++i // increment index then assign". > Spinning things off into function calls often makes refactoring easier. You're breaking a rule in keeping things simple. Don't anticipate. You are refactoring ahead of the refactoring, most likely making the wrong decision in the process. And you're also coding defensively against your fellow developers. This tends to be a problem in low- to average-skill workplaces. Instead why you just follow cheakins and show your junior developers what they are doing wrong? That is how I learned to program from some amazing developers. It wasn't from them coding defensively against me, but from the tap on the shoulder I got asking me why I did something on my last checkin. And I'm not advocating "meaty" functions. They should be divided on logical pieces of functionality that aren't so small where every line qualifies, but not so large as to span more than a page or more do a single thing. Before they even hit the page mark, most likely other reasons to will dominate though like the single responsibility principle or something else. Me and you might just come from different worlds.
- MaulingMonkey 10y ago> If you have a one-line function that is used one place (or even a few places) and it doesn't mean it is easier to understand just because you made it look like English. More likely you just obfuscated it: update_string_with_value(string,val) is NOT easier to understand than string.append(val). We agree your sample has been obfuscated. /// <summary> /// Case Insensitive IRC Name /// </summary> public struct CIIrcName { private string Data; public static implicit operator CIIrcName( string s ) { return new CIIrcName() { Data=s }; } public static implicit operator string( CIIrcName n ) { return n.Data; } public string ToLower() { return (Data??"").ToLowerInvariant().Replace('{','[').Replace('}',']').Replace('|','\\'); } // Silly scandinavians :< public static bool operator==( CIIrcName lhs, CIIrcName rhs ) { return lhs.ToLower() == rhs.ToLower(); } public static bool operator!=( CIIrcName lhs, CIIrcName rhs ) { return lhs.ToLower() != rhs.ToLower(); } public override bool Equals( object obj ) { return obj is CIIrcName && (CIIrcName)obj == this; } public override int GetHashCode() { return ToLower().GetHashCode(); } public override string ToString() { return Data; } } Every single function of this is a one-liner. Are any of them obfuscated? (IRC servers sometimes talk about the same user or channel in multiple cases, this was a quick drop-in to prevent these from being mistaken as different users/channels by my IRC client. The "Silly scandinavians" comment references the fact that, yes, IRC thinks '[' is lowercase '{'. And yes, I've seen that matter in practice.) Most of the changes I'd make here have nothing to do with method length: 1) Conversion (at least back to string) should probably be made explicit at this point (was originally implicit for ease-of-use when replacing string s) 2) ToLower could be made private (only used inside the type) and have the comment replaced with the above blurb (instead of being a comment to myself that only I'll properly decode.) 3) Rename CIIrcName to CaseInsensitiveIrcName? 4) Rename Data to OriginalCaseName? > And you're also coding defensively against your fellow developers. This tends to be a problem in low- to average-skill workplaces. My experience has been just the opposite. Low/average skill tends to mean less asserts, less static analysis, ignored warnings instead of warnings-as-errors, no unit tests, no thread safety annotations... forget my coworkers - I add these things to code defensively against future-me, and I've found it super effective on larger codebases. > This goes all the way back to when they learned it was a good idea to comment their code "next = ++i // increment index then assign". We agree this is bad too. A different perspective though - those are perfectly reasonable comments when you're new enough to programming that you don't remember what "++i" does! But it's something to grow out of and remove.
- cle 10y agoIn my experience, there's a costly dimension to DRY that doesn't get enough love: dependencies. DRY incurs a significant dependency cost. This has been painful for me, since I work on distributed systems a lot. When you have an ardent follower of "DRY" who refactors a common utility so that many subsystems of a distributed system have a code dependency on the same piece of code, then changing that piece of code can become very costly. When that cost gets too high (e.g. requires coordinated deployments and lots of cat herding with owners of other systems), then the DRY has just painted you into a corner where the cost of changing your code is higher than it's worth, and you end up with a rotting code base because the cost of change is too high. Please, please factor that cost into your decision to wield DRY in distributed/service-oriented systems! Code dependencies have diminishing returns in increasingly distributed systems...try to find a way to encode the common functionality as data instead of code and you'll be way better off.
- icebraining 10y agoIn that case, can't you then just un-DRY (hydrate?) the code by branching it for each subsystem? Then each team can keep its own pace, and still cherry-pick improvements from each other, which would be difficult to do if they were simply pieces of code copied all around the codebase.
- taejavu 10y agoThe opposite of DRY is WET (Write Everything Twice) ;)
- dpark 10y agoYes and no. Once a half dozen components take a common dependency, that dependency tends to grow large and complex. Where each component needed a simple string templating class (for example), the aggregate dependency ends up doing too much and is overengineered. It has multiple layers of abstraction to make the various string manipulation classes "look the same" and therefore justify DRY refactoring. So yes, you can just "rehydrate" the dependency into each component, but you often end up needing a fair bit of work for each component to untangle the mess and create the minimal dependency that each component actually needs. If you just want to copy the dependency directly, then sure, that's typically pretty easy. But that's also typically a pretty terrible outcome as now you have tons of duplicated code and modifying the dependency implies duplicate work, and worse, duplicate pointless work as you must maintain the pieces your component doesn't even use.
- mozumder 10y agoI'm firmly in the anti-DRY camp since it causes everything to be slow, including coding speeds. You spend far too much time trying to figure out something new, when you could have just cut-and-pasted the answer from the previous problem. Repeat yourself. It's good for you, and makes you code faster. You can refactor out later if code becomes bloated.
- meddlepal 10y agoAnd then one day you realize there are bugs duplicated all over the place and you pray that nobody hits them. I'm not religious about DRY, but it's not a bad thing when kept in the toolbox with KISS and YAGNI.
- mozumder 10y ago> And then one day you realize there are bugs duplicated all over the place and you pray that nobody hits them. If you can copy-paste repeated code, you can copy-paste debugging as well. Duplicate bugs are a single bug. Obviously not all DRY is bad.
- meddlepal 10y agoAnd then I can explain to my boss why I'm working on stuff I'm not supposed to when I could have just wrapped it up once in a library and fixed it once on all those projects. So now not only have I created a bug, duplicated it N times, but my boss thinks I'm an idiot and wasting time for not applying DRY and good engineering hygiene.
- deleted 10y ago[deleted]
- rsyntax 10y ago> Repeat yourself. It's good for you, and makes you code faster. Maintainable goes out the window...
- richdougherty 10y agoPoint 1 is how I do it. I think it balances the trade off between too-early abstraction and too-much duplication. The only thing I'd add is that you should think about including some comments when you duplicate code so that people realise how the different bits of code relate to each other. If you've thought of a potential way to abstract the code, but didn't proceed with it for some reason, you can add a note describing how it could be done. This helps new people understand why they're seeing duplication and how they can factor the code if they choose to.
- dllthomas 10y ago> Don't refactor out single lines unless there a lot going on on that line or you have the experience to know (don't lie to yourself either) that this will be an important and growing line. On the contrary, you might want to factor out single tokens, if they are likely to change together. You can view concerns over magic numbers as a special case of this. If you have "10" in 10 places, you probably want it in a constant with a name. But perhaps you want it in several constants with different names - does the "10" here represent the same piece of knowledge as the "10" there? There's no reason the same doesn't apply if we want to perform the same operation in 10 places. Those that represent the same piece of knowledge ("this is how we render a text box", "we have capacity for 4 widgets", "we compute the total price of a basket like this") should likely be consolidated. Those that represent different pieces of knowledge should probably not.
- InclinedPlane 10y agoThis is another good point. Often times you want to make sure that "magic numbers" are replaced by well named constants or configurable values, even if they're only used once. Most importantly to make sure to differentiate them. It becomes less obvious what those numbers do when they're just numbers, and it becomes much less obvious that two things using the same number in close code proximity just have the same values by coincidence. This can be true for logic as well. If you have a very specific conditional it can help to put it into a function so that it's purpose is clear, especially so if it's a piece of business logic that might change. Interestingly this can be "anti-DRY" as well because you might have the same logic duplicated in different places, but the fact that they are the same is a coincidence, and you want them to change depending on their own requirements.
- dllthomas 10y ago> Interestingly this can be "anti-DRY" Which is funny, because as originally coined, it is exactly "DRY". "Every piece of knowledge must have a single, unambiguous, authoritative representation within a system". https://en.wikipedia.org/wiki/Don't_repeat_yourself https://en.wikipedia.org/wiki/Don't_repeat_yourself
- BurningFrog 10y agoYour first point is known as the Rule Of Three: https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming) https://en.wikipedia.org/wiki/Rule_of_three_(computer_progra...
- jnordwick 10y agoYeah! I always love seeing when my ideas align with one of those I trust, and Fowler often gets my nod of approval. A month or so ago I was caught agreeing with Gil Tene, so I'm on a roll.
- adrianratnapala 10y agoI agree with all three of your caveats. But I want to emphasise and re-phrase #1 Premature abstraction is as bad as premature optimisation. In fact their effect on the code base is rather similar: complexity and obscurity in the name of some theory about a future benefit.
- aptwebapps 10y agoI agree. One aspect that catches me out sometimes is that often the first iteration of something is in such obvious need of abstraction that it's hard to let it go for the time being. The reason to do so, of course, is that you don't know what the proper abstraction is yet, but that doesn't make it pleasant.
- lowbloodsugar 10y agoYour "Corollary to #1" is YAGNI. I agree with 1. My advice to my teams for #1 is identical. The complaint about readability in TFA is inexcusable. If I do not have a system for visualizing (or otherwise comprehending) the complexity of my system, then there's not much hope. Dependency graphs and "Find references" in my IDE help. If the only tool in the toolbox is looking at the code that fits in a vim console, then I'm going to have problems writing large applications. I am not that kind of genius. I have met and worked with that kind of genius (i.e. holding all the code in their head) and it turned out that around 1998 codebases hit a size that even those geniuses hit a wall, with project-ending consequences.
- benji-york 10y agoI am interested in what tools you would suggest for said visualizing/comprehending.
- rsyntax 10y agoI would add to NOT practice DRY in your test cases/code. More often than not this will get you in trouble.