commit 08-2025
This commit is contained in:
Kevin Adametz
parent
9ae662f63e
commit
480fdc65ed
404 changed files with 65310 additions and 2600431 deletions
557
dev/code-review/TreeCalcBot_review.md
Normal file
557
dev/code-review/TreeCalcBot_review.md
Normal file
|
|
@ -0,0 +1,557 @@
|
|||
# Code Review: TreeCalcBot.php
|
||||
|
||||
## Zusammenfassung der Analyse
|
||||
|
||||
Die `TreeCalcBot` Klasse ist ein komplexes Service-Layer-Tool zur Berechnung und Darstellung von Multi-Level-Marketing (MLM) Geschäftsstrukturen. Die Klasse verarbeitet Benutzer-Hierarchien, berechnet Verkaufsvolumen und generiert HTML-Trees für die Frontend-Darstellung.
|
||||
|
||||
**Hauptprobleme:**
|
||||
- Massive Performance-Probleme durch N+1 Abfragen und fehlende Lazy Loading
|
||||
- Vermischung von Datenzugriff, Geschäftslogik und Präsentationsschicht
|
||||
- Über 390 Zeilen Code in einer einzigen Klasse (Single Responsibility Principle verletzt)
|
||||
- Keine Fehlerbehandlung bei kritischen Operationen
|
||||
- HTML-Generierung direkt in der Service-Klasse
|
||||
- Ineffiziente rekursive Strukturverarbeitung ohne Memory-Management
|
||||
|
||||
**Risiko-Level:** 🔴 HOCH - Potenzielle Systemausfälle bei großen Datenmengen
|
||||
|
||||
## Detaillierte Verbesserungsvorschläge
|
||||
|
||||
### 1. Performance-Optimierung: N+1 Problem beheben
|
||||
|
||||
**IST-Zustand (Zeile 151-159):**
|
||||
```php
|
||||
private function readRootUsers(){
|
||||
$users = User::with('account')->select('users.*')
|
||||
->where('users.deleted_at', '=', null)
|
||||
->where('users.id', '!=', 1)
|
||||
->where('users.admin', "<", 4)
|
||||
->where('users.m_level', "!=", null)
|
||||
->where('users.m_sponsor', "=", null)
|
||||
->where('users.payment_account', "!=", null)
|
||||
->where('users.active_date', "<=", $this->date->end_date)
|
||||
->get();
|
||||
if($users){
|
||||
foreach($users as $user){
|
||||
$BusinessUserItem = new BusinessUserItem($this->date);
|
||||
$BusinessUserItem->makeUser($user->id); // HIER: N+1 Problem!
|
||||
$BusinessUserItem->addUserID();
|
||||
$this->business_users[] = $BusinessUserItem;
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**SOLL-Vorschlag:**
|
||||
```php
|
||||
private function readRootUsers(){
|
||||
$users = User::with([
|
||||
'account',
|
||||
'userLevel',
|
||||
'userBusiness' => function($query) {
|
||||
$query->where('month', $this->date->month)
|
||||
->where('year', $this->date->year);
|
||||
}
|
||||
])->select('users.*')
|
||||
->where('users.deleted_at', '=', null)
|
||||
->where('users.id', '!=', 1)
|
||||
->where('users.admin', "<", 4)
|
||||
->where('users.m_level', "!=", null)
|
||||
->where('users.m_sponsor', "=", null)
|
||||
->where('users.payment_account', "!=", null)
|
||||
->where('users.active_date', "<=", $this->date->end_date)
|
||||
->chunk(100, function($users) {
|
||||
foreach($users as $user){
|
||||
$BusinessUserItem = new BusinessUserItem($this->date);
|
||||
$BusinessUserItem->makeUserFromModel($user); // Verwende bereits geladene Daten
|
||||
$BusinessUserItem->addUserID();
|
||||
$this->business_users[] = $BusinessUserItem;
|
||||
}
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
**Begründung:** Verhindert das N+1-Problem durch Eager Loading und reduziert Memory-Verbrauch durch Chunking. Anzahl der DB-Abfragen reduziert sich von N+1 auf 1 pro Chunk.
|
||||
|
||||
### 2. Single Responsibility Principle: HTML-Generierung auslagern
|
||||
|
||||
**IST-Zustand (Zeile 250-391):**
|
||||
```php
|
||||
public function makeHtmlTree(){
|
||||
$deep = 0;
|
||||
$ret = '<ol class="dd-list">';
|
||||
foreach($this->business_users as $business_user){
|
||||
$ret .= $this->addItem($business_user, $deep);
|
||||
}
|
||||
$ret .= '</ol>';
|
||||
return $ret;
|
||||
}
|
||||
|
||||
private function addItem($item, $deep){
|
||||
// 55 Zeilen HTML-String-Concatenation...
|
||||
}
|
||||
```
|
||||
|
||||
**SOLL-Vorschlag:**
|
||||
```php
|
||||
// Neue Klasse: App\Services\BusinessPlan\TreeHtmlRenderer.php
|
||||
class TreeHtmlRenderer
|
||||
{
|
||||
private $initFrom;
|
||||
|
||||
public function __construct($initFrom = 'member')
|
||||
{
|
||||
$this->initFrom = $initFrom;
|
||||
}
|
||||
|
||||
public function renderTree(array $businessUsers): string
|
||||
{
|
||||
return view('admin.business.components.tree', [
|
||||
'business_users' => $businessUsers,
|
||||
'init_from' => $this->initFrom
|
||||
])->render();
|
||||
}
|
||||
|
||||
public function renderParentless(array $parentless): string
|
||||
{
|
||||
return view('admin.business.components.parentless', [
|
||||
'parentless' => $parentless,
|
||||
'init_from' => $this->initFrom
|
||||
])->render();
|
||||
}
|
||||
}
|
||||
|
||||
// In TreeCalcBot.php:
|
||||
public function makeHtmlTree(): string
|
||||
{
|
||||
$renderer = new TreeHtmlRenderer($this->init_from);
|
||||
return $renderer->renderTree($this->business_users);
|
||||
}
|
||||
```
|
||||
|
||||
**Begründung:** Trennt Darstellungslogik von Geschäftslogik, verbessert Testbarkeit und Wartbarkeit. HTML-Templates sind wiederverwendbar und können einfacher angepasst werden.
|
||||
|
||||
### 3. Memory-Management: Große Datenmengen effizienter verarbeiten
|
||||
|
||||
**IST-Zustand (Zeile 176-192):**
|
||||
```php
|
||||
private function readParentlessUser(){
|
||||
$users = User::with('account')->select('users.*')
|
||||
->where('users.deleted_at', '=', null)
|
||||
->where('users.id', '!=', 1)
|
||||
->where('users.admin', "<", 4)
|
||||
->where('users.payment_account', "!=", null)
|
||||
->where('users.active_date', "<=", $this->date->end_date)
|
||||
->get(); // Lädt ALLE User in Memory!
|
||||
|
||||
foreach($users as $user){
|
||||
if(!isset(self::$userIDs[$user->id])){
|
||||
$BusinessUserItem = new BusinessUserItem($this->date);
|
||||
$BusinessUserItem->makeUser($user->id);
|
||||
$this->parentless[] = $BusinessUserItem;
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**SOLL-Vorschlag:**
|
||||
```php
|
||||
private function readParentlessUser(){
|
||||
User::with('account')->select('users.*')
|
||||
->where('users.deleted_at', '=', null)
|
||||
->where('users.id', '!=', 1)
|
||||
->where('users.admin', "<", 4)
|
||||
->where('users.payment_account', "!=", null)
|
||||
->where('users.active_date', "<=", $this->date->end_date)
|
||||
->whereNotIn('users.id', array_keys(self::$userIDs))
|
||||
->lazy(100) // Lazy Loading für Memory-Effizienz
|
||||
->each(function($user) {
|
||||
$BusinessUserItem = new BusinessUserItem($this->date);
|
||||
$BusinessUserItem->makeUserFromModel($user);
|
||||
$this->parentless[] = $BusinessUserItem;
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
**Begründung:** Lazy Loading verhindert Memory-Overflow bei großen Datenmengen. `lazy()` lädt nur jeweils 100 Datensätze in Memory, anstatt alle auf einmal.
|
||||
|
||||
### 4. Robuste Fehlerbehandlung implementieren
|
||||
|
||||
**IST-Zustand (Zeile 229-236):**
|
||||
```php
|
||||
public function readSponsorUser($user_id){
|
||||
$user = User::find($user_id);
|
||||
$userSponsor = User::find($user->m_sponsor); // Potenzielle null-pointer Exception!
|
||||
if($userSponsor){
|
||||
$this->sponsor = new BusinessUserItem($this->date);
|
||||
$this->sponsor->makeUser($userSponsor->id);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**SOLL-Vorschlag:**
|
||||
```php
|
||||
public function readSponsorUser($user_id){
|
||||
try {
|
||||
$user = User::find($user_id);
|
||||
if (!$user) {
|
||||
\Log::warning("TreeCalcBot: User not found: {$user_id}");
|
||||
return;
|
||||
}
|
||||
|
||||
if (!$user->m_sponsor) {
|
||||
return; // Kein Sponsor definiert
|
||||
}
|
||||
|
||||
$userSponsor = User::find($user->m_sponsor);
|
||||
if ($userSponsor) {
|
||||
$this->sponsor = new BusinessUserItem($this->date);
|
||||
$this->sponsor->makeUserFromModel($userSponsor);
|
||||
} else {
|
||||
\Log::warning("TreeCalcBot: Sponsor not found: {$user->m_sponsor} for user: {$user_id}");
|
||||
}
|
||||
} catch (\Exception $e) {
|
||||
\Log::error("TreeCalcBot: Error reading sponsor for user {$user_id}: " . $e->getMessage());
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Begründung:** Verhindert Systemabstürze durch null-pointer Exceptions und bietet aussagekräftige Logging-Informationen für Debugging.
|
||||
|
||||
### 5. Dependency Injection für bessere Testbarkeit
|
||||
|
||||
**IST-Zustand (Zeile 25-35):**
|
||||
```php
|
||||
public function __construct($month, $year, $init_from = 'member')
|
||||
{
|
||||
$this->date = new stdClass();
|
||||
$date = Carbon::parse($year.'-'.$month.'-1');
|
||||
$this->date->month = $month;
|
||||
$this->date->year = $year;
|
||||
$this->date->start_date = $date->format('Y-m-d H:i:s');
|
||||
$this->date->end_date = $date->endOfMonth()->format('Y-m-d H:i:s');
|
||||
$this->init_from = $init_from;
|
||||
}
|
||||
```
|
||||
|
||||
**SOLL-Vorschlag:**
|
||||
```php
|
||||
use Carbon\Carbon;
|
||||
use Illuminate\Contracts\Logging\Log;
|
||||
|
||||
public function __construct(
|
||||
int $month,
|
||||
int $year,
|
||||
string $init_from = 'member',
|
||||
Log $logger = null
|
||||
) {
|
||||
$this->validateInput($month, $year);
|
||||
|
||||
$this->date = new stdClass();
|
||||
$date = Carbon::parse($year.'-'.$month.'-1');
|
||||
$this->date->month = $month;
|
||||
$this->date->year = $year;
|
||||
$this->date->start_date = $date->format('Y-m-d H:i:s');
|
||||
$this->date->end_date = $date->endOfMonth()->format('Y-m-d H:i:s');
|
||||
$this->init_from = $init_from;
|
||||
$this->logger = $logger ?? app(Log::class);
|
||||
}
|
||||
|
||||
private function validateInput(int $month, int $year): void
|
||||
{
|
||||
if ($month < 1 || $month > 12) {
|
||||
throw new \InvalidArgumentException("Invalid month: {$month}");
|
||||
}
|
||||
|
||||
if ($year < 2020 || $year > date('Y') + 1) {
|
||||
throw new \InvalidArgumentException("Invalid year: {$year}");
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Begründung:** Ermöglicht Unit-Testing durch Dependency Injection und validiert Eingabeparameter zur Laufzeit.
|
||||
|
||||
### 6. Static Property für bessere Performance
|
||||
|
||||
**IST-Zustand (Zeile 19-23):**
|
||||
```php
|
||||
private static $userIDs = [];
|
||||
|
||||
public static function addUserID($id){
|
||||
self::$userIDs[$id] = $id;
|
||||
}
|
||||
```
|
||||
|
||||
**SOLL-Vorschlag:**
|
||||
```php
|
||||
private array $processedUserIDs = [];
|
||||
|
||||
public function addUserID(int $id): void
|
||||
{
|
||||
$this->processedUserIDs[$id] = true; // Boolean statt ID als Wert spart Memory
|
||||
}
|
||||
|
||||
public function isUserProcessed(int $id): bool
|
||||
{
|
||||
return isset($this->processedUserIDs[$id]);
|
||||
}
|
||||
|
||||
public function clearProcessedUsers(): void
|
||||
{
|
||||
$this->processedUserIDs = [];
|
||||
}
|
||||
```
|
||||
|
||||
**Begründung:** Nicht-statische Properties vermeiden Memory-Leaks zwischen verschiedenen Instanzen und Boolean-Werte sparen Speicher.
|
||||
|
||||
## Lösung für langlaufende Cron-Jobs (Asynchrone Verarbeitung)
|
||||
|
||||
### Konzept
|
||||
|
||||
Die aktuelle Implementierung läuft synchron und verarbeitet alle Business-User-Strukturen in einem einzigen Request, was bei großen Datenmengen zu Timeouts führt. Die beste Lösung ist die Auslagerung in Laravel Queues, die folgende Vorteile bietet:
|
||||
|
||||
- **Asynchrone Verarbeitung:** Keine Timeout-Probleme
|
||||
- **Skalierbarkeit:** Mehrere Worker können parallel arbeiten
|
||||
- **Fehlerbehandlung:** Automatische Retry-Mechanismen bei Fehlern
|
||||
- **Monitoring:** Queue-Status kann überwacht werden
|
||||
- **Memory-Management:** Jeder Job hat eigene Memory-Grenzen
|
||||
|
||||
### Schritt-für-Schritt-Anleitung
|
||||
|
||||
#### 1. Job-Klasse erstellen
|
||||
|
||||
```bash
|
||||
php artisan make:job ProcessBusinessStructure
|
||||
```
|
||||
|
||||
#### 2. Job-Implementierung
|
||||
|
||||
```php
|
||||
<?php
|
||||
// app/Jobs/ProcessBusinessStructure.php
|
||||
|
||||
namespace App\Jobs;
|
||||
|
||||
use App\Cron\BusinessUsersStore;
|
||||
use Illuminate\Bus\Queueable;
|
||||
use Illuminate\Contracts\Queue\ShouldQueue;
|
||||
use Illuminate\Foundation\Bus\Dispatchable;
|
||||
use Illuminate\Queue\InteractsWithQueue;
|
||||
use Illuminate\Queue\SerializesModels;
|
||||
use Illuminate\Support\Facades\Log;
|
||||
|
||||
class ProcessBusinessStructure implements ShouldQueue
|
||||
{
|
||||
use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;
|
||||
|
||||
public $timeout = 300; // 5 Minuten pro Job
|
||||
public $tries = 3; // 3 Versuche bei Fehlern
|
||||
|
||||
private $month;
|
||||
private $year;
|
||||
private $step;
|
||||
|
||||
public function __construct(int $month, int $year, string $step = 'structure')
|
||||
{
|
||||
$this->month = $month;
|
||||
$this->year = $year;
|
||||
$this->step = $step;
|
||||
}
|
||||
|
||||
public function handle()
|
||||
{
|
||||
Log::info("Processing Business Structure: Month {$this->month}, Year {$this->year}, Step: {$this->step}");
|
||||
|
||||
$businessStore = new BusinessUsersStore($this->month, $this->year);
|
||||
|
||||
try {
|
||||
switch ($this->step) {
|
||||
case 'structure':
|
||||
$businessStore->storeUserBusinessStructure();
|
||||
// Nächsten Schritt als neuen Job dispatchen
|
||||
ProcessBusinessStructure::dispatch($this->month, $this->year, 'details')
|
||||
->delay(now()->addSeconds(10));
|
||||
break;
|
||||
|
||||
case 'details':
|
||||
$businessStore->storeBusinessUsersDetail();
|
||||
// Finalisierung als neuen Job dispatchen
|
||||
ProcessBusinessStructure::dispatch($this->month, $this->year, 'complete')
|
||||
->delay(now()->addSeconds(10));
|
||||
break;
|
||||
|
||||
case 'complete':
|
||||
$businessStore->storeBusinessCompleted();
|
||||
Log::info("Business Structure processing completed for {$this->month}/{$this->year}");
|
||||
break;
|
||||
}
|
||||
} catch (\Exception $e) {
|
||||
Log::error("Business Structure processing failed: " . $e->getMessage());
|
||||
throw $e; // Re-throw für Queue Retry-Mechanismus
|
||||
}
|
||||
}
|
||||
|
||||
public function failed(\Throwable $exception)
|
||||
{
|
||||
Log::error("Business Structure Job failed permanently: " . $exception->getMessage());
|
||||
// Optional: E-Mail-Benachrichtigung an Admins
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
#### 3. User-spezifische Job-Klasse für Details
|
||||
|
||||
```bash
|
||||
php artisan make:job ProcessUserBusinessDetails
|
||||
```
|
||||
|
||||
```php
|
||||
<?php
|
||||
// app/Jobs/ProcessUserBusinessDetails.php
|
||||
|
||||
namespace App\Jobs;
|
||||
|
||||
use App\User;
|
||||
use App\Services\BusinessPlan\TreeCalcBot;
|
||||
use Illuminate\Bus\Queueable;
|
||||
use Illuminate\Contracts\Queue\ShouldQueue;
|
||||
use Illuminate\Foundation\Bus\Dispatchable;
|
||||
use Illuminate\Queue\InteractsWithQueue;
|
||||
use Illuminate\Queue\SerializesModels;
|
||||
|
||||
class ProcessUserBusinessDetails implements ShouldQueue
|
||||
{
|
||||
use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;
|
||||
|
||||
public $timeout = 120; // 2 Minuten pro User
|
||||
public $tries = 2;
|
||||
|
||||
private $userId;
|
||||
private $month;
|
||||
private $year;
|
||||
|
||||
public function __construct(int $userId, int $month, int $year)
|
||||
{
|
||||
$this->userId = $userId;
|
||||
$this->month = $month;
|
||||
$this->year = $year;
|
||||
}
|
||||
|
||||
public function handle()
|
||||
{
|
||||
$user = User::find($this->userId);
|
||||
|
||||
if (!$user) {
|
||||
\Log::warning("User {$this->userId} not found for business details processing");
|
||||
return;
|
||||
}
|
||||
|
||||
$treeCalcBot = new TreeCalcBot($this->month, $this->year, 'admin');
|
||||
$treeCalcBot->initBusinesslUserDetail($user);
|
||||
|
||||
if ($treeCalcBot->business_user) {
|
||||
// Store logic hier implementieren
|
||||
\Log::info("Processed business details for user {$this->userId}");
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
#### 4. Artisan Command anpassen
|
||||
|
||||
```php
|
||||
<?php
|
||||
// app/Console/Commands/BusinessStore.php
|
||||
|
||||
namespace App\Console\Commands;
|
||||
|
||||
use App\Jobs\ProcessBusinessStructure;
|
||||
use Illuminate\Console\Command;
|
||||
|
||||
class BusinessStore extends Command
|
||||
{
|
||||
protected $signature = 'business:store {month?} {year?}';
|
||||
protected $description = 'Process business structure asynchronously via queues';
|
||||
|
||||
public function handle()
|
||||
{
|
||||
$month = $this->argument('month') ?? date('n');
|
||||
$year = $this->argument('year') ?? date('Y');
|
||||
|
||||
$this->info("Dispatching business structure processing for {$month}/{$year}");
|
||||
|
||||
// Job in Queue einreihen
|
||||
ProcessBusinessStructure::dispatch($month, $year);
|
||||
|
||||
$this->info("Job dispatched successfully. Check queue:work for progress.");
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
#### 5. Queue-Konfiguration
|
||||
|
||||
**config/queue.php anpassen:**
|
||||
```php
|
||||
'connections' => [
|
||||
'database' => [
|
||||
'driver' => 'database',
|
||||
'table' => 'jobs',
|
||||
'queue' => 'default',
|
||||
'retry_after' => 600,
|
||||
'after_commit' => false,
|
||||
],
|
||||
],
|
||||
```
|
||||
|
||||
**Migration für Jobs-Tabelle:**
|
||||
```bash
|
||||
php artisan queue:table
|
||||
php artisan migrate
|
||||
```
|
||||
|
||||
#### 6. Queue Worker starten
|
||||
|
||||
```bash
|
||||
# Produktiv:
|
||||
php artisan queue:work --queue=default --tries=3 --timeout=300
|
||||
|
||||
# Mit Supervisor für automatischen Neustart:
|
||||
# /etc/supervisor/conf.d/laravel-worker.conf
|
||||
[program:laravel-worker]
|
||||
process_name=%(program_name)s_%(process_num)02d
|
||||
command=php /path/to/project/artisan queue:work --sleep=3 --tries=3 --timeout=300
|
||||
autostart=true
|
||||
autorestart=true
|
||||
user=www-data
|
||||
numprocs=2
|
||||
redirect_stderr=true
|
||||
stdout_logfile=/path/to/project/storage/logs/worker.log
|
||||
```
|
||||
|
||||
#### 7. Cron-Integration
|
||||
|
||||
```php
|
||||
// app/Console/Kernel.php
|
||||
protected function schedule(Schedule $schedule)
|
||||
{
|
||||
// Einmal monatlich am 1. um 02:00 Uhr
|
||||
$schedule->command('business:store')
|
||||
->monthlyOn(1, '02:00')
|
||||
->withoutOverlapping()
|
||||
->onOneServer();
|
||||
}
|
||||
```
|
||||
|
||||
### Monitoring und Fehlerbehandlung
|
||||
|
||||
```bash
|
||||
# Queue Status überwachen
|
||||
php artisan queue:monitor database --max=100
|
||||
|
||||
# Failed Jobs anzeigen
|
||||
php artisan queue:failed
|
||||
|
||||
# Failed Job erneut versuchen
|
||||
php artisan queue:retry all
|
||||
```
|
||||
|
||||
Diese Lösung stellt sicher, dass auch bei großen Datenmengen die Verarbeitung erfolgreich abgeschlossen wird, ohne dass Timeout-Probleme auftreten.
|
||||
Loading…
Add table
Add a link
Reference in a new issue