Feedback

[nielsdelestinne]

API

1. Goed rekening gehouden met exceptions die vanuit lagerliggende lagen gegooid kunnen worden. Deze wil je altijd 'verpakken' naar iets dat je kan teruggeven aan de consumers/clients van jouw API. Dit doe je goed (e.g. BadRequest). Je kan wel eventueel kritisch kijken naar wanneer je een 4xx statuscode teruggeeft (e.g. een BadRequest) versus een 5xx internal server error. Een statuscode startend met een 4 wilt zeggen dat de 'schuld' bij de client ligt en hij in principe zijn call moet corrigeren om dan opnieuw te proberen. Een statuscode startend met een 5 wijst op een internal server error. De schuld ligt bij de server. Er is op zich weinig wat de client hier aan kan doen (hij weet dus dat hij niet opnieuw moet proberen).
2. I.v.m. Exception handling. Kijk eens aan de 'Middelware' oplossing die Kenny had voorgesteld: https://docs.microsoft.com/en-us/aspnet/core/fundamentals/middleware/?tabs=aspnetcore2x&view=aspnetcore-2.1#ordering & http://netitude.bc3tech.net/2017/07/31/using-middleware-to-trap-exceptions-in-asp-net-core/
3. Wat logging betreft: zorg dat je zeker in al je catch blokken van jouw try-catch statements aan logging doet. (in plaats van bijvoorbeeld: _customerLogger.LogInformation("hoi");. Gebruik in dat geval (voor het loggen van exceptions) niet LogInformation maar LogError.
4. Goeie mapping, dto's.

Services

1. In bijvoorbeeld de ItemService, methode CreateNewItem, kan je de grote if/else check extracten naar een methode (bijvoorbeeld: AssertValidValuesForProperties). Op die manier wordt de flow van methode CreateNewItem duidelijker. (validate --> map To Item --> Save Item --> return Dto)

Domain

1. Nog teveel public setters. Ik kan bijvoorbeeld op een Item het ID nadien setten op een nieuwe waarde. Zeer gevaarlijk.
2. Goed dat je van Address een apart object hebt gemaakt. Ik zou hier zelf wel geen aparte 'DB' hebben voorzien. Een customer heeft simpelweg een address object. Wil ik het address, dan moet ik via de customer (id) gaan.
3. OrderReport hoeft niet te bestaan in je domain. Uiteindelijk is het report gewoon een verzameling van data die in je ander domeinobjecten leeft (in Order, ItemGroup,...). Ik zie OrderReport dus meer als een DTO, een resultaat van een aggregatie van informatie. Een OrderReport zou je in principe ook nooit as-such gaan opslaan / persisteren. Je zou het altijd herbereken.

Integration / Unit test

1. Mooie suite van tests geschreven!
2. Unit tests: dit zit op zich best goed. Je mapper test is bijvoorbeeld een zeer nuttige en niet zo moeilijk te schrijven.
3. Bij de integratie tests (eigenlijk echt de controller end-to-end tests), assert je nu puur de statusCode. Dit is op zich niet slecht. In de plaats zou je ook, in de situaties waarin je een object terugkrijgt (een Dto), kunnen asserten dat deze DTO dan de waardes bevat die je verwacht.

Kleine opmerkingen

1. Bij het rename van een project (bv. naar Oder.Api moet je ook de achterliggende foldernaam aanpassen. Die is bijvoorbeeld nu nog gewoon Oder. Projectname != foldername)
2. Commits zonder een zinnige message (e.g. je hebt een commit met message t, zijn bad practice.. :) )