Hi,

if (you don't want to read a semi rant)
exit 0;

I have noticed a number of useless comments in makepkg and have decided
that they are worse than no comments. Examples:


1) This inspired the rant!

download_sources
# we can only check checksums if we have all files
check_checksums

Huh, why are we not checking for all files then? Because
download_sources exits when it fails...


2) and there are a lot of these:

# fix flyspray feature request #2978
# fix flyspray bug #5923
# Fixes FS#10039
# fix flyspray #6246
#fix flyspray feature request #5223
# fix flyspray bug #5973

and I am guilty here... but I was at the airport with no internet
access so I had no idea if these are important. And most of these
appeared to be non-obscure features/fixes so did not need a comment
justifying their inclusion.


3) overly obvious comments

# do we have a changelog?
if [ -f "$startdir/ChangeLog" ]; then

If you do not understand that test, then leave the code alone...


I will patch these out eventually but I like to share my frustration. I
am just a generous guy!


Allan




_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://www.archlinux.org/mailman/listinfo/pacman-dev

@@ -1,2 +1,3 @@
+# Check to see if you want to read a semi rant
if (you don't want to read a semi rant)
exit 0;


On Sun, Mar 8, 2009 at 2:06 AM, Allan McRae <allan@archlinux.org> wrote:
> Hi,
>
> if (you don't want to read a semi rant)
> * exit 0;
>
> I have noticed a number of useless comments in makepkg and have decided that
> they are worse than no comments. *Examples:
>
> 1) *This inspired the rant!
>
> * download_sources
> * # we can only check checksums if we have all files
> * check_checksums
>
> Huh, why are we not checking for all files then? *Because download_sources
> exits when it fails...
>
> 2) *and there are a lot of these:
>
> * # fix flyspray feature request #2978
> * # fix flyspray bug #5923
> * # Fixes FS#10039
> * # fix flyspray #6246
> * #fix flyspray feature request #5223
> * # fix flyspray bug #5973
>
> and I am guilty here... *but I was at the airport with no internet access so
> I had no idea if these are important. *And most of these appeared to be
> non-obscure features/fixes so did not need a comment justifying their
> inclusion.
>
> 3) overly obvious comments
>
> * # do we have a changelog?
> * if [ -f "$startdir/ChangeLog" ]; then
>
> If you do not understand that test, then leave the code alone...
>
>
> I will patch these out eventually but I like to share my frustration. *I am
> just a generous guy!
>
> Allan
>
>
>
>
> _______________________________________________
> pacman-dev mailing list
> pacman-dev@archlinux.org
> http://www.archlinux.org/mailman/listinfo/pacman-dev
>
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://www.archlinux.org/mailman/listinfo/pacman-dev

Allan McRae wrote:

Hi,


Hi. I think much of the problem you are perceiving comes down to a
difference in comment philosophy between people. I also got significant
push back on comments in my changes to the pacman source code and to be
honest, it surprised me.


For some complex operations, typically encompassed by a single function,
as in the libalpm _resolve_deps function and similar places, when I
write such code, I often like my comments to be a "parallel stream"
describing the steps involved in solving the problem. Sometimes this
makes the comments seem obvious because they're explaining what the code
they immediately precede is clearly doing. But for me, I like to be
able to ignore the code and read through the comments in sequence to get
a general overview of the structure of the code. Then I like to ignore
the comments and read the code to see how it's actually implemented.
This means some necessary redundancy between "obvious comments" and the
code that it is commenting on.


Some people might suggest that such comments are best in the function
header comment, and to some people, that may be true. I like function
header comments to be more descriptive of the purpose of the function
than details of its implementation, however.


Also, it helps alot that my emacs coloring scheme puts comments in a
darker color than code (code is always bright white for me, with some
keywords highlighted in other bright colors, and comments are always
sort of a washed-out blue-grey, and don't stand out so much on my dark
background), so it's pretty easy for me to ignore them. My eyes seem to
be able to easily and naturally skip to either comments or code
depending on which I want to see; I'm not sure how much of this is due
to the color scheme, and how much is due to years of practice reading
code this way, but I'm pretty much never annoyed by "useless comments"
because I just don't read them, except when I want to.


My biggest pet peeve with code is when it is under-commented and messy.
Because 90% of developers tend to skip comments whenever they can get
away with it, I never discourage comments, even when they seem useless
to me. Useless comments can always be removed pretty easily, because
it's a simple keystroke to blow them away, but adding comments to
uncommented code takes actual work. It would be great if everyone could
comment their code perfectly, but given the unattainability of this
goal, I'd rather that people err on the side of too many comments, than
too few.


That's how I see it, anyway.
Bryan

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://www.archlinux.org/mailman/listinfo/pacman-dev

On Sun, Mar 8, 2009 at 7:06 AM, Allan McRae <allan@archlinux.org> wrote:
> Hi,
>
> if (you don't want to read a semi rant)
> * exit 0;
>
> I have noticed a number of useless comments in makepkg and have decided that
> they are worse than no comments. *Examples:
>
> 1) *This inspired the rant!
>
> * download_sources
> * # we can only check checksums if we have all files
> * check_checksums
>
> Huh, why are we not checking for all files then? *Because download_sources
> exits when it fails...
>

The problem with this comment is maybe more that it could be
misleading. It could let us think that check_checksums assumes that
all the files are here. But that is not the case.
But there is still one interesting information here, it's what you
said in your comment : "if download_sources return, it means all files
are here"
So the comment could be :

download_sources
# we have all files now, so check their integrity
check_checksums

> 2) *and there are a lot of these:
>
> * # fix flyspray feature request #2978
> * # fix flyspray bug #5923
> * # Fixes FS#10039
> * # fix flyspray #6246
> * #fix flyspray feature request #5223
> * # fix flyspray bug #5973
>
> and I am guilty here... *but I was at the airport with no internet access so
> I had no idea if these are important. *And most of these appeared to be
> non-obscure features/fixes so did not need a comment justifying their
> inclusion.
>

If one of these fixes is not totally obvious, I would say it would be
better having some comments explaining quickly what is done and why
rather than having a link to flyspray.
If it is obvious, I agree that no comment and no fs link is needed.

> 3) overly obvious comments
>
> * # do we have a changelog?
> * if [ -f "$startdir/ChangeLog" ]; then
>
> If you do not understand that test, then leave the code alone...
>
>

Now that's a typical example of useless comment, much more than the above
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://www.archlinux.org/mailman/listinfo/pacman-dev