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 2 down vote favorite
1

I have the following split function in Delphi works perfect the problem is I want to improve the code to not use repeat-until more but not that another way to do it.

type
  TSarray = array of string;

function Split(Texto, Delimitador: string): TSarray;

var
  o: integer;
  PosDel: integer;
  Aux: string;

begin

  o := 0;
  Aux := Texto;
  SetLength(Result, Length(Aux));

  repeat

    PosDel := Pos(Delimitador, Aux) - 1;

    if PosDel = -1 then
    begin
      Result[o] := Aux;
      break;
    end;

    Result[o] := copy(Aux, 1, PosDel);
    delete(Aux, 1, PosDel + Length(Delimitador));
    inc(o);
  until Aux = '';
end;

Example: 

var texto,deli:string;
    all_array:TSarray;
begin
deli := 'test';
texto := deli+'hi world 1'+deli+'hi world 2'+deli;
end;

all_array := Split(texto,deli);
ShowMessage(all_array[1]);
ShowMessage(all_array[2]);
end;

My plan is to use no classes, only the "uses" default

What alternatives do I have to repeat-until?

share|improve this question
    
Can you please add some example inputs / outputs of what your code is doing? –  Simon Forsberg♦ Feb 24 at 15:01
    
Ok , post updated. –  NickFuryy Feb 24 at 15:10
    
I meant add some example inputs / outputs in pure text form, for those that do not have a Delphi compiler around. –  Simon Forsberg♦ Feb 24 at 16:40
    
Did you consider using recursion? –  Mawg Apr 13 at 12:15
    
Recursion with undefined depth, that is unknown count of delimiter occurrences here, is a bad idea. –  René Hoffmann Jun 25 at 13:43

1 Answer 1

up vote 1 down vote

First, I don't quite understand what you mean with not using repeat-until, because in order to split string you have to use some kind of loop.

I assume that you want to have some speed improvements of your code.

If you are not changing string parameters passed to routine you should declare them as const parameters to make call run faster

function Split(const Texto, Delimitador: string): TSarray;

You are reserving way too much memory with SetLength(Result, Length(Aux)); You can freely increase length of resulting array as you go SetLength(Result, Length(Result) + 1); and it will also run faster.

And finally, copying string you are splitting and then deleting parts of the string is not optimal implementation. You should be using overloaded version of Pos function where third argument is offset from which it starts searching, and eliminate Aux string completely.

Also, using o as variable name is not recommended because it can easily be misread as 0, so I replaced it with i, that is commonly used as index variable.

So final code with all optimizations implemented might look like this:

function Split(const Texto, Delimitador: string): TSarray;
var
  i: integer;
  Len: integer;
  PosStart: integer;
  PosDel: integer;
begin
  i := 0;
  SetLength(Result, 1);
  Len := Length(Delimitador);
  PosStart := 1;
  PosDel := Pos(Delimitador, Texto);
  while PosDel > 0 do
    begin
      Result[i] := Copy(Texto, PosStart, PosDel - PosStart);
      PosStart := PosDel + Len;
      PosDel := Pos(Delimitador, Texto, PosStart);
      inc(i);
      SetLength(Result, i + 1);
    end;
  Result[i] := Copy(Texto, PosStart, Length(Texto));
end;
share|improve this answer

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.