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?

Improve this question
0

1 Answer 1

Reset to default
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.

Improve this answer
11
  • thanks @candied_orange for the detailed answer. I especially like the last section.
    – Urs Meili
    Dec 6, 2019 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 6, 2019 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 6, 2019 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 6, 2019 at 21:44
  • 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 6, 2019 at 22:04

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