Sunday, 8 December 2024

Should naming procedures Setup and Teardown be reserved for unit testing?

I recently came across some code a developer had done where they had named 2 public procedures 'Setup' and 'Teardown', when I fist saw these I instinctively thought of unit tests and thought that class might relate to unit testing. After looking into the code it was apparent that these procedures where meant to be called after an object of the class was created and before the object was freed, but had nothing to do with unit testing.

Even though there are no restrictions on using these words for procedures I do think it is best practice not to use them in production code and name them something different like 'InitializeResources' and 'CleanupResources'. I think keeping procedures 'Setup' and 'Teardown' specifically for testing maintains a clear distinction between testing and application logic, which can be beneficial for maintainability.

Wednesday, 24 July 2024

Why change how a Web Broker service works?

I have a web service (web broker) that works fine and each request is sent to a corresponding object that relates to the request and the response is returned, for example the /customerinfo request (GET) calls an object that is private to the TWebModule that returns customer information.

procedure TMyWebModule.CustomerInfoAction(Sender: TObject; Request: TWebRequest; Response: TWebResponse; var Handled: Boolean);
begin
    Response.Content := FCustomers.CustomerInfo(Request);
end; 

BTW, this is not the actual code, but is just an example. This works fine and has been working fine for some time. However, another developer has come up with what they consider a better way fo doing this. First, they add a property of TWebRequest to all objects that use TWebRequest, so in this example the FCustomer object has a WebRequest property. Then in the BeforeDispatch set the WebRequest property of all the web module objects (not just the one that relates to the request) to the web request. The advantage of this is so that you do not need to pass in the WebRequest in the action e.g.

procedure TMyWebModule.CustomerInfoAction(Sender: TObject; Request: TWebRequest; Response: TWebResponse; var Handled: Boolean);
begin
    Response.Content := FCustomers.CustomerInfo;
end; 

They have also proposed that we could also set the TWebResponse on all the objects to that then all you need to do is this.

procedure TMyWebModule.CustomerInfoAction(Sender: TObject; Request: TWebRequest; Response: TWebResponse; var Handled: Boolean);
begin
    FCustomers.CustomerInfo;
end; 

The other change that is being proposed is to add validation to the TWebRequest, by having a new class that descends from TWebRequest, lets call it TMyWebRequest and this will have validation code, so that you can call something like MyWebRequest.Validate it will do some validation. Currently there is a validation class that takes the TWebRequest, but this would be removed and the TMyWebRequest would validate itself.

In the AfterDispatch method it will iterate through all the objects and set the MyWebRequest property to nil.

There are a few things I am not keen about with making these changes. 

  • I don't like the idea of descending from TWebRequest just to add some validation methods, and much prefer having validation classes that are responsible for doing the validation.
  • Currently passing TWebRequest makes it clear and explicit that the parameter is expected, making it easier to trace.
  • The current way keeps the responsibility of handling the request and response with the action.
  • It is setting the MyWebRequest property of all the objects, even though it is only required for the object that will be called, for example it sets the MyWebRequest property on the Order, Product, System and Customer objects even though it is only the Customer object that will be called with that requrest.
  • It means that a developer can use both methods, this could cause confusion.
  • It will take some time to change all the services to work this new way. 

The benefits I can see from doing this are:

  • Reduce the number of parameters passed.
I cannot see any real benefits from making these changes, or am I missing something?