One of the hardest decisions I think we have to make when maintaining software is to decide whether the change we’re making can be done in a more modern style, or using a new idiom, and if so what is the tension between doing that and following the patterns in the existing code.
Vive la Différence!
In some cases the idiom can be a language feature. When I was writing mostly C++ a colleague started using the newer “bind” helper functions. They were part of the standard and I wasn’t used to seeing them so the code looked weird. In this particular case I still think they look weird and lead to unreadable code (when heavily used), but you have to live with something like that for a while to see if it eventually pans out.
Around the same time another colleague was working on our custom grid control [1] and he came up with this weird way of chaining method calls together when creating a cell style programmatically:-
Style style = StyleManager.Create().SetBold().SetItalic().SetFace(“Arial”).SetColour(0, 0, 0).Set...;
At the time I was appalled at the idea because multiple statements like this were a nightmare to put breakpoints on if you needed to debug them. Of course these days Fluent Interfaces are all the rage and we use techniques like TDD that makes debugging far less of an occurrence. I myself tried to add SQL-like support to an in-memory database library we had written that relied on overloading operators and other stuff. It looked weird too and was slightly dangerous due to classic C++ lifetime issues, but C# now has LINQ and I’m writing code like that every day.
They say that programmers should try and learn a number of different programming languages because it’s seeing how we solve problems in other languages that we realise how we can cross-pollinate and solve things in our “primary” languages in a better way. The rise in interest in functional programming has probably had one of the most significant effects on how we write code as we learn the hard way how to deal with the need to embrace parallelism. It’s only really through using lambdas in C# that I’ve begun to truly appreciate function objects in C++. Similarly pipelines in PowerShell brought a new perspective on LINQ, despite having written command shell one-liners for years!
Another side-effect is how it might affect your coding style - the way you layout your code. Whilst Java and C# have fairly established styles C++ code has a mixture. The standard obviously has its underscores but Visual C++ based code (MFC/ATL) favours the Pascal style. I’ve worked on a few C++ codebases where the Java camelCasing style has been used and from what I see posted on the Internet from other languages I’d say that the Java style seems to have become the de-facto style. My personal C++ codebase has shifted over the last 20 years from it’s Microsoft/Hungarian Notation beginnings to a Java-esque style as I finally learnt the error of my ways.
When In Rome…
The counter argument to all this is consistency. When we’re touching a piece of code it’s generally a good idea not to go around reformatting it all as it makes reading diffs really hard. Adopting new idioms is probably less invasive and in the case of something like unique_ptr / make_shared it could well make your code “more correct”. There is often also the time element - we have more “important” things to do; there are always more reasons not to change something than to change it and so it’s easier to stick with what we know.
The use of refactoring within a codebase makes some of this more palatable and with good test coverage and decent refactoring tools it can be pretty easy much of the time. Unless it’s part of the team culture though this may well just be seen as another a waste of time. For me, when it’s out of place and continues to be replicated making the problem worse, then I find more impetus to change it.
For example, when I started on my first C# project it was a greenfield project and we had limited tools (i.e. just Visual Studio). A number of the team members had a background in C++ and so we adopted a few classic C/C++ styles, such as using all UPPERCASE names for constants and a “m_” prefix on member variables. Once we eventually started using ReSharper we found that these early choices were beginning to create noise which was annoying. We wanted to adopt a policy of ensuring there were no serious warnings from ReSharper [2] in the source code as we were maintaining it, and also that as we went forward it was with the accepted C# style. We didn’t want to just #pragma stuff to silence R# as we also knew that without the sidebar bugging us with warnings things would never change.
One particularly large set of constants (key names for various settings) continued to remain in all upper case because no one wanted to take the hit and sort them out. My suggestion that we adopt the correct convention for new constants going forward was met with the argument that it would make the code inconsistent. Stalemate! In the end I started factoring them out into separate classes anyway as they had no business being lumped together in the first place. This paved the way for a smaller refactoring in the future.
Whilst this particular example seems quite trivial, others, such as adopting immutability for data model classes or using an Optional<T> style class for cases where a null reference might be used are both more contentious and probably require a longer period of inconsistency whilst the new world order is adopted. They also tend not to bring any direct functional benefit and so its incredibly hard to put a value on them. Consistent code doesn’t stick out like a sore thumb and so it’s less distracting to work with, but there is also no sense in doing things inefficiently or even less correctly just to maintain the status quo.
[1] Writing a grid control is like writing a unit test framework - it’s a Rite of Passage for programmers. Actually ours was a lot less flickery than the off-the-shelf controls so it was worth it.
[2] The serious ones that is. We wanted to leverage the static analysis side of R#' to point out potential bugs, like the “modified closure” warning that would have saved me the time I lost in “Lured Into the foreach + lambda Trap”.
And once you've modified your own preferences of style and language/library features to use you have the rest of your team to convince.
ReplyDeleteI've found the size of the team makes a lot of difference. Even in an established project if the team is small (maybe up to 4 developers) there can be a lot of flexibility to change the style and features used from one release to the next. But with a large team it's much harder to get a consensus so either the project gets locked into an old fashioned style and using old libraries and tools, or a small number of developers push forward without bringing the rest of the team with them which can give great short term gains but can lead to maintenance problems later. This becomes a built-in drag on improvements. Handling it becomes an organisation or training issue almost as complicated as the original technical issue.
As mainly a C++ developer it's interesting how fast various places are adapting to C++ 11.
I've started looking for buy-in from the team before adopting some changes like this because, as you say, if your teammates are not behind you you'll just make things worse.
ReplyDeleteI also struggle with deciding whether some changes are really that valuable or not and so look for confirmation from my teammates that it's really worth embarking on. They may not know, but by gauging their reaction I can at least get a feel for whether it should just be dropped.