Leaderboard

Actually I guess is the other way around! The more expert I get the more refactoring I do! As @Lars Fosdal said, cryptic code, WTF was I thinking code, Code that could benefit from modern RTL code if there's no performance penalty. For example: For lPair in SomeObjectList is slower than for i:= 0 to SomeobjectList.count-1. So I won't refactor it. Depending where the code is placed, I will use one form or the other. Replacing TStringList (used as dictionary) with a TDictionary/TDictionaryObject makes a huge diference!

"Do you have tests for it?"

The problem is that OTL is unfortunately Windows only, where it wins hands down. On the other hand, PPL with all its quirks is cross-platform. When porting to Linux, I had to write two versions of the same code, ifdef'd to use OTL on Windows and PPL on Linux. Not an elegant solution but I did not want to touch the working Windows code...

PPL is a library too. It's also quite big. Why does it matter how big a library is? It would seem odd to me that there would be no problem using a library known to have many defects still, over a high quality library. Who in their right mind would choose the lower quality library facing this choice?

Actually both. I needed to write a small interface between SAP (text files with a +/- ten thousand records) and another system that needed to check if a RFID card number was in a list. Depending on some card property (returned by the associated class, I had to call a certain REST API). I was using a simple sorted StringList, when I switched to TDictionary the performance boost was noticeable. Routines dropped from minutes to seconds. I replaced: idx := fCardList.indexof( aCardNumber ); if idx>-1 then lObj := TMyObject( fCardList.Objects[idx] ) with if fCardlist.TryGetValue( aCardNumber, lObj ) then case lObj.RoleType of [..] end; Faster and more readable! It depends. When performance is the key, I usually write a lot of comment to compensate the lack of readability. In those cases I know the routine must be lean and mean I will use the fastest data structure. When the routine is simpler, not called very much, or performance is not an issue then I move to a "more readable" coding approach.

I have been guilty of doing that myself but given this and other examples it shows why calling virtual methods from a ctor is considered a code smell by some people. And from what I just learned C++ even refuses to do a virtual call as per https://isocpp.org/wiki/faq/strange-inheritance#calling-virtuals-from-ctors And also "Injection Constructors should be simple"

In general you can't do this. Anti-aliasing requires knowledge of the background at the point where the text is rendered. If the original background is a single colour then it may be possible, but will require clever processing. What you ought to do is simply render the text again on the new background.

Same way like other approaches (besides interface) by moving construction of TSomeOtherClass instance to TMyClass constructor. Example would depend on DI framework used... I would look at Spring4D... If code is used in places where out of memory is not recoverable error, then ignoring this potential leak is also viable approach in combination with assigning instance at beginning of constructor like @Uwe Raabe suggested

Yes, platform and framework doesn't matter, VCL, FMX, iOS, Linux, etc.

If allocating memory for the object instance fails then destructor is not called. I would probably use try..except block for releasing the object in case of out of memory error. Of course, that works only if constructing TMyClass instance itself is the only thing that can cause out of memory in that process. But I don't really like that whole try...finally block with nil assignment. Cleaner approaches would include DI, interfaces, passing meta class to TMyClass constructor and constructing TSomeOtherClass instance within the constructor, if the construction is too complex then I would use anonymous method instead and use it as factory.

Another challenge is knowing when NOT to refactor. "This code works, but it looks awful... should I refactor?"

The TBannerAd component creates a native control. Because of the way FireMonkey renders everything to a canvas, native components will always render on top of FireMonkey components. It’s a side-effect of the way it works. The way I got round this is to hide and show the banner component when in other screens (e.g. tabs of a page control or frames which display as screens depending on your UI architecture), so it doesn’t get it the way when I don’t want it to. I also suggest including a bottom padding to your multi view menus to push the contents above the banner. I hope this helps

I've been using Delphi for 25 years, I'm working on a 20 year old code base, I'll let you know when I'm done refactoring

Even Chuck Norris refactors his code (with a roundhouse kick though but still...)

I personally use nr 2 really often. Imagine a simple encoder: TBaseEncoder = Class [...] TEncoderV1 = Class(TBaseEncoder) [...] TEncoderV2 = Class(TBaseEncoder) [...] TEncoderV3 = Class(TEncoderV2) [...] Basically you have a bunch of encoders, containing new methods, improvements, etc. Now let's say you want your application to be backwards compatible and be able to use previous encoders. If you add: TBaseEncoderClass = Class Of TBaseEncoder; ...and define your application as: TMyApplication = Class strict private _myencoder: TBaseEncoder; public Constructor Create(inMyEncoderClass: TBaseEncoderClass); ReIntroduce; End; Constructor TMyApplication.Create(inMyEncoderClass: TBaseEncoderClass); Begin inherited; _myencoder := inMyEncoderClass.Create; End; then you simply can call: TMyApplication.Create(TBaseEncoder); or TMyApplication.Create(TEncoderV3); to create and make your application to use the specified version of your encoder.