I'm learning about SOLID principles, and ISP states that:
Clients should not be forced to depend upon interfaces that they do not use.
Does using default methods in interfaces violate this principle?
I have seen a similar question but I'm posting here with an example to get a clearer picture if my example violate ISP. Say I have this example:
public interface IUser{
void UserMenu();
String getID();
default void closeSession() {
System.out.println("Client Left");
}
default void readRecords(){
System.out.println("User requested to read records...");
System.out.println("Printing records....");
System.out.println("..............");
}
}
With the following classes implementing IUser Interface
public class Admin implements IUser {
public String getID() {
return "ADMIN";
}
public void handleUser() {
boolean sessionIsOpen = true;
while (sessionIsOpen) {
switch (Integer.parseInt(in.readLine())) {
case 1 -> addNewUser();
case 2 -> sessionIsOpen=false;
default -> System.out.println("Invalid Entry");
}
}
closeSession();
}
private void addNewUser() {
System.out.println("Adding New User..."); }
}
}
Editor Class:
public class Editor implements IUser {
public String getID() {
return "EDITOR";
}
public void handleUser() {
boolean sessionIsOpen=true;
while (sessionIsOpen){
switch (Integer.parseInt(in.readLine())) {
case 1 -> addBook();
case 2 -> readRecords();
case 3 -> sessionIsOpen=false;
default ->
System.out.println("Invalid Entry");
}
}
closeSession();
}
private void addBook() {
System.out.println("Adding New Book..."); }
}
}
Viewer Class
public class Viewer implements IUser {
public String getID() {
return "Viewer";
}
public void handleUser() {
boolean sessionIsOpen=true;
while (sessionIsOpen){
switch (Integer.parseInt(in.readLine())) {
case 1 -> readRecords();
case 2 -> sessionIsOpen=false;
default ->
System.out.println("Invalid Entry");
}
}
closeSession();
}
}
Since editor and viewer class use readRecords() method and Admin class doesn't provide an implementation for that method, I implemented it as a default method in IUser Interface to minimize code repetition (DRY Principle).
Am I violating the interface segregation principle in the above code by using default methods in IUser because the Admin class does not use the read method?
Can someone explain please, because I think I'm not forcing Admin class to use methods/interfaces that they do not use.
does using default methods in interfaces violate the principle?
No, not if they're used correctly. In fact, they can help to avoid violating ISP (see below).
Does your example of using default methods violate ISP?
Yes! Well, probably. We could have a debate about exactly how badly it violates ISP, but it definitely violates a bunch of other principles, and isn't good practice with Java programming.
The problem is that you're using a default method as something for the implementing class to call. That's not their intent.
Default methods should be used to define methods that:
Your example appears to break several conditions.
The first condition is there for a simple reason: all inheritable methods on Java interfaces are public, so they always can be called by users of the interface. To give a concrete example, the below code works fine:
Admin admin = new Admin();
admin.closeSession();
admin.readRecords();
Presumably, you don't want this to be possible, not just for Admin, but for Editor and Viewer too? I would argue that this is a sort-of violation of ISP, because you are depending on users of your classes not calling those methods. For the Admin class, you could make readRecords() 'safe' by overriding it and giving it a no-op implementation, but that just highlights a much more direct violation of ISP. For all other methods/implementations, including the classes that do make use of readRecords(), you're screwed. Rather than thinking of this in terms of ISP, I'd call it API or implementation leakage: it allows your classes to be used in ways that you didn't intend (and may wish to break in the future).
The second condition I stated might need further explanation. By aggregate functionality, I mean that the methods should probably call (either directly or indirectly) one or more of the abstract methods on the interface. If they don't do that, then the behaviour of those methods can't possibly depend on the state of the implementing class, and so could probably be static, or moved into a different class entirely (i.e. see the Single-responsibility principle). There are examples and use cases where it's OK to relax this condition but they should be thought about very carefully. In the example you give, the default methods are not aggregate, but it looks like sanitized code for the sake of Stack Overflow, so maybe your "real" code is fine.
It's debatable whether 2/3 implementers counts as "most" with with regards to my third condition. However, another way to think about it is you should know in advance of writing the implementing classes whether or not they should have that method with that functionality. How certainly can you say whether in the future, if you need to create a new class of User, they will require the functionality of readRecords()? Either way, it's a moot point as this condition only really needs to be thought about if you haven't violated the first 2.
There are examples in the standard library of good uses default methods. One would be java.util.function.Function with its andThen(...) and compose(...) methods. These are are useful pieces of functionality for users of Functions, they (indirectly) make use of the Function's abstract apply(...) method, and importantly, it's highly unlikely that an implementing class would ever wish to override them, except maybe for efficiency in some highly specialized scenarios.
These default methods do not violate ISP, as classes that implement Function have no need to call or override them. There may be many use-cases where concrete instances of Function never have their andThen(...) method called, but that's fine – you don't break ISP by providing useful but non-essential functionality, as long as you don't encumber all those use-cases by forcing them to do something with it. In the case of Function, providing these methods as abstract rather than default would violate ISP, as all implementing classes would have to add their own implementations, even when they know it's unlikely to ever be called.
Use an abstract class!
Abstract classes have been poo-pooed a lot in discussions about good Java practice, because they were frequently misunderstood, misued and abused. It wouldn't surprise me if at least some programming best-practice guides like SOLID were published in reaction to this misuse. A very frequent issue I've seen is having an abstract class provide a "default" implementation for tons of methods that is then overridden almost everywhere, often by copy-pasting the base implementation and changing 1 or 2 lines. Essentially, this is breaking my third condition on default methods above (which also applies to any method on an intended-to-be-subclassed type), and it happens A LOT.
However, in this scenario, abstract classes are probably just what you need.
Something like this:
interface IUser {
// Add all methods here intended to be CALLED by code that holds
// instances of IUser
// e.g.:
void handleUser();
String getID();
// If some methods only make sense for particular types of user,
// they shouldn't be added.
// e.g.:
// NOT void addBook();
// NOT void addNewUser();
}
abstract class AbstractUser implements IUser {
// Add methods and fields here that will be USEFUL to most or
// all implementations of IUser.
//
// Nothing should be public, unless it's an implementation of
// one of the abstract methods defined on IUser.
//
// e.g.:
protected void closeSession() { /* etc... */ }
}
abstract class AbstractRecordReadingUser extends AbstractUser {
// Add methods here that are only USEFUL to a subset of
// implementations of IUser.
//
// e.g.:
protected void readRecords(){ /* etc... */ }
}
final class Admin extends AbstractUser {
@Override
public void handleUser() {
// etc...
closeSession();
}
public void addNewUser() { /* etc... */ }
}
final class Editor extends AbstractRecordReadingUser {
@Override
public void handleUser() {
// etc...
readRecords();
// etc...
closeSession();
}
public void addBook() { /* etc... */ }
}
final class Viewer extends AbstractRecordReadingUser {
@Override
public void handleUser() {
// etc...
readRecords();
// etc...
closeSession();
}
}
Note: Depending on your situation, there may be better alternatives to abstract classes that still achieve DRY:
If your common helper methods are stateless (i.e. don't depend on fields in the class), you could use an auxiliary class of static helper methods instead (see here for an example).
You might wish to use composition instead of abstract class inheritance. For example, instead of creating the AbstractRecordReadingUser as above, you could have:
final class RecordReader {
// Fields relevant to the readRecords() method
public void readRecords() { /* etc... */ }
}
final class Editor extends AbstractUser {
private final RecordReader r = new RecordReader();
@Override
void handleUser() {
// etc...
r.readRecords();
// etc...
}
}
// Similar for Viewer
This avoids the problem that Java doesn't allow multiple inheritance, which would become an issue if you tried to have multiple abstract classes containing different pieces of optional functionality, and some final classes needed to use several of them. However, depending on what state (i.e. fields) the readRecord() method needs to interact with, it might not be possible to separate it out into a separate class cleanly.
You could just put your readRecords() method in AbstractUser and avoid having the additional abstract class. The Admin class isn't obliged to call it, and as long as the method is protected, there's no risk that anyone else will call it (assuming you have your packages properly separated). This doesn't violate ISP as even though Admin can interact with readRecords(), it isn't forced to. It can pretend that method doesn't exist, and everyone is fine!