hamid
160 lines
7.4 KiB
Markdown
160 lines
7.4 KiB
Markdown
# Code quality
|
|
|
|
The four rules that apply identically to both projects: no dead code, comment the *why*, no starter
|
|
scaffolding, and a mock is only a mock behind a seam.
|
|
|
|
> Last verified: 2026-07-30 against commit `d3ec723`.
|
|
|
|
---
|
|
|
|
## 1. No dead code
|
|
|
|
Unused variables, imports/usings, parameters, and private members are removed — not left behind, not
|
|
commented out, and **not suppressed**.
|
|
|
|
| Project | How it surfaces | Gate |
|
|
| --- | --- | --- |
|
|
| Client | `@typescript-eslint/no-unused-vars`, raised from eslint-config-next's default `warn` to **`error`** in `eslint.config.mjs` | Dead code **fails `npm run check`** |
|
|
| Server | `CS0168` (declared, never used), `CS0219` (assigned, never read), `CS0169` (private field never used), `IDE0005` (unnecessary `using`) | The gate is **zero new warnings**, so dead code is a gate failure |
|
|
|
|
**Delete it — don't silence it.** No `#pragma warning disable`, no throwaway discards, no `_ =`
|
|
assignments to quiet an analyzer, no file-wide ESLint disable.
|
|
|
|
Two sanctioned opt-outs, both narrow:
|
|
|
|
- **Client:** a deliberately-unused binding is prefixed with `_` — `_event`, `catch (_err)`.
|
|
- **Server:** a parameter that must exist to satisfy an interface or delegate signature but is genuinely
|
|
unused stays, named conventionally, with a one-line `// why` only if the reason isn't obvious.
|
|
|
|
When a lint disable is genuinely correct — a deliberate browser-only read after mount that trips
|
|
`react-hooks/set-state-in-effect` is the real example in this codebase — use a scoped
|
|
`// eslint-disable-next-line <rule>` with a one-line reason on the line above. Never a file-wide disable,
|
|
and never in preference to fixing the code.
|
|
|
|
---
|
|
|
|
## 2. Comment the *why*, never the *what*
|
|
|
|
Code that needs a comment to be understood usually needs a **better name** instead. Reach for the name
|
|
first, then a small helper, then a comment.
|
|
|
|
**Don't** write a comment that restates what the code already says:
|
|
|
|
```csharp
|
|
// ❌ restates the obvious
|
|
// increment the retry counter
|
|
retryCount++;
|
|
```
|
|
|
|
```tsx
|
|
// ❌ restates the obvious
|
|
// set the access token
|
|
setClientCookie(COOKIE_NAMES.ACCESS_TOKEN, token);
|
|
```
|
|
|
|
No XML-doc or JSDoc that merely echoes a function's name either.
|
|
|
|
**Do** add a tight comment where a non-obvious decision, constraint, business rule, workaround, ordering
|
|
or security requirement, or deliberate deviation is *not* evident from the code. Explain the reasoning,
|
|
not the mechanics:
|
|
|
|
```csharp
|
|
// ✅ captures a constraint the code can't express on its own
|
|
// Payment gateway rejects amounts above 50M IRR per call; split larger settlements upstream.
|
|
if (amount > MaxPerCallRial) …
|
|
```
|
|
|
|
The models to follow in this codebase:
|
|
|
|
| File | What its comment earns |
|
|
| --- | --- |
|
|
| `client/src/app/[locale]/layout.tsx` | Why `<html>` lives in the `[locale]` layout and not above it |
|
|
| `client/src/lib/auth/token.ts` | Why the JWT `exp` check is UX-only and never a security boundary |
|
|
| `client/src/layout/config.ts` | Why the two chrome-bar heights are measured rather than guessed, and must stay in sync with the bars |
|
|
| `client/middleware.ts` | Why the matcher lists bare `'/'` explicitly alongside the catch-all regex |
|
|
|
|
Delete comments that no longer match the code. A wrong comment costs more than no comment.
|
|
|
|
---
|
|
|
|
## 3. Don't reintroduce starter scaffolding
|
|
|
|
Both projects were derived from open-source starters. Their branding, demo/showcase pages, and
|
|
`_TITLE_`/`_DESCRIPTION_` placeholders were **intentionally removed**. Don't add them back — not as a
|
|
convenience, not while editing docs, not as an example.
|
|
|
|
Specifically:
|
|
|
|
- No placeholder page, showcase route, or "example component" gallery.
|
|
- No `_TITLE_` / `_DESCRIPTION_` / lorem-ipsum copy anywhere, including in message files.
|
|
- No starter README boilerplate reinstated into a project README.
|
|
- `PlaceholderScreen` exists for a genuinely not-yet-built screen and must not be reachable from a shell's
|
|
navigation. `/admin/notifications` is the current example: it is a placeholder, and it is deliberately
|
|
absent from `AdminLayout`'s nav for that reason.
|
|
|
|
---
|
|
|
|
## 4. A mock is only a mock behind a seam
|
|
|
|
Some integrations are intentionally out of scope and must be **mocked, not invented**: real PSP and BNPL
|
|
connections, the Shahkar / MoH / INO / criminal-record vendors, MinIO/S3 credentials, the سامانه مودیان
|
|
enrollment. Reaching one is not a blocker.
|
|
|
|
The only sanctioned form of "not real yet" is:
|
|
|
|
1. **An interface.** Server: an interface in `Application/Contracts/`, implemented twice, selected by
|
|
configuration in `AddCrossCuttingSeams` — and **the default is the mock, with a typo falling closed to
|
|
the mock**. Client: the domain's `Api` interface in `services/{domain}/types.ts`, implemented by
|
|
`clientApi.ts` and `mockApi.ts`, selected in `apis/index.ts` by `USE_{DOMAIN}_MOCK`.
|
|
2. **Selection by registration, never by branching.** No `if (mock)` inside a handler, hook, or component.
|
|
Swapping a mock for the real thing is a one-line registration change and touches no caller.
|
|
3. **A record**, in `docs/status/`: the seam (interface name + file), what is faked, why, the config keys
|
|
it reads, and **step-by-step how to make it real** — which provider, which settings, which methods,
|
|
what to test.
|
|
|
|
An unrecorded mock is a defect, because the next agent cannot tell a deliberate stand-in from a bug.
|
|
|
|
Two mocks in this repo are **deliberate MVP endpoints, not stand-ins waiting for a vendor**:
|
|
`ICredentialVerifier` / `ILicenseVerificationService` stay mock because manual MoH / INO / eNamad review
|
|
*is* the intended MVP — there is no public B2B API. Don't "finish" them.
|
|
|
|
---
|
|
|
|
## 5. Scale and cost are part of correctness
|
|
|
|
Every decision should consider what it costs at scale, not only whether it works once:
|
|
|
|
**Server** — indexing, pagination on every unbounded list, caching read-heavy and reference data behind
|
|
the cache seam, idempotency and locks on the money path, the DB constraint as the authoritative backstop
|
|
behind every friendly pre-check.
|
|
|
|
**Client** — query caching with a deliberate `staleTime` so you never refetch what you already hold,
|
|
invalidation on mutation, re-render cost (stable references, `select` to subscribe to a slice, state
|
|
colocated low), and bundle size.
|
|
|
|
And in both: the seam that lets a mock become real without touching a caller.
|
|
|
|
---
|
|
|
|
## 6. Configuration lives in files, not a secret store
|
|
|
|
`dotnet user-secrets` is **not used** in this repo, and the `<UserSecretsId>` was removed from
|
|
`Baya.Web.Api.csproj`, so that store **is not read at all**. Any instruction telling you to set a value
|
|
with `dotnet user-secrets` is stale.
|
|
|
|
| Where config lives | What |
|
|
| --- | --- |
|
|
| `server/src/API/Baya.Web.Api/appsettings.*.json` | Server config, including dev crypto keys |
|
|
| `client/.env.development` / `.env.production` | Client config |
|
|
| root `docker-compose.yml` | The deployment's container-specific overrides |
|
|
|
|
This is a deliberate pre-launch trade for a demo deployment, which means **the repo contains live
|
|
credentials**. Before onboarding real users they must be rotated and the secret half moved out of git —
|
|
see [`DEPLOY.md`](../../../DEPLOY.md) "Going to Production". Never hardcode a secret in code either way:
|
|
keys, connection strings, and tokens come from configuration bound to typed settings, never a literal in
|
|
a handler, service, or component.
|
|
|
|
One value is load-bearing and must never change: `Seams:FieldEncryption:Key` / `:HashKey` decrypt all
|
|
existing PII and derive the phone-lookup hash. Changing them makes every PII read throw and every phone
|
|
lookup miss.
|