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.

Here is a small library which manages writing into and reading out a byte array. It uses TValue arrays to get and put data, its my first time using them. Its crudely written, and poorly optimized but I tried to make the best solution in the 3 hrs I was given to build and debug it. Right now it only supports Integer, String, TGUID, TBytes and TDateTime as data inputs.

I want your thoughts on it, any areas that may be improved with know how of yours. And for the raging ones, I am not trying to be faster than delphi!

Library

unit AgBuffer;

interface

uses System.SysUtils, System.Rtti, Windows;

type

{$SCOPEDENUMS ON}

TAgData = ( Int, Str, GUID, Bytes, Date );

TAgBuffer = class ( TObject )  

  private

  fSize   : Int64;  

  procedure CheckGrowNecessity ( const AElementSize: Int64 );

  procedure EmbedInteger  ( const AInteger: Integer ); inline;
  procedure EmbedString   ( const AString: String ); inline;
  procedure EmbedGUID     ( const AGUID: TGUID ); inline;
  procedure EmbedBytes    ( const ABytes : TBytes ); inline;
  procedure EmbedDateTime ( const ADateTime: TDateTime ); inline;

  function  ExtractInteger: Integer; inline;
  function  ExtractString: String; inline;
  function  ExtractGUID: TGUID; inline;
  function  ExtractBytes: TBytes; inline;
  function  ExtractDateTime: TDateTime; inline;

  public

  Buffer      : TBytes;
  InitialSize : Int64;
  Position    : Int64;
  Values      : TArray <TValue>;

  constructor Create ( const ABuffer: TBytes = nil );

  procedure Reinitialize ( const ABuffer: TBytes );

  procedure Add ( const AData: Array of TValue );
  procedure Extract ( const ADataTypes: Array of TAgData );
  procedure Clear;

  property  Size   : int64  read fSize;

end;

implementation

constructor TAgBuffer.Create ( const ABuffer: TBytes = nil );
begin

  if Assigned ( ABuffer ) then
  begin

    Buffer := ABuffer;
    fSize  := Length ( ABuffer );
    InitialSize := fSize;

  end;

end; 

procedure TAgBuffer.Reinitialize ( const ABuffer: TBytes );
begin  

  Buffer      := ABuffer;
  fSize       := Length ( Buffer );
  InitialSize := fSize;
  Position    := 0;
  SetLength   ( Values, 0 ); 

end;

procedure TAgBuffer.CheckGrowNecessity ( const AElementSize: Int64 );
begin

  fSize := fSize + AElementSize;
  if fSize > InitialSize then
  begin
    InitialSize := InitialSize + 512;
    SetLength ( Buffer, InitialSize );
  end;

end;

procedure TAgBuffer.EmbedInteger ( const AInteger: Integer );
begin    
  CheckGrowNecessity ( 4 );
  Move ( AInteger, Buffer [Position], 4 );
  Position := Position + 4;    
end;

procedure TAgBuffer.EmbedString ( const AString: String );
var
  StringLength : DWORD;
  StringSize   : Int64;
begin

  StringLength := Length ( AString );
  StringSize   := StringLength * 2;
  CheckGrowNecessity ( StringSize + 4 );

  Move ( StringLength, Buffer [Position], 4 );
  Move ( AString [1], Buffer [Position + 4], StringSize );
  Position := Position + StringSize + 4;

end;

procedure TAgBuffer.EmbedGUID ( const AGUID: TGUID );
begin    
  CheckGrowNecessity ( 16 );
  Move ( AGUID, Buffer [Position], 16 );
  Position := Position + 16;    
end;

procedure TAgBuffer.EmbedBytes   ( const ABytes : TBytes );
var
  StringLength : DWORD;
begin

  StringLength := Length ( ABytes );
  CheckGrowNecessity ( StringLength + 4 );

  Move ( StringLength, Buffer [Position], 4 );
  Move ( ABytes [0], Buffer [Position + 4], StringLength );
  Position := Position + StringLength + 4;

end;

procedure TAgBuffer.EmbedDateTime ( const ADateTime: TDateTime );
begin    
  CheckGrowNecessity ( 8 );
  Move ( ADateTime, Buffer [Position], 8 );
  Position := Position + 8;    
end;

procedure TAgBuffer.Add ( const AData: Array of TValue );
var
  I        : DWORD;
  TypeChar : Char;
begin

  if Length ( AData ) <> 0 then
  begin

    // Initialization
    Position    := fSize;
    InitialSize := fSize + 1024;
    SetLength ( Buffer, InitialSize );

    for I := 0 to Length ( AData ) - 1  do
    begin

      // Preparation
      TypeChar := Char ( AData [I].TypeInfo.Name [2] );

      // Type determination
      if TypeChar = 'n' then
        EmbedInteger ( AData [I].AsInteger )

      else if TypeChar = 't' then
        EmbedString ( AData [I].AsString )

      else if TypeChar = 'G' then
        EmbedGUID ( AData [I].AsType <TGUID> )

      else if TypeChar = 'A' then
        EmbedBytes ( AData [I].AsType <TBytes> )

      else if TypeChar = 'D' then
        EmbedDateTime ( AData [I].AsType <TDateTime> );

    end;
    // Freeing left over space
    SetLength ( Buffer, fSize );

  end;

end;

function  TAgBuffer.ExtractInteger: Integer;
begin    
  Move ( Buffer [Position], Result, 4 );
  Position := Position + 4;    
end;

function  TAgBuffer.ExtractString: String;
var
  StrLength : DWORD;
  StrSize   : int64;
begin

  Move      ( Buffer [Position], StrLength, 4 );
  SetLength ( Result, StrLength );

  StrSize   := StrLength * 2;
  Move      ( Buffer [Position + 4], Result [1], StrSize );
  Position := Position + 4 + StrSize;

end;

function  TAgBuffer.ExtractGUID: TGUID;
begin    
  Move ( Buffer [Position], Result, 16 );
  Position := Position + 16;    
end;

function  TAgBuffer.ExtractBytes: TBytes;
var
  ArrayLength: DWORD;
begin

  Move      ( Buffer [Position], ArrayLength, 4 );
  SetLength ( Result, ArrayLength );
  Move      ( Buffer [Position + 4], Result [0], ArrayLength );
  Position := Position + 4 + ArrayLength;

end;

function  TAgBuffer.ExtractDateTime: TDateTime;
begin    
  Move ( Buffer [Position], Result, 8 );
  Position := Position + 8;    
end;

procedure TAgBuffer.Extract ( const ADataTypes: Array of TAgData );
var
  I : DWORD;
begin

  if Length ( ADataTypes ) <> 0 then
  begin

    SetLength ( Values, Length ( ADataTypes ));

    for I := 0 to Length ( ADataTypes ) - 1  do
      case ADataTypes [I] of

        TAgData.Int   : Values [I] := ExtractInteger;
        TAgData.Str   : Values [I] := ExtractString;
        TAgData.GUID  : Values [I] := TValue.From <TGUID> ( ExtractGUID );
        TAgData.Bytes : Values [I] := TValue.From <TBytes> ( ExtractBytes );
        TAgData.Date  : Values [I] := TValue.From <TDateTime> ( ExtractDateTime );

      end;

  end;

end;

procedure TAgBuffer.Clear;
begin

  SetLength   ( Buffer, 0 );
  SetLength   ( Values, 0 );
  fSize       := 0;
  InitialSize := 0;
  Position    := 0;

end;
end.

Example

To use it, I am giving an example which embeds a header of sorts of 2 ints and 2 guids:-

uses
  AgBuffer, System.SysUtils, System.Rtti;

---

var
  Packet1, Packet2           : TAgBuffer;
  Int1, Int2, Int3, Int4     : Integer;
  GUID1, GUID2, GUID3, GUID4 : TGUID;

---

// Embedding Data

Int1 := 4;
Int2 := 56000;

CreateGUID ( GUID1 );
CreateGUID ( GUID2 );

Packet1 := TAgBuffer.Create;

Packet1.Add
( [Int1, Int2, 
   TValue.From <TGUID> ( GUID1 ), TValue.From <TGUID> ( GUID2 )] );

// Packet1.Buffer can now be used for any transmission

// Extracting Data

Packet2 := TAgBuffer.Create ( Packet1.Buffer );
Packet2.Extract ( [AGB_INT, AGB_INT, AGB_GUID, AGB_GUID] );

Int3  := Packet.Values [0].AsInteger;
Int4  := Packet.Values [1].AsInteger;
GUID3 := Packet.Values [2].AsType <TGUID>;
GUID4 := Packet.Values [3].AsType <TGUID>;
share|improve this question
    
Why do you have all of those commented lines? Is this by convention? Because it's ugly! –  Max Nov 28 '13 at 8:53
    
The commenting is done, well for comments sakes, the lines are there only to fragment code. This site is deserted, and people on SO were saying to paste such here, how can they recommend this... –  Umair Ahmed Nov 28 '13 at 9:00
    
You asked for our thoughts and that's my thought. You don't "fragment" code by adding huge lines in the middle of the code. –  Max Nov 28 '13 at 9:08
    
@UmairAhmed Welcome to Code Review! We have less users than SO indeed, but have a few Delphi programmers around (codereview.stackexchange.com/tags/delphi/topusers). Wait and see, sorry! By the way, smaller snippets of code are easier to review, so if you're able to reduce your code to a specific part of the library, it would probably help too. –  Quentin Pradet Nov 28 '13 at 9:10
    
Sorry, they were not meant for fragmenting code for me, I just put them to emphasize different code parts for people here. I'll remove them if you people don't like them. –  Umair Ahmed Nov 28 '13 at 9:11

2 Answers 2

up vote 3 down vote accepted

Some suggestions:

Replace this

const
  AGB_INT  = 0;
  AGB_STR  = 1;
  AGB_GUID = 2;
  AGB_BUFF = 3;
  AGB_DATE = 4;

By this

{$SCOPEDENUMS ON} // Enabling scoped enumerations

TDataKind = (
  IntegerData,
  StringData,
  GUIDData,
  BufferData,
  DateData
);

Enumerations are the way in Delphi. It seems you have a background on C or C++, but the use of const in Delphi is kind of rare. In code you will use one of those constants like this: TDataKind.IntegerData, wht is good for code readability.

Replace this

public
  Buffer      : TBytes;
  InitialSize : Int64;
  Position    : Int64;
  Values      : TArray <TValue>;

By this

public
  property Buffer: TBytes read FBuffer write SetBuffer;
  property InitialSize: Int64 read FInitialSize write SetInitialSize;
  property Position: Int64 read FPosition write SetPosition;
  property Values[aIndex: Integer]: TValue read GetValues write SetValues;
  property ValuesCount: Integer read GetValuesCount;

Never expose class data directly, rather use properties to control the access outside code has over the class internals.

It seems to me that you are trying to mimic the behavior of a stream. Delphi already has many streaming classes, all subclasses of TStream. In particular, I guess you would like to know TMemoryStream and eventually use it as the ancestor to your class.

I hope it helps!

share|improve this answer
    
(+1) ...just a thought about Delphi streaming: it does a good job, but it's not neutral to the platform/tools you use. –  Wolf Nov 28 '13 at 13:49
    
I have used Memory Stream many times before but the issue with it is that it extends itself on every new element add. Also a lot functionality comes along with it which is not necessary for making packets. I want as much less going on as possible. With this one can extend TValue fields to accommodate any type of classes. –  Umair Ahmed Nov 28 '13 at 15:01
    
Thanks for the suggestions, and yes I have lived with C/C++ far more than delphi. –  Umair Ahmed Nov 28 '13 at 15:07

I'm not a Delphi programmer, but here are a two comments based on what I understood.

Use an enumeration type instead of characters ('n', 't'...) to determine the type.

It seems that TArray is a dynamic array, that is, it shrinks and grows automatically and in a smart way based on usage. "Smart" here means that the array size does not change too often: when you need more space, the capacity is doubled, avoiding costy reallocations. It guarantees a small cost for such updates. That's a part of TArray that you did not 'emulate'.

share|improve this answer
    
I did to some extent from the CheckGrowNecessity method which allocates 1KB blocks to the array whenever a new element demands more space than the array has already. Due to the packets being sent over the internet, the add method also finally removes any additional free space allocated. –  Umair Ahmed Nov 28 '13 at 11:43

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.