General code clean-up

[mnlevy1981]

At some point, we need to take step back from development and talk about coding standards and really flesh out the wiki. Some topics that have come up while @klindsay28 and I review code:

1. If we have an implicit none statement at the module level (and all modules should have this!), then we do not need additional implicit none statements at the subroutine level
2. We need standards for when to use a variable at the module level vs at the subroutine level; two possible solutions, both with pros and cons:
   1. Determine a threshold N (perhaps N=1? definitely a small number), and if more than N subroutines use the same variable bring it in at the module level otherwise have them at the subroutine level
   2. Regardless of how many subroutines need a variable, only use it at the module level if a module variable needs it (e.g. kind type); otherwise bring in at the subroutine level
3. What should the max number of characters in a line be?
   1. Related: what should the divider used between subroutines look like?
4. How many spaces should we use when we indent?
5. Formatting of use statements: one variable per line, or multiple?
6. Formatting of subroutine calls and declarations: one argument per line or multiple?
7. Remove unused variables -- NAG provides warnings about variables that are declared but not used, which will be very useful once the code is stable. Also, if we can reach a point where the code compiles without warnings it would be nice to turn on a compile flag to elevate warnings to errors in the test system to ensure the code stays that way.
8. Do we align commas or text in variable declarations? I.e.

    integer,  intent(in) :: n
    real(r8), intent(in) :: x

vs

    integer , intent(in) :: n
    real(r8), intent(in) :: x

I hope this issue will be a catch-all for other similar items as we encounter them, and we can close this issue when the wiki and code are both updated. This should definitely not happen until we reach a point where we can call for a code freeze, I can imagine the merge conflicts if we do this while there are still active development branches.

[mnlevy1981]

Another comment I forgot to add to the above list...

Variables should not change name when passing through an interface (e.g. there are many places where autotrophs is passed in as the auto_meta argument, which makes it hard to trace a variable in the code)

[klindsay28]

For each variable, use the same case in all usage of the variable name, even though Fortran is case insensitive.

[mnlevy1981]

Declaring arrays, we should choose between

real(r8), dimension(N) :: x

and

real(r8) :: x(N)

Rather than having some of each scattered throughout the code

[klindsay28]

Consistent usage of .eq. vs == and .ne. vs /=

usage of assumed shape vs. assumed size for non-scalar subroutine/function arguments

[mnlevy1981]

' vs " for defining strings

[mnlevy1981]

endif vs end if (similar for end do, end where, and what not)

[mnlevy1981]

CamelCase vs seperating_by_underscores

[mnlevy1981]

We have not been consistent about abbreviating variable / routine names... we use configuration in some places, config in others (and I used conf in at least one comment); also short_name is used in some places while sname appears in others.

[mnlevy1981]

We tend to line up & for continuation lines in column 79 (or some other column in the area). Is the extra whitespace useful, or should we just use a simple & following the last character?

          call this%add_var_1d_int(sname, lname, units, group,                &
                            grazing(m,n)%auto_ind(1:cnt),                     &
                            marbl_status_log,                                 &
                            add_newline=(grazing_config(m,n)%zoo_ind_cnt.eq.0))

vs

          call this%add_var_1d_int(sname, lname, units, group, &
                            grazing(m,n)%auto_ind(1:cnt), &
                            marbl_status_log, &
                            add_newline=(grazing_config(m,n)%zoo_ind_cnt.eq.0))

[mnlevy1981]

Avoid compile-time settings as much as possible. Currently, we define the following at build-time:

1. tracer count (currently just base tracer count, but soon to be CISO tracers as well)
2. autotroph count
3. zooplankton count
4. grazer (prey) count

We also use build-time macros to tell MARBL if some optional libraries are available:

1. The MARBL timing library can use either the timers in CIME's share code or MPI_Wtime() if CIME or MPI are available, respectively
2. We want to be able to use the CIME share code for vector math (we currently have some ifdef statements in marbl_co2calc_mod.F90 that should relate to CIME instead of being compiler-specific)

In general, though, we want MARBL to be run-time configurable.

[mnlevy1981]

I will add a straw-man set of guidelines to the developer's guide over at https://marbl-ecosys.github.io/MARBL/html/ and let folks know when that is available.

[klindsay28]

consisting formatting of comments is desirable

[mnlevy1981]

Get rid of un-used kinds (r4, i4, i8)

[klindsay28]

change diagnostic index [>0, /=-1] checks to be based on flags, such as autotrophs(n)%silicifier

introduce autotrophs(n)%calcifier = autotrophs(n)%imp_calcifier .or. autotrophs(n)%exp_calcifier to simplify calcifier checks

[mnlevy1981]

marbl_mod is using autotrophs from marbl_settings_mod but also passing it as an explicit argument to other subroutines. On one hand, this lets us explicitly specify intent(in), but on the other hand it is makes it awfully hard to figure out where variables are coming from.

[mnlevy1981]

I often use the write function to concatenate strings, e.g.

write(str_new, "(2A)"), str1, str2

but we should stick with the // operator

str_new = str1 // str2