2
\$\begingroup\$

The code below groups a list by either number or letter, as seen below:

1A, 1B, 2A, 2B, 3A, 3B, 4A, 4B, 5A, 5B

or

1A, 2A, 3A, 4A, 5A, 1B, 2B, 3B, 4B, 5B

The option is configurable by the user via the loopOrder variable. The code essentially sets the first loop (i loop) or third loop (k loop) to 1 to negate it and then sets the other loop to 2.

Code below:

unit Unit2;

interface

uses
  Winapi.Windows, Winapi.Messages, System.SysUtils, System.Variants, System.Classes, Vcl.Graphics,
  Vcl.Controls, Vcl.Forms, Vcl.Dialogs, Vcl.StdCtrls;

type
  TForm2 = class(TForm)
    ListBox1: TListBox;
    procedure FormCreate(Sender: TObject);
  private
    { Private declarations }
  public
    { Public declarations }
  end;

var
  Form2: TForm2;

implementation

{$R *.dfm}

procedure TForm2.FormCreate(Sender: TObject);
var
  loopOrder, i, j, k, x, y: integer;
  outputList: array [1 .. 5, 1 .. 2] of string;
begin

  loopOrder := 2; //1 for 1A, 2A, 3A etc. 2 for 1A, 1B, 2A etc.

  for i := 1 to loopOrder do
  begin
    for j := 1 to 5 do
    begin
      x := 2;
      if loopOrder = 2 then
        x := 1;

      for k := 1 to x do
      begin
        if loopOrder = 1 then
          y := k
        else
          y := i;

        if y = 1 then
          outputList[j, y] := inttostr(j) + 'A'
        else
          outputList[j, y] := inttostr(j) + 'B';

        ListBox1.Items.Add(outputList[j, y]);
      end;
    end;
  end;
end;

end.

Is there a cleaner way of doing this that perhaps uses less variables?

\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

I don't speak the language you are using so I won't try coding in it, but I would have thought you could simplify the code by something like

if loopOrder = 1 then  
    sort by 1st character //x=2 implies k=1 then 2 - unroll for loop
    k=1
    …
    k=2
    …
else
    sort by second character // x=1 implies k=1 - no loop required
    k=1
    …

and cut out a lot of testing and variables.
for example x=3-loopOrder //no need for x
hence k is either 1 (when loopOrder=2) or 1 then 2 (when loopOrder=1)
Unroll the for loops as in one case k can only be 1 and in the other just 1 or 2
and so on.

Later thought

Since you always execute the code for k=1 regardless of loopOrder you can do it and then execute the code for k=2 if and only if loopOrder = 2. This might save some code repeat.

\$\endgroup\$
3
\$\begingroup\$

For me, loops in a constructor are a red flag. I would move the loop logic into another method (possibly called "LoadListbox"). That would loop for the number you want to add, but not do it directly. Instead call a method GenerateListboxItem (below). This gives you something you can test in isolation. Ultimately it is a math problem, so solving it that way allows one to handle any number of values easily once you have the bounds (passed as parameters to the function defaulting to 5 and 2).

function GenerateListboxItem(aIndex:Integer;aSortLetter:boolean;
         aNumberCount:integer=5;aLetterCount:integer=2):String;
var
  num : integer;
  chr : integer;
begin
  assert(aIndex >= 0, 'Requested index is less than 0');
  assert(aIndex <= aNumberCount * aLetterCount,'Requested index is out of range of possible values');
  if aSortLetter then
    begin
      num := (aIndex MOD aNumberCount)+1;
      Chr := (aIndex DIV aNumberCount);
    end
  else
    begin
      num := (aIndex DIV aLetterCount)+1;
      Chr := (aINdex MOD aLetterCount);
    end;
  Result := IntToStr(num)+Char(Ord('A')+Chr);
end;

This can then be tested with the following:

for ix := 0 to 9 do
  begin
    if ix <> 0 then write(', ');
    Write(GenerateListBoxItem(ix,True));
  end;
writeln;
for ix := 0 to 9 do
  begin
    if ix <> 0 then write(', ');
    Write(GenerateListBoxItem(ix,False));
  end;

which generates the requested values:

1A, 2A, 3A, 4A, 5A, 1B, 2B, 3B, 4B, 5B
1A, 1B, 2A, 2B, 3A, 3B, 4A, 4B, 5A, 5B

Because its a simple function, there is no memory allocation required for larger sets. For example:

GenerateListBoxItem(10000000,False,10000000,5)

returns 2000001A without the expensive loop to generate this value. This is very helpful if your going to display the data in a virtual list control such as TControlList.

Edit

One more thing. If you need to store "extra" data each unique item can be addressed by num * chr. So you can use it to reference another list/array of more detailed data. This is based on the principle that x * y = y * x. So you could have an array containing the values in the order of 1A, 2A, 3A, 4A, 5A, 1B, 2B, 3B, 4B, 5B and then directly reference that array to get the value out. This does mean that the output can be anything, provided the number of the two parts can be counted and sorted prior to referencing and are ordered by the ord(first field) * ord(second field). (note ord here means numerical value of the position of the field in a unique sorted list of field values)

\$\endgroup\$
2
\$\begingroup\$

It's been quite a while since I've used Delphi - great times :) - so consider any code I write as pseudo code.

There are several problems I see here:

It's quite clear what exactly your task or goal here is. You basically speak of "grouping a list", and I first interpreted that as sorting an existing list. However the code is actually creating a list.

Then the structure of the code is quite confusing. You have one single procedure that "does everything", including directly filling the TListBox.

This includes the input. Basically everything (the "loopOrder", and the structure of the list elements) is hard-coded.

I'd first suggest breaking things down into multiple procedures, especially just as @PeterJennings suggests, into two procedures for the two variants. Then - if you need it - rewrite to support differently structured lists later.

The input could be changed to something like:

const 
    ListStructure: array of array of string = (
        ('1', '2', '3', '4', '5'),
        ('A', 'B')
    );
\$\endgroup\$
1
  • \$\begingroup\$ "It's quite clear" - or unclear? \$\endgroup\$ Commented May 6, 2024 at 19:09

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.