This is a prime candidate for the Strategy pattern.
For example, this code:
if ($this->isDisputeAccepted($order)) { //returns true if dispute was accepted
$order->setStatus('accepted');
$order->refund(); //refunds the order on ebay and internally in my system
$this->insertRecordInOrderHistoryTable($order,'refunded');
} else if ($this->isDisputeCancelled($order)) { //returns true if dispute was cancelled
$order->setStatus('cancelled');
$this->insertRecordInOrderHistory($order,'cancelled');
$order->rollBackRefund(); //cancels the refund on ebay and internally in my system
} else if ($this->isDisputeOlderThan7Days($order)) { //returns true if 7 days elapsed since the dispute was opened
$order->closeDispute(); //closes the dispute on ebay
$this->insertRecordInOrderHistoryTable($order,'refunded');
$order->refund(); //refunds the order on ebay and internally in my system
}
Could be reduced to
var $strategy = $this.getOrderStrategy($order);
$strategy->preProcess();
$strategy->updateOrderHistory($this);
$strategy->postProcess();
where getOrderStrategy wraps the order in a DisputeAcceptedStrategy, DisputeCancelledStrategy, DisputeOlderThan7DaysStrategy, etc. each of which know how to handle the given situation.
Edit, to answer question in comments.
could you please elaborate more on your code. What I understood is that getOrderStrategy is a factory method that returns a strategy object depending on the order status, but what are the preProcess() and preProcess() functions. Also why did you pass $this to updateOrderHistory($this)?
You're focussing on the example, which could be completely inappropriate for your case. I don't have enough detail to be sure of the best implementation, so I came up with a vague example.
The one common piece of code you have is insertRecordInOrderHistoryTable, so I chose to use that (with a slightly more generic name) as the central point of the strategy. I pass $this to it, because it is calling a method on this, with $order and a different string per strategy.
So, basically, I envision each of those looking like this:
public function updateOrderHistory($auth) {
$auth.insertRecordInOrderHistoryTable($order, 'cancelled');
}
Where $order is a private member of the Strategy (remember I said it should wrap the order) and the second argument is different in each class. Again, this might be entirely inappropriate. You might want to move insertRecordInOrderHistoryTable to a base Strategy class and not pass the Authorization class in. Or you might want to do something entirely different, it was just an example.
Likewise, I limited the rest of the differing code to pre- and postProcess methods. This is almost certainly not the best you can do with it. Give it more appropriate names. Split it down into multiple methods. Whatever makes the calling code more readable.
You might prefer to do this:
var $strategy = $this.getOrderStrategy($order);
$strategy->setStatus();
$strategy->closeDisputeIfNecessary();
$strategy->refundIfNecessary();
$strategy->insertRecordInOrderHistoryTable($this);
$strategy->rollBackRefundIfNecessary();
And have some of your Strategies implement empty methods for the "IfNecessary" methods.
Whatever makes the calling code more readable.
There are different patterns in software development; MVP, MVVM, MVC, etc. are some of the well-known ones. However, you have to define the specific problem or technology that you are intending to solve or use.
Each of these patterns is good to solve some specific sets of problems. For example, the MVP (Model View Presenter) pattern helps to introduce separation of concerns in ASP.NET WebForms development. It consists of splitting up the responsibilities for gathering, displaying, and storing data from a web page into separate objects: a Model object, a View object, and a Presenter object.
The most famous general cookbook of design patterns is Gang of Four (GoF) design patterns.
Edit: i suppose that you are more interested in implementing design patterns on .NET platform
Best Answer
Considerations from your questions and comments.
Tight Coupling:
You stated:
(1) is tightly coupling two parts of the system in a way which they don't belong together.
(1) also contradicts the code example you have:
As the two are opposite approaches.
I can understand the thought as to why they're related/joined as per (2), as you are using Login things in Signup together, and they have some real world similarity in that Signup and Login have some shared notions to the end user. You also want to check they're not logged in before signing them up.
For checking they're not logged in, this is possibly something that happens on all pages, so you would already know. But if not, then the Signup controller would call a separate service to find out if they are logged in. This other service would be re-used on various pages, so you define once in one class what is "logged in" and it's re-used throughout your code.
Classes do have dependencies all the time, bringing other objects in (eg via DI) however, in this case in the system these controllers are not directly related at all.
Bringing objects into a class is one thing and will often have some form of coupling. Classes will have dependencies from other parts of the system otherwise we'd not be able to adhere to SRP. But "extend" is a very tightly coupled relationship and shouldn't be seen as some kind of first port of call a developer should do unless there is very good reason, and then it's usually very obvious.
Classes can only extend one class. Signup has used that privilege with "Login Class". But doesn't Signup class need a validation class to validate the Signup form data? And a database class (eg a service calling a repository), to determine if the username is not in use? Etc.
So why extend Login class instead of database class? Well the answer is not "which is more important to choose which to extend" but "should this be coupled or not". Chances are you don't extend any of these, you would use composition.
A good but simple example of Composition
Also see Composition over Inheritance
Re your statement (1) - You cannot login before signing up, so Login has no business asking Signup to come to its party as then you're stating that Login is in charge of Signup, which is very wrong.
It would arguably be more logical to state the other way around "Signup gets a Login object" (re your code example), but in fact even Signup has no business asking Login to come to its party even when Signup is completed. This is because they are separate parts of the system, and Signup does not need Login to handle it's own single responsibility - Signup!
The requirements are separately "Signup" then "Login", so combining those specific requirements is "Login after Signup", not "use Signup to be able to eventually Login".
This said, Login and Signup should share some common ground from the system, such as using the same code for Login form and user credential validation, as these should be shared for re-use to maintain DRY. However such a common ground does not mean you should pin the controlling classes together like this (1) or even bring in an instance of the other's object.
In some cases some sort of mediator might make sense, that would bring together shared requirements and objects (such as a service), but in this case you'd just hand over to the next part of the system to log the user in after Signup.
In a different slightly stretched example just to reiterate the above point:
Say you send them to the UserProfile page after Login, you wouldn't bring the UserLogin object into the UserProfilePageController so that once Login is done the system is already in the profile controller ready to go.
The login system would be called, and then the login controller would hand over to whatever resource was requested - a page prior to Login that redirected them to Login, or simply the user profile page, etc. (Although this is more likely to be something else controlling this, front controller or internal wiring etc.)
Is tightly coupled so bad?
The problem with tightly coupling these things together with (statement 1) is when you just want to simply log someone in (they signed up months ago) your Login is still bringing in Signup object when you don't need it.
Or worse/equally as bad, you have to make a 2nd Login class just for pure Login that is not coupled to Signup. Then you have violated DRY as the two Login classes will be doing the same thing, they're just duplicated to (wrongly) cater for your bad design in coupling things.
The other issue is when you want to refactor the Login or the Signup you have to consider the other one because they're tightly coupled to each other's objects.
Of course such tight coupling can be valid, and dependencies are very much shared between classes. The key is good separation and de-coupling wherever possible means parts of the system run independent of each other and changes only effect that one part of the system.
The likely better approach:
Signup Request
SignupForm
andLoginForm
SignupFormValidation
andLoginFormValidation
Login Request
LoginForm
LoginFormValidation
See how Signup and Login both call the same
LoginForm
andLoginFormValidation
? This means (e.g.) if you decide that username can be 20 chars instead of 18, you make change in a centralised place (LoginForm
andLoginFormValidation
) and Signup and Login both inherit the same changes.To log them in when they've signed up, I would suggest a mediator of some kind, such as a service. Probably called something like
LoginAfterSignupService
(I'm open to this being debated as there may be a better approach).Signup will pass the username and password to this service who's job is to hand over to the Login system. It will pass the username/password (etc) to the Login system , which will just do it's normal thing and verify the username and password and log them in.
The login system will always need a username and password to verify the user, whether than comes from a login form or a mediator service that the signup system can call shouldn't matter. The login system shouldn't care where that comes from.
The reason I suggested a mediator service instead of Signup just handing over to the login system is because you need something to handle the specific case that, we're not on a login page, we're not on Signup, we're in the middle. So if login fails you are in a separate service who's job is solely to handle the middle ground of "login after signup" and can do some specific checks if login returns false (fails).
For example, login page would just state "sorry wrong username or password", but we've just signed up and that message would be confusing to the user. The specific service could instead handle it more appropriately (whatever would be appropriate depends on your system).
Quick note on your code in your question:
Sorry but this is a bad smell:
"Login" class should not have a method "Logout". The code to use this would look awkward:
Just make a separate logout class so you would do this:
You may find now or in the future that your logout process changes, to sending an email, or checking something specific (etc etc). If this is a class these new requirements are valid methods in that class. If "logout" is a method in "login" class, apart from being a violation of SRP, you now have to stuff all kinds of "logout" requirements into a single "logout" method in the "login" class.
This is very roughly what I think it would look like
You'd probs want some sanity checking in there, and throw some exceptions perhaps (or the services this class calls might throw them, like in the RegisterUserService and LogUserInAfterSignupService).