Code-Kata: Cache-Klasse. Wer produziert den besten Code

[Dejan Vu]

Zitat von idefix2:

Das Interface der Klasse habe ich bewusst etwas anders gestaltet als in der Vorgabe

Wieso hältst Du dich nicht an die Vorgabe? Wieso soll sich ein Cache um das Schreiben kümmern? Wieso soll er Statistiken pflegen?

Durchgefallen.

Im Ernst: Sollen wir für deine Sonderklasse extra eigene Test- und Prüfroutinen schreiben?


Schreib Deine doch bitte um, dann kann man die mit den MRU-Ansätzen vergleichen. Dein Algorithmus ist wirklich sehr interessant.

Ein Tipp bezüglich der function TCache.FindPlaceInCache: integer; : Du rennst mit einer For-Schleife durch die Liste? Das ist jetzt aber nicht so sonderlich performant, denn ein Cache verwaltet i.a. Millionen von Elementen.
Diese Routine wird bei jedem Cache-Aufruf einmal komplett durch alle Elemente laufen. Das kannst Du mit einer Priority-Queue (bzw. einem Baum) viel effizienter Lösen. Der wird bei jedem Zugriff sortiert bzw. mit wenigen Handgriffen angepasst, sodass dein 'worst element' immer automatisch 'oben' ist. Aber die Implementierung ist natürlich ungemein komplexer.

Zitat von Namenloser:

Ein paar Worte zu meinen Designentscheidungen:
- Ich prüfe nicht explizit ob die Aufrufe gültig sind (z.B. ob bei Get der Key auch wirklich existiert) und werfe daher auch keine Exceptions. Denn offensichtlich ist die Schnittstelle so angelegt, dass der Benutzer vorher selbst mit Contain prüfen soll, ob der Key existiert....

Gut gesehen.

Zitat:

...Die ganze Schnittstelle ist meiner Meinung nach nicht optimal...

Gegenvorschlag?

Zitat:

Ich verwende grundsätzlich (fast) nie private sondern immer protected (bis auf sehr wenige Ausnahmen, bei denen es wirklich gute Gründe gibt, die dagegen sprechen).

Welche Gründe sprechen gegen private Felder? Properties sehe ich ein, aber Felder sollten so strict private sein, wie es nur geht.
Ich deklariere Felder immer privat (außer bei DTO) aber den Zugriff per Property als protected. Vermutlich ist das das Gleiche, aber mit mehr Tipparbeit verbunden.

Das 'MakePair' würde ich dem TKeyValuePair als Create spendieren. Gehört -finde ich- dorthin.

Delphi-Quellcode:

// function TCache.MakePair(Key: Integer; Value: TObject): TKeyValuePair;
// begin
//   Result.Key := Key;
//   Result.Value := Value;
// end;

constructor TKeyValuePair.Create(aKey: Integer; aValue: TObject)
begin
  Self.Key := aKey;
  Self.Value := aValue;
end;

Das Freigeben des Cache-Items gehört (finde ich) in den Destruktor des TCacheItem.... Aber wieso gibst Du dort auch das 'Value' frei? Das ist doch das zu cachende Objekt? Damit hat doch der Cache nix am Hut. Oder hab ich mich verlesen?

Delphi-Quellcode:

procedure TCache.Evict(Item: TCacheItem);
begin
  ...
  CacheItem.Value.Value.Free; // <<<--- Mööp, ich glaube, da ist ein Value. zu viel
  CacheItem.Free;
end;

Hier würde ich zwei zusätzliche kleine Methoden draus machen (Lesbarkeit, Clean Code).

Delphi-Quellcode:

// procedure TCache.Put(ID: Integer; Item: TObject);
// var
//   CacheItem: TCacheItem;
// begin
//   Assert(not Contains(ID));
// 
//   if GetCurrentNumberOfElements >= FMaxSize then
//     Evict(FMRU.Last);
// 
//   CacheItem := TCacheItem.Create(MakePair(ID, Item));
//   FMap[ID] := CacheItem;
//   Bump(CacheItem);
// end;

procedure TCache.RemoveLeastUsedElementFromCache;
begin
  if GetCurrentNumberOfElements >= FMaxSize then
    Evict(FMRU.Last);
end;

procedure TCache.AddItemToCache(ID: Integer; Item: TObject);
var
  CacheItem: TCacheItem;

begin
  CacheItem := TCacheItem.Create(MakePair(ID, Item));
  FMap[ID] := CacheItem;
  Bump(CacheItem);
end;

procedure TCache.Put(ID: Integer; Item: TObject);
begin
  Assert(not Contains(ID));

  RemoveLeastUsedElementFromCache;
  AddItemToCache(ID, Item); 
end;

'Bump' und 'Evict' sind für mich ungewöhnliche Begriffe. Ich würde zumindest 'Bump' anders benennen. Aber vermutlich ist das bei Dir/euch Standard. Dann müsste ich mich anpassen

Deckt sich ziemlich mit meiner Implementierung, nur das ich keine Generics habe und die MRU selbst implementieren muss.

PS: Es gibt keine Vorgabe, das eine MRU verwendet werden soll! ICH hätte das so gelöst. Wie ihr das macht, ist mir schnuppe (im Gegenteil: Sehr interessant)