I believe that most of the developers try to write code to be readable as much as possible. I will try to explain how I try to achieve this. From my point of view one of the most important things is to write public methods as stories. This means when somebody is reading your public method its implementation should tell with the sentence like methods what it tries to achieve. This way one can concentrate on the business logic that the method tries to achieve and not how that is achieved.
So I try to follow few short rules:
- Instead of code within public method call number of private methods
- Avoid loops
- Try to minimize if statements
- If private method is hard to read apply this rules to private method
Following this rules your public methods are easy to understand. It is easier to test if you are doing black-box testing. What may happen to your code is that you will have number of private methods if you implement mentioned rules also on private methods. But I don't consider this to be bad because in that case even private methods are easy to understand so if one should change your code it will be easier.
As opinion is easier to understand through example, I will show one method from my http://www.flexiblefeeds.com site.
Although example is in groovy I believe it will be easily understandable for all developers. Currently method looks like this:
def currentUserVote(Long articleId, boolean upVoting) {
boolean canVote = canCurrentUserVote(articleId)
if (!canVote) {
return
}
vote(articleId, upVoting)
registerVoting(articleId)
}I believe that is is easy to understand what the method does from the code itself. But to be sure, first it is checked if current user can vote. If user cannot vote method returns. If user can vote voting is done and then voting is registered.
Now let us see how this method can look like if the code would be embedded into this public method.
def currentUserVote(Long articleId, boolean upVoting) {
// decide if user can vote
if (!loggedInUserIsAdministrator()) {
return
}
// logged in user can vote if he didn't voted
if (userIsLoggedIn()) {
return !voted(loggedInUser().id, articleId)
}
// not logged in user can vote if he didn't voted and data is stored in session
if(votedInSession(articleId)) {
return
}
// perform voting
try {
String sql
if (upVoting) {
sql = "SQL_FOR_UP_VOTING"
} else {
sql = "SQL_FOR_DOWN_VOTING"
}
Article.executeUpdate(sql, [id:articleId])
} catch (Exception ex) {
log.error("Failed to vote up for article ${articleId}", ex)
throw ex;
}
// register voting
if (loggedInUser()) {
def a = Article.get(articleId)
try {
ArticleVoting voting = new ArticleVoting(user:loggedInUser(), article:a)
voting.save(flush:true)
} catch (Exception ex) {
log.error(ex)
throw ex;
}
} else {
if (!session().votedIds) {
def votedIds = [] as Set
session().votedIds = votedIds
}
session().votedIds.add(articleId)
}
}Having look at this method you can notice it is possible to understand what is method doing. But beside understanding what is method doing you are reading code. It means you are doing two things at the same time. Trying to understand business logic and trying to understand how this business logic is achieved.Therefore in all cases I would recommend to refactor such code and to extract parts of the public methods into private methods.
