Dalija Prasnikar

Why? It serves no purpose whatsoever. I am not familiar with OTL enough to comment other code.

Can you please translate your logic to English. It is extremely hard to follow what you are doing. Especially, since TTask.Run code is basically empty and it requires way more context.

Understandable. I would do the same. GPX format is ideal for SAX parser. Avoiding allocations of thousands of XML nodes would be my best bet for optimization.

Unrelated to the profiling, there are other optimizations. First, SAX parsing is generally more performant than DOM parsing, especially when DOM is based on interfaces. If you don't need XML DOM, then building your business classes directly during parsing will be more efficient. But not all structures cane be equally easy parsed by SAX. Next, IXmlDoc works on top of standard IDOM interfaces, so you have additional slowdown there. If you cannot use SAX, modifying code to work directly with IDOM interfaces might be a solution. Or using different DOM parser.

Sampling profiler will give you better insight. https://www.delphitools.info/samplingprofiler/

I cannot say whether something in logs changed, but as far as I can remember FastMM works in Sidney the same way it worked before. I don't have the Tokyo installed for comparison. And it shows the stack trace correctly so I can follow to the line where issue happened. Yes, I know that in more complex code it can be harder to figure out real culprit, but that has always been the case.

Would you elaborate that? Memory manager works fine in Sydney and detects memory leaks.

I am talking about your comment to @Wagner Landgraf where he mentioned memory management and you are saying this thread is not about memory management. I am not trying to prove anything, just wanted to say that memory management is indirectly involved in this conversation, and Wagner didn't post in the wrong thread.

The way I am reading that is: if you pass parameters in constructor to initialize fields and you validate them there, raising exception if they are not valid, then you can avoid checking those fields when you are using them in other places because you know they cannot hold invalid values.

try...finally implies cleanup which in Delphi terms implies memory management Maybe memory management is not main topic, but it is certainly related.

Depends on the context. If you open some file for processing and it turns out to be a too large so processing it raises out of memory, you just clean up and tell to the user: "Sorry, your file is too large and you don't have enough memory." But after proper cleanup your application will be in fine condition to process some other smaller file. On the other hand, there are situations where out of memory is not recoverable. You still may be able to show message to the user there is not enough memory, but you will just have to kill the application after that.

What do you mean by plan B?

If you want to handle the exception on site, then the second option is the wrong way to do it for several reasons. Construction of an object can always fail because of OutOfMemory, so if you wanted to handle all exceptions at that point you have failed to do so. If you are fine to bubble up that exception further on and if except part (handling the exception) cannot raise exceptions, you don't need try..finally at all.

You tell me. It doesn't feel bad to me at all I think you are overcomplicating. If you have class that requires some setup (that will commonly not be changed once object is created) to function properly then passing those parameters through constructor and raising exception in constructor is the best and simplest thing to do. Use the approach that requires the least amount of code for safely using such class and where you cannot easily forget to do something vital. Usually that will be the best code.

Actual purpose of try...finally in above code is to ensure cleanup of the MyThing instance if MyThing.DoIt raises exception. If constructor raises the exception there will be automatic cleanup and MyThing.Free will never be, nor it should be called.

No, you don't. If the instance is never constructed it will be nil in the destructor and you can just call Free on such instance. You only need to consider that any instance in the destructor can be nil, so if you are calling any other methods you need to check for nil before calling those in destructor.

Nobody said that you must validate in the constructor. It all depends whether this is critical or not and at which point. There are no wrong or right answers here, it all depends how class functionality and how it is used. Each approach has downsides and upsides.

Yes. All fields are zero initialized. There is no double safety if something cannot ever happen. If you have extra initialization code then such unnecessary code can obscure other important code. Keep it simple. If you are not sure what is initialized and when, it is better to check and learn than to use code just in case. It is fine to construct as many instances as you need and protect them with single try...finally. You don't need to pay attention to the order of destruction, unless there is some special reason where one instance still holds reference to other one you are destroying sooner, where you might have problem with dangling pointers. There is no functional difference, only one nil assignment more in your case - if you feel like you can more easily follow the code if multiple instances are all initialized before try..finally, that is fine. Remember, try...finally is used just for cleanup in case of exception, not for handling the exception. If exception is raised inside try..finally it will be propagated to the next exception handler block - try...except just the same it would if it is raised outside try..finally. Again, if there is exception raised in constructor, assignment to the variable will never happen. So if LA is nil and the you call LA := TRaiseOnMinus.Create(-2) in finally block LA will still be nil. Automatic cleanup for that unassigned object instance is happening in hidden code inserted by compiler. If the instance is successfully created, then automatic cleanup will not happen. Raising exceptions in constructor is not evil nor bad practice. It is perfectly fine thing to do. Whether you will raise exception in constructor or not, is matter of each particular class functionality. If you pass invalid parameter to a constructor it makes sense that such constructor raises exception, because otherwise your object is not properly functional instance, If you don't raise exception then, you would have to sprinkle your code all over inside that class and where you use it with checks for that particular invalid condition. When you have some condition that causes failure failing soon is always better than failing later on. If you have some reasons not to raise exception or there is nothing special that would require raising exception that is also fine. Constructing objects can always raise OutOfMemory so you always need to be prepared to do the cleanup anyway. Take for instance TFileStream - if constructor fails to open or create file it will raise an exception. And you can handle that failure at that point. If you don't have to handle it at that point, that means you have opened file too soon.

Some bullet points: if Assigned(Obj) then Obj.Free is redundant, you can just call Obj.Free - Free is static and can be called on nil instance and also Free has nil check within before calling destructor constructors are allowed to raise exceptions destructors must never raise exceptions (without handling them within with try...except) destructors can be called on half constructed instance (if the constructor fails, destructor chain will be automatically called) and must be able to handle such scenarios without causing trouble - raising exceptions when constructor fails assignment to variable will never happen local object variables are never automatically initialized and they can contain random garbage object instance fields are always zero initialized when constructor chain is being called Because of the above correct construction pattern is var LA: TRaiseOnMinus; begin LA := TRaiseOnMinus.Create( -2 ); try //some code finally LA.Free; end; end; With multiple constructors there are multiple options, but I prefer using only single try...finally because destructors must not raise exceptions, single try finally is faster and cleaner var LA, LB: TRaiseOnMinus; begin LB := nil; LA := TRaiseOnMinus.Create( -2 ); try LB := TRaiseOnMinus.Create( -3 ); //some code finally LB.Free; LA.Free; end; end; Correct constructor and destructor for your class would be: constructor TRaiseOnMinus.Create( AParam : Integer ); begin inherited; if AParam < 0 then raise Exception.Create( 'Bum' ); FMyList := TStringList.Create; end; destructor TRaiseOnMinus.Destroy; begin FMyList.Free; inherited; end; Only if you need to do some additional work with FMyList in destructor, then you must check it for nil before using it. In such cases you can find call to Free within the same check, but not because it wouldn't work outside. destructor TRaiseOnMinus.Destroy; begin if Assigned(FMyList) then begin FMyList.DoSomethingImportant; FMyList.Free; end; inherited; end;

Yes, there is. I forgot about that. I have a need for large number of projects and files and when I get into a trouble with the menu, it is easier for me to remove specific files than to have smaller number of files remembered.

This issue seems to happen when you have too many files in the recent menu. Open properties - most bottom option in the menu and delete few files (how many depends on the size of your screen) from the recent list. That should fix it. If it is not fixed, you will need to remove more files. Reported as https://quality.embarcadero.com/browse/RSP-36702

Instead of anonymous methods, you can also use plain TNotifyEvent and pass list item as Sender. Also if you don't like the code in completion handler because of TThread.Queue, this call can be moved to UpdateItem. There are many ways to write this functionality, this is just one.

But it is completely decoupled in that code. The only connection is in attached completion handler that can hold any kind of code and is not part of processing class.

Yes, you can use that approach, but it is not flexible because your processing function depends on the specific UI type. I wouldn't create task inside Execute method because processing functionality itself should be implemented as clean synchronous code - that gives you more flexibility to combine its functionality as needed. I would use something like: TMyClass = class public procedure Execute(OnCompleted: TProc); end; procedure TMyClass.Execute(OnCompleted: TProc); begin // process data //... if Assigned(OnCompleted) then OnCompleted; end; procedure TMyForm.ProcessItem(li: TListViewItem); begin TTask.Run( procedure begin TMyClass(li.Data).Execute( procedure begin TThread.Queue(nil, procedure begin UpdateItem(li); end); end); end); end; procedure TMyForm.UpdateItem(li: TListViewItem); begin li. ... end; procedure TMyForm.ProcessView; begin for var li in lview1.Items do begin ProcessItem(li); end; end;

You are right that the processing logic and updating UI don't belong together. When I have processing functionality as part of the class (and I usually do) I also declare completion handlers in that class. And then you can easily attach any kind of additional logic when processing is finished - including updating UI. The most important part is that all individual pieces of code can be easily unit tested (except for UI) and that there is minimal gluing code where you can make mistakes when you put all of the code together.