Goodbye, Clean Code

> As an apropos to the article, and touched upon therein, checking in a fairly major change to de-duplicate some code without consulting the original author/team is a wee bit rude.

This sounds like the outcome of bad culture. Ownership of the code should be shared to the point where it should never be considered rude to improve the code. Any part of the code.

> Ask your colleagues first why such code still exists before barging in and making these changes,

If I had to synchronise with others all of my improvements to existing code (which was frequently written in a hurry to meet a deadline, so with shortcuts taken intentionally, or with incomplete knowledge of future use cases) I would get at most half as much done.

> they may already have some concerns as to why refactoring to a helper method or some abstraction isn't in their game plan yet.

If there are alluring "improvements" that don't work for such subtle reasons, this should be documented in the code. If it's not, one has only oneself to blame when someone goes in and changes it.

Edit: I realise now that I'm talking about teams where everyone is reasonably senior. It could be different with junior members on the team, to which many changes might look like improvements when a senior engineer would at most see the change as equivalent. In that case I think you're right, but for a different reason: junior engineers should always check in with senior engineers about things in order to learn more of the craft!

Your boss made a good point. My boss also made the exact same good point. But between those two same good points, there isn't a great line. ;)

I think the author got the wrong message from this situation. Yes, the refactor is bad, but I think they misidentified WHY it is bad.

> Firstly, I didn’t talk to the person who wrote it. I rewrote the code and checked it in without their input. Even if it was an improvement (which I don’t believe anymore), this is a terrible way to go about it. A healthy engineering team is constantly building trust. Rewriting your teammate’s code without a discussion is a huge blow to your ability to effectively collaborate on a codebase together.

Yes, but that's a social problem not a code problem. It doesn't mean your code was wrong, it means the process by which you integrated that code was wrong.

> Secondly, nothing is free. My code traded the ability to change requirements for reduced duplication, and it was not a good trade. For example, we later needed many special cases and behaviors for different handles on different shapes. My abstraction would have to become several times more convoluted to afford that, whereas with the original “messy” version such changes stayed easy as cake.

It's telling that there isn't a code example for these "special cases". My guess is that allowing them as configuration points in the de-duplicated calls would not have been that complicated.

I have heard this "problem" brought up before, and in my experience, it's not nearly the problem people claim it is. Yes, there is a balance to be struck: you don't want to remove duplication before you understand what's really duplicated, hence the rule of three (and I'm even fine going well beyond three). But I've yet to work on a codebase that wasn't well on the "too much duplication" side of that.

The REAL problem I see with the refactor is that the new data structures don't correspond to business objects, the way the user thinks about them.

> I don't want to ask of permission for an improvement. If there is a need to have formal authorization of maintainers responsible for parts of the code, they setup just that. Otherwise, I'm certainly going to improve "your" code, in some rare cases without even telling you (that's not a goal in itself to do it behind your back, obviously, and I also value collaboration -- but that should not be a problem). The question that remains is: is it really an improvement. If you are not sure, then maybe don't commit. If you are, do it (following your local rules), and if it ends up being a mistake, yes, it will be reverted, and so what?

The issue remains that "improvements" and "good code" are too a large extent subjective. And the issue with doing a major refactor of code someone else wrote just days before on your own without consulting the original author or the team late at night as is described in the article (and doing it repeatedly) is showing poor social skills at best and being actively dangerous at worst.

Here are some reasons that you should consult the team and or the original author before doing a large scale refactor of recent code:

* Start off with the principle of charity and assume that the person who wrote that code and the people who reviewed it aren't complete idiots and you aren't some kind of savant who is able to see something they didn't while working on it for a much longer period of time.

* Since that person has been working on it for days they might have a perfectly valid reason for writing the code the way they did. One example is that they are working with the stakeholder and this is the first of a series of changes to deliver specific functionality and the code is the way it is because it facilitates those upcoming changes that might only be days away.

* If you have a habit of doing this and your changes getting reverted "ask forgiveness than permission" as you said, the team will begin to lose trust in you and it is bad for team morale. Consulting people before you make a huge change to their recent code makes them feel part of the process as opposed to something that is being done around them with them as spectators. On any team where that happens with any degree of frequency is not going to be a happy team in the long run. And managers will prioritize a happy team with high morale as opposed to a "rockstar refactorer" on the team who is impacting morale regardless of whether their refactorings are error free or not.

* It is fundamentally against the spirit of collaboration. When someone releases some new code there is a degree of inherent collaboration in there, it might involve multiple developers, stakeholders, implied functionality that was not part of the original story and at the very least the code reviewer. You unilaterally making a change without even taking 10 mins of your time to discuss it with them is impolite at best and wasting company resources at worst if your "improvement" ends up being reverted and all that work and time could have been saved had you bothered to have a 10 min convo before starting.

Oliver Steele describes "Instance First Development", which the language he designed, OpenLaszlo, supported through the "Instance Substitution Principle". I've written about it here before, and here are some links and excerpts.

https://news.ycombinator.com/item?id=14418108

In the right context, prototypes can enable Instance-First Development, which is a very powerful technique that allows you to quickly and iteratively develop working code, while delaying and avoiding abstraction until it's actually needed, when the abstraction requirements are better understood and informed from experience with working code.

That approach results in fewer unnecessary and more useful abstractions, because they follow the contours and requirements of the actual working code, instead of trying to predict and dictate and over-engineer it before it even works.

Instance-First Development works well for user interface programming, because so many buttons and widgets and control panels are one-off specialized objects, each with their own small snippets of special purpose code, methods, constraints, bindings and event handlers, so it's not necessary to make separate (and myriad) trivial classes for each one.

Oliver Steele describes Instance-First Development as supported by OpenLaszlo here:

Instance-First Development

http://blog.osteele.com/2004/03/classes-and-prototypes/

[...]

https://news.ycombinator.com/item?id=21841054

[...] The mantle of constraint based programming (but not Instance First Development) has been recently taken up by "Reactive Programming" craze (which is great, but would be better with a more homoiconic language that supported Instance First Development and the Instance Substitution Principle, which are different but complementary features with a lot of synergy). The term "Reactive Programming" describes a popular old idea: what spreadsheets had been doing for decades. [...]

https://news.ycombinator.com/item?id=7756215

Oliver Steele (one of the architects of OpenLaszlo, and a great Lisp programmer) describes how OpenLaszlo supports "instance first development" and "rethinking MVC":

http://osteele.com/archives/2004/03/classes-and-prototypes

http://osteele.com/archives/2003/08/rethinking-mvc

[...] I've used OpenLaszlo a lot, and I will testify that the "instance first" technique that Oliver describes is great fun, works very well, and it's perfect for the kind of exploratory / productizing programming I like to do. (Like tacking against the wind, first exploring by creating instances, then refactoring into reusable building block classes, then exploring further with those...)

OpenLaszlo's declarative syntax, prototype based object system, xml data binding and constraints support that directly and make it easy.

OpenLaszlo's declarative syntax and compiler directly support instance first development (with a prototype based object system) and constraints (built on top of events and delegates -- the compiler parses the constraint expressions and automatically wires up dependences), in a way that is hard to express elegantly in less dynamic, reflective languages. (Of course it was straightforward for Garnet to do with Common Lisp macros!)

https://news.ycombinator.com/item?id=17360883

Instance-First Development:

https://blog.osteele.com/2004/03/classes-and-prototypes/

>The equivalence between the two programs above supports a development strategy I call instance-first development. In instance-first development, one implements functionality for a single instance, and then refactors the instance into a class that supports multiple instances.

>[...] In defining the semantics of LZX class definitions, I found the following principle useful:

>Instance substitution principal: An instance of a class can be replaced by the definition of the instance, without changing the program semantics.

In OpenLaszlo, you can create trees of nested instances with XML tags, and when you define a class, its name becomes an XML tag you can use to create instances of that class.

That lets you create your own domain specific declarative XML languages for creating and configuring objects (using constraint expressions and XML data binding, which makes it very powerful).

The syntax for creating a bunch of objects is parallel to the syntax of declaring a class that creates the same objects.

So you can start by just creating a bunch of stuff in "instance space", then later on as you see the need, easily and incrementally convert only the parts of it you want to reuse and abstract into classes.

What is OpenLaszlo, and what's it good for?

https://web.archive.org/web/20080211173607/http://www.donhop...

Constraints and Prototypes in Garnet and Laszlo:

https://web.archive.org/web/20080211173607/http://www.donhop...