3
\$\begingroup\$

As a little standard exercise, I wrote an integer to roman numerals converter in F03. The result looks already neat enough, for my standards, but I wonder whether it could be made snappier without sacrificing clarity. For folks not familiar with the simple syntax of modern Fortran, I suggest the following quick modern Fortran tutorial.

module mRomanStrings

  implicit none
  private
  character(len=*), parameter :: RM_CHARS  = "IVXLCDMvxlcdm_"
  integer         , parameter :: MAX_ORDER = len(RM_CHARS)/2-1
  character(len=4)            :: ROMAN_STRING(0:9,0:MAX_ORDER)
  logical                     :: mRomanStringsIsNotInitialized = .TRUE.
  public :: IntegerToRoman

contains

  function IntegerToRoman(i) result(rm)
    integer         , intent(in)  :: i
    character(len=:), allocatable :: rm
    integer :: n
    if( mRomanStringsIsNotInitialized ) call mRomanSring_init()
    rm = " "
    do n = min(MAX_ORDER,int(log(dble(i))/log(10.d0)+1.d-10)), 0, -1
       rm = trim(rm)//trim(ROMAN_STRING(mod(i/10**n,10),n))
    enddo
  end function IntegerToRoman

  subroutine mRomanSring_init()
    character :: a0, a1, a2, a3
    integer :: n
    do n = 0, MAX_ORDER
       a0 = " "
       a1 = RM_CHARS(2*n+1:2*n+1)
       a2 = RM_CHARS(2*n+2:2*n+2)
       a3 = RM_CHARS(2*n+3:2*n+3)
       ROMAN_STRING(0:9,n) = [&
            a0//a0//a0//a0, a1//a0//a0//a0, a1//a1//a0//a0, a1//a1//a1//a0, a1//a2//a0//a0, &
            a2//a0//a0//a0, a2//a1//a0//a0, a2//a1//a1//a0, a2//a1//a1//a1, a1//a3//a0//a0 ]
    enddo
    mRomanStringsIsNotInitialized = .FALSE.
  end subroutine mRomanSring_init
     
end module mRomanStrings

program ConvertIntegersToRoman
  use, intrinsic :: iso_fortran_env
  use mRomanStrings
  implicit none
  integer :: i
  do i=1,10000
     write(OUTPUT_UNIT,"(i6,x,a)") i, IntegerToRoman(i)
  enddo
end program ConvertIntegersToRoman
\$\endgroup\$
2
  • \$\begingroup\$ There is an error in your the initialization routine. 2 * MAX_ORDER + 3 > size(RM_CHARS) \$\endgroup\$
    – mcocdawc
    Jan 12 '21 at 14:32
  • \$\begingroup\$ If you compile with gfortran -g -Wall -Wextra -Warray-temporaries -Wconversion -fimplicit-none -fbacktrace -ffree-line-length-0 -fcheck=all -ffpe-trap=zero,overflow,underflow -finit-real=nan You get automatically warned about boundary violations. \$\endgroup\$
    – mcocdawc
    Jan 12 '21 at 14:33
3
\$\begingroup\$

It is overall good code, but there are of course some points to improve.

Objectively wrong and severe:

  1. Always compile with as much debug options as possible with your compiler. It detects errors such as

    2 * MAX_ORDER + 3 == 2 * (size(RM_CHARS) / 2 - 1) + 3 > size(RM_CHARS)
    

    in your initialization loop.

  2. The function expects a number that is larger than zero and smaller than 10^6. This is nowhere documented nor checked.

Small things that could be made better:

  1. There are unnecessary many trim calls when you build up the string. If you only append trimmed strings, you know that the result is trimmed.

  2. There is log10 for the decadic logarithm.

  3. It helps sometimes to introduce additional variable names for self documentation.

  4. I would recommend to use the words magnitude, exponent, and mantissa in the code. This is more self-explaining than "order".

  5. It is not necessary to start from Zero for the first dimension of ROMAN_STRING

  6. I would never name a logical variable with its negated form. You can always use .not. and it quickly becomes hard to read if you doubly negate it. The name could be shorter in addition.

    mRomanStringsIsNotInitialized == .not. mRomanStringInitialized
    .not. mRomanStringsIsNotInitialized == mRomanStringInitialized
    
  7. I would use floor instead of int. It is a common error to use int instead of nint so it is nice to explicitly say floor.

Architecture:

At the moment you have runtime checks in your function to ensure initialization of ROMAN_STRING. This is not necessary since ROMAN_STRING could be made a compile time constant (with parameter). If you do this then your function does not depend anymore on a global variable that can be mutated at run time, can be made pure and is probably a tiny bit faster.

Opinion based:

I would add more whitespace around your operators and after commata and use an indentation of at least 4 spaces. You can format Fortran code pretty much according to the PEP8 guidelines of python, to achieve convincing results. (Especially since a lot of people in scientific computing use both languages.)

Everything together leads to:

  module mRomanStrings
  
    implicit none
    private
    integer         , parameter :: MAX_MAGNITUDE = 5 
    character(len=4), parameter :: ROMAN_STRING(9, 0 : MAX_MAGNITUDE) = & 
          reshape([character(4) :: &
          'I', 'II', 'III', 'IV', 'V', 'VI', 'VII', 'VIII', 'IX', &
          'X', 'XX', 'XXX', 'XL', 'L', 'LX', 'LXX', 'LXXX', 'XC', &
          'C', 'CC', 'CCC', 'CD', 'D', 'DC', 'DCC', 'DCCC', 'CM', &
          'M', 'MM', 'MMM', 'Mv', 'v', 'vM', 'vMM', 'vMMM', 'Mx', &
          'x', 'xx', 'xxx', 'xl', 'l', 'lx', 'lxx', 'lxxx', 'xc', &
          'c', 'cc', 'ccc', 'cd', 'd', 'dc', 'dcc', 'dccc', 'cm'], &
          [9, MAX_MAGNITUDE + 1]) 
    public :: IntegerToRoman
  
  contains
  
      !> Expects an integer 1 <= n <= (10^6 - 1) and returns
      !>  the Roman number literal as string.
      pure function IntegerToRoman(n) result(rm)
          integer, intent(in) :: n
          character(len=:), allocatable :: rm
          integer :: largest_magnitude, mag, mantissa
          if (n <= 0) error stop
  
          largest_magnitude = floor(log10(dble(n)))
          if (largest_magnitude > MAX_MAGNITUDE) error stop
  
          rm = ""
          do mag = largest_magnitude, 0, -1
              mantissa = mod(n / 10**mag, 10) 
              if (mantissa > 0) then
                  rm = rm // trim(ROMAN_STRING(mantissa, mag))
              end if
          end do
      end function IntegerToRoman
  
  end module mRomanStrings
  
  program ConvertIntegersToRoman
      use, intrinsic :: iso_fortran_env, only: OUTPUT_UNIT
      use mRomanStrings
      implicit none
      integer :: i
      do i = 1, 99999
          write(OUTPUT_UNIT,"(i6,x,a)") I, IntegerToRoman(i)
      end do
  end program ConvertIntegersToRoman
\$\endgroup\$
9
  • \$\begingroup\$ Thanks for the detailed and informative feedback mcocdawc. Not sure about the four-space indentation: I like the default in emacs. By the way, it looks like julia should be a more natural companion to people in the fortran community than python. Any thoughts? \$\endgroup\$ Jan 13 '21 at 9:19
  • \$\begingroup\$ When I see floor instead of int I am starting to look what is going on with negative numbers and get confused when it is in fact nothing. \$\endgroup\$
    – Vladimir F
    Jan 13 '21 at 14:26
  • \$\begingroup\$ @LucaArgenti I worked once with Julia for a numeric task when it had version 0.6. Even then I liked it, and I guess they have improved the things that I did not like. (It was fast after JIT compilation, but the first execution of a function was really slow. Since you could not easily precompile functions, it was slower than python if you only executed functions once.) If I started a new numerics project now, I would definitely consider Julia, but since our number crunching tasks rely on existing Fortran code with more than 10^6 lines of code you just don't rewrite it. \$\endgroup\$
    – mcocdawc
    Jan 13 '21 at 16:23
  • \$\begingroup\$ For postprocessing/plotting I have good workflow with numpy/pandas/matplotlib and speed does not matter, so I won't change it. I assume it is the case for a lot of colleagues. Nevertheless I am happy to have at least written a short serious program with Julia, because their multiple dispatch paradigm is really nice. It definitely improved my code to use a bit less OOP and a bit more overloaded but free functions. (free as in not a method) PS: With vim you write much better Fortran code. ;-) \$\endgroup\$
    – mcocdawc
    Jan 13 '21 at 16:29
  • \$\begingroup\$ @VladimirF I am sorry, but I don't really get your argument floor(-3.5) == -4 is IMHO not nothing. \$\endgroup\$
    – mcocdawc
    Jan 13 '21 at 16:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.