0

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.

  1. Gettting client using service
  2. Changes the visibility if client status is true or false
  3. 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? – gnat Jan 3 at 18:25
  • 4
    You're taking SRP too far. That method is ~10 lines long and is perfectly readable/maintainable. – 17 of 26 Jan 3 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 3 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 3 at 21:20
  • This code seems fine to me... – TheCatWhisperer Jan 3 at 21:34
4

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 3 at 21:01
  • @BerinLoritsch: yep, but for my point about the SLA not really important, I think. – Doc Brown Jan 3 at 22:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.