Have I properly commented this piece of my query?

Question

I feel like this deserves a good comment but I get the feeling that what I have is merely a distraction.

1. Does this deserve a comment?
2. If so, can this comment be improved?

Please note that I am working with a poorly normalized application.

(   
    /* Remove Time Portion From ActivityDate */  
    DateAdd(Day, DateDiff(Day, 0, Activity.ActivityDate), 0) Not Between        
    /* Remove Time Portion From @MinimumDate Or SQL Server Min DateTime */   
    DateAdd(Day, DateDiff(Day, 0, Coalesce(@MinimumDate, '1753-01-01')), 0) And
    /* Remove Time Portion From @MaximumDate Or SQL Server Max DateTime */  
    DateAdd(Day, DateDiff(Day, 0, Coalesce(@MaximumDate, '9999-12-31')), 0) 
)

Answer 1

Personally, I think you don't need comments that say removes X from Y or increments X by Y etc...

That being said, if you feel like you need to comment a particular section of code, I'd try to focus on the intent and big picture of the functionality. For instance, why were things implemented this way? That way, the next guy that comes along behind you will have a fighting chance when he has to make changes.

For instance, I know what your snippet of code does functionally, but I have no idea why it is there or how it relates to the rest of the application. A possible improvement could be something along the lines of an explanation of the functionality that you would give someone new to the code, or someone who might not care how it's implemented, but why.

I always have to remind myself that programming is communicating and the more clearly you can communicate your intent to other programmers, the better off you'll be. If you can find a way to add comments that improves the quality of communication between developers, then go for it. I'd argue that comments that tell you exactly what the code is doing is counterproductive and hurts communication in the end.

Answer 2

The comments are helpful to me, because I would have had to think about what those lines are doing if they were not there. Although as Robert Greiner says, why you are doing this would also be good to know.

Answer 3

I'd wrap the dateadd/datediff in a scalar udf with a self commenting name and then pass in Activity.ActivityDate or Coalesce(@MinimumDate, '1753-01-01')) as parameters

So you'd have this:

(   
    dbo.ufnGetDateOnly (Activity.ActivityDate) NOT BETWEEN        
         dbo.ufnGetDateOnly (COALESCE(@MinimumDate, '1753-01-01')) AND
         dbo.ufnGetDateOnly (COALESCE(@MaximumDate, '9999-12-31')) 
)

You could also have a "date if null" parameter and deal with COALESCE in the udf if it's common enough in the SQL code

(   
    dbo.ufnGetDateOnly (Activity.ActivityDate, DEFAULT) NOT BETWEEN        
         dbo.ufnGetDateOnly (@MinimumDate, '1753-01-01') AND
         dbo.ufnGetDateOnly (@MaximumDate, '9999-12-31') 
)

Now it's obvious...no?

Answer 4

Do not document what you do. It must be self explanatory what a single line of code does. Document:

• What is the primary role, implemented logic of a block? (e.g. a function, a loop)
• Constraints, assumptions (null value?, thread safe? read-only?)
• Why is it like this and not elsehow? (e.g. Architectural decisions, we are going to use this XML parses because we need namespace support and because that other one does not support EBDIC encoding)
• Where is this in the big picture? Where is it called from?
• Schema (including DB constraints, assumptions, e.g. escaped?)
• Implications (e.g. security considerations, side effects)
• Dependencies (e.g. libraries, headers, a magic lock file on a floppy disk)