How to use DRY principle rightly?

1

Good day!
Usually I try to make my code more compact and I formatting similar code parts to the procedures and functions.
But in some cases I see that with my programming skills this refactoring can make my code more difficult.
I will show typical case from my project. This procedure move selected items from inventory to the equipment and and vice versa.

procedure THeroItemsInterface.SortItems(Sender : TObject);
var
  I : Integer;
begin         
  if Length(Inventory) > 0 then
  begin
    for I := 0 to High(Inventory) do
      if Inventory[I].Button.Pressed then
      begin
        SetLength(fHero.Equipped.Items, Length(fHero.Equipped.Items)+1);
        fHero.Equipped.Items[High(fHero.Equipped.Items)] := FHero.Inventory.Items[I].CopyItem();
      end;
    for I := High(Inventory) downto 0 do
      if Inventory[I].Button.Pressed then
        FHero.Inventory.DelItem(I);
  end;

  if Length(Equipped) > 0 then
  begin
    for I := 0 to High(Equipped) do
      if Equipped[I].Button.Pressed then
      begin
        SetLength(fHero.Inventory.Items, Length(fHero.Inventory.Items)+1);
        fHero.Inventory.Items[High(fHero.Inventory.Items)] := FHero.Equipped.Items[I].CopyItem();
      end;
    for I := High(Equipped) downto 0 do
      if Equipped[I].Button.Pressed then
        FHero.Equipped.DelItem(I);
  end;

  Redraw;
end;

In the one hand, I can make additional procedure with 2 parameters: GUI of the inventory and real inventory bounded with this GUI… But GUI is just array of items interfaces and I need to create new type to use them as parameter of procedure.
So I think the current code huge and some ugly, but it have more clear logic for me then potential code after refactoring.
What you think about situation like this? I think this problem seems little, but it very typical for me.
(You can also say about another problems of this procedure and my coding :upside_down_face:).

Thanks!

2

I would strive for code like this:

procedure THeroItemsInterface.SortItems(Sender : TObject);

  { Items from InputList (that have Pressed in PressedList) are moved to OutputList. }
  procedure MovePressed(const PressedList: TItemButtonList; 
    const InputList, OutputList: TItemsList); 
  var
    I: Integer;
  begin
    for I := 0 to PressedList.Count - 1 do
      if PressedList[I].Button.Pressed then
      begin
        OutputList.Add(InputList[I]);
        InputList.Delete(I);
      end;
  end;

begin
  MovePressed(Inventory, fHero.Inventory, fHero.Equipped);
  MovePressed(Equipped, fHero.Equipped, fHero.Inventory);
end;

But this is not a ready solution – it requires having list types TItemButtonList, TItemsList that actually allow this.

Maybe you can use generics for this. To be simplest, make things instances of classes, and just have TItemsList = {$ifdef FPC}specialize{$endif} TObjectList<TItem>. I’m speculating here. But I guess my point is – “if you can change your data structures to make the code simpler, do it”.

My main priorities, reviewing your code, is thus:

  1. Do not repeat the logic two times – we have 2x the same logic for Inventory and Equipped. That’s what I tried to achieve with MovePressed, just called 2x.
  2. Do both adding and removing in one iteration. Not for speed, but because it does seem simpler. Things look simple when adding to OutputList and removing from InputList are close to each other.

Also, I don’t know the context – but are indexes of the lists you use 100% guaranteed equal (Inventory, fHero.Inventory)? Your code seems to assume it. Is there opportunity to not assume this? This would be my next priority: remove the assumption in code that the order of buttons and fHero.Xxx lists match. E.g. this seems even better:

procedure THeroItemsInterface.SortItems(Sender : TObject);

  { Items from InputList (that have Pressed in PressedList) are moved to OutputList. }
  procedure MovePressed(const PressedList: TItemButtonList; 
    const InputList, OutputList: TItemsList); 
  var
    Item: TItem;
    Pressed: TItemButton;
  begin
    for Pressed in PressedList do
      if Pressed.Button.Pressed then
      begin
        Item := ItemFromButton(Pressed.Button);
        OutputList.Add(Item);
        InputList.Remove(Item);
      end;
  end;

begin
  MovePressed(Inventory, fHero.Inventory, fHero.Equipped);
  MovePressed(Equipped, fHero.Equipped, fHero.Inventory);
end;

… but it implies you can implement ItemFromButton(...). If you create buttons from code, maybe you can make

TMyItemButton = class(TCastleButton)
public
  Item: TItem;
end;

Or maybe you can overuse TComponent.Tag, available at buttons too: MyItem := TItem(Button.Tag) – it requires typecast, thus does not look super clean, but it is a common solution to avoid mapping TComponent (designed visually) to your own logic. I would say it’s better to use Tag this way, than to rely on 2 lists having matching order.

Note: This is all “food for thought”! It’s easy to rearrange things when I write pseudo-code, not knowing at context, not knowing what you’re prepared to refactor in the context. So don’t treat my suggestion as this has to look like this – I’m just pointing out how it would seem to look more obvious to me. But we cannot always have perfection. “Done is better than perfect” :slight_smile: So maybe you will find inspiration in my reworked code above, but discard it freely if it’s nonsense / doesn’t fit your other design.

1 Like
3

P.S. And I guess I didn’t really just think about “DRY” principle :slight_smile: I was more like “how to make it obvious what happens, and remove assumptions that seem risky” - treating your question as a code review. DRY is part of this, but I guess a bunch of other guidelines I have in my head entered too.

And remember this (what I would find cleanest code) is subjective, and my suggestions are disconnected from what you can actually do (how much you’re prepared to refactor your data structures). So I hope this is helpful, but it’s by no means something you should consider “this must be like this”. I would not not consider my own suggestions as anything more than “suggestions” when doing this code. Maybe I would strive for the code to look like I envisioned, but resign from this design if other circumstances tell me it’s too much work for too little gain.

1 Like