HowTo: LMDZ code guidelines : Différence entre versions

De LMDZPedia
Aller à : navigation, rechercher
m
m
 
(9 révisions intermédiaires par le même utilisateur non affichées)
Ligne 1 : Ligne 1 :
 
   /!\ This page is a WORK IN PROGRESS. Nothing in this page should be considered correct or consensual for now. /!\
 
   /!\ This page is a WORK IN PROGRESS. Nothing in this page should be considered correct or consensual for now. /!\
 +
 +
''Note: this documents draws from various sources, including [http://broken.link.todo.replace the OPA guidelines], [https://fortranwiki.org/fortran/show/Modernizing+Old+Fortran Modernizing Old Fortran], [http://pages.swcp.com/~walt/F/F_bnf.html F syntax rules], [https://github.com/fortran-lang/stdlib/blob/master/STYLE_GUIDE.md the stdlib style guide], [https://github.com/codee-com/fortran-modernization CODEE Fortran recommendations].
  
 
== Example code ==
 
== Example code ==
Ligne 7 : Ligne 9 :
 
''Note: of course, there's always exceptions to "Always", but they should '''systematically''' documented by a comment, with a proper explanation.''
 
''Note: of course, there's always exceptions to "Always", but they should '''systematically''' documented by a comment, with a proper explanation.''
  
 +
<syntaxhighlight lang="fortran" line>
 +
!! File: lmdz_mymodule.f90
 +
!!        ^ all module files should start with their component name, (e.g. "lmdz")
 +
!!                      ^ use ".f90" for all free-form files, unless they contain preprocessor directives (then use ".F90").
 +
 +
!! At the top of the file, we provide a comment to explain what this modules does
 +
! implementation of my_useful_stuff.
 +
! ----------------------------------
 +
 +
!! Unless specified otherwise, all files should contain a single module with the same name which encapsulates all their content
 +
MODULE lmdz_mymodule
 +
!!  ^ all Fortran keywords are in CAPS, everything else is in lowercase.
 +
  USE my_other_module, ONLY: function_a, function_b
 +
  !!                    ^ Always specify explicitely which functions you use from a module.
 +
!!^ use indentation (2 spaces) to provide visual clarity
 +
  IMPLICIT NONE; PRIVATE
 +
  !!                ^ Always make your module private, and expose explicitely which functions are public
 +
  !!    ^ Always use IMPLICIT NONE at the module-level
 +
  PUBLIC my_var, my_subroutine
 +
 +
  REAL, PARAMETER :: my_var
 +
  REAL            :: a, b, c, d, e, f
 +
  !!              ^ use "::" even if it's not strictly necessary
 +
  !!      ^ Alignment of "::" is not necessary, but should be attempted whenever it provides readability and is relevant (e.g. there's some link between the aligned variables)
 +
       
 +
CONTAINS
 +
 
 +
!! Put at least a blank line before/after each function/subroutine2
 +
!! A routine should have a clear, unambiguous name, describing what the routine does.
 +
  SUBROUTINE my_subroutine(arg1, arg2, arg3) ! handle event xxxx
 +
  !!                            ^ comments should always be right before, or on the same line as what they're commenting
 +
  !!                            ^ as for modules, all functions should be documented in a comment
 +
    INTEGER, INTENT(IN) :: arg1
 +
    !!          ^ Always provide intent
 +
    REAL, INTENT(IN)    :: arg2
 +
 +
    INTEGER, INTENT(INOUT) :: arg3
 +
 +
    REAl, INTENT(OUT) :: out1
 +
    !!^ Group together IN, then INOUT, then OUT
 +
 +
    IF (arg2 > arg1) THEN
 +
    !!      ^ .ge., .le., etc are deprecated. Use >, <, ==, etc instead
 +
      ! ...
 +
    END IF
 +
    !! ^ Use END IF, END DO rather than ENDIF, ENDDO (coherence with fortran-lang.org)
 +
  END SUBROUTINE my_subroutine
 +
!!        ^ always use named blocks for END
 +
 
 +
 +
END MODULE my_module
 +
!!      ^ always use named blocks for END
 +
</syntaxhighlight>
 +
 +
== Comments and Documentation ==
 +
 +
Documentation consists of putting information both inside and outside the source code. On large, formal projects, most of the documentation is outside the source code. However because the internal documentation is most closely associated with the code, it is the part most likely to remain correct as the code is modified. The thing to be really careful with is that misleading comments are even worse than having no comments at all. So, please report any misleading comments you found.
 +
 +
Try to avoid having superfluous comments such as: <code>A = A + 1 ! Increment A by one.</code> This is not adding anything that is not immediately obvious. A person looking through your code should only have to scan your comments to be able to get a good idea of what the code does and where to look for any special activity.
 +
 +
Good comments do not just repeat verbally what is happening in the code or just explain it. They clarify its intent. They should explain at a higher level of abstraction what the code or what you are trying to do. In addition, you should comment:
 +
* Everything that gets around an error or an undocumented feature in a language or environment.
 +
* Any violations of good programming style should be justified. This will ensure that any well-intentioned programmer does not break your code by changing the source to implement a “better” style.
 +
* The lines before a control structure, e.g. IF, CASE, loop, etc. are a natural spot to comment and explain what these constructs are about to do.
 +
* The start of '''any''' subroutine or function whose usage is not downright obvious.
 +
 +
== Misc ==
 +
 +
=== Fortran standard ===
 +
 +
Fortran 2008 is now widely supported on all [https://fortranwiki.org/fortran/show/Fortran+2008+status major compilers], and should be taken as a reference.
 +
 +
=== Allocatable arrays ===
 +
 +
Allocatable arrays can be relevant, but should be substituted by automatic arrays (= passing the array size as an argument) whenever possible, for performance and debugging reasons.
 +
 +
=== Compiler warnings ===
 +
 +
Compiler warnings are useful - unless there's too many of them. Effort should be made such that during the compiling phase, no warning appears, so that user can be easily alerted of potential bugs.
 +
when some appear in their configuration.
 +
 +
In particular, all component must be able to run when a compile-time and/or run-time array bounds checking option is enabled.
 +
Use of the (*) construct in array dimensioning to circumvent this problem is forbidden because it effectively disables array bounds checking.
 +
 +
=== Side effects ===
 +
 +
Whenever possible, write functions without side-effects, and label them accordingly as <code>PURE</code> or <code>ELEMENTAL</code>.
 +
 +
=== SAVE ===
 +
 +
Since all files contain a <code>MODULE</code>, attribute <code>SAVE</code> is banned. Variables whose value have to be preserved between two calls should be declared at the module level.
 +
 +
=== Deprecated features ===
 +
 +
In terms of keeping code up to date and easier to maintain the code should always follow the current standards of FORTRAN and ANSI C. We decide to restrict the languages use to the elements, which are not obsolete or deleted -- even if they are still available with almost all compilers.
 +
 +
''Note: just because a feature is deprecated doesn't mean it '''must''' be converted in old, working code - this can be tricky and bug-prone in complex instances.''
 +
 +
Some notable deprecated features:
 +
 +
* <code>COMMON</code> blocks. Instead, use <code>MODULE</code> initialisation.
 +
* <code>EQUIVALENCE</code> should be replaced by pointers or derived data types.
 +
* Any kind of <code>GOTO</code> has absolutely no place in modern programming. Use <code>CASE</code> whenever relevant rather than <code>IF</code>.
 +
* Arithmetic <code>IF</code> statements. Use block <code>IF</code> or <code>CASE</code> instead.
 +
* <code>CONTINUE</code> statement:  use <code>IF, CASE, DO WHILE, EXIT, CYCLE</code> statements or a contained <code>SUBROUTINE</code> instead.
 +
* I/O routines <code>END</code> and <code>ERR</code> - use <code>IOSTAT</code> instead
 +
* <code>FORMAT</code> statements: use character parameters or explicit format- specifiers inside the <code>READ</code> or <code>WRITE</code> statement instead.
 +
* <code>ENTRY</code> statements: a subprogram must only have one entry point
 +
* ''Alternate'' <code>RETURN</code> is obsolescent, and <code>RETURN</code> at the end of a function is useless clutter.
 +
* Statement functions have been replaced by corresponding functions or subroutines in the <code>CONTAIN</code> block of the current scope.
 +
* <code>DATA</code> and <code>BLOCK DATA</code> should be replaced by initializers.
 +
 +
=== Preprocessor ===
 +
 +
Limit '''to the bare minimum''' the use of preprocessor <code>#ifdef</code> keys. Those decrease lisibility, disable automatic code analysis, and generally make code a pain to manage and read. Ideally, a given preprocessor key should be used only once, or if not possible, in a single module for the entire codebase.
 +
 +
=== INCLUDE ===
 +
 +
Limit the use of <code>include</code> headers to the bare minimum. Whenever not possible, wrap those include in a module, and import that module instead. This provides much more flexibility, e.g. when using <code>ONLY: ...</code>.
 +
 +
== Recent examples of improper LMDZ code ==
 +
 +
''Note: The purpose of this section is '''obviously not''' to blame anyone, but to stress that "bad"/perfectible code in LMDZ isn't limited to old legacy code, and that even current code is being written in a way that can be greatly improved.''
 +
 +
==== 07/24: Unhelpful comment ====
 +
 +
<syntaxhighlight lang="fortran" line>
 +
IF (iflag_ice_thermo == 0) THEN 
 +
          !pas necessaire a priori
 +
</syntaxhighlight>
 +
 +
This comment fails to convey a clear indication. What is necessary or not ? In which condition ? "A priori" further conveys a sense of uncertainty that may make sense to the writer, but certainly not to an external reader.
 +
 +
==== 07/24: Unclear conventions, no PURE, useless RETURN ====
 +
 +
<syntaxhighlight lang="fortran" line>
 +
function grid_index(lon_deg,lat_deg)
 +
!--------------------------------------------------------------------------------
 +
! Get local index of grid point of longitude,latitude lon_deg,lat_deg
 +
! Author : XXX 2024/07/XX
 +
! Please do not put this function in a m*odule not to complexify the replay script
 +
!--------------------------------------------------------------------------------
 +
USE dimphy, only : klon
 +
USE geometry_mod, ONLY: latitude_deg, longitude_deg
 +
implicit none
 +
real, intent(in) :: lon_deg,lat_deg
 +
integer :: grid_index
 +
integer i
 +
grid_index=0
 +
do i=1,klon
 +
  if ( abs(lon_deg-longitude_deg(i)) < 0.02 .and.  abs(lat_deg-latitude_deg(i)) < 0.02 ) then
 +
        grid_index=i
 +
        exit
 +
  endif
 +
enddo
 +
return
 +
end
 +
</syntaxhighlight>
 +
 +
This function is actually mostly great ! It is documented, the author put a clear statement to indicate a deviation from standards (no module), <code>USE</code> is used in conjuction with <code>ONLY</code>, <code>INTENT</code> is declared.
 +
However, it is also lacking in some areas, notably:
 +
* This function should be declared as <code>PURE</code>
 +
* This function has inconsistent syntax: mix of uppercase and lowercase for Fortran keywords, sometimes uses <code>::</code> in declarations and sometimes not
 +
* The function ends with a useless <code>RETURN</code>
 +
 +
==== 07/24: Duplicated, uncommented code ====
 +
 +
<syntaxhighlight lang="fortran" line>
 +
          zx_tmp_fi3d=0.
 +
          IF (vars_defined) THEN
 +
            DO nsrf=1,nbsrf
 +
                DO k=1,klev
 +
                  zx_tmp_fi3d(:,k)=zx_tmp_fi3d(:,k) &
 +
                        +pctsrf(:,nsrf)*tke_shear(:,k,nsrf)
 +
                ENDDO
 +
            ENDDO
 +
          ENDIF
 +
 +
          CALL histwrite_phy(o_tke_shear, zx_tmp_fi3d)
 +
 +
          zx_tmp_fi3d=0.
 +
          IF (vars_defined) THEN
 +
            DO nsrf=1,nbsrf
 +
                DO k=1,klev
 +
                  zx_tmp_fi3d(:,k)=zx_tmp_fi3d(:,k) &
 +
                        +pctsrf(:,nsrf)*tke_buoy(:,k,nsrf)
 +
                ENDDO
 +
            ENDDO
 +
          ENDIF
 +
 +
          CALL histwrite_phy(o_tke_buoy, zx_tmp_fi3d)
 +
 +
 +
          zx_tmp_fi3d=0.
 +
          IF (vars_defined) THEN
 +
            DO nsrf=1,nbsrf
 +
                DO k=1,klev
 +
                  zx_tmp_fi3d(:,k)=zx_tmp_fi3d(:,k) &
 +
                        +pctsrf(:,nsrf)*tke_trans(:,k,nsrf)
 +
                ENDDO
 +
            ENDDO
 +
          ENDIF
 +
 +
          CALL histwrite_phy(o_tke_trans, zx_tmp_fi3d)
 +
</syntaxhighlight>
 +
 +
This code:
 +
* Duplicates three times essentially the same code. Refactoring into a routine would be much more readable and reduce the potential of typos.
 +
* Contains no comment justifying such behavior, or what is even being done here, which to an external reader is fairly obscure.
 +
 +
==== 06/24: Undocumented variable ====
 +
 +
<syntaxhighlight lang="fortran" line>
 +
INTEGER                              :: nsrf, k, iq, iff, i, j, ilev, itr, itrb
 +
!!                                                                          ^ Added this
 +
!! (...)
 +
itrb = 0
 +
!! (...)
 +
if(tracers(iq)%name(1:3)=='BIN') then
 +
              itrb = itrb + 1
 +
</syntaxhighlight>
 +
 +
Just because a variable is a "dumb" counter doesn't mean it shouldn't be documented. In this particular example, the variable name is devoid of any obvious meaning.
 +
Properly documenting this variable would also make it clear to others whether it would make sense to reuse it later on in the code.
 +
 +
==== 06/24: Comments as code storage ====
 +
 +
<syntaxhighlight lang="fortran" line>
 +
!CALL writefield_phy('latitude',latitude_deg,1)
 +
!CALL writefield_phy('pressure_hl',pressure_hl,klev+1)
 +
!CALL writefield_phy('Ldecorel',PDECORR_LEN_EDGES_M,klev)
 +
</syntaxhighlight>
  
−
!! File: lmdz_mymodule.f90
+
'''Comments are ''NOT'' for code storage'''. Unfortunately, the LMDZ code is ''littered'' with bits of commented code without any indication of when or why the code was commented.
−
!!        ^ all module files should start with their component name, (e.g. "lmdz")
+
As a result, there are in a lot of places such comments, many dating from 20+ years ago, for which we have no clue whether we can remove them or not. This greatly hinders readability and maintainability.
−
!!                      ^ use ".f90" for all free-form files, unless they contain preprocessor directives (then use ".F90").
 
−
 
−
!! At the top of the file, we provide a comment to explain what this modules does
 
−
! implementation of my_useful_stuff.
 
−
! ----------------------------------
 
−
 
−
!! Unless specified otherwise, all files should contain a single module which encapsulates all their content
 
−
MODULE my_module
 
−
!!  ^ all Fortran keywords are in CAPS, everything else is in lowercase.
 
−
  USE my_other_module, ONLY: function_a, function_b
 
−
  !!                    ^ Always specify explicitely which functions you use from a module.
 
−
!!^ use indentation (2 spaces) to provide visual clarity
 
−
  IMPLICIT NONE; PRIVATE
 
−
  !!                ^ Always make your module private, and expose explicitely which functions are public
 
−
  !!    ^ Always use IMPLICIT NONE at the module-level
 
−
  PUBLIC my_var, my_subroutine
 
−
 
−
  REAL, PARAMETER :: my_var
 
−
  REAL            :: a, b, c, d, e, f
 
−
  !!              ^ use "::" even if it's not strictly necessary
 
−
  !!      ^ Alignment of "::" is not necessary, but should be attempted whenever it provides readability and is relevant (e.g. there's some link between the aligned variables)
 
−
       
 
−
CONTAINS
 
−
 
 
−
  !! Put at least a blank line before/after each function/subroutine
 
−
  SUBROUTINE my_subroutine(arg1, arg2, arg3) ! handle event xxxx
 
−
  !!                            ^ comments should always be right before, or on the same line as what they're commenting
 
−
  !!                            ^ as for modules, all functions should be documented in a comment
 
−
    INTEGER, INTENT(IN) :: arg1
 
−
    !!          ^ Always provide intent
 
−
    REAL, INTENT(IN)    :: arg2
 
−
 
−
 
−
    INTEGER, INTENT(INOUT) :: arg3
 
−
 
−
    REAl, INTENT(OUT) :: out1
 
−
    !!^ Group together IN, then INOUT, then OUT
 
−
 
−
    ! ... body of my_subroutine
 
−
    IF (arg2 > arg1) THEN
 
−
    !!      ^ .ge., .le., etc are deprecated. Use >, < ==, etc instead
 
−
      ! ...
 
−
    END IF
 
−
    !! ^ Use END IF, END DO rather than ENDIF, ENDDO (coherence with fortran-lang.org)
 
−
  END SUBROUTINE my_subroutine
 
−
!!        ^ always use named blocks for END
 
−
 
 
−
 
−
END MODULE my_module
 
−
!!      ^ always use named blocks for END
 
  
 +
Of course, in ''some'' cases, it is warranted to keep some code at hand, e.g. for diagnostic purpose. In such case, please '''make sure to always clearly document ''why, when and by whom'' the code was commented out''', so that it can be cleaned up at a later date when it becomes irrelevant.
  
 +
== Questions à élucider ==
  
−
Some comments:
+
* Prescrit-on une langue (Français / Anglais) ?
−
* Limit '''to the bare minimum''' the use of preprocessor <code>#ifdef</code> keys. Those decrease lisibility, disable automatic code analysis, and generally make code a pain to manage and read. Ideally, a given preprocessor key should be used only once, or if not possible, in a single module for the entire codebase.
+
* Longueur des lignes: dans la limite du raisonable, pourquoi se limiter à 100-140 chars ? Le wrapping fonctionne très bien.
−
* Limit the use of <include> headers to the bare minimum. Whenever not possible, wrap those include in a module, and import that module instead. This provides much more flexibility, e.g. when using <code>ONLY: ...</code>.
 

Version actuelle en date du 1 août 2024 à 08:17

 /!\ This page is a WORK IN PROGRESS. Nothing in this page should be considered correct or consensual for now. /!\

Note: this documents draws from various sources, including the OPA guidelines, Modernizing Old Fortran, F syntax rules, the stdlib style guide, CODEE Fortran recommendations.

Example code

Note: in this example, we added "meta-comments" indicated by !!

Note: of course, there's always exceptions to "Always", but they should systematically documented by a comment, with a proper explanation. 

 1 !! File: lmdz_mymodule.f90
 2 !!         ^ all module files should start with their component name, (e.g. "lmdz")
 3 !!                      ^ use ".f90" for all free-form files, unless they contain preprocessor directives (then use ".F90").
 4 
 5 !! At the top of the file, we provide a comment to explain what this modules does
 6 ! implementation of my_useful_stuff.
 7 ! ----------------------------------
 8 
 9 !! Unless specified otherwise, all files should contain a single module with the same name which encapsulates all their content
10 MODULE lmdz_mymodule
11 !!  ^ all Fortran keywords are in CAPS, everything else is in lowercase.
12   USE my_other_module, ONLY: function_a, function_b
13   !!                     ^ Always specify explicitely which functions you use from a module.
14 !!^ use indentation (2 spaces) to provide visual clarity
15   IMPLICIT NONE; PRIVATE
16   !!                ^ Always make your module private, and expose explicitely which functions are public
17   !!    ^ Always use IMPLICIT NONE at the module-level
18   PUBLIC my_var, my_subroutine
19 
20   REAL, PARAMETER :: my_var
21   REAL            :: a, b, c, d, e, f
22   !!               ^ use "::" even if it's not strictly necessary
23   !!       ^ Alignment of "::" is not necessary, but should be attempted whenever it provides readability and is relevant (e.g. there's some link between the aligned variables)
24         
25 CONTAINS
26   
27  !! Put at least a blank line before/after each function/subroutine2
28  !! A routine should have a clear, unambiguous name, describing what the routine does.
29   SUBROUTINE my_subroutine(arg1, arg2, arg3) ! handle event xxxx
30   !!                            ^ comments should always be right before, or on the same line as what they're commenting
31   !!                            ^ as for modules, all functions should be documented in a comment
32     INTEGER, INTENT(IN) :: arg1
33     !!          ^ Always provide intent 
34     REAL, INTENT(IN)    :: arg2
35 
36     INTEGER, INTENT(INOUT) :: arg3
37 
38     REAl, INTENT(OUT) :: out1
39     !!^ Group together IN, then INOUT, then OUT
40 
41     IF (arg2 > arg1) THEN
42     !!       ^ .ge., .le., etc are deprecated. Use >, <, ==, etc instead
43       ! ...
44     END IF
45     !! ^ Use END IF, END DO rather than ENDIF, ENDDO (coherence with fortran-lang.org)
46   END SUBROUTINE my_subroutine
47 !!         ^ always use named blocks for END
48   
49 
50 END MODULE my_module
51 !!       ^ always use named blocks for END

Comments and Documentation

Documentation consists of putting information both inside and outside the source code. On large, formal projects, most of the documentation is outside the source code. However because the internal documentation is most closely associated with the code, it is the part most likely to remain correct as the code is modified. The thing to be really careful with is that misleading comments are even worse than having no comments at all. So, please report any misleading comments you found.

Try to avoid having superfluous comments such as: A = A + 1 ! Increment A by one. This is not adding anything that is not immediately obvious. A person looking through your code should only have to scan your comments to be able to get a good idea of what the code does and where to look for any special activity.

Good comments do not just repeat verbally what is happening in the code or just explain it. They clarify its intent. They should explain at a higher level of abstraction what the code or what you are trying to do. In addition, you should comment:

  • Everything that gets around an error or an undocumented feature in a language or environment.
  • Any violations of good programming style should be justified. This will ensure that any well-intentioned programmer does not break your code by changing the source to implement a “better” style.
  • The lines before a control structure, e.g. IF, CASE, loop, etc. are a natural spot to comment and explain what these constructs are about to do.
  • The start of any subroutine or function whose usage is not downright obvious.

Misc

Fortran standard

Fortran 2008 is now widely supported on all major compilers, and should be taken as a reference.

Allocatable arrays

Allocatable arrays can be relevant, but should be substituted by automatic arrays (= passing the array size as an argument) whenever possible, for performance and debugging reasons.

Compiler warnings

Compiler warnings are useful - unless there's too many of them. Effort should be made such that during the compiling phase, no warning appears, so that user can be easily alerted of potential bugs. when some appear in their configuration.

In particular, all component must be able to run when a compile-time and/or run-time array bounds checking option is enabled. Use of the (*) construct in array dimensioning to circumvent this problem is forbidden because it effectively disables array bounds checking.

Side effects

Whenever possible, write functions without side-effects, and label them accordingly as PURE or ELEMENTAL.

SAVE

Since all files contain a MODULE, attribute SAVE is banned. Variables whose value have to be preserved between two calls should be declared at the module level.

Deprecated features

In terms of keeping code up to date and easier to maintain the code should always follow the current standards of FORTRAN and ANSI C. We decide to restrict the languages use to the elements, which are not obsolete or deleted -- even if they are still available with almost all compilers.

Note: just because a feature is deprecated doesn't mean it must be converted in old, working code - this can be tricky and bug-prone in complex instances.

Some notable deprecated features:

  • COMMON blocks. Instead, use MODULE initialisation.
  • EQUIVALENCE should be replaced by pointers or derived data types.
  • Any kind of GOTO has absolutely no place in modern programming. Use CASE whenever relevant rather than IF.
  • Arithmetic IF statements. Use block IF or CASE instead.
  • CONTINUE statement: use IF, CASE, DO WHILE, EXIT, CYCLE statements or a contained SUBROUTINE instead.
  • I/O routines END and ERR - use IOSTAT instead
  • FORMAT statements: use character parameters or explicit format- specifiers inside the READ or WRITE statement instead.
  • ENTRY statements: a subprogram must only have one entry point
  • Alternate RETURN is obsolescent, and RETURN at the end of a function is useless clutter.
  • Statement functions have been replaced by corresponding functions or subroutines in the CONTAIN block of the current scope.
  • DATA and BLOCK DATA should be replaced by initializers.

Preprocessor

Limit to the bare minimum the use of preprocessor #ifdef keys. Those decrease lisibility, disable automatic code analysis, and generally make code a pain to manage and read. Ideally, a given preprocessor key should be used only once, or if not possible, in a single module for the entire codebase.

INCLUDE

Limit the use of include headers to the bare minimum. Whenever not possible, wrap those include in a module, and import that module instead. This provides much more flexibility, e.g. when using ONLY: ....

Recent examples of improper LMDZ code

Note: The purpose of this section is obviously not to blame anyone, but to stress that "bad"/perfectible code in LMDZ isn't limited to old legacy code, and that even current code is being written in a way that can be greatly improved.

07/24: Unhelpful comment

1 IF (iflag_ice_thermo == 0) THEN   
2            !pas necessaire a priori

This comment fails to convey a clear indication. What is necessary or not ? In which condition ? "A priori" further conveys a sense of uncertainty that may make sense to the writer, but certainly not to an external reader.

07/24: Unclear conventions, no PURE, useless RETURN

 1 function grid_index(lon_deg,lat_deg)
 2 !--------------------------------------------------------------------------------
 3 ! Get local index of grid point of longitude,latitude lon_deg,lat_deg
 4 ! Author : XXX 2024/07/XX
 5 ! Please do not put this function in a m*odule not to complexify the replay script
 6 !--------------------------------------------------------------------------------
 7 USE dimphy, only : klon
 8 USE geometry_mod, ONLY: latitude_deg, longitude_deg
 9 implicit none
10 real, intent(in) :: lon_deg,lat_deg
11 integer :: grid_index
12 integer i
13 grid_index=0
14 do i=1,klon
15    if ( abs(lon_deg-longitude_deg(i)) < 0.02 .and.  abs(lat_deg-latitude_deg(i)) < 0.02 ) then
16         grid_index=i
17         exit
18    endif
19 enddo
20 return
21 end

This function is actually mostly great ! It is documented, the author put a clear statement to indicate a deviation from standards (no module), USE is used in conjuction with ONLY, INTENT is declared. However, it is also lacking in some areas, notably:

  • This function should be declared as PURE
  • This function has inconsistent syntax: mix of uppercase and lowercase for Fortran keywords, sometimes uses :: in declarations and sometimes not
  • The function ends with a useless RETURN

07/24: Duplicated, uncommented code

 1           zx_tmp_fi3d=0.
 2           IF (vars_defined) THEN
 3              DO nsrf=1,nbsrf
 4                 DO k=1,klev
 5                    zx_tmp_fi3d(:,k)=zx_tmp_fi3d(:,k) &
 6                         +pctsrf(:,nsrf)*tke_shear(:,k,nsrf)
 7                 ENDDO
 8              ENDDO
 9           ENDIF
10 
11           CALL histwrite_phy(o_tke_shear, zx_tmp_fi3d)
12 
13           zx_tmp_fi3d=0.
14           IF (vars_defined) THEN
15              DO nsrf=1,nbsrf
16                 DO k=1,klev
17                    zx_tmp_fi3d(:,k)=zx_tmp_fi3d(:,k) &
18                         +pctsrf(:,nsrf)*tke_buoy(:,k,nsrf)
19                 ENDDO
20              ENDDO
21           ENDIF
22 
23           CALL histwrite_phy(o_tke_buoy, zx_tmp_fi3d)
24 
25 
26           zx_tmp_fi3d=0.
27           IF (vars_defined) THEN
28              DO nsrf=1,nbsrf
29                 DO k=1,klev
30                    zx_tmp_fi3d(:,k)=zx_tmp_fi3d(:,k) &
31                         +pctsrf(:,nsrf)*tke_trans(:,k,nsrf)
32                 ENDDO
33              ENDDO
34           ENDIF
35 
36           CALL histwrite_phy(o_tke_trans, zx_tmp_fi3d)

This code:

  • Duplicates three times essentially the same code. Refactoring into a routine would be much more readable and reduce the potential of typos.
  • Contains no comment justifying such behavior, or what is even being done here, which to an external reader is fairly obscure.

06/24: Undocumented variable

1 INTEGER                               :: nsrf, k, iq, iff, i, j, ilev, itr, itrb
2 !!                                                                           ^ Added this
3 !! (...)
4 itrb = 0
5 !! (...)
6 if(tracers(iq)%name(1:3)=='BIN') then
7                itrb = itrb + 1

Just because a variable is a "dumb" counter doesn't mean it shouldn't be documented. In this particular example, the variable name is devoid of any obvious meaning. Properly documenting this variable would also make it clear to others whether it would make sense to reuse it later on in the code.

06/24: Comments as code storage

1 !CALL writefield_phy('latitude',latitude_deg,1)
2 !CALL writefield_phy('pressure_hl',pressure_hl,klev+1)
3 !CALL writefield_phy('Ldecorel',PDECORR_LEN_EDGES_M,klev)

Comments are NOT for code storage. Unfortunately, the LMDZ code is littered with bits of commented code without any indication of when or why the code was commented. As a result, there are in a lot of places such comments, many dating from 20+ years ago, for which we have no clue whether we can remove them or not. This greatly hinders readability and maintainability.

Of course, in some cases, it is warranted to keep some code at hand, e.g. for diagnostic purpose. In such case, please make sure to always clearly document why, when and by whom the code was commented out, so that it can be cleaned up at a later date when it becomes irrelevant.

Questions à élucider

  • Prescrit-on une langue (Français / Anglais) ?
  • Longueur des lignes: dans la limite du raisonable, pourquoi se limiter à 100-140 chars ? Le wrapping fonctionne très bien.
Récupérée de « http://lmdz-forge.lmd.jussieu.fr/mediawiki/LMDZPedia/index.php?title=HowTo:_LMDZ_code_guidelines&oldid=484 »