current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite
1

The following pascal function (compiled with Delphi) will split strings. It works perfectly, but how to improve the code? For example, to avoid using the repeat-until loop.

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 '15 at 15:01
    
Ok , post updated. – Matt Olsen Feb 24 '15 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 '15 at 16:40
    
Did you consider using recursion? – Mawg Apr 13 '15 at 12:15
    
Recursion with undefined depth, that is unknown count of delimiter occurrences here, is a bad idea. – René Hoffmann Jun 25 '15 at 13:43
up vote 3 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.