I have been reading the "Clean code" book and particularly the part on Hybrid classes and the difference between data structures and objects.
I have an SFTPUtil class with one method for deleting a file from an sftp server.
Currently it looks like what is described as a Hybrid class like so:
String host;
int port;
String rootDirectory;
String username;
String password;
public void deleteFile(String filename){
// Code using the instance variables above
}
//GETTERS AND SETTERS FOR INSTANCE VARIABLES
But based on this book, it would seem better to just have:
public void deleteFile(String filename, String host, int port, String rootDirectory, String username, String password){
// Code using the instance variables above
}
And no instance variables.
Is this a correct assumption? What are peoples thoughts on this and are there better ways again to design this?
The approach depends on the need to reuse your instance variables or your objects. If you need to set your instance variables to use them only once, you are better off without instance variables. However, if you plan on reusing the the values of your instance variables, you are better off with a class to hold their values.
There is a third approach, which strikes a balance between reusing instances and not passing too many parameters. You can define a class to hold SFTPLoginCredentials, put instance variables in it, and then provide static methods in the utilities class that use variables from the credentials object:
public class SFTPLoginCredentials {
private final String host;
private final int port;
private final String rootDirectory;
private final String username;
private final String password;
// Add a constructor and getters for all these instance variables
}
public static class SFTPUtils {
public static void deleteFile(String filename, SFTPLoginCredentials credentials);
public static void uploadFile(InputStream data, String filename, SFTPLoginCredentials credentials);
public static void downloadFile(String filename, OutputStream data, SFTPLoginCredentials credentials);
}
Now you can create a single SFTPLoginCredentials object, and keep using it in your SFTPUtils class, which can become stateless (a major driving factor behind the desire to not introduce unnecessary instance variables).
First of all: it's mainly a matter of personal preference. If you feel like you can work easier with one way than the other, then you've got your answer.
That being said you will want to take a look at how you intend to use this class.
I see two main possibilities on how you will use this:
- Remove many files from a server
- Remove a file from many servers
If you have the first use case then it will make a lot of sense to use instance variables for the connection and a method that just takes a filename. This won't force you to keep specifying the connection information and might feel cleaner.
If you have the second use case then it will not make much sense to store them in fields because you will barely use it.
All this combined I'd probably lean towards the first approach myself. A method should have one responsibility and both making the connection AND deleting a file doesn't feel right and by taking the first approach you will also be 'safe' from the code smell that would appear should you have chosen the second approach and it turns out you want to delete many files from the servers in the future (as described in the first approach).
If this is something that is not likely to change keep it as simple as possible (the book approach looks fine in that regard), but it really depends what is the requirement and how do you anticipate the code to change in the future.
Lets say you want to add more sftp commands, add caching of connections or have the option to change the implementation in the future this all effects how you design a class. but I think the key point here is don't over design.
