mifriis / edge-of-kuiper

Textbased space rpg somewhat based on real science
Other
7 stars 3 forks source link

GameTime improvements. #33

Closed Nicoolai closed 2 years ago

Nicoolai commented 3 years ago

Currently, GameTime is kind of hard to test, as it uses DateTime.Now for ElapsedTime calculations. We can't really be sure the same amount of time has passed, from initialising the class, when testing. This means that anything that uses elapsed game time, can have varied results, during unit tests.

Also, should we change it into a service instead of a static? Register it in the IOC container?

mifriis commented 3 years ago

The issue with IOC is it makes it impossible to reach from our domain entities.

You can't call from a Domain entity to a Service, only the other way. So either the entity must be created with GameTime or access it via a Static.

For mocking Random i used a static factory: https://github.com/mifriis/edge-of-kuiper/blob/mifriis/moremining/kuiper-game/Services/DiceFactory.cs

Either it defaults to creating a new instance (or gets set/created on game load) or you set it to a Mock manually in your test. IGameTIme could be implemented in a similiar way. Here's some tests that show it in practice: https://github.com/mifriis/edge-of-kuiper/blob/mifriis/moremining/kuiper-tests/DiceRollerShould.cs

18 goes a bit deeper into if having Domain enties that have so much behaviour as they do now, is a problem or not.

larsbaunwall commented 3 years ago

Well well, guess I worked on something similar while you were typing 😉 PR #34

Nicoolai commented 3 years ago

The issue with IOC is it makes it impossible to reach from our domain entities.

You can't call from a Domain entity to a Service, only the other way. So either the entity must be created with GameTime or access it via a Static.

Shouldn't there just be a service that ties it together then? Maybe it's just a matter of designing a service-layer that makes sense and making sure you pass in the necessary dependencies?

I am a big fan of IOC and not much of a fan of static classes :)

mifriis commented 3 years ago

The issue with IOC is it makes it impossible to reach from our domain entities. You can't call from a Domain entity to a Service, only the other way. So either the entity must be created with GameTime or access it via a Static.

Shouldn't there just be a service that ties it together then? Maybe it's just a matter of designing a service-layer that makes sense and making sure you pass in the necessary dependencies?

I am a big fan of IOC and not much of a fan of static classes :)

There must be some examples of best practice of how to build behavior rich domain objects and still be able to read info from "parents" without going anemic and doing it all in one big service just to avoid statics.

GameTime, DiceRoll/Random, the captains name or a list of objects in the solar system are all things i have a hard time seeing us parse around when they could just be fetched from a Static. If testability is the problem we can use the locator pattern.

larsbaunwall commented 3 years ago

Maybe you are looking for a Domain Service (also this SO answer)?

The pure domain model should not be allowed to know anything about encapsulating objects, e.g. application services or other infrastructure, I agree.

Nicoolai commented 3 years ago

Domain Services : Encapsulates business logic that doesn't naturally fit within a domain object, and are NOT typical CRUD operations – those would belong to a Repository.

Sounds perfect, doesn't it? SolarSystemService as an example, it handles logic around planets, which the planets shouldn't know or be able to do. They just need to know what they are, who they orbit and what their own position is. Maybe they don't even need to know their own position and in fact, they don't really know it before SolarSystemService tells them how much time has passed, from their initial position.

To me, but I am also by no means an expert or even proficient in this, domain services sounds like a perfect fit. I think the challenge is just designing them in a good way, with the right splits.

Nicoolai commented 3 years ago

If the domain model shouldn't know about the encapsulating objects, and can't touch IOC container, is it allowed to call those statics?

mifriis commented 3 years ago

@Nicoolai @larsbaunwall It's not that I insist on using Statics. But can we get what we have working before we begin to refactor it? There needs to be a working state to start from and end from. If you don't have concepts like set course, start an event like mining and such you don't have all the puzzle pieces needed to know if the refactor is a success. We don't have the vertical working yet

larsbaunwall commented 3 years ago

If the domain model shouldn't know about the encapsulating objects, and can't touch IOC container, is it allowed to call those statics?

I would say no. Instead, an application service should execute a command on the domain model with all the information needed embedded in the command.

larsbaunwall commented 3 years ago

@Nicoolai @larsbaunwall It's not that I insist on using Statics. But can we get what we have working before we begin to refactor it? There needs to be a working state to start from and end from. If you don't have concepts like set course, start an event like mining and such you don't have all the puzzle pieces needed to know if the refactor is a success. We don't have the vertical working yet

I think #34 is the first iteration of that journey. I agree - we could just leave it at that.

I would consider separating the point-in-time data container (GameTime) from the Now() calculation sometime soon before it gets out of hands though?

mifriis commented 3 years ago

@Nicoolai @larsbaunwall It's not that I insist on using Statics. But can we get what we have working before we begin to refactor it? There needs to be a working state to start from and end from. If you don't have concepts like set course, start an event like mining and such you don't have all the puzzle pieces needed to know if the refactor is a success. We don't have the vertical working yet

I think #34 is the first iteration of that journey. I agree - we could just leave it at that.

I would consider separating the point-in-time data container (GameTime) from the Now() calculation sometime soon before it gets out of hands though?

Agree, with this vertical i think we have a lot of the corner cases in there.

Nicoolai commented 3 years ago

@Nicoolai @larsbaunwall It's not that I insist on using Statics. But can we get what we have working before we begin to refactor it? There needs to be a working state to start from and end from. If you don't have concepts like set course, start an event like mining and such you don't have all the puzzle pieces needed to know if the refactor is a success. We don't have the vertical working yet

Yep, good point. Fine by me.

mifriis commented 2 years ago

IGameTimeService is a thing in main now and GameTime the static is dead and gone. It's still a DateTime underneath, but you can set it's GameTimeService.Now() output to whatever. I used the solarsystem tests to verify the distance tests now being completely predictable on every run.