[fpc-pascal] Re: StrUtils.RomanToInt oddities

[Lukasz Sokol]

On 23/09/13 17:18, Bart wrote:
> On 9/23/13, Lukasz Sokol <el.es.cr at gmail.com> wrote:
> 
>> function TryRomanToInt(AInput:String; out AResult: integer):boolean;
>> var i, Len, N, Np1 : integer;
> [snip]
>>
>>
>>         if N >= Np1 then AResult := AResult + N
>>         else AResult := AResult - N;
>>
>>       end;
>>     else // i = Len-1 = last char we just add (consider : MCM : add 1000,
> 
> Compiler doesn't like the else here. I removed the ; after the end 1 line above.
I said below it's only mind-compiled ;)

> 
>> sub 100, add 1000 = 1900)
>>      begin
>>        AResult := AResult + N;
>>      end;
>>   end; // for
>>   // if the above, after all characters are through, has resulted in 0 or
>> less,
>>   // we invalidate everything at the end (consider : CMLM, IIIM )
>>
>>   Result := AResult > 0;
>>   if not Result then AResult := 0;
>> end;
>>
>> (only mind-compiled ;) tests welcome ;) )
>>
> 
> It produces "odd" output though:
> 
> C:\Users\Bart\LazarusProjecten\ConsoleProjecten>test
> Enter Roman numeral: LL  //should be illegal
> TryRomanToInt('LL') -> 50
> StrUtils.RomanToInt('LL') = 100
Oh I see the loop needs to run from 0 to Len-1 ?.... it missed the last (or first) char

> 
> Enter Roman numeral: MM
> TryRomanToInt('MM') -> 1050  //obviously should be 2000
But this looks like you're adding the results? Or are you reusing the
same variable ? In that case stick AResult := 0 at the top of the function please

> StrUtils.RomanToInt('MM') = 2000
> 
> Enter Roman numeral: II
> TryRomanToInt('II') -> 1051  // nice one!  
Yeah totally added to former Aresult, and missed last char ...

> StrUtils.RomanToInt('II') = 2
> 
> So, it's a little bit flawed.

:)

function TryRomanToInt(AInput:String; out AResult: integer):boolean;
var i, Len, N, Np1 : integer;

  function TryRomanCharToInt(AIn: Char; out AOut: integer): boolean;
  begin
    case AIn of
      'M' : AOut := 1000;
      'D' : AOut := 500;
      'C' : AOut := 100;
      'L' : AOut := 50;
      'X' : AOut := 10;
      'V' : AOut := 5;
      'I' : AOut := 1;
    else
      AOut := 0;
    end;
    Result := (AOut > 0);
  end;

begin
  // if it looks like shameless c&p, it's because it is ;)
  Result := False;
  AResult := 0; // << here you've reused the variable passed to AResult somewhere
  AInput := UpperCase(AInput);  //don't use AnsiUpperCase please
  Len := Length(AInput);
  if (Len = 0) then Exit;
  // 
  i := 1;
  for i := 0 to Len-1 do // which char am I actually missing here??? 
  begin
    if not TryRomanCharToInt(AInput[i], N) then
    begin
      AResult := -1; // invalidate everything
      exit;
    // writeln('Not a valid roman numeral at position ',i);
    end;

    if (i > Len-1) then 
      begin 
        if not TryRomanCharToInt(AInput[i+1], Np1) then
        begin
          AResult := -1; // invalidate everithing
          exit;
        // writeln('Not a valid roman numeral at position ',i+1);
        end;
         
	// according to wikipedia, .
        if not (((N = 100) and (Np1 > 100)) or  // C can be placed before L or M
                ((N = 10) and (Np1 > 10) and (Np1 <= 100)) or // X can be placed before L or C
                ((N = 1) and (Np1 > 1) and (Np1 <= 50)))  // I can be placed before V and X << here too
        then
          begin
            AResult := -1; // invalidate everithing: catches MDCLXVIVXLDM
            exit;
            // writeln('Not a valid roman numeral combination at position ',i, ' and ',i+1);
          end;
  

        if N >= Np1 then AResult := AResult + N
        else AResult := AResult - N;
        
      end // i > Len-1, you're right about the ; 
    else // i = Len-1 = last char we just add (consider : MCM : add 1000, sub 100, add 1000 = 1900)
     begin // alternatively this should exit the loop maybe on the N-1th character 
       AResult := AResult + N;
     end;
  end; // for
  // if the above, after all characters are through, has resulted in 0 or less,
  // we invalidate everything at the end (consider : CMLM, IIIM )
  
  Result := AResult > 0;
  if not Result then AResult := 0;
end;


> 
> Anyhow, let's not focus upon what the new code should be.
> The question was: is current behaviour (accepting IIMIIC etc.) a bug or not.
> 
> Bart
> _______________________________________________
> fpc-pascal maillist  -  fpc-pascal at lists.freepascal.org
> http://lists.freepascal.org/mailman/listinfo/fpc-pascal
>