2

I recently switched jobs to a new company which develops a .NET framework based product which has been on the market for like 15 years already. Some of the engineers are in this project almost from the beginning, while most of them are in the company for a shorter period of time.

I was used to write lots of tool / extension classes which would encapsulate small pieces of often-used code, such as

public static void DeleteByColumn(this SqlConnection conn, string tableName, 
                                  string columnName, object value) {

   var cmd=new SqlCommand($"DELETE FROM {tableName} WHERE {columName}=@value",conn);
   cmd.Parameters.AddWithValue("@value",value);
   cmd.ExecuteNonQuery();

}

(This example is only for illustrative purposes, we [mostly] don't use direct SQL commands in this project).

The senior engineers however mostly seem to think that we shouldn't encapsulate such code fragments consisting of just two or three lines in tool methods. Instead, it'd be a better idea to just put the respective code right where it is executed. They argue that the code is "clearer" this way, since when reading the code, you don't have to go forth and back between methods.

What do you people think of this approach?

Urs Meili
  • 129
  • 2

1 Answers1

10

Save me from published junk drawers.

If you want to put small, potentially often used piece of code in a shared library then that library needs a good organizing principle. That is, it needs a good name. One that makes clear what goes in it and what doesn't. I do not need a dumping ground that represents nothing other then stuff you decided to extract. Put stuff together that has a good reason to be together.

I'm concerned you may be following the brain dead form of DRY. This bad habit has you scour the code base for similar looking code to factor out without ever considering if the duplication exists for a good reason.

DRY encourages you not to spread a design decision around by lazily copy and pasting. But it never asked you to ensure that no two lines of code look the same. Duplication is most appropriate when it represents a different idea, one that might change independently, that simply happens to look the same.

If you're carefully considering that when you target code for extraction I'm still with you.

The senior engineers however mostly seem to think that we shouldn't encapsulate such code fragments consisting of just two or three lines in tool methods.

I have to disagree with this. Little methods with damn good names are a god send.

Instead, it'd be a better idea to just put the respective code right where it is executed. They argue that the code is "clearer" this way, since when reading the code, you don't have to go forth and back between methods.

This complaint is a valid concern. This seems to fear that you're going to take them on a jumping fest when they read the code. You just might, even if you've chosen code that really should be extracted.

The way you avoid that is with good names, good abstraction, and well proven code. Do that and they wont be looking inside your method. They'll trust it the way they trust Console.WriteLine("Hello World!");. Make them look inside over and over and of course they'll hate it.

A good name means when they look inside they'll find what they expect. A good abstraction means it works how they expect. Looking inside and looking at documentation should teach them nothing they didn't expect.

Do that and there wont be any going 'forth and back between methods'. They'll know what you mean when you call it.

So now the question is, does DeleteByColumn(conn, "students", "grade", "F"); live up to that?

I must confess I had to look inside this method before I felt like I understood what it does. I'm only guessing that I'm using it correctly. But I do think DeleteByColumn() is a name I could get used to.

Keep in mind though any library you put this in should be focused on the kinds of things this is focused on. I'd tend to create a persistence adapter where I dump logic that knows how to talk to the DB so the rest of the application doesn't have to know that.

If you found say some oft used string manipulation code that has nothing to do with the DB I would not group it with this.

A shared library is a significant boundary that often reaches across applications. If you're proposing that this code is ready to be that then keep in mind you're biting off a lot. If you want to write a DB access library you're up against guys like this: JOOQ. That's a lot to live up to.

It's far easier to create code that is meant for your application rather then every application. With a DB adapter you're only on the hook for the features your app really needs. No need to be all things to every DB user. Reusability is a nice idea but don't kill yourself creating things no one has asked for. Just keep in mind that your application can change. Be ready to change.

Show that you're willing temper your plans so it doesn't seem like you're just rewriting because it's easier than reading. Your team may be more welcoming of your changes. If they aren't here's my advice:

It's perfectly OK to assist yourself, when trying to understand code, by extracting code into little functions to push nosy details away so you can see what is really going on at a glance. I do it all the time. Sometimes after I sit with someone I find out that what I created is good enough to keep. Sometimes it's not. When it's not I simply don't check it in. I toss it in my personal junk drawer.

candied_orange
  • 102,279
  • 24
  • 197
  • 315
  • thanks @candied_orange for the detailed answer. I especially like the last section. – Urs Meili Dec 06 '19 at 21:31
  • Actually my question goes further. There is the DRY principle, as you also mentioned it in your comment. Let's say I write some code where I come across having to delete many records in a database which have value X in column Y. Should I think "I'm sure that somebody else will have a to do a similar thing in the near future" and already create a tool method "DeleteByColumn" which does this in a generic way (altough I am _currently_ the only person doing that), or do I leave this task to the next person - hoping that he will find my solution, refactor it out and turn it into a generic method? – Urs Meili Dec 06 '19 at 21:39
  • In my experience, most fellow developers do _not_ search the entire code base for similar existing implementations (and care to refactor it), but just implement the same stuff again, and again, so in the end we have lots of similar code doing "almost" the same stuff. So I tend to write those tool methods already in "advance", altough it violates the KISS principle. – Urs Meili Dec 06 '19 at 21:43
  • Rather than focus on repetition focus on keeping methods and classes small. The limiting factor here isn't repetition. It's your vocabulary. Because lots of little methods are only nice when they have good names. Give them bad names and I'd rather you stick me with a 300 line nightmare function. – candied_orange Dec 06 '19 at 21:44
  • Actually writing them ahead of any need doesn't violate KISS. It violates YAGNI. You're far better off learning to refactor existing code then trying to predict the future. If you have that super power stop writing code and manage my stock portfolio. Look up the [rule of three](https://en.m.wikipedia.org/wiki/Rule_of_three_(computer_programming)) – candied_orange Dec 06 '19 at 21:47
  • Let's say I write some potentially reusable code today and don't refactor it out because of YAGNI. Two months away from now, some other developer needs to write similar code. He has no chance to find my piece of code inside our code base (which is > 200 projects) if it is buried somehow deep in an arbitrary project. He will write it again, and again not in a tool method because he isn't even aware the code already exists, and because of YAGNI. If I would have written a tool method instead in the first place, he would have found the method and not implemented it again. – Urs Meili Dec 06 '19 at 21:58
  • 1
    Extract it into a junk drawer and the other dev has no chance of finding it. Put it in a DB adapter package with other DB code under a good name and they just might find it. Leave it un-extracted and the way they find it is by imitating the code you left it in and changing only what needs to change. It's called copy and mutate. It's not the best, it's not fancy, but it works and it happens a lot. Which means that's what you have to be easier than if you want people to use your stuff. – candied_orange Dec 06 '19 at 22:04
  • "Leave it un-extracted and the way they find it is by imitating the code you left it in and changing only what needs to change" - What do you mean by this? For me it would be "leave it un-extracted and they don't find it at all, but code it again from scratch - or copy & paste it from the internet" – Urs Meili Dec 07 '19 at 18:09
  • If they have to copy from the internet something that is already in your code base then your project structure needs serious work. I know google has an awesome search algorithm but I still look for local examples to follow before I hunt the internet. – candied_orange Dec 07 '19 at 19:00
  • Excellent answer - hasty DRY usually results in obscure shared libraries with no ownership and diverging needs from consumers. It is *so hard* to argue against this mindset of "banish every scent of duplication." Just don't. – Ant P Dec 11 '19 at 07:25
  • @AntP the best argument I have against mindless duplication eradication is: it's ok for both x and y to equal 1. Duplicating values or implementations is not the sin. The sin is duplicating an idea and so spreading a single design decision around. It's better if changing an idea can happen in one place. But it should only be one idea. – candied_orange Dec 11 '19 at 13:13