[RESOLVED] Need some advice with refactoring

[MrNorth]

Hi!

We are developing a desktop application that uses WCF services as a service layer. Service proxies are injected into the application viewmodels, making them pretty slim, mostly just commands, revents and viewmodel specific properties.

We are having a problem with the services, they are getting very bloated.

For example, we have a SaveMeasurementAsync(measurement as Measurement) method in the service layer, that takes an object full of sampled measurements. Currently that method does the following things:

* Saves the measurements in the database (through a repository)

* Sends a MSMQ message

* Composes the MSMQ message by 15 lines of string concatenation

* Sending an email (by using another EmailHelper class)

* Has some logic that determines if to send an email or not


All this in the 320 line method, which makes it a living hell to unit test. My question to you guys is, how to refactor? The obvious thing would be to break out stuff into separate classes and inject those into the MeasurementService (we use Unity as DI container), but that may cause another problem that the MesaurementService is getting way too many dependencies. And both me and my colleague are lousy at coming up names for classes to break up the code into. Usually we just move stuff into private methods, but that is an anti pattern itself and doesn't really solve the problem, the MeasurementService still is a "god" class, only the single method has less code.

So, some solid questions:

1) Exactly what should the MeasurementService class do?

2) What other classes could the logic be split up into? I would really appreciate some suggestions on names, or some patterns to follow? We really want to follow the single responsibility principle, but we have noticed that it is easier said than done.


We have another project with the exact same symptomes, but there we don't use WCF, so in that case the viewmodels are the ones that does "everything". One viewmodel has 3000 lines of code and 29 private methods. No one has even thought about writing unit tests for that one.


We are getting desperate here, and would really appreciate your help guys!!!

/Henrik

[techgnome]

* Saves the measurements in the database (through a repository)

* Sends a MSMQ message

* Composes the MSMQ message by 15 lines of string concatenation

* Sending an email (by using another EmailHelper class)

* Has some logic that determines if to send an email or not

There is nothing in any of that which suggests to me multiple classes... methods, sure... classes, no.

this is all opinion and personal preference, but I'd have a public method that becomes your service endpoint.
Each of the items above becomes private methods... the public method then calls each one as needed.
in very rough terms it looks like this:

public class MeasurementService

private Sub SaveMeasurements({parameters})
.. stuff to store the measurements
End Sub

private Sub CreateMSMQMessage({parameters})
.. create the MSMQ message...
End Sub

private Function ComposeMSMQMessage({parameters}) as string (or what ever you want)
.. Compose the message
  Return MSMQMessage
End Function

private function SendEmailCheck({parameters}) as Boolean
... do logic to determine if you should send the email
  Return True/False
End Function

Private Sub SendEmail({parameters})
... send your email here
End Sub

Public SaveMeasurementAsync({parameters})
  SaveMeasurements({parameters})

  dim MsmqMessage As String = ComposeMSMQMessage({parameters})
  CreateMSMQMessage(MsmqMessage, {parameters})

  If SendEmailCheck({parameters}) Then
    SendEmail({parameters})
  End If

End Sub

End Class

Each method now does one thing... even the endpoint is doing one thing - it's now managing the process rather than being the process itself.


Now for testing... if you're using test-driven development, you create the shell, no code... just the stubs... you call the endpoint... then you start filling in the code for each method - start with the Save method... then call the endpoint... did the data save? and add in each as you go, running the tests in between each round.

If you're refactoring and re-designing... you break it all out, then I'd block out everything in the endpoint... then add them back in one call at a time, running the tests in between each round (similar to the TDD above, but instead of adding code, we're simply switching on code as we go).

-tg

[MrNorth]

Yeha I guess you are right, the class is not big enough for an "extract class" refactoring here, even though the class does more than it should (why should it compose a message for example?) That should be the responsibility of a MeasurementMessaging-class maybe?

/H

[techgnome]

Maybe... is there another area that also calls it? if not, I'm not sure I'd bother as it's still part of the same overall singular process.

There's a fine line before you tip over in to the over-engineered realm.

-tg