[gobolinux-users] Gobo wants you! (to comment its scripts)

[Jonas Karlsson]

Note, when I give references to the code, I refer to line numbers in the
current svn, that is, with the patch applied.

On 11/04/2008, Michael Homer <michael at gobolinux.org> wrote:
> On Fri, Apr 11, 2008 at 8:51 PM, Daniele Maccari <gobo.users at gmail.com> wrote:
>  > Mmm, well, I'm doing to change anything anybody will find erroneous/not
>  > clear, so keep suggestion coming.
>
> It's funny you should say that. After some discussions on IRC:
>  - Some of the (inline) comments are a bit unnecessary ("Print out
>  notes for this script, if any.", "Quiet suppresses output"). I think
>  they're remnants of when you were commenting for your own
>  understanding, so they could be removed.
I too agree that the comments are a bit verbose. The reason, not the
code, should be explained by comments, unless one can't see what
the specific code does without explanation. Michael gives a good
example below with "Print out examples".

>  - For if/then, the comment should be either on the same line as the
>  then, or on the subsequent lines, not in between the if and then
>  (there are two of these)

My opinion is that one liners should be on the same line as the
then/else/fi while multiple lines should come after (except for "fi" where
no multiple lines should be used).

>  - Jonas doesn't like you correcting his grammar. I think you should do
>  it anyway, because I like consistency.
I don't mind my grammar being corrected, if it's wrong. My comment was
that unnecessary changes to comments adds too much noice and adds
no value. My example was (around line 860):

               if [ "$opt" = "--${optionsLongList[j]}" ]
               then
-                  # we found a valid (known) option
+                  # We've found a valid (known) option

English is not my native language, but I thought that the original
sentence was correct and the change provided no value. A more valid
example would be the changes just some lines below:

                     if echo "$option" | grep -q = -
                     then
-                        # long option value assignment with =
+                        # Long option value assignment with =.

where the changes add no value at all imo.

>  - The general principle was that the code should stand on its own
>  except when it's unclear, or it's necessary to explain why it's doing
>  what it is. "Read every line of the example and print it indented." is
>  good, because the following line isn't terribly clear without a lot of
>  thought, but "Print out examples" above it is obvious from `if [
>  "$scriptExample" ] ;then echo "Examples: ", so it isn't really
>  necessary.
See above.

>  - The blank "#<space>" lines in function comments (which are otherwise
>  great) take up a bit much room. You can only fit so many lines on a
>  screen, so blank ones lose you some real estate.
Also some newlines added besides the comments coincidence with
indentation level changes, where the indentation itself allows the code to
be easy read (imo).

>  - It was also resolved to general agreement that I should say these
>  things *before* I commit patches. I suppose that was a valid point,
>  all things considered.
>
Yes. :)
Besides the points made above there are also some errors made in the
comments and changes that, to me, doesn't clarify the code. I don't have
time to review the code thoroughly, but at a glance I can give one
example of both.

Around line 850:

-            # Go through the list of available options
+            # Loop through options.

"loop" adds value, while "available" is dropped, so either comment is
bad. "Loop through <available|valid> options" would be better.

Around line 970:

-         fi
-      else # dash found
+         fi # dash found
+      else
+         # The parameter isn't an option.

the "dash found" should be at the "else", not the "fi".

-- 
/Jonas