[task #16073] --copykeys of Fits program takes keyword names also

[Mohammad Akhlaghi]

Follow-up Comment #6, task #16073 (project gnuastro):

Thanks a lot Jash. Here are some comments:

In particular, you will see some of them are stylistic that don't affect the
program's running. Nevertheless, when contributing to a new code-base, it is
very important to use the existing conventions, otherwise the code will become
unreadable as more people contribute ;-).

* I noticed that your Git commit name is 'Jash-Shah'. Having a SPACE between
your first and family names is fine. If "Jash" is your first name and "Shah"
your family name, it helps readability (and future inclusion in the second
page of Gnuastro's PDF manual
<https://www.gnu.org/software/gnuastro/manual/gnuastro.pdf>) if you use a
simple SPACE instead of a dash like the template below (note the SPACE between
"your" and "Name"):


git config --global user.name "Your Name"
git config --global user.email "youremail@yourdomain.com"


* Before pushing your commit, run a 'git log -p -1' and see if Git has marked
any of the regions in red. These are common bad programming styles. For
example, when a line ends in several white spaces. I saw one such case in your
changes in 'ui_check_copykeys'.

* According to Gnuastro's coding conventions
<https://www.gnu.org/software/gnuastro/manual/html_node/Coding-conventions.html>,
all lines within the source file should be at most 75 characters. But I saw
some very long comment lines in 'ui_check_copykeys'. If you use GNU Emacs as
your text editor, you can simply press M-q (or ALT+q on most keyboards) to do
this for you.

* Staying on comments, I noticed that in some places (like above
'keywords_copykeys_str', you had put an empty line before and after the
comment description. As you see in the rest of the Gnuastro's code, this is
not the convention used in Gnuastro. So please follow a similar convention in
all new comments.

* When defining blocks, you had indented the curly-braces at the same level as
the 'if', like the bad code below. But according to the GNU Coding Standards
<https://www.gnu.org/prep/standards/standards.html#Formatting>, you should use
the good indentation style below. As you read the existing Gnuastro code, you
should have noticed how different your code-blocks look ;-). If you use Emacs,
simply pressing TAB on each line will fix the indentation to the GNU style.


/* Bad */
if (isalpha(*pt))
{
....
}

/* Good */
if (isalpha(*pt))
  {
    ....
  }


* In terms of design, I noticed that in the high-level 'keywords' function (of
'bin/fits/keywords.c'), you had implemented a completely independent process
for the name-based copying. This is not good practice ;-). We already have all
the necessary low-level steps to do the writing in a later part of that
function. Duplicating the effort is not good in programming. Also, mixing
low-level and high-level steps is not good. You should use the concept of
modularity effectively. In other words, each one of the operations in the
'keywords' function is very different from each other, and each have their own
internal complexities at a low-level. But when you are looking in the
high-level 'keywords', you shouldn't see those complexities! So hide all the
complexities of integer-based or name-based keyword copying in the
'keywords_copykeys' function and keep the 'keywords' function untouched ;-).

After you implement these, I'll take a closer look at the lower-level details
(this has already become relatively long) ;-).


    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/task/?16073>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/