bpi-dk/bpi-codereview — pull request #1 by kasperg (2013-11-27 12:11:46+00:00)
Title: Code review

I have written my comments and questions here inline with the code. I also added a few simple changes myself.

Some general comments (in Danish):

### Kommentarer
Mit overordnede indtryk af koden isoleret set:

* Selve kodestrukturen i modulet er generelt veldokumenteret. Alle funktioner, konstater etc. har tilknyttet en kodeblok, og der er skrevet løbende kommentarer igennem koden. Der er få steder, hvor koden bliver kompleks, der godt kunne trænge til en yderligere forklaring.
* Koden er inddelt i filer af passende og funktionelt afgrænset størrelse. Det gør kildekoden lettere at overskue.
* Det virker som en god idé brugervenlighedsmæssigt at implementere dele af funktionaliten vha. AJAX. Udviklerne bemærker dog flere steder at ikke-AJAX varianten ikke er implementeret endnu.
* Jeg har ikke fået adgang til selve BPI PHP klienten så denne kan jeg ikke reviewe.

### Konkrete anbefalinger
En række af de hyppigst forekommende problemer som jeg mener bør rettes:

* En række elementer i koden, konstanter, funktioner og permissions er ikke prefixet med modulets navn. Det bør ske for at undgå sammenstød med andre moduler.
* Håndtering af exceptions fra kald vha. BPI klienten bør ske vha. watchdog_exception. Husk også at give feedback til brugeren om at det er opstået en fejl.
* Refaktorer initialisering af et BPI klient objekt til én metode.
* Husk alle dependencies (ctools, ting_reference) eller opdater kode og make-fil så der ikke er en implicit afhængighed.
* Brug query parametre i url’en og form status til at viderebringe data mellem sideskift. Den nuværende brug af session-variable bliver hurtigt kompliceret og skrøbelig.

Se derudover de enkelte kommentarer.