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?

Possible duplicate of [Should a method do one thing and be good at it?](https://softwareengineering.stackexchange.com/questions/137941/should-a-method-do-one-thing-and-be-good-at-it) — gnat, Jan 03 '19 at 18:25
You're taking SRP too far. That method is ~10 lines long and is perfectly readable/maintainable. — 17 of 26, Jan 03 '19 at 18:31
The method is short enough that you could come up with a better name and not change the behavior. — Berin Loritsch, Jan 03 '19 at 21:07
"... is ~10 lines long." Method LOC is not a criterion for SRP. In fact SRP naturally results in short, concise, and yes, understandable methods. Done consistently this will be true all the way to the top, and especially at the top, of a hierarchy of complex class composition and inheritance: I've seen a user-facing class of almost exclusively 1 to 3 line methods that was a joy to read and work with. Details organized and pushed down at appropriate levels yields concise class API's that keeps client code concise - turtles all the way up and down. @DocBrown answer is precisely that. — radarbob, Jan 03 '19 at 21:20
is this WPF? you should remove the GetClient call from the constructor and using binding to set stuff like visibility — Ewan, Jan 04 '19 at 09:57
@Ewan yes it is WPF. GetClient will work initialized main form. If I remove it, my initial process does not work. I could not understand you say. — barteloma, Jan 06 '19 at 15:22
@barteloma you should refactor to MVVM and bind your xaml to a view model rather than using code behind. you will find it simplifies your code somewhat — Ewan, Jan 06 '19 at 15:41
@gnasher729 This is so true. It is one of the best examples of a well-intentioned idea being taken too far and causing unexpected damage by unintended side-effects - a beautiful, ironic symphony about the reality of software development. — T. Sar, Jun 05 '19 at 18:30
@gnasher729: I know what you trying to tell us, but can hardly agree. I have seen way too much code in too huge functions and classes in the past which could heavily benefit from applying a little bit more "SRP" than the other way round. And I have to admit, some of that code was written by me. — Doc Brown, Jun 05 '19 at 18:56

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.

You could argue that can be shortened using the ternary operator: `Visibility = client.Status ? Visibility.Visible : Visibility.Collapsed`. And if this is WPF code, you can simply use the built in `BooleanToVisibilityConverter` in your binding and not even have this check here. — Berin Loritsch, Jan 03 '19 at 21:01
@BerinLoritsch: yep, but for my point about the SLA not really important, I think. — Doc Brown, Jan 03 '19 at 22:19
@BerinLoritsch Please do not sacrifice readability to lessen the number of line codes. A ternary is a nice tool in theory but it has terrible maintenance later on - specially for newbies. — T. Sar, Jun 05 '19 at 18:33

How can I get rid of this antipattern

1 Answers1