current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite
1

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;
share|improve this question

1 Answer 1

up vote 1 down vote accepted

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)

share|improve this answer
    
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? –  Xor-el Dec 30 '14 at 16:22
    
@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" –  xmojmr Dec 30 '14 at 16:57
    
thanks a lot for clearing my doubts –  Xor-el Dec 30 '14 at 17:02
    
@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 –  xmojmr Dec 30 '14 at 18:36
1  
@xmomjr, nice habit i must say. –  Xor-el Dec 31 '14 at 14:11

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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