Closed mifriis closed 2 years ago
To better understand events, i will link the review comments in "order" here:
@visualbean @larsbaunwall perhaps one of you have an idea on how i could rework events to gain access to the services from an events execute method.
A very simple local test approach could be:
ship --setcourse
captain --save
You can use the same savefile again and again now provided you dont change the Event class (or something like the Captain for example)
To better understand events, i will link the review comments in "order" here:
Event interface https://github.com/mifriis/edge-of-kuiper/pull/36#discussion_r750579414
The first event https://github.com/mifriis/edge-of-kuiper/pull/36#discussion_r750580039
Add to GameEvent list https://github.com/mifriis/edge-of-kuiper/pull/36#discussion_r750582145
After a reload: https://github.com/mifriis/edge-of-kuiper/pull/36#discussion_r750577992
Iterate through all events https://github.com/mifriis/edge-of-kuiper/pull/36#discussion_r750578479
Execute event https://github.com/mifriis/edge-of-kuiper/pull/36#discussion_r750581331
But it fails due to lack of dependency injection: https://github.com/mifriis/edge-of-kuiper/pull/36#discussion_r750580910
How i hoped i could set it up in the DI registry: https://github.com/mifriis/edge-of-kuiper/pull/36#discussion_r750582946
@visualbean @larsbaunwall perhaps one of you have an idea on how i could rework events to gain access to the services from an events execute method.
A very simple local test approach could be:
Run the ConsoleApp (F5)
Type in a captain name, for example LongLars or BrilliantAlex (remember what you choose though)
ship --setcourse
Choose 2 (or whatever...)
An event is now added in the background.
captain --save
Stop the console app and start it again
Type in the same captain name as before, you do remember it right?
As part of the load it will now start executing the events that happened while you were gone.
You can use the same savefile again and again now provided you dont change the Event class (or something like the Captain for example)
You COULD pass IServiceProvider into all of them, as a dependency of the runner. Gaining access through a servicelocator pattern instead of direct injection.
Otherwise you could create an "EventContext" type which would then be assembled through dependency injection and passed in through the runner (so basically take any type you need and add it to the ctor of the context and save it to a property) note that there might be issues with singelton vs transient services in this case.
You COULD pass IServiceProvider into all of them. Gaining access through a servicelocator pattern instead of direct injection.
Otherwise you could create an "EventContext" type which would then be assembled through dependency injection and passed in directly (so basically take any type you need and add it to the ctor of the context as save it to a property) note that there might be issues with singelton vs transient services in this case.
Won't ServiceLocator have the same problem though?
For example:
IShipService _shipService;
public SetCourseEvent(IContainer container)
{
_shipService = container.GetInstance<IShipService>();
}
As per https://jasperfx.github.io/lamar/documentation/ioc/concepts/
Here they dependency inject the Container and then use it to get the instance needed.
But i suppose where i need to include it from instead of the constructor, is the execute method:
public void ExecuteEvents(DateTime eventsBefore)
{
foreach(var gameEvent in GameEvents)
{
if(gameEvent.EventTime <= eventsBefore)
{
gameEvent.Execute(IContainer);
}
}
}
So EventService would dependency inject IContainer and pass it with Execute method into the Event who can then resolve whatever it needs to function. It's a bit dirty but i suppose it might work? I will give it a shot tomorrow. Appreciated!
You COULD pass IServiceProvider into all of them. Gaining access through a servicelocator pattern instead of direct injection.
Otherwise you could create an "EventContext" type which would then be assembled through dependency injection and passed in directly (so basically take any type you need and add it to the ctor of the context as save it to a property) note that there might be issues with singelton vs transient services in this case.
Won't ServiceLocator have the same problem though?
For example:
IShipService _shipService; public SetCourseEvent(IContainer container) { _shipService = container.GetInstance<IShipService>(); }
As per https://jasperfx.github.io/lamar/documentation/ioc/concepts/
Here they dependency inject the Container and then use it to get the instance needed.
But i suppose where i need to include it from instead of the constructor, is the execute method:
public void ExecuteEvents(DateTime eventsBefore) { foreach(var gameEvent in GameEvents) { if(gameEvent.EventTime <= eventsBefore) { gameEvent.Execute(IContainer); } } }
So EventService would dependency inject IContainer and pass it with Execute method into the Event who can then resolve whatever it needs to function. It's a bit dirty but i suppose it might work? I will give it a shot tomorrow. Appreciated!
Exactly. ServiceLocator is generally considered bad practice though. It makes it really hard to find usages of services, especially for unit testing. Where the context way i find to be VERY similar, but a bit less "open to interpretation" so to speak
@Nicoolai if you are up for it, i think this is a good place to get a review.
I would love to get your feedback! I think with these basics in place it will be possible to create other systems like commodities/trade, mining, ship upgrades, crew or even story/tutorial missions.
Overall Result: ✔️ Pass
Pass Rate: 100%
Run Duration: 1s 467ms
Date: 2021-11-19 14:24:50 - 2021-11-19 14:24:52
Framework: .NETCoreApp,Version=v6.0
Total Tests: 64
✔️ Passed | ❌ Failed | ⚠️ Skipped |
---|---|---|
64 | 0 | 0 |
100% | 0% | 0% |
Result | Test | Duration |
---|---|---|
✔️ Passed | Kuiper.Tests.Unit.Services.SolarSystemServiceShould.ReturnBodyBasedOnName | 105ms |
✔️ Passed | Kuiper.Tests.Unit.Services.SolarSystemServiceShould.ReturnCorrectKmDistanceBetweenBodies | 2ms |
✔️ Passed | Kuiper.Tests.Unit.Services.SolarSystemServiceShould.ReturnSattelitesAroundABody | 9ms |
✔️ Passed | Kuiper.Tests.Unit.Services.SolarSystemServiceShould.ReturnCorrectAuDistanceBetweenBodies | 2ms |
✔️ Passed | Kuiper.Tests.Unit.Services.SolarSystemServiceShould.ReturnAllBodiesUnderAStar | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.CaptainServiceShould.SaveSuccessfully | 136ms |
✔️ Passed | Kuiper.Tests.Unit.Repositories.JsonFileSolarSystemRepositoryShould.SetCorrectName | 7ms |
✔️ Passed | Kuiper.Tests.Unit.Repositories.JsonFileSolarSystemRepositoryShould.SetCorrectOrbitRadius | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.CaptainServiceShould.LoadCaptainWhenSavesFound | 15ms |
✔️ Passed | Kuiper.Tests.Unit.Services.CaptainServiceShould.SetupANewCaptainIfNoSavesFound | 2ms |
✔️ Passed | Kuiper.Tests.Unit.Services.CaptainServiceShould.ReturnCaptainIfAlreadySetup | 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.CaptainServiceShould.ThrowIfCaptainIsNotSet | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipServiceShould.DoNothingWhenTheDestinationIsFubar | 2ms |
✔️ Passed | Kuiper.Tests.Unit.Repositories.JsonFileSolarSystemRepositoryShould.CreateStarBodyTypeFromJson | 1ms |
✔️ Passed | Kuiper.Tests.Unit.Repositories.JsonFileSolarSystemRepositoryShould.SetCorrectOrbitVelocity | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Repositories.JsonFileSolarSystemRepositoryShould.CreatePlanetBodyTypeFromJson | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Repositories.JsonFileSolarSystemRepositoryShould.SetCorrectOriginDegrees | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Repositories.JsonFileSolarSystemRepositoryShould.CreateMoonBodyTypeFromJson | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Repositories.JsonFileSolarSystemRepositoryShould.NotFailIfJsonFileExist | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Repositories.JsonFileSolarSystemRepositoryShould.CreateDwarfPlanetBodyTypeFromJson | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Repositories.JsonFileSolarSystemRepositoryShould.CreateGasGiantBodyTypeFromJson | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Repositories.JsonFileSolarSystemRepositoryShould.FailIfJsonFileDoesNotExist | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Repositories.JsonFileSolarSystemRepositoryShould.CreateCorrectAmountOfBodiesFromJson | 3ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipServiceShould.UpdateShipStatusWhenDestinationValid | 14ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipServiceShould.UpdateShipStatusWhenDestinationReached | 1ms |
✔️ Passed | Kuiper.Tests.Unit.Systems.ConsoleCommandParseShould.ParseAndExecute_WithNullGroupedCommand_ParsesWithNameAsGroup | 15ms |
✔️ Passed | Kuiper.Tests.Unit.Systems.ConsoleCommandParseShould.ParseAndExecute_WithArgs_ExecutesWithArgs | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Systems.ConsoleCommandParseShould.ParseAndExecute_WithoutArgs_ExecutesWithNullArgs | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Systems.ConsoleCommandParseShould.Parse_WithoutArgs_ParsesWithNullArgs | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Systems.ConsoleCommandParseShould.Parse_WithNullGroupedCommand_ParsesWithNameAsGroup | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Systems.ConsoleCommandParseShould.Parse_WithArgs_ParsesWithArgs | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.SetCorrectOrbitRadius | 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipServiceShould.CalculatedVToTarget | 11ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipServiceShould.ReturnMergedListOfPlanetAndMoons | 1ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.ReturnItsPositionInSpaceAtAGivenTime | 2ms |
✔️ Passed | Kuiper.Tests.Unit.Systems.GameTimeServiceShould.ThrowExceptionIfRealStartTimeNeverSet | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Systems.GameTimeServiceShould.ReturnGamDateInNextWeekWhenStartingRealYesterday | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.AddBodyAsSatelliteIfItHasAParent | 16ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.CreateAMoon | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.CreateBodyWithOriginDegrees | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.ReturnItsInitialPositionInSpace | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.CreateBodyWithVelocity | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.CreateAGasGiant | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.SetCorrectOrbitVelocity | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.CreateAStar | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.CreateADwarfPlanet | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.SetCorrectName | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.CreateAPlanet | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Domain.CelestialBodyShould.SetCorrectOriginDegrees | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipShould.CalculateAccelerationGsCorrectly | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipShould.DeductFuelBasedOnSpentDVCorrectly | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipShould.CalculateDeltaVelocityCorrectly | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipShould.EmptyTheTankOnFullDeltaVSpent | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipShould.ReturnSpentFuel | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipShould.CalculateAccelerationCorrectly | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipShould.RefuelWithoutOverFilling | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.SetCourseCommandShould.SetCourseAndAddEventOnExecute | 19ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipEngineShould.CalculateThrustToWeightRationCorrectly | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipEngineShould.CalculateExhaustVelocityCorretly | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipShould.NotPossibleToForceSpendingMoreFuelThanAvailiable | 7ms |
✔️ Passed | Kuiper.Tests.Unit.Services.ShipShould.RefuelWithoutFillingTheTank | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.EventServiceShould.ExecuteAllEventsThatHappenedBeforeNow | 30ms |
✔️ Passed | Kuiper.Tests.Unit.Services.EventServiceShould.AddEventToEventList | < 1ms |
✔️ Passed | Kuiper.Tests.Unit.Services.EventServiceShould.RemoveEventFromEventList | < 1ms |
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.4.3+1b45f5407b (64-bit .NET 6.0.0-rtm.21522.10)
[xUnit.net 00:00:00.51] Discovering: kuiper-tests
[xUnit.net 00:00:00.55] Discovered: kuiper-tests
[xUnit.net 00:00:00.55] Starting: kuiper-tests
[xUnit.net 00:00:00.85] Finished: kuiper-tests
Created using Liquid Test Reports
# Summary | |
---|---|
Generated on: | 11/19/2021 - 14:24:52 |
Parser: | CoberturaParser |
Assemblies: | 1 |
Classes: | 31 |
Files: | 30 |
Covered lines: | 397 |
Uncovered lines: | 230 |
Coverable lines: | 627 |
Total lines: | 1238 |
Line coverage: | 63.3% (397 of 627) |
Covered branches: | 50 |
Total branches: | 72 |
Branch coverage: | 69.4% (50 of 72) |
Covered methods: | 86 |
Total methods: | 140 |
Method coverage: | 61.4% (86 of 140) |
Name | Covered | Uncovered | Coverable | Total | Line coverage | Covered | Total | Branch coverage | Covered | Total | Method coverage |
---|---|---|---|---|---|---|---|---|---|---|---|
kuiper-game | 397 | 230 | 627 | 1311 | 63.3% | 50 | 72 | 69.4% | 86 | 140 | 61.4% |
Kuiper.Domain.Account | 5 | 21 | 26 | 73 | 19.2% | 0 | 0 | 1 | 6 | 16.6% | |
Kuiper.Domain.Captain | 16 | 0 | 16 | 29 | 100% | 0 | 0 | 7 | 7 | 100% | |
Kuiper.Domain.CelestialBodies.CelestialBody | 46 | 0 | 46 | 82 | 100% | 4 | 4 | 100% | 12 | 12 | 100% |
Kuiper.Domain.SaveFile | 4 | 0 | 4 | 14 | 100% | 0 | 0 | 4 | 4 | 100% | |
Kuiper.Domain.Ship | 48 | 1 | 49 | 87 | 97.9% | 6 | 6 | 100% | 14 | 15 | 93.3% |
Kuiper.Domain.ShipEngine | 18 | 1 | 19 | 36 | 94.7% | 0 | 0 | 6 | 7 | 85.7% | |
Kuiper.Domain.ShipStatusExtensions | 0 | 6 | 6 | 24 | 0% | 0 | 4 | 0% | 0 | 1 | 0% |
Kuiper.Domain.Transaction | 0 | 12 | 12 | 73 | 0% | 0 | 0 | 0 | 5 | 0% | |
Kuiper.MainLoopWorker | 0 | 17 | 17 | 37 | 0% | 0 | 4 | 0% | 0 | 3 | 0% |
Kuiper.MainRegistry | 0 | 24 | 24 | 39 | 0% | 0 | 0 | 0 | 1 | 0% | |
Kuiper.Program | 0 | 6 | 6 | 20 | 0% | 0 | 0 | 0 | 2 | 0% | |
Kuiper.Repositories.JsonFileSolarSystemRepository | 25 | 14 | 39 | 72 | 64.1% | 6 | 8 | 75% | 3 | 6 | 50% |
Kuiper.Services.AccountService | 0 | 14 | 14 | 34 | 0% | 0 | 0 | 0 | 5 | 0% | |
Kuiper.Services.CaptainService | 58 | 0 | 58 | 93 | 100% | 6 | 6 | 100% | 5 | 5 | 100% |
Kuiper.Services.EventService | 33 | 0 | 33 | 55 | 100% | 10 | 10 | 100% | 5 | 5 | 100% |
Kuiper.Services.GameTimeService | 13 | 3 | 16 | 44 | 81.2% | 2 | 2 | 100% | 4 | 5 | 80% |
Kuiper.Services.ShipService | 36 | 5 | 41 | 68 | 87.8% | 4 | 4 | 100% | 6 | 7 | 85.7% |
Kuiper.Services.SolarSystemService | 27 | 0 | 27 | 61 | 100% | 0 | 0 | 8 | 8 | 100% | |
Kuiper.Systems.AccountCommand | 0 | 20 | 20 | 38 | 0% | 0 | 4 | 0% | 0 | 3 | 0% |
Kuiper.Systems.CaptainBaseCommand | 0 | 6 | 6 | 20 | 0% | 0 | 0 | 0 | 2 | 0% | |
Kuiper.Systems.CaptainsConsole | 0 | 11 | 11 | 29 | 0% | 0 | 0 | 0 | 2 | 0% | |
Kuiper.Systems.CommandInfrastructure.ConsoleCommandParser | 28 | 12 | 40 | 69 | 70% | 9 | 12 | 75% | 3 | 5 | 60% |
Kuiper.Systems.ConsoleCommandBase | 0 | 1 | 1 | 11 | 0% | 0 | 0 | 0 | 1 | 0% | |
Kuiper.Systems.ConsoleWriter | 8 | 0 | 8 | 19 | 100% | 0 | 0 | 2 | 2 | 100% | |
Kuiper.Systems.DestinationsCommand | 0 | 21 | 21 | 37 | 0% | 0 | 4 | 0% | 0 | 3 | 0% |
Kuiper.Systems.Events.SetCourseEvent | 3 | 7 | 10 | 24 | 30% | 0 | 0 | 3 | 4 | 75% | |
Kuiper.Systems.LocationCommand | 0 | 7 | 7 | 19 | 0% | 0 | 0 | 0 | 3 | 0% | |
Kuiper.Systems.SaveCommand | 0 | 9 | 9 | 22 | 0% | 0 | 0 | 0 | 3 | 0% | |
Kuiper.Systems.SetCourseCommand | 23 | 4 | 27 | 42 | 85.1% | 3 | 4 | 75% | 2 | 3 | 66.6% |
Kuiper.Systems.ShipBaseCommand | 6 | 1 | 7 | 20 | 85.7% | 0 | 0 | 1 | 2 | 50% | |
Kuiper.Systems.TimeCommand | 0 | 7 | 7 | 20 | 0% | 0 | 0 | 0 | 3 | 0% |
I fucked it all up trying to sign an old commit.
Failing forward to #39
DOTNET SIIIIX!
Based on VisualBeans command setup i have refactored much of the current code to completely use dependency injection (with only few exceptions)
SolarSystem has been rewritten a bit, it's now designed to work with JSON serialization now.
Events are introduced! When you set a course in your ship an event is added to the GameEvents list marking your date of arrival. On load all events are evaluated and executed if they "happened" while you were away.
SaveFile
introduced as a domain object. The noble goal of making a root aggregate hasn't worked. This way we can control exactly what is saved, how and where to put it back on load.