Integer to Roman Numeral conversion

Question

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

Answer 1

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