Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I'm currently developing a web-app by myself and have made it a point to use descriptive variable and method names (sometimes at the expense of brevity) in order to minimize commenting. The plan was to code each method using this strategy, then comment after I've completed the method I was currently working on.

However, I've found that after (more or less) all of the methods I've completed, in-line comments seemed superfluous. I still follow Javadoc conventions for classes and methods, but as of now my code is for the most part, completely devoid of any in-line comments.

Fortunately, I'm still relatively early in the development process and all of the methods' workings are fresh in my head, should a situation arise where I need to write in-line comments.

Is this a good strategy? If not, where should I include in-line comments? I've included one of my methods below in order to illustrate how self-documenting it currently is.

(I'm using Pelops, which is a Java library used to access a Cassandra database. Without going in too deep in to Cassandra's data model, a row basically corresponds to a relational database tuple, and a column corresponds a column in a relation database table).

I'd like to think that even without knowledge of Pelops or Cassandra, that one will be able to understand what is going on in the code. Is this the case? If not, where can I insert comments to make it crystal clear? I hope to abstract any suggestions on comment placement in this code in to all the other methods I've written so far.

/**
     * Gathers a user's ID and first name from the database, creates a random activation code for that user and stores
     * it in the database, and sends an activation e-mail to that user.

     * @param eMail                           a String representation of the e-mail of the user that the e-mail is being sent for
     * @throws MyAppConnectionException      if any of the queries is unable to be executed or a problem arises because of one, or if a problem arises while sending the e-mail
     */
    public static void generateAndSendActivation(String eMail) throws MyAppConnectionException, MyAppActivationException
    {
        eMail = eMail.toLowerCase();

        try
        {
            Selector userIDSelector = Pelops.createSelector(pool);
            Column userIDColumn = userIDSelector.getColumnFromRow("Users_By_Email", eMail, "User_ID", ConsistencyLevel.ONE);
            String userIDString = new String(userIDColumn.getValue());

            Selector firstNameAndStatusSelector = Pelops.createSelector(pool);
            SlicePredicate firstNameAndStatus = Selector.newColumnsPredicate("First_Name", "Status");
            List<Column> firstNameAndStatusColumns = firstNameAndStatusSelector.getColumnsFromRow("Users", userIDString, firstNameAndStatus, ConsistencyLevel.ONE);

            char statusChar = Selector.getColumnValue(firstNameAndStatusColumns, "Status").toChar();

            if(statusChar == 'N')
            {
                String firstNameString = Selector.getColumnStringValue(firstNameAndStatusColumns, "First_Name");

                String activationCode = createRandomCode();
                String activationHash = BCrypt.hashpw(activationCode, BCrypt.gensalt());

                Mutator storeActivationHashMutator = Pelops.createMutator(pool);
                storeActivationHashMutator.writeColumn("Users", userIDString,
                        storeActivationHashMutator.newColumn("Activation_Code", activationHash)
                        );

                storeActivationHashMutator.execute(ConsistencyLevel.ONE);
                sendEmail(firstNameString, userIDString, eMail, activationCode, "sendActivationCode");
            }
            else
            {
                throw new MyAppActivationException("User with ID " + userIDString + "tried to activate an already activated account");
            }

        }
        catch(NotFoundException nfe)
        {
            MyAppQueryException bqe = new MyAppQueryException("Account for user with e-mail " + eMail + " not found. Cannot send activation code", nfe);
            bqe.accountDoesNotExist = true;
            throw bqe;
        }
        catch(PelopsException pe)
        {
            MyAppQueryException be = new MyAppQueryException("Unable to carry out one of the operations required to generate and store activation code", pe);
            throw be;
        }
        catch(MyAppMailException bme)
        {
            throw bme;
        }
    }
share|improve this question

migrated from stackoverflow.com May 24 '11 at 13:35

This question came from our site for professional and enthusiast programmers.

1  
I'd say your method is too long to be self documenting. I'd start with putting the whole of the try block into a method and take it from there. Also, if a method has 'and' in the name that implies it's doing two things, try splitting this into two methods. –  Anonymous May 23 '11 at 18:01
    
You think so? I don't think the method is particularly long. Approximately half of the length is creating and throwing exceptions. I don't consider that "real code" :). And it is split in to two methods, it calls "sendEmail" to send the actual eMail. Since I consider creating, storing, and sending an activation code one action, I decided to have this function call the e-mail function, rather than have the caller invoke 2 separate functions for an action I deem to be atomic. –  Kevin May 23 '11 at 18:04
    
You should only need to comment a section of code is you feel somebody who has never seen your code before wouldn't understand what your code is doing. –  Ramhound May 23 '11 at 18:14
    
Yes it's too long and it's doing more than one thing at different levels of abstraction - you can see this in the exceptions it's catching - what's a PelopsException got to do with a MyAppEmailException? –  Anonymous May 23 '11 at 18:22
    
This is noise: "Gathers a user's ID and first name from the database". Skip this part. If I need to know that I can read the code. This function activates a user, sending an E-mail to the user. –  kevin cline May 25 '11 at 7:44
add comment

3 Answers

First of all, I think you should split such method into several and avoid magic numbers /chars

for example it can be smt like

User user = retrieveUser(email);
if(user.isNew()){
     String activationCode = calcActivationCode();
     addUserToDb(activationCode );
     sendEmailWithActivationCode(email, activationCode);
}

And, then implement retrieveUser, addUserToDb, addsendEmail

share|improve this answer
1  
Good point on that "user.isNew()" (over magic constants). –  StaxMan May 23 '11 at 18:09
    
@Stas Kurilin: "user.isNew()" is a good suggestion. The creation of the activation code and the sending of the e-mail are already invoked as functions though. –  Kevin May 23 '11 at 18:14
    
@Kevin, main idea is to separate flow logic from working with DB. And so on. Decomposition rulez) –  Stas Kurilin May 23 '11 at 18:21
    
Yes, that's how you make code self documenting. –  Martin Wickman May 24 '11 at 13:48
add comment

I often write code just like this. With very little, often nothing, written for comments in-line. A good practice would be to have your tests be your documentation. If you want to know what something does take a look at its unit tests as they will tell the best story.

Also, you may want to create an abstraction around your data access implementation. You could create a DAO to hide the actual implementation in the code that uses the data access. This would make it more clear that something is being created/updated/deleted/read quite easily in the code that uses it. If they need to know what is going on inside the actual implementation, they will have to find which is being used (likely just your one implementation), and at that point, the names of the methods should be fairly descriptive.

Ultimately do what works for you. If you code changes often, it may not make sense to have lots of comments, as you may find it hard to update the documentation so often. Then again, if you are fine with changing documentation often, this wouldn't be a problem.

share|improve this answer
add comment

I think use of good names, use of per-class/per-method javadocs where useful, and keeping methods short enough are good starting points.

As to inline comments: as all commenting, there are no hard and fast rules; and sometimes little bit goes a long way. I do not, however, believe in that one never needs them, nor that this should be a goal. Obviously you shouldn't try to add any specific number of comments (either by absolute or relative measure).

For what it is worth, I think inline comments seem most useful to me when they explain control flow. In your case, for example, one might consider commenting above that 'if(statusChar == 'N')' statement. Problem with per-method comments are that they are not right next to exact place where things happen.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.