I have a process that starts with importing data from file and then executes a series of procedures, but at anytime it can find a problem and should stop executing rest of them and run another set.
Here is my example where every procedure sets global gStop variable that indicates stopping the process. If it was stopped, I need run some code for that at the end.
var gStop:boolean;
procedure Run;
begin
gStop:=False;
Import; // imports data from file
If Not gStop Then
AfterImport1;
If Not gStop Then
AfterImport2;
If Not gStop Then
AfterImport3;
If Not gStop Then
AfterImport4;
If Not gStop Then
If fTypeOfData = cMSSQL Then // function returns type of imported data
begin
ProcessMSSQLData1;
If not gStop Then
ProcessMSSQLData2;
If not gStop Then
ProcessMSSQLData3;
If not gStop Then
If fObjectAFoundInData Then // function checks if ObjectA was found in imported data
ProcessObjectA;
If not gStop Then
ProcessMSSQLData4;
end;
If Not gStop Then
AfterImport5;
...
// If stopped at anytime
If gStop then
begin
LogStoppedProcess;
ClearImportedData;
...
end;
end;
In my case it actually goes over 200 lines of code, so I have to scroll up and down when I maintain this part of code.
Any way I can improve this procedure to be more readable, easier to maintain or just is there any other way I can stop the process without all the IFs?
EDIT 1:
Every procedure can find a faulty data and can set gStop := True;
and the process should skip all the rest of the procedure and just execute the part of the code at the end when gStop = True;
EDIT 2:
I would like to keep the workflow controlled from main procedure (Run) so I can see all the tasks that are run after the main Import. I only see more confusion and less readability and maintainability if I break the execution into many smaller procedure. Then I could just have:
procedure Run;
begin
gStop:=False;
Import; // imports data from file
RunEverytingAferImport; // execute ALL tasks after import
// If stopped at anytime
If gStop then
begin
LogStoppedProcess;
ClearImportedData;
...
end;
end;
This workflow doesn't seem designed correctly. I want to know what are main tasks running after Import and not go on discovery trip every time I need to review it. All tasks are already group into the procedure by purpose, what they do and how they do it, and results.
CONCLUSION:
Even though not the best option, I decided to go with Raising Exception when stopping the process is needed. I 'kind of' understand the implications that this approach brings (more will be known once I implement it), but it seems a logical step towards some-day better implementation of the whole process. Now I will not see all these IFs for every task execution. Good! The code will be more readable, maintainable.
I read the links provided to explain the pitfalls of using Exceptions in stopping workflow execution, but, as Dalija Prasnikar explained in comments, this is not a performance related part of the task; process is only executed once per application run; tasks are already quite structured by what they do; tasks already include multiple IF statements where stopped process is checked and so on, so I think exceptions don't fall into a really bad solution to my problem.
Also, if I turn tasks into functions with returning results, I think I will have the same problem, checking values of every task and stop or continue the process based on that.
So, Raising exception is what I choose.
You should use custom exception and raise it when ever you encounter reason to break your work and go to Stopped
code. In your Import
, AfterImport1
and other code logic just call Stop
procedure and it will execute Stopped
procedure. On the other hand if all goes well, Stopped
will not be called.
You can derive your exception from EAbort creating silent exception
type
EStopException = class(EAbort);
or derive from base Exception class to use regular type exception.
type
EStopException = class(Exception);
procedure Stop(const Msg: string);
begin
raise EStopException.Create(Msg);
end;
procedure Import;
var sl: TStringList;
begin
sl := TStringList.Create;
try
// your code logic
if NeedToStop then Stop('something happened');
finally
// perform any cleanup code needed here
sl.Free;
end;
end;
procedure Stopped;
begin
end;
procedure Run;
begin
try
Import;
AfterImport1;
AfterImport2;
except
on e: EStopException do
Stopped;
end;
end;
Similar to RTTI-based example of Jens Borrisholt, but without RTTI. And thus not bound to a single super-object containing all methods.
type TAfterImportActor = reference to procedure (var data: TImportData; var StopProcess: boolean);
TAfterImportBatch = TList< TAfterImportActor >;
var Batch1, Batch2, BatchMSSQL: TAfterImportBatch; // don't forget to create and free them.
procedure InitImportBatches;
begin
Batch1 := TAfterImportBatch.Create;
Batch2 := TAfterImportBatch.Create;
BatchMSSQL := TAfterImportBatch.Create;
Batch1.Add( AfterImport1 );
Batch1.Add( SomeObject.AfterImport2 ); // not only global procedures
Batch1.Add( SomeAnotherObject.AfterImport3 ); // might be in different modules
Batch1.Add( AfterImport4 );
Batch2.Add( AfterImport5 );
...
Batch2.Add( AfterImport123 );
BatchMSSQL.Add( ProcessMSSQLData1 );
...
BatchMSSQL.Add( ProcessMSSQLData5 );
end;
procedure ProcessBatch(const Batch: TAfterImportBatch; var data: TImportData; var StopProcess: Boolean);
var action: TAfterImportActor;
begin
if StopProcess then exit;
for action in Batch do begin
action( data, StopProcess );
if StopProcess then break;
end;
end;
procedure Run;
var gStop: boolean;
data: TImportData;
begin
gStop:=False;
Import(data, gStop); // imports data from file
ProcessBatch( Batch1, data, gStop );
If fTypeOfData = cMSSQL Then // function returns type of imported data
ProcessBatch( BatchMSSQL, data, gStop );
ProcessBatch( Batch2, data, gStop );
...
// If stopped at anytime
If gStop then
begin
LogStoppedProcess;
ClearImportedData;
...
end;
end;
PS. This framework ( and RTTI framework above ) lack any exception control, so if any of import processors would raise some not-caught exception - the execution would jump out of the main process loop without calling clean-up routines. So that means you still have to caught possible exceptions within every actor (fragile) or within the Run
procedure. But in the latter case you might totally omit gStop
variable and instead raise your custom exception. Personally i'd prefer the exception-based way to Boolean flag. And even EurekaLog might be of use, if your failing afterimport
procedure would add to the exception some meaningful message as to why exactly the import was aborted.
PPS. I'd also split gStop into TWO different variables/exceptions: Batch Cancel and Import Abort ones. Then the If fTypeOfData = cMSSQL Then
- or any other prerequisite - check could be just the first actor in the batch. Then the batches could be just combined into 2-nd tier array/collection.
I also think EurekaLog would ignore your custom exceptions would you inherit them from EAbort
- http://docwiki.embarcadero.com/RADStudio/XE8/en/Silent_Exceptions
type TAfterImportActor = reference to procedure (var data: TImportData; var CancelBatch, AbortImport: boolean);
TAfterImportBatch = TList< TAfterImportActor >;
var Batch1, Batch2, BatchMSSQL: TAfterImportBatch;
// don't forget to create and free them.
ImportBatches: TArray<TAfterImportBatch>;
procedure MSSQLCheck(var data: TImportData; var CancelBatch, AbortImport: boolean);
begin
CancelBatch := data.fTypeOfData <> cMSSQL;
end;
procedure InitImportBatches;
begin
Batch1 := TAfterImportBatch.Create;
Batch2 := TAfterImportBatch.Create;
BatchMSSQL := TAfterImportBatch.Create;
Batch1.Add( AfterImport1 );
Batch1.Add( SomeObject.AfterImport2 ); // not only global procedures
Batch1.Add( SomeAnotherObject.AfterImport3 ); // might be in different modules
Batch1.Add( AfterImport4 );
Batch2.Add( AfterImport5 );
...
Batch2.Add( AfterImport123 );
BatchMSSQL.Add( MSSQLCheck ); // If fTypeOfData = cMSSQL Then Run This Batch
BatchMSSQL.Add( ProcessMSSQLData1 );
...
BatchMSSQL.Add( ProcessMSSQLData5 );
ImportBatches := TArray<TAfterImportBatch>.Create
( Batch1, BatchMSSQL, Batch2);
end;
procedure ProcessBatch(const Batch: TAfterImportBatch; var data: TImportData; var StopProcess: Boolean);
var action: TAfterImportActor; CancelBatch: boolean;
begin
if StopProcess then exit;
CancelBatch := false;
for action in Batch do begin
action( data, CancelBatch, StopProcess );
if StopProcess or CancelBatch then break;
end;
end;
procedure Run;
var gStop: boolean;
data: TImportData;
CurrentBatch: TAfterImportBatch;
begin
gStop := False;
Import(data, gStop); // imports data from file
for CurrentBatch in ImportBatches do begin
if gStop then break;
ProcessBatch( CurrentBatch, data, gStop );
end;
...
// If stopped at anytime
If gStop then
begin
LogStoppedProcess;
ClearImportedData;
...
end;
end;
PPPS. You would also maybe want to have a look at http://www.uweraabe.de/Blog/2010/08/16/the-visitor-pattern-part-1/
Focusing how different action-makers are registered and invoked. This might give you some ideas, though it is not exactly your problem.
Another thing to consider might be Multi-cast events like those in Spring4D library.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With