Winery exercice - Partie 2

Dans la partie 1, on avait fait passer TestWineCreation et promis de « rendre justice à ce fameux COLOR_RED = "" ».
C’est le moment : place à ClassifyByColor, le deuxième test du Winery exercice. 🍷

💡 Tout le code est sur GitHub : pcavezzan/2026-july-technical-interview-sample, branche result/winery.

🎯 Ce que le test attend

ClassifyByColor doit ranger une Cellar (une slice de Wine) dans un map[string]Cellar, une entrée par couleur :

func TestClassifyByColor(t *testing.T) {
    cellar := openWineCatalog(t)
    assert.Equal(t, cellar.Length(), 6)

    byColor := cellar.ClassifyByColor()
    redWines := byColor[COLOR_RED]
    whiteWines := byColor[COLOR_WHITE]
    roseWines := byColor[COLOR_ROSE]

    assert.Equal(t, 4, redWines.Length())
    assert.Equal(t, 2, whiteWines.Length())
    assert.Nil(t, roseWines)

    cellar = append(cellar, Wine{
        Name:   "Château Puech Haut",
        Region: "Languedoc",
        Year:   2018,
        Price:  18.95,
        Color:  COLOR_ROSE,
    })

    byColor = cellar.ClassifyByColor()
    redWines = byColor[COLOR_RED]
    whiteWines = byColor[COLOR_WHITE]
    roseWines = byColor[COLOR_ROSE]

    assert.Equal(t, 4, redWines.Length())
    assert.Equal(t, 2, whiteWines.Length())
    assert.Equal(t, 1, roseWines.Length())

    assert.Equal(t, "0,2,3,4", redWines.dump())
    assert.Equal(t, "1,5", whiteWines.dump())
}

Deux choses à noter avant même d’écrire une ligne de code :

• Le catalogue de wines.json ne porte le champ color que sur les blancs. Les 4 rouges n’ont aucune clé color. Après passage par NewWine (partie 1), leur champ Color vaut donc la valeur zéro d’un string : "". C’est très exactement COLOR_RED. 🎯
• roseWines doit être nil tant qu’aucun rosé n’existe, puis devenir une vraie Cellar de longueur 1 une fois un rosé ajouté. Pas de case spéciale à écrire pour ça : on y revient plus bas.

🧩 Le point de départ (code fourni)

func (c Cellar) ClassifyByColor() map[string]Cellar {
    var res map[string]Cellar

    // TODO: Candidate Codes

    return res
}

var res map[string]Cellar déclare un map nil, pas une map vide. La nuance a l’air cosmétique. Elle ne l’est pas. 🪤

🛠️ Le réflexe naturel… et le panic qui va avec

L’implémentation la plus courte à laquelle on pense, c’est une seule boucle qui accumule chaque vin dans le bucket de sa couleur :

func (c Cellar) ClassifyByColor() map[string]Cellar {
    var res map[string]Cellar

    for _, wine := range c {
        res[wine.Color] = append(res[wine.Color], wine)
    }

    return res
}

Ça compile. go vet ne dit rien. Et go test explose :

panic: assignment to entry in nil map [recovered, repanicked]

En Go, lire une clé absente d’une map nil renvoie tranquillement la valeur zéro (res["rouge"] sur un map nil, c’est un Cellar nil, aucun souci). Mais écrire dedans (res["rouge"] = ...) panique, toujours. Le skeleton fourni construit délibérément une map nil et laisse le candidat remplir la boucle : c’est le même genre de piège que le shadowing de la partie 1, version « runtime » plutôt que « logique ». ⚠️

🛠️ Le fix : une ligne

func (c Cellar) ClassifyByColor() map[string]Cellar {
    res := make(map[string]Cellar)

    for _, wine := range c {
        res[wine.Color] = append(res[wine.Color], wine)
    }

    return res
}

make(map[string]Cellar) alloue une map vide et modifiable. Le reste de la fonction ne change pas d’une virgule. C’est tout ce que demande le test. 🧼

🥊 L’aparté du jour, le linter et la copie de variable de boucle

Le jour de l’entretien, j’ai réécrit cette boucle par réflexe. Sur les projets de mon client, mon linter me remonte en permanence l’erreur rangeValCopy, le réflexe est resté, même là où il n’avait pas lieu d’être :

for i, _ := range c {
    wine := c[i]
    res[wine.Color] = append(res[wine.Color], wine)
}

…plutôt que la forme idiomatique for _, wine := range c.

Mon interlocuteur ne croyait pas à l’intérêt de la manœuvre.
👉 Verdict, à froid et benchmark à l’appui : il avait raison de douter, et j’avais tort de m’accrocher à ce correctif précis. 😅

Ce que le linter reproche, en vrai

rangeValCopy de gocritic existe bel et bien (source), avec ce résumé : « Detects loops that copy big objects during each iteration », et un remède suggéré très précis :

// Before
for _, x := range xs { /* ... */ }
// After
for i := range xs { x := &xs[i]; /* ... */ }

Deux détails la rendent peu probable ici : son seuil par défaut est de 128 octets, alors que Wine (id, name, price, year, color, region) n’en pèse que 72 sur cette architecture : en dessous du seuil, elle ne se déclenche pas sans configuration explicite.
Et surtout, elle ne conseille jamais x := xs[i] (une nouvelle copie par index, ce que j’ai écrit) mais x := &xs[i], un pointeur. Cellar est un []Wine, pas un []*Wine : append(res[wine.Color], wine) a besoin d’une valeur, jamais d’un pointeur. Le remède que la règle propose ne s’applique même pas à cette fonction.

En clair, ClassifyByColor n’est pas une lecture seule qu’on pourrait alléger par un pointeur.

type Record struct {
    Data [1024]byte
    Seen bool
}

// Ne change rien : r est une copie, records reste intact
for _, r := range records {
    r.Seen = true
}

// Fonctionne : on mute l'élément réel via son adresse
for i := range records {
    records[i].Seen = true
}

Le correctif ne supprime aucune copie

Compare les deux formes :

// Idiomatique
for _, wine := range c {
    res[wine.Color] = append(res[wine.Color], wine)
}

// Écrit en entretien
for i, _ := range c {
    wine := c[i]
    res[wine.Color] = append(res[wine.Color], wine)
}

Dans les deux cas, c[i] est lu une fois et sa valeur atterrit dans une variable nommée wine, avant d’être copiée une seconde fois dans le slice de destination par append. Le second bloc ne fait que déplacer le point de copie de la clause range vers une ligne d’indexation explicite, sans le faire disparaître. Trois variantes, benchmarkées sur une cave de 1000 vins (go test -bench=. -benchmem -count=5, Apple M5 Max, Go 1.26) :

Les trois formes tournent au coude à coude : l’écart est dans le bruit de mesure (moins de 3 %), et allocations et octets par opération sont rigoureusement identiques. Le compilateur Go fait déjà ce travail d’élimination de copie mort-née pour un cas aussi simple qu’une boucle sans fermeture ni goroutine.

Petite ironie pour finir : for i, _ := range c introduit lui-même un nouveau smell : le _ est superflu, et un linter de simplification (type staticcheck, règle S1005) préférera for i := range c.
Le correctif né d’un linter en a discrètement fait naître un autre, sans gagner un seul cycle CPU. 🙃

💡 Le code de ce benchmark est visible ici.

🧪 Deux détails à corriger côté test, tant qu’on y est

En repassant sur TestClassifyByColor, deux choses détonnent avec ce qu’on avait mis en place dans la partie 1.

L’ordre des arguments de assert.Equal

assert.Equal(t, cellar.Length(), 6)

La signature de testify est assert.Equal(t, expected, actual, ...). Ici c’est inversé : cellar.Length() est passé comme expected et 6 comme actual. Le test passe quand même (l’égalité est symétrique), mais si jamais il casse, le message d’échec ment sur son propre sens :

expected: 5
actual  : 6

…alors que c’est bien la cave qui contient 5 vins, pas la valeur 6 attendue qui vaut 5. Un détail qui ne mord jamais tant que tout va bien, et qui complique le debug pile le jour où ça casse. Le reste du fichier (redWines.Length(), whiteWines.Length()) respecte le bon ordre : seule cette ligne d’ouverture a le sens inversé.

assert là où require s’impose

Comme dans la partie 1 : cellar.Length() est un prérequis. Si le catalogue ne contient pas 6 vins, tous les .dump() qui suivent ("0,2,3,4", "1,5") n’ont plus aucun sens : ils décrivent des indices qui ne correspondent plus à rien. Un require.Equal à cet endroit stoppe le test proprement au lieu de laisser dérouler des échecs en cascade illisibles.

func TestClassifyByColor(t *testing.T) {
    cellar := openWineCatalog(t)
    require.Equal(t, 6, cellar.Length())

    // ... le reste du test peut rester en assert
}

✅ Résultat

Une ligne de fix (var res → res := make(...)), zéro logique supplémentaire, et le deuxième test du Winery exercice passe. Ce qui rend le test costaud, ce n’est pas la fonction elle-même (elle tient en une boucle), c’est la manière dont le typage du domaine (Color string, valeur zéro "") et les nuances Go (nil map en écriture, nil slice vs slice vide) s’articulent pour que le comportement attendu tombe juste, sans code spécial pour aucun cas.

Dans la partie 3, direction SortByPrice : trier une Cellar sans jamais perturber l’originale. 🍇