TODOs in comments

[Sudheesh Katkam <sk...@maprtech.com>]

Hey y’all, 

For consistency across code, Chris had recommended using TODO(DRILL-####) format for todos in comments, where #### is the JIRA number. If there are no objections, let’s try to stick to that format.

Thank you,
Sudheesh

Re: TODOs in comments

[Parth Chandra <pc...@maprtech.com>]

+1 on asking who is likely to fix your TODO !
In general you tend to ignore a TODO that you did not put in. Even with
TODO's that you yourself put in, action never gets taken unless there is a
_concrete_ action item to follow up. As far as I can see, a JIRA is the
closest we will get to having a concrete action item and so it perhaps
makes sense to put in TODO's only if there is a JIRA associated.
My personal opinion is that otherwise TODO's end up adding to kruft and
confusion.
As an additional data point, there are 853 TODO's in the Drill code. Some
are over a year old. I don't see them being cleared, so clearly they have
not served the purpose.

On Tue, May 5, 2015 at 10:58 PM, Julian Hyde <ju...@gmail.com> wrote:

> My boss at Oracle used to say “There’s a TODO in this code. Why are you
> checking it in? It isn’t finished.”
>
> A bit harsh, but he had a good point.
>
> A TODO can indicate something that is not implemented because it is
> outside the current specification. If something isn’t supported
> functionality yet,
>
>    throw new AssertionError(“cannot whizzbang yet”);
>
> seems better, in my opinion, than
>
>   // TODO: fix this
>   return null;
>
> Also, ask yourself who is likely to fix your TODO. Most likely it will be
> you. Or maybe no one will ever get round to it. Either way, you are
> cluttering up the code for everyone else who is not going to fix it.
>
> Lastly, there are genuinely cases where you wrote the simplest thing that
> could possibly work and you did not write a more efficient but more complex
> algorithm. Well done. This is good software engineering. You want to tell
> the world that your algorithm might be found to be slow, and you want to
> receive a little credit for having anticipated the problem. Also totally
> fine. But don’t write “// TODO: convert this bubble sort to a merge sort”.
> That makes it sounds as if there is a bug in your code, and your code is
> perfect (unless and until someone finds a performance issue). Just write a
> nice paragraph explaining your design choices, possible issues and how they
> would manifest themselves.
>
> I would not ban TODOs outright, but I think developers should think three
> times before checking one in.
>
> Julian
>
>
> > On May 5, 2015, at 10:14 PM, Daniel Barclay <db...@maprtech.com>
> wrote:
> >
> > I think that requiring every TODO note to have an associated JIRA report
> > number would be too restrictive, increasing the friction to record notes
> > about things to be looked at later.
> >
> > Making it so that one can't leave a note without the overhead of filing
> > a whole JIRA report is going to cause some things to go unnoted.  That
> > seems significantly worse than not having every note indexed in JIRA.
> >
> > Encouraging having the JIRA number sounds fine; requiring it, at least
> > without having some alternative mark for less-formal things, doesn't
> > seem good.
> >
> >
> >
> >
> >
> >
> > Daniel
> >
> > Sudheesh Katkam wrote:
> >> Yes, TODOs must have an associated JIRA, with the specified format.
> >>
> >>> On May 5, 2015, at 1:14 PM, Julian Hyde <ju...@gmail.com> wrote:
> >>>
> >>> Is the proposal to disallow TODOs that do not have a JIRA case number?
> I’d be +1 to that.
> >>>
> >>> I’m much less concerned with the problem that TODO(DRILL-abcd) might
> linger after in the code after DRILL-abcd has been fixed.
> >>>
> >>> Julian
> >>>
> >>>
> >>> On May 5, 2015, at 12:38 PM, Jason Altekruse <al...@gmail.com>
> wrote:
> >>>
> >>>> It could optionally be passed manually as a flag, but we already have
> the
> >>>> build step that is generating the git.properties file, we could issue
> the
> >>>> same type of call to git to try to pull the JIRA number out of the
> commit
> >>>> message.
> >>>>
> >>>> On Tue, May 5, 2015 at 12:34 PM, Chris Westin <
> chriswestin42@gmail.com>
> >>>> wrote:
> >>>>
> >>>>> I like that idea, but how would the build know what JIRA you're
> working on?
> >>>>>
> >>>>> On Tue, May 5, 2015 at 12:25 PM, Jason Altekruse <
> altekrusejason@gmail.com
> >>>>>>
> >>>>> wrote:
> >>>>>
> >>>>>> We should also consider adding something to the build that will
> automate
> >>>>>> the process of checking for todo comments containing the JIRA number
> >>>>> being
> >>>>>> fixed. This would allow reviewers to easily verify that a JIRA being
> >>>>> closed
> >>>>>> is not leaving related TODOs in the code (possibly unit tests added
> by
> >>>>> the
> >>>>>> reporter of the issue, or comments mentioned in another patch that
> wanted
> >>>>>> to relate a problem they saw in a known outstanding JIRA). This can
> be
> >>>>>> mitigated by mentioning the relevant areas in the code when filing a
> >>>>> JIRA,
> >>>>>> but this would also be a helpful safety net to keep the code
> cleaner.
> >>>>>>
> >>>>>> - Jason
> >>>>>>
> >>>>>> On Tue, May 5, 2015 at 12:07 PM, Sudheesh Katkam <
> skatkam@maprtech.com>
> >>>>>> wrote:
> >>>>>>
> >>>>>>> Hey y’all,
> >>>>>>>
> >>>>>>> For consistency across code, Chris had recommended using
> >>>>> TODO(DRILL-####)
> >>>>>>> format for todos in comments, where #### is the JIRA number. If
> there
> >>>>> are
> >>>>>>> no objections, let’s try to stick to that format.
> >>>>>>>
> >>>>>>> Thank you,
> >>>>>>> Sudheesh
> >>>>>>
> >>>>>
> >>>
> >>
> >
> >
> > --
> > Daniel Barclay
> > MapR Technologies
>
>

Re: TODOs in comments

[Julian Hyde <ju...@gmail.com>]

My boss at Oracle used to say “There’s a TODO in this code. Why are you checking it in? It isn’t finished.”

A bit harsh, but he had a good point.

A TODO can indicate something that is not implemented because it is outside the current specification. If something isn’t supported functionality yet,

   throw new AssertionError(“cannot whizzbang yet”);

seems better, in my opinion, than

  // TODO: fix this
  return null;

Also, ask yourself who is likely to fix your TODO. Most likely it will be you. Or maybe no one will ever get round to it. Either way, you are cluttering up the code for everyone else who is not going to fix it.

Lastly, there are genuinely cases where you wrote the simplest thing that could possibly work and you did not write a more efficient but more complex algorithm. Well done. This is good software engineering. You want to tell the world that your algorithm might be found to be slow, and you want to receive a little credit for having anticipated the problem. Also totally fine. But don’t write “// TODO: convert this bubble sort to a merge sort”. That makes it sounds as if there is a bug in your code, and your code is perfect (unless and until someone finds a performance issue). Just write a nice paragraph explaining your design choices, possible issues and how they would manifest themselves.

I would not ban TODOs outright, but I think developers should think three times before checking one in.

Julian


> On May 5, 2015, at 10:14 PM, Daniel Barclay <db...@maprtech.com> wrote:
> 
> I think that requiring every TODO note to have an associated JIRA report
> number would be too restrictive, increasing the friction to record notes
> about things to be looked at later.
> 
> Making it so that one can't leave a note without the overhead of filing
> a whole JIRA report is going to cause some things to go unnoted.  That
> seems significantly worse than not having every note indexed in JIRA.
> 
> Encouraging having the JIRA number sounds fine; requiring it, at least
> without having some alternative mark for less-formal things, doesn't
> seem good.
> 
> 
> 
> 
> 
> 
> Daniel
> 
> Sudheesh Katkam wrote:
>> Yes, TODOs must have an associated JIRA, with the specified format.
>> 
>>> On May 5, 2015, at 1:14 PM, Julian Hyde <ju...@gmail.com> wrote:
>>> 
>>> Is the proposal to disallow TODOs that do not have a JIRA case number? I’d be +1 to that.
>>> 
>>> I’m much less concerned with the problem that TODO(DRILL-abcd) might linger after in the code after DRILL-abcd has been fixed.
>>> 
>>> Julian
>>> 
>>> 
>>> On May 5, 2015, at 12:38 PM, Jason Altekruse <al...@gmail.com> wrote:
>>> 
>>>> It could optionally be passed manually as a flag, but we already have the
>>>> build step that is generating the git.properties file, we could issue the
>>>> same type of call to git to try to pull the JIRA number out of the commit
>>>> message.
>>>> 
>>>> On Tue, May 5, 2015 at 12:34 PM, Chris Westin <ch...@gmail.com>
>>>> wrote:
>>>> 
>>>>> I like that idea, but how would the build know what JIRA you're working on?
>>>>> 
>>>>> On Tue, May 5, 2015 at 12:25 PM, Jason Altekruse <altekrusejason@gmail.com
>>>>>> 
>>>>> wrote:
>>>>> 
>>>>>> We should also consider adding something to the build that will automate
>>>>>> the process of checking for todo comments containing the JIRA number
>>>>> being
>>>>>> fixed. This would allow reviewers to easily verify that a JIRA being
>>>>> closed
>>>>>> is not leaving related TODOs in the code (possibly unit tests added by
>>>>> the
>>>>>> reporter of the issue, or comments mentioned in another patch that wanted
>>>>>> to relate a problem they saw in a known outstanding JIRA). This can be
>>>>>> mitigated by mentioning the relevant areas in the code when filing a
>>>>> JIRA,
>>>>>> but this would also be a helpful safety net to keep the code cleaner.
>>>>>> 
>>>>>> - Jason
>>>>>> 
>>>>>> On Tue, May 5, 2015 at 12:07 PM, Sudheesh Katkam <sk...@maprtech.com>
>>>>>> wrote:
>>>>>> 
>>>>>>> Hey y’all,
>>>>>>> 
>>>>>>> For consistency across code, Chris had recommended using
>>>>> TODO(DRILL-####)
>>>>>>> format for todos in comments, where #### is the JIRA number. If there
>>>>> are
>>>>>>> no objections, let’s try to stick to that format.
>>>>>>> 
>>>>>>> Thank you,
>>>>>>> Sudheesh
>>>>>> 
>>>>> 
>>> 
>> 
> 
> 
> -- 
> Daniel Barclay
> MapR Technologies

Re: TODOs in comments

[Daniel Barclay <db...@maprtech.com>]

I think that requiring every TODO note to have an associated JIRA report
number would be too restrictive, increasing the friction to record notes
about things to be looked at later.

Making it so that one can't leave a note without the overhead of filing
a whole JIRA report is going to cause some things to go unnoted.  That
seems significantly worse than not having every note indexed in JIRA.

Encouraging having the JIRA number sounds fine; requiring it, at least
without having some alternative mark for less-formal things, doesn't
seem good.






Daniel

Sudheesh Katkam wrote:
> Yes, TODOs must have an associated JIRA, with the specified format.
>
>> On May 5, 2015, at 1:14 PM, Julian Hyde <ju...@gmail.com> wrote:
>>
>> Is the proposal to disallow TODOs that do not have a JIRA case number? I’d be +1 to that.
>>
>> I’m much less concerned with the problem that TODO(DRILL-abcd) might linger after in the code after DRILL-abcd has been fixed.
>>
>> Julian
>>
>>
>> On May 5, 2015, at 12:38 PM, Jason Altekruse <al...@gmail.com> wrote:
>>
>>> It could optionally be passed manually as a flag, but we already have the
>>> build step that is generating the git.properties file, we could issue the
>>> same type of call to git to try to pull the JIRA number out of the commit
>>> message.
>>>
>>> On Tue, May 5, 2015 at 12:34 PM, Chris Westin <ch...@gmail.com>
>>> wrote:
>>>
>>>> I like that idea, but how would the build know what JIRA you're working on?
>>>>
>>>> On Tue, May 5, 2015 at 12:25 PM, Jason Altekruse <altekrusejason@gmail.com
>>>>>
>>>> wrote:
>>>>
>>>>> We should also consider adding something to the build that will automate
>>>>> the process of checking for todo comments containing the JIRA number
>>>> being
>>>>> fixed. This would allow reviewers to easily verify that a JIRA being
>>>> closed
>>>>> is not leaving related TODOs in the code (possibly unit tests added by
>>>> the
>>>>> reporter of the issue, or comments mentioned in another patch that wanted
>>>>> to relate a problem they saw in a known outstanding JIRA). This can be
>>>>> mitigated by mentioning the relevant areas in the code when filing a
>>>> JIRA,
>>>>> but this would also be a helpful safety net to keep the code cleaner.
>>>>>
>>>>> - Jason
>>>>>
>>>>> On Tue, May 5, 2015 at 12:07 PM, Sudheesh Katkam <sk...@maprtech.com>
>>>>> wrote:
>>>>>
>>>>>> Hey y’all,
>>>>>>
>>>>>> For consistency across code, Chris had recommended using
>>>> TODO(DRILL-####)
>>>>>> format for todos in comments, where #### is the JIRA number. If there
>>>> are
>>>>>> no objections, let’s try to stick to that format.
>>>>>>
>>>>>> Thank you,
>>>>>> Sudheesh
>>>>>
>>>>
>>
>


-- 
Daniel Barclay
MapR Technologies

Re: TODOs in comments

[Sudheesh Katkam <sk...@maprtech.com>]

Yes, TODOs must have an associated JIRA, with the specified format.

> On May 5, 2015, at 1:14 PM, Julian Hyde <ju...@gmail.com> wrote:
> 
> Is the proposal to disallow TODOs that do not have a JIRA case number? I’d be +1 to that.
> 
> I’m much less concerned with the problem that TODO(DRILL-abcd) might linger after in the code after DRILL-abcd has been fixed.
> 
> Julian
> 
> 
> On May 5, 2015, at 12:38 PM, Jason Altekruse <al...@gmail.com> wrote:
> 
>> It could optionally be passed manually as a flag, but we already have the
>> build step that is generating the git.properties file, we could issue the
>> same type of call to git to try to pull the JIRA number out of the commit
>> message.
>> 
>> On Tue, May 5, 2015 at 12:34 PM, Chris Westin <ch...@gmail.com>
>> wrote:
>> 
>>> I like that idea, but how would the build know what JIRA you're working on?
>>> 
>>> On Tue, May 5, 2015 at 12:25 PM, Jason Altekruse <altekrusejason@gmail.com
>>>> 
>>> wrote:
>>> 
>>>> We should also consider adding something to the build that will automate
>>>> the process of checking for todo comments containing the JIRA number
>>> being
>>>> fixed. This would allow reviewers to easily verify that a JIRA being
>>> closed
>>>> is not leaving related TODOs in the code (possibly unit tests added by
>>> the
>>>> reporter of the issue, or comments mentioned in another patch that wanted
>>>> to relate a problem they saw in a known outstanding JIRA). This can be
>>>> mitigated by mentioning the relevant areas in the code when filing a
>>> JIRA,
>>>> but this would also be a helpful safety net to keep the code cleaner.
>>>> 
>>>> - Jason
>>>> 
>>>> On Tue, May 5, 2015 at 12:07 PM, Sudheesh Katkam <sk...@maprtech.com>
>>>> wrote:
>>>> 
>>>>> Hey y’all,
>>>>> 
>>>>> For consistency across code, Chris had recommended using
>>> TODO(DRILL-####)
>>>>> format for todos in comments, where #### is the JIRA number. If there
>>> are
>>>>> no objections, let’s try to stick to that format.
>>>>> 
>>>>> Thank you,
>>>>> Sudheesh
>>>> 
>>> 
> 

Re: TODOs in comments

[Julian Hyde <ju...@gmail.com>]

Is the proposal to disallow TODOs that do not have a JIRA case number? I’d be +1 to that.

I’m much less concerned with the problem that TODO(DRILL-abcd) might linger after in the code after DRILL-abcd has been fixed.

Julian


On May 5, 2015, at 12:38 PM, Jason Altekruse <al...@gmail.com> wrote:

> It could optionally be passed manually as a flag, but we already have the
> build step that is generating the git.properties file, we could issue the
> same type of call to git to try to pull the JIRA number out of the commit
> message.
> 
> On Tue, May 5, 2015 at 12:34 PM, Chris Westin <ch...@gmail.com>
> wrote:
> 
>> I like that idea, but how would the build know what JIRA you're working on?
>> 
>> On Tue, May 5, 2015 at 12:25 PM, Jason Altekruse <altekrusejason@gmail.com
>>> 
>> wrote:
>> 
>>> We should also consider adding something to the build that will automate
>>> the process of checking for todo comments containing the JIRA number
>> being
>>> fixed. This would allow reviewers to easily verify that a JIRA being
>> closed
>>> is not leaving related TODOs in the code (possibly unit tests added by
>> the
>>> reporter of the issue, or comments mentioned in another patch that wanted
>>> to relate a problem they saw in a known outstanding JIRA). This can be
>>> mitigated by mentioning the relevant areas in the code when filing a
>> JIRA,
>>> but this would also be a helpful safety net to keep the code cleaner.
>>> 
>>> - Jason
>>> 
>>> On Tue, May 5, 2015 at 12:07 PM, Sudheesh Katkam <sk...@maprtech.com>
>>> wrote:
>>> 
>>>> Hey y’all,
>>>> 
>>>> For consistency across code, Chris had recommended using
>> TODO(DRILL-####)
>>>> format for todos in comments, where #### is the JIRA number. If there
>> are
>>>> no objections, let’s try to stick to that format.
>>>> 
>>>> Thank you,
>>>> Sudheesh
>>> 
>> 

Re: TODOs in comments

[Jason Altekruse <al...@gmail.com>]

It could optionally be passed manually as a flag, but we already have the
build step that is generating the git.properties file, we could issue the
same type of call to git to try to pull the JIRA number out of the commit
message.

On Tue, May 5, 2015 at 12:34 PM, Chris Westin <ch...@gmail.com>
wrote:

> I like that idea, but how would the build know what JIRA you're working on?
>
> On Tue, May 5, 2015 at 12:25 PM, Jason Altekruse <altekrusejason@gmail.com
> >
> wrote:
>
> > We should also consider adding something to the build that will automate
> > the process of checking for todo comments containing the JIRA number
> being
> > fixed. This would allow reviewers to easily verify that a JIRA being
> closed
> > is not leaving related TODOs in the code (possibly unit tests added by
> the
> > reporter of the issue, or comments mentioned in another patch that wanted
> > to relate a problem they saw in a known outstanding JIRA). This can be
> > mitigated by mentioning the relevant areas in the code when filing a
> JIRA,
> > but this would also be a helpful safety net to keep the code cleaner.
> >
> > - Jason
> >
> > On Tue, May 5, 2015 at 12:07 PM, Sudheesh Katkam <sk...@maprtech.com>
> > wrote:
> >
> > > Hey y’all,
> > >
> > > For consistency across code, Chris had recommended using
> TODO(DRILL-####)
> > > format for todos in comments, where #### is the JIRA number. If there
> are
> > > no objections, let’s try to stick to that format.
> > >
> > > Thank you,
> > > Sudheesh
> >
>

Re: TODOs in comments

[Chris Westin <ch...@gmail.com>]

I like that idea, but how would the build know what JIRA you're working on?

On Tue, May 5, 2015 at 12:25 PM, Jason Altekruse <al...@gmail.com>
wrote:

> We should also consider adding something to the build that will automate
> the process of checking for todo comments containing the JIRA number being
> fixed. This would allow reviewers to easily verify that a JIRA being closed
> is not leaving related TODOs in the code (possibly unit tests added by the
> reporter of the issue, or comments mentioned in another patch that wanted
> to relate a problem they saw in a known outstanding JIRA). This can be
> mitigated by mentioning the relevant areas in the code when filing a JIRA,
> but this would also be a helpful safety net to keep the code cleaner.
>
> - Jason
>
> On Tue, May 5, 2015 at 12:07 PM, Sudheesh Katkam <sk...@maprtech.com>
> wrote:
>
> > Hey y’all,
> >
> > For consistency across code, Chris had recommended using TODO(DRILL-####)
> > format for todos in comments, where #### is the JIRA number. If there are
> > no objections, let’s try to stick to that format.
> >
> > Thank you,
> > Sudheesh
>

Re: TODOs in comments

[Jason Altekruse <al...@gmail.com>]

We should also consider adding something to the build that will automate
the process of checking for todo comments containing the JIRA number being
fixed. This would allow reviewers to easily verify that a JIRA being closed
is not leaving related TODOs in the code (possibly unit tests added by the
reporter of the issue, or comments mentioned in another patch that wanted
to relate a problem they saw in a known outstanding JIRA). This can be
mitigated by mentioning the relevant areas in the code when filing a JIRA,
but this would also be a helpful safety net to keep the code cleaner.

- Jason

On Tue, May 5, 2015 at 12:07 PM, Sudheesh Katkam <sk...@maprtech.com>
wrote:

> Hey y’all,
>
> For consistency across code, Chris had recommended using TODO(DRILL-####)
> format for todos in comments, where #### is the JIRA number. If there are
> no objections, let’s try to stick to that format.
>
> Thank you,
> Sudheesh