Duplicate code detected

PleegWat

Automated tooling is accusing me of duplicate code. Java.

And I agree with it. Textually, the code is duplicate. Actually, it's a chain of invocations setting the same properties on different builder types.

while(rs.next()) {
    switch(Config.ConfigType.fromString(rs.getString("CONFIG_TYPE"))) {
        case FooConfig:
            results.add(
                    FooConfigSummary.builder()
                            .id(rs.getString("ID"))
                            /* Enough shared fields to trigger duplicate code detection */
                            .fooValue(rs.getString("FOO"))
                            .build()
            break;
        case BarConfig:
             /* Etc */
    }
}

The builders are autogenerated. There is no inheritance relation or shared interface between FooConfig.Builder.id() and BarConfig.Builder.id() other than the fact FooConfig and BarConfig both extend Config (which does not have a nested Builder class).

Of course I could probably turn the duplicate detection off. But that feels like giving up and I'm gonna end up having dozens of these in a few years so I'd rather nip any clutter in the bud.

BernieTheBernie

@PleegWat said in Duplicate code detected:

turn the duplicate detection off

DOes your tool allow you to turn it off for a small section of code only?
With Resharper, there were some special kinds of comments for enabling/disabling specific warnings.

Jaime

@PleegWat What tool is this? Besides turning it off, sometimes a little insight into the actual rule will give you some ideas of exactly what the rule is trying to prevent.

Personally, the idea of grabbing the id from rs and setting it on a new foo, and doing this for every property, by a reference with a string that susceptible to typos, seems to be a lot of risky code. I know the builder pattern is all the rage nowadays, but a good old FooConfig.fromSomething(rs) would move all of the duplicated code out of the switch and probably get rid of your duplicate detection. If that code is really repeated, as in get same gets from rs for every section of the switch, then I would deserialize those in one place.

PleegWat

@Jaime FooConfig is generated code, and I have no control over the generator at all nor choice on whether or not to use it. I did move the body of the case statement to a helper function (less indentation → longer lines → fewer lines after autoformatting → still above the threshold → ☹). A separate model converter class might dodge the scanner, but I'd avoid that. I haven't looked into exactly how to turn off the warning yet; I've previously suppressed warnings using the @SuppressWarning annotation.

And it was strings or offsets - rs is ResultSet from a DB query.

Jaime

@PleegWat said in Duplicate code detected:

And it was strings or offsets - rs is ResultSet from a DB query.

Or an ORM. Or some sort of micro-ORM that at least analyzes the query and generates the boilerplate code for you.

One of the things I tell all of my programmers - if you are still using a database access methodology that requires you to pull data out of a result set by index or column name, then you need to look at some of the other code we have to figure out an alternative. Generated code combined with a decent language will 100% eliminate spelling errors, data type mismatches, and nullability problems.

dkf

@Jaime said in Duplicate code detected:

@PleegWat said in Duplicate code detected:

And it was strings or offsets - rs is ResultSet from a DB query.

Or an ORM. Or some sort of micro-ORM that at least analyzes the query and generates the boilerplate code for you.

One of the things I tell all of my programmers - if you are still using a database access methodology that requires you to pull data out of a result set by index or column name, then you need to look at some of the other code we have to figure out an alternative. Generated code combined with a decent language will 100% eliminate spelling errors, data type mismatches, and nullability problems.

It's more important to get away from doing things by index, as that is actively fragile; binding by name is significantly more robust, in that it either works or fails properly. (JDBC is pretty bad that way; it has no way to bind parameters by name that I can see. My error rate decreased when I found a way to bind positionally without explixit numbering.) And make sure to test very thoroughly.

The key problem with ORMs is more subtle; they make it very easy to have code that does far more work than it should (e.g., fetching a whole table just to count the rows in it). That problem shows up in a lot of web service tooling as well. The main symptom is a program that works but which feels terribly slow.

Jaime

@dkf said in Duplicate code detected:

The key problem with ORMs is more subtle; they make it very easy to have code that does far more work than it should (e.g., fetching a whole table just to count the rows in it). That problem shows up in a lot of web service tooling as well. The main symptom is a program that works but which feels terribly slow.

Which is why I suggested a micro-ORM. All of them will allow you to specify your own SQL. From this, they'll do two important things: Validate that the query parses, and wrap the inputs and outputs in strongly-typed code.

People who know SQL very well and prefer to work closer to the database should probably stay away from the more sophisticated ORMs. They provide isolation that such a person would neither benefit from nor desire, and they hide complexity.

boomzilla

@Jaime said in Duplicate code detected:

People who know SQL very well and prefer to work closer to the database should probably stay away from the more sophisticated ORMs. They provide isolation that such a person would neither benefits from nor desire, and they hide complexity.

For simple CRUD type stuff they're still super handy. But yes, bad at complicated stuff and don't scale well.

Tsaukpaetra

@Jaime said in Duplicate code detected:

a micro-ORM.

*hides under the table*