refactor: optimize AcademicTermService methods and improve query handling
This commit is contained in:
parent
f69ed0bc15
commit
8055d4ec5d
@ -241,18 +241,26 @@ ## 5. Kode Ringkas & Idiomatis Laravel
|
|||||||
| `App::make('Class')` | `app('Class')` |
|
| `App::make('Class')` | `app('Class')` |
|
||||||
|
|
||||||
- **Jangan panggil `env()` di luar file config.** Simpan dulu ke `config/*.php`, lalu akses lewat `config('nama.key')`. Ini berlaku juga untuk kredensial/flag baru yang ditambahkan.
|
- **Jangan panggil `env()` di luar file config.** Simpan dulu ke `config/*.php`, lalu akses lewat `config('nama.key')`. Ini berlaku juga untuk kredensial/flag baru yang ditambahkan.
|
||||||
- **Tidak pakai DocBlock** untuk mendeskripsikan fungsi. Gunakan nama method yang deskriptif + return type/param type hint dari PHP. DocBlock hanya boleh dipakai kalau memang dibutuhkan untuk generic type (`@return Collection<int, Student>`) yang tidak bisa diekspresikan native PHP — bukan untuk narasi.
|
- **DocBlock boleh dipakai kalau memang dibutuhkan dan penting** — bukan default yang dipasang di semua method. Untuk method standar (CRUD biasa, atau apa pun yang sudah jelas maksudnya dari nama method + return/param type PHP), **jangan** tambah DocBlock — itu pemborosan. DocBlock baru layak ditulis untuk kasus yang memang penting, misalnya:
|
||||||
```php
|
1. **Generic type** yang tidak bisa diekspresikan native PHP, mis. `@return Collection<int, Student>`, `@param array<int, string>`.
|
||||||
// Bad
|
2. **Perilaku non-obvious** yang bisa bikin pembaca salah paham kalau tidak dijelaskan: aturan bisnis tersembunyi, constraint yang tidak kelihatan dari nama method, workaround keterbatasan interface/library.
|
||||||
/**
|
|
||||||
* The function checks if given string is a valid ASCII string
|
|
||||||
* @param string $string
|
|
||||||
* @return bool
|
|
||||||
*/
|
|
||||||
public function checkString($string) { }
|
|
||||||
|
|
||||||
// Good
|
```php
|
||||||
public function isValidAsciiString(string $string): bool { }
|
// Bad — DocBlock generik yang cuma mengulang nama method, tidak nambah informasi
|
||||||
|
/**
|
||||||
|
* Get the validation rules that apply to the request.
|
||||||
|
*/
|
||||||
|
public function rules(): array { ... }
|
||||||
|
|
||||||
|
// Good — tanpa DocBlock sama sekali, karena nama + return type sudah cukup jelas
|
||||||
|
public function rules(): array { ... }
|
||||||
|
|
||||||
|
// Good — DocBlock dipertahankan karena memang menjelaskan hal yang tidak
|
||||||
|
// kelihatan dari signature method (fallback logic yang bisa mengejutkan)
|
||||||
|
/**
|
||||||
|
* Falls back to the one with the latest start date if none is explicitly active.
|
||||||
|
*/
|
||||||
|
public function getActive(): ?AcademicTerm { ... }
|
||||||
```
|
```
|
||||||
- Komentar kode hanya untuk menjelaskan **kenapa**, bukan **apa** — kalau nama variabel/method sudah jelas, jangan tambah komentar.
|
- Komentar kode hanya untuk menjelaskan **kenapa**, bukan **apa** — kalau nama variabel/method sudah jelas, jangan tambah komentar.
|
||||||
|
|
||||||
|
|||||||
@ -24,7 +24,7 @@ public function index(PaginatedRequest $request): Response
|
|||||||
'academicTerms' => $this->service->paginated(
|
'academicTerms' => $this->service->paginated(
|
||||||
...$request->validatedWithDefaults(),
|
...$request->validatedWithDefaults(),
|
||||||
semester: $request->validated('semester'),
|
semester: $request->validated('semester'),
|
||||||
isActive: $request->filled('is_active') ? filter_var($request->validated('is_active'), FILTER_VALIDATE_BOOLEAN) : null,
|
isActive: $request->filled('is_active') ? $request->boolean('is_active') : null,
|
||||||
),
|
),
|
||||||
'filters' => $request->only(['semester', 'is_active']),
|
'filters' => $request->only(['semester', 'is_active']),
|
||||||
]);
|
]);
|
||||||
|
|||||||
@ -9,30 +9,14 @@
|
|||||||
|
|
||||||
class AcademicTermService
|
class AcademicTermService
|
||||||
{
|
{
|
||||||
public function getAllForSelect(): Collection
|
|
||||||
{
|
|
||||||
return AcademicTerm::select(['id', 'academic_year', 'semester', 'start_date', 'end_date', 'is_active', 'open_semesters'])->latest()->get();
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
|
||||||
* The academic term currently marked active, falling back to the one with
|
|
||||||
* the latest start date if none is explicitly active.
|
|
||||||
*/
|
|
||||||
public function getActive(): ?AcademicTerm
|
|
||||||
{
|
|
||||||
return AcademicTerm::where('is_active', true)->first()
|
|
||||||
?? AcademicTerm::orderByDesc('start_date')->first();
|
|
||||||
}
|
|
||||||
|
|
||||||
public function paginated(int $perPage = 25, string $search = '', ?string $semester = null, ?bool $isActive = null): LengthAwarePaginator
|
public function paginated(int $perPage = 25, string $search = '', ?string $semester = null, ?bool $isActive = null): LengthAwarePaginator
|
||||||
{
|
{
|
||||||
return AcademicTerm::query()
|
return AcademicTerm::query()
|
||||||
->select(['id', 'academic_year', 'semester', 'start_date', 'end_date', 'is_active', 'open_semesters'])
|
->when($search, fn ($q) => $q->where('academic_year', 'like', "%{$search}%"))
|
||||||
->when($search, fn($q) => $q->where('academic_year', 'like', "%{$search}%"))
|
->when($semester, fn ($q) => $q->where('semester', $semester))
|
||||||
->when($semester, fn($q) => $q->where('semester', $semester))
|
->when($isActive !== null, fn ($q) => $q->where('is_active', $isActive))
|
||||||
->when($isActive !== null, fn($q) => $q->where('is_active', $isActive))
|
|
||||||
->latest()
|
->latest()
|
||||||
->paginate($perPage);
|
->paginate($perPage, ['id', 'academic_year', 'semester', 'start_date', 'end_date', 'is_active', 'open_semesters']);
|
||||||
}
|
}
|
||||||
|
|
||||||
public function create(array $data): AcademicTerm
|
public function create(array $data): AcademicTerm
|
||||||
@ -45,13 +29,14 @@ public function create(array $data): AcademicTerm
|
|||||||
|
|
||||||
public function update(AcademicTerm $academicTerm, array $data): AcademicTerm
|
public function update(AcademicTerm $academicTerm, array $data): AcademicTerm
|
||||||
{
|
{
|
||||||
$academicTerm->academic_year = $data['academic_year'];
|
$academicTerm->update([
|
||||||
$academicTerm->semester = $data['semester'];
|
'academic_year' => $data['academic_year'],
|
||||||
$academicTerm->start_date = $data['start_date'];
|
'semester' => $data['semester'],
|
||||||
$academicTerm->end_date = $data['end_date'];
|
'start_date' => $data['start_date'],
|
||||||
$academicTerm->is_active = $data['is_active'];
|
'end_date' => $data['end_date'],
|
||||||
$academicTerm->open_semesters = $this->normalizeOpenSemesters($data['open_semesters'] ?? []);
|
'is_active' => $data['is_active'],
|
||||||
$academicTerm->update();
|
'open_semesters' => $this->normalizeOpenSemesters($data['open_semesters'] ?? []),
|
||||||
|
]);
|
||||||
|
|
||||||
return $academicTerm;
|
return $academicTerm;
|
||||||
}
|
}
|
||||||
@ -72,6 +57,21 @@ public function updateActiveStatus(AcademicTerm $academicTerm, bool $isActive):
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
public function getAllForSelect(): Collection
|
||||||
|
{
|
||||||
|
return AcademicTerm::latest()->get(['id', 'academic_year', 'semester', 'start_date', 'end_date', 'is_active', 'open_semesters']);
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The academic term currently marked active, falling back to the one with
|
||||||
|
* the latest start date if none is explicitly active.
|
||||||
|
*/
|
||||||
|
public function getActive(): ?AcademicTerm
|
||||||
|
{
|
||||||
|
return AcademicTerm::where('is_active', true)->first()
|
||||||
|
?? AcademicTerm::orderByDesc('start_date')->first();
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @param array<int, mixed> $openSemesters
|
* @param array<int, mixed> $openSemesters
|
||||||
* @return array<int, int>
|
* @return array<int, int>
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user