Full disclosure: Roman of SourceOddity was kind enough to offer me a free license of FixInsight in return for writing a blog post about it. There are no conditions on the review, of course – his exact words were “your own opinion”.
The short review: Every so often a tool comes along where you don’t realise how useful it is until you use it. FixInsight is just such a tool.
The long review:
Code analysis
There’s a long history of static code analysis tools for a variety of languages. I’m probably not the only one who remembers PC-Lint’s ancient print magazine advertisements, which had a C code snippet and asked you to spot the problem with it. A compiler, of course is an example of just this: its hints and warnings are unnecessary but useful output. It doesn’t have to emit them – it really only needs to emit errors – but we appreciate when it does. Often, in fact, I personally wish that Delphi emitted more hints and warnings than it does.
There are a number of tools that analyze Delphi code and produce reports based on it (Icarus, Delphi Unit Dependency Scanner, ModelMaker’s Unit Dependencies, and Castalia might count as well) but only two products perform static analysis and report possible code errors. They are Pascal Analyzer (which emits warnings of various strengths, which I’ve never used but which had a good short review here) and this new plugin, FixInsight. The person behind SourceOddity is Roman Yankovsky, a name you might recognise from the Delphi Developers G+ group or elsewhere.
FixInsight’s analysis
FixInsight is a tool that analyses all the code in your Delphi project, and produces a set of warnings and other messages where it thinks the code might have a problem.
It does this in two categories:
- Conventions – that is, Delphi style
- Warnings – that is, code that might have problems
Each item it checks for can be individually turned on or off, so if you have an idiosyncratic Delphi codebase that doesn’t match canonical style, you can avoid output noise by turning off some or all of FixInsight’s convention messages, for example.
Diving in
The plugin installs itself into the Project menu with a menu item ‘Run FixInsight’. Clicking it shows this dialog:
On the left are all the items it can check, in the two sections of Conventions and Warnings; on the right, where applicable, are settings for the specific check. Click Run to start analysis. The dropdown allows you to either analyze the entire project, or just the current unit.
By the way, the documentation is quite good and includes a list of explanations of what each rule means. I’d like to see this included in the dialog above, perhaps – the right-hand side is often empty and so could be put to good use showing that information.
Warnings
In my opinion, these are the most valuable of the checks FixInsight produces. Almost all of those warnings indicate something odd is happening in your code – either odd, or downright wrong.
I guarantee that if you run FixInsight, it will show problems in your code that you were not aware of. Why? Because I did.
The main thing here is that I don’t think it has enough warnings. I have thought of several I would like added – mostly messages I wish the compiler itself emitted – and Roman has heard those suggestions and may implement them. I suspect over time this tool will grow.
Conventions
I have mixed feelings about these: while the code style checks are very good (I prefix for interface types, etc) the others, such as a method being too long or having too many lines, do almost always find some warnings. The reason is that while methods should in general be short, single-purpose, have few parameters, etc, in a codebase you will generally have a few key places where this isn’t the case. Is reporting this noise? It depends on your code: if it only shows those three places that are consciously written like this and have been reviewed, the results are unwanted. For me this is the case and in my own code, in the projects I’ve tested this on, which I tend to feel I keep in fairly good shape, the code is this way for a reason.
However, it’s certainly valuable knowing where these areas are. The codebases I tested on are fairly new (less than a year old each) and I strongly suspect running these checks on old codebases or codebases you personally are new to would show many areas that break these rules, which would be excellent targets for refactoring. This is a strength of this sort of tool: running it on a codebase that is large or old will show problems, for sure, ones that you will not have already seen, otherwise they’d be fixed. That is valuable.
Analyzing its output
So what do you get when you run it?
There is where I go out on a potentially embarrassing limb and show you what it looks like running on my own code. The above was run on the Parnassus Bookmarks plugin. Wait! The code is not as bad quality as that output makes it look.
This output pane is fully integrated into the IDE, and double-clicking a message takes you to the line of code, just as with the inbuilt compiler messages.
Let’s analyze those warnings:
- Variable is assigned twice successively: usually this would be strange. In this case, it’s valid, since it’s a property (TTimer.Enabled) and the assignment has side effects – setting it to false then true resets the timer.
- Suspect Free call: There is a call to Free in a form’s method, which frees the form inside some logic. That sounds odd just writing it, and FixInsight reckons I might have wanted to free something else, not the form, since there’s what to it is a random Free sitting around inside a method in a place it doesn’t understand, which is a very good thing to warn about. (Update: it should be Release, instead. FixInsight was right.) There are many places in my code that call Free (of course) and this is the only spot that caused this warning: the code analysis here must be quite sophisticated.
This is a good example of where this tool would be useful in a code review, because it has found the sort of method that should be reviewed. - Methods too long: probably bad style, but I would defend not breaking those methods up.
- Empty except block: in any other code this would be a major red flag, but when writing an IDE plugin there are danger areas where exceptions might occur and you truly want to just ignore it and continue. I know. Really. But it’s unavoidable. (I have written and deleted a defence of that particular code several times. It’s off-topic. Please just accept it really is necessary there.) But is this a good thing to warn about? Absolutely yes, because ignoring exceptions is a terrible, terrible thing to do and in any normal code this should be fixed straight away.
But do you see the third line from the bottom? The missing inherited call in a destructor? Ouch. Yes, that was indeed a problem, and I fixed it. Thankyou, FixInsight.
False positives
The problem with any static analysis tool is that it may produce false positives, either because the rule is strict (and there are reasons for your code to break it) or because it cannot tell for sure if the code is how it is meant to be, and so warns you anyway. The above rules are a mix of these. In the above cases, of the eight errors shown in that screenshot, one was absolutely wrong (the inherited destructor.) One indicates code that should be reviewed (suspect Free call.) The others, as it happens, were okay. But that’s just my code. What are the chances that in anything other than an IDE plugin, an empty except block is ever correct? Very little.
In general I’ve found its warnings to be well judged. When FixInsight has flagged code as possibly wrong, it either has been wrong, or in rare cases it has been deliberately written to be like that but it is code where I can clearly see why it is flagged.
This leads to a conundrum: the output is valuable but you don’t want to see it all the time. Therefore, while you might run it occasionally, or over a project that you want to clean up, what do you do in order to regularly maintain code quality?
Enter the command-line tool.
Command-line and CI
FixInsight comes with a command-line version that can be run over a project, with various options, and this I think is where it shines. Why? Because you can run it as part of continuous integration.
Once you have reviewed your source code once, you aren’t interested in any remaining warning the tool produces. What you are interested in is changes to those warnings: new ones added, or old ones removed.
The command-line tool allows you to analyze a project and produce output either as normal text (very similar to the compiler’s formatted output, so probably parseable with the same tools) or as XML. You could easily integrate this into a build server or CI system as an automated tool to increase your code quality.
The tool doesn’t yet do this, but a nice addition would be if it could analyse two of its own logs and output if there is a significant change. Line numbers for a build may change every time it’s run, so a naive diff of its output might change every time. An intelligent context-aware diff that could be run by a CI server might be a neat addition.
Overview
When will you use this tool?
- Once-off, for an old project you want to maintain
- Once-off, when you are new to a project
- Occasional runs over code you’re writing as you write it, or before you check it in (maybe not every day)
- For code reviews
- For continuous integration, to flag potentially dangerous code
When will you not use this tool?
- In your IDE every day or every single (local) build.
My main feeling about the tool is both that it is useful – it’s found errors for me, which is embarrassing, but better than not finding them at all – and a feeling of potential for what else it can add in future. The plugin is brand new and development so far as I understand has understandably focused on creating its first version, and so now it’s out, it can expand. There are several things I would really like to see it check for. (For example, uninitialised managed result types in a method – the compiler only warns about uninitialised unmanaged types – or mixing interface and object referencing.) Just today Roman added a Feedback feature to the website to allow for feature requests, and I suspect and hope he will add new checks and warnings to the product steadily over the next few months.
For a tool that you won’t use directly every day (even if your build server uses it many times a day) the price of 90 euros seems high. But humans are notoriously bad at valuation. If it finds you just one or two errors that saves an hour of debugging time, that 90 euros is worth it already.
Pros:
- Finds code errors you aren’t aware of, and does a good job at it
- Nicely integrated into the IDE
- Command-line tool is a nice addition, especially if integrated into your automated / CI build server
Cons:
- False positives (a problem with any tool of this type, and the warnings it generates are good ones)
- I would like to see more warnings added
Is it worth buying? Yes. It’s worth running just once over your code, so get it for that (and you can get a trial version too) but to me the biggest thing is including it into code reviews and automated continuous integration on your build server. You won’t notice it’s there… until it finds something that you really did want to notice.
- FixInsight for Delphi is available from SourceOddity, and you can download a trial here.