How can I get rid of this antipattern

Question

I have a main window and I amgetting data from http client service while form load.

public class MainWindow: Window
{
    private IClientService service;     
    public MainWindow(IClientService service)
    {
        this.service = service;                 
        GetClient();            
    }       
    public async Task<Client> GetClient()
    {
        try
        {
            IsDownloading = true;    
            var client = await service.GetClient();             
            if(client!=null)
            {
                if (client.Status)
                    Visibility = Visibility.Collapsed;
                else
                    Visibility = Visibility.Visible;                        
                ShowClientRegistrationForm();               
            }else{
                HideClientRegistrationForm();
            }
        }
        catch
        {
            MessageBox.Show("Access error.", "Error", MessageBoxButton.OK, MessageBoxImage.Error);    
            throw;
        }
        finally
        {
            IsDownloading = false;
        }
    }
}

My GetClient() method does 3 operations.

Gettting client using service
Changes the visibility if client status is true or false
Shows the registration form to user, if client object is null.

I think this is an antipattern. And violates the single responsibility principle. How can I get rid of it?

Doc Brown · Accepted Answer · 2019-01-03T20:29:11.163

SRP is not an end in itself, it is a means to an end.

If you want to have your GetClient outside the GUI layer, it would make sense to get rid of any GUI specific code inside that method, and use callbacks or events to call the visibility changing code, the registration form code, and the MessageBox code. Same if you want to automatically test GetClient without any interfering GUI code.

However, in the current form GetClient does not look like a method worth automatic testing to me. It just coordinates some UI actions around the service.GetClient method. If you want to know if that behaviour is correct or not, you will have to test it manually either, and look how the GUI reacts.

The only thing one could consider to improve here is IMHO the "Single Level of Abstraction" principle. That could mean to put the part

           if (client.Status)
                Visibility = Visibility.Collapsed;
            else
                Visibility = Visibility.Visible;

into a method on its own and call it like SetVisibilityAccordingToStatus(client.Status).

I think it is debatable if that is currently worth the hassle, I would probably defer that decision until the method grows a few lines longer and readability starts to decrease, or if the code snippet above occurs in your code in more than just one place.

How can I get rid of this antipattern

1 Answers1