search

MaKiPL / OpenVIII-monogame

Open source Final Fantasy VIII engine implementation in C# working on Windows and Linux (Android and iOS planned too!) [Monogame]
MIT License
645 stars 59 forks source link

Unit tests and OpenVIII #160

Open FlameHorizon opened 4 years ago

FlameHorizon commented 4 years ago

Is there any reason why this project does not have unit tests?

I was looking at Core/Saves/ATBTimer.cs and thought - I might start my help here by adding unit tests to this class and continue refactoring.

Are there any red flags regarding unit tests in this project which I'm not aware of?

Ps. I'm aware about this Testing suite but I can't get it to work.

jonasrk commented 4 years ago

If I see some examples for unit tests, maybe I can also help with adding a few. :) Having some unexpected freetime. ;)

Sebanisu commented 4 years ago

Someone tried to add unit tests. Though I donno how to use them.

FlameHorizon commented 4 years ago

Today’s update (mostly to clear my head) - I started working on test cases for Core/Saves/Damageable.cs and Core/Battle/Enemies/Enemy.cs which inheritances from Damageable. And oh boy, the dependencies are really. Although I’ve managed to write a few test cases for public bool Damageable.ChangeHP(int dmg). I hope either tomorrow or on Saturday Sunday I will have some to show.

FlameHorizon commented 4 years ago

Today I’ve slowly started making some non-breaking changes in #163 . This will help me to understand project and people who are working on it better.

To the owners - can I help you in any ways implement a coding convention for this project?

Sebanisu commented 4 years ago

I had been using reshaper and codemaid to try to clean up some code the last few weeks. I haven't committed to master yet. I'm not sure if there is a way to define the conventions. Though reshaper has some default ones I was using. I like reshaper because I load a file and everything is highlighted like spelling errors and things that don't follow the naming convention. So then I just need to go through the file and make it happy. So then I get a green checkmark. Codemaid just sorts everything and groups the methods, fields, properties, etc. I'm kinda a noob. I know like c++ has a clang tidy file. I don't know if something like that exists for c#.

FlameHorizon commented 4 years ago

I've moved discussion about coding style to a separate issue #165

FlameHorizon commented 4 years ago

Great effort! I will start writing today.

FlameHorizon commented 4 years ago

Update. Now, with this recent PR, it is even harder...

Sebanisu commented 4 years ago

harder to test?

FlameHorizon commented 4 years ago

Is there a reason why do you have a static CreateInstance methods with private or protected constructors?

A lot of classes (like Saves.CharacterData) are contected with global static class Memory .

Class Saves.CharacterData contains information about character's data as well as operations on binary data. Probably there are just too many things which are happening in one place.

Sebanisu commented 4 years ago

I am told new keywords should be hidden behind a factory method. https://refactoring.guru/design-patterns/factory-method/csharp/example

Though maybe I just did a poor job at implementation.

Sebanisu commented 4 years ago

Only should be two ways to make a character data instance. Loading a save or reading init.out. Enemy data stored in enemy dat files so they're loaded at the start of battle. You could have the test run after loading init.out or just feed the code random binary data and it will break and throw errors. Maybe we could break the monolith class into smaller sub classes. I was wondering if we should get rid of the damageable abstract class. It's job could be handled by an interface. I wanted to have atb and hp functions universal between the entities.