4
\$\begingroup\$

I am currently working on a little binary patcher. My current code works fine but I do want my code to be optimized (fast) and as clean as possible. Could you all please help me review this?

IDE: Delphi 7 and XE7

const
  OriginalByte: array [0 .. 55] of Byte = ($64, $69, $6E, $67, $20, $6D, $65,
    $20, $68, $69, $73, $00, $00, $00, $00, $27, $00, $00, $28, $00, $00, $00,
    $21, $00, $00, $00, $63, $6F, $6D, $70, $75, $74, $65, $72, $2E, $00, $00,
    $00, $00, $00, $00, $00, $00, $00, $00, $00, $00, $00, $27, $00, $00, $28,
    $00, $00, $00, $00);

  BytetoWrite: array [0 .. 55] of Byte = ($63, $68, $6F, $67, $20, $6D, $65,
    $20, $68, $69, $73, $00, $00, $00, $00, $27, $00, $00, $28, $00, $00, $00,
    $21, $00, $00, $00, $63, $6F, $6D, $70, $75, $74, $65, $72, $2E, $00, $00,
    $00, $00, $00, $00, $00, $00, $00, $00, $00, $00, $00, $27, $00, $00, $28,
    $00, $00, $00, $06);

function CompareByteArrays(FirstArray, SecondArray: array of Byte): Boolean;

var
  Counter: Integer;
begin

  Result := True;
  if Length(FirstArray) <> Length(SecondArray) then
  begin
    Result := False;
    Exit;
  end;

  for Counter := Low(FirstArray) to High(FirstArray) do

  begin
    if (FirstArray[Counter]) <> (SecondArray[Counter]) then
    begin
      Result := False;
      Exit;
    end

  end;

end;

procedure DoMyPatch();
var

  i: Integer;
  FileName: string;
  input: TFileStream;

  FileByteArray, ExtractedByteArray: array of Byte;

begin
  FileName := 'Cute1.res';

  try
    input := TFileStream.Create(FileName, fmOpenReadWrite);
  except
    on E: Exception do
    begin
      ShowMessage(E.Message);
      Exit;
    end
  end;

  input.Position := 0;
  SetLength(FileByteArray, input.size);
  input.Read(FileByteArray[0], Length(FileByteArray));
  for i := Low(FileByteArray) to High(FileByteArray) do
  begin

    ExtractedByteArray := Copy(FileByteArray, i, Length(OriginalByte));

    if CompareByteArrays(ExtractedByteArray, OriginalByte) = True then
    begin
      // Begin Patch
      input.Free;
      try
        Win32Check(CopyFile(PChar(FileName), PChar(ChangeFileExt(FileName,
          '.BAK')), False));
      except
        on E: Exception do
        begin
          ShowMessage(E.Message);
          Exit;
        end
      end;

      try
        input := TFileStream.Create(FileName, fmOpenReadWrite);
      except
        on E: Exception do
        begin
          ShowMessage(E.Message);
          Exit;
        end
      end;

      input.Seek(i, SoFromBeginning);

      input.Write(BytetoWrite[0], Length(BytetoWrite));

      ShowMessage('Patch Success');
      input.Free;
      Exit;
    end;
  end;
  ShowMessage('Patch Fail');

  input.Free;

end;
\$\endgroup\$
2
\$\begingroup\$

Some notes:

  1. add definition of the undeclared variables BytetoWrite, OriginalByte and how they are used/initialized
  2. format your code for better readability. Left indentation is not aligned. Too many void blank lines
  3. using

    resource := CreateResource;
    try
        // do something
    finally
        FreeAndNil(resource);
    end

    pattern is easy to follow and it prevents unintended memory leaks nicely

  4. for byte-to-byte memory compare in CompareByteArrays better use built-in CompareMem function instead of custom array traversal. In an old language runtime source codes this function was implemented in optimized assembly language
  5. using Integer for file-offset is not very future-friendly idea considering that it is normal (for average end-user) today to work with video files in 4K resolution where single file size can be >4GB
  6. using Memory Mapped Files for read-only file access will give you simpler code (whole file is an in-memory array) and faster I/O operations (on-demand reading managed by OS kernel code). This method can give you significant boost if the original file is big and the searched-pattern is located near the beginning of the file
  7. the loop should end at High(FileByteArray) - Length(OriginalByte) (+/-1) as beyond this boundary the pattern can not be found anyway
  8. you can get some speed by comparing against the original byte array just offset by the i index. Creation of the intermediate ExtractedByteArray would not be necessary (throw away, safe some memory allocation and memory copy CPU-clocks). Like this:

    if CompareByteArrays(ExtractedByteArray, OriginalByte) = True then
    if CompareMem(@FileByteArray[i], @OriginalByte[0], Length(OriginalByte)) = True then

    assuming the i...i + Length(OriginalByte) - 1 boundaries are withing the range Low(FileByteArray)...High(FileByteArray) as mentioned in (7)

\$\endgroup\$
14
  • \$\begingroup\$ thanks for your comments, and sorry i can't upvote you (not enough reputation) but i do have one question. if i use resource := CreateResource; try try // do something except raise exit; end finally FreeAndNil(resource); end the "finally" segment of the code doesn't get executed because of the "exit" command right? \$\endgroup\$
    – Xor-el
    Dec 30 '14 at 16:22
  • \$\begingroup\$ @Xor-el finally block will execute always (unless your application is crashing irrecoverably or the computer is going out of e-power). See Stack Overflow: Does the statements in the Finally block still execute in this piece of code? for some "proof" \$\endgroup\$
    – xmojmr
    Dec 30 '14 at 16:57
  • \$\begingroup\$ @Xor-el thanks for your accept/vote although it was probably too fast as some better answer may appear once the new year's eve holidays are over. Looking at your code after the edit it seems that the patcher is single-purpose so the (5) does not apply. Using (6) can give you significant boost if the original file is big and the searched-pattern is located near the beginning of the file. (7) the loop should end at High(FileByteArray) - Length(OriginalByte) (+/-1) otherwise you'll face the (random) access violation problem if the pattern is not there \$\endgroup\$
    – xmojmr
    Dec 30 '14 at 18:36
  • \$\begingroup\$ i don't think so because i have tested it with a pattern that does not exist and it works fine (it does not crash) and informs me that pattern was not found (patch fail). \$\endgroup\$
    – Xor-el
    Dec 30 '14 at 21:24
  • 1
    \$\begingroup\$ @xmomjr, nice habit i must say. \$\endgroup\$
    – Xor-el
    Dec 31 '14 at 14:11

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.