Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Components without attributes to EmptyComponent #5190

Closed
jdrueckert opened this issue Dec 5, 2023 · 6 comments
Closed

Refactor Components without attributes to EmptyComponent #5190

jdrueckert opened this issue Dec 5, 2023 · 6 comments
Labels
Category: Test/QA Requests, Issues and Changes targeting tests and quality assurance Good First Issue Good for learners that are new to Terasology Size: M Medium-sized effort likely requiring some research and work in multiple areas Topic: Stabilization Requests, Issues and Changes related to improving stablity and reducing flakyness Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity

Comments

@jdrueckert
Copy link
Member

jdrueckert commented Dec 5, 2023

Motivation

During a recent omega-wide refactoring, I noticed that we have a lot of components in engine and module land that implement gestalt's Component but do not involve any attributes (class fields).
These components should instead extend gestalt's EmptyComponent which avoids the need to implement copyFrom.

Furthermore, the refactoring will help to determine packages with many empty components which indicates further improvement potential as mentioned in the java doc of EmptyComponent:

if you are finding you have a lot of empty components you might consider whether they would be better handled by another component having boolean attributes.

Proposal

Search engine and modules (repos in https://github.com/Terasology) for classes that implement Component<T> but do not define any class fields. For instance, in Intellij, you can search for usages of gestalt's Component interface in extends/implements clauses.
Refactor the identified classes to extend EmptyComponent<T> instead and remove the implementation of copyFrom.

As a follow-up (not relevant for the completion of this issue), packages with a lot of empty components should be considered for refactoring into a single component with boolean attributes.

Helpful Examples

An example for a non-empty component that does not need refactoring is Health's DamageResistComponent.

It defines a component attribute (class field) damageTypes, so it needs to implement Component<T> which it correctly does and implement the override for copyFrom which it also correctly does.

An example for an empty component that does not need refactoring is Health's BlockDamagedComponent.

It does not define any component attributes (class fields), so it needs to extend EmptyComponent<T>which it correctly does and doesn't need to implement the override for copyFrom.

An example for an empty component that does need refactoring is WorkstationInGameHelp's ParticipateInItemCategoryInGameHelpComponent:

// Copyright 2021 The Terasology Foundation
// SPDX-License-Identifier: Apache-2.0
package org.terasology.workstationInGameHelp.components;

import org.terasology.gestalt.entitysystem.component.Component;

/**
 * Component for items that have help information for a workstation help system.
 * Entities that have this component are processed in {@link org.terasology.workstationInGameHelp.systems.WorkstationItemsInGameHelpCommonSystem}.
 */
public class ParticipateInItemCategoryInGameHelpComponent implements Component<ParticipateInItemCategoryInGameHelpComponent> {
    @Override
    public void copyFrom(ParticipateInItemCategoryInGameHelpComponent other) {

    }
}

It implements Component<T> and the override for copyFrom but does not define any attributes (class fields), so it would be a candidate for refactoring. A respective refactoring could look like the following:

// Copyright 2021 The Terasology Foundation
// SPDX-License-Identifier: Apache-2.0
package org.terasology.workstationInGameHelp.components;

import org.terasology.gestalt.entitysystem.component.EmptyComponent;

/**
 * Component for items that have help information for a workstation help system.
 * Entities that have this component are processed in {@link org.terasology.workstationInGameHelp.systems.WorkstationItemsInGameHelpCommonSystem}.
 */
public class ParticipateInItemCategoryInGameHelpComponent extends EmptyComponent<ParticipateInItemCategoryInGameHelpComponent> {
}
@jdrueckert jdrueckert added Good First Issue Good for learners that are new to Terasology Size: M Medium-sized effort likely requiring some research and work in multiple areas Topic: Stabilization Requests, Issues and Changes related to improving stablity and reducing flakyness Category: Test/QA Requests, Issues and Changes targeting tests and quality assurance Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity labels Dec 5, 2023
@spookynutz
Copy link

Hello, I'm new to contributing but I would be interested in trying to work on this issue. Would that be ok ?

@spookynutz
Copy link

spookynutz commented Dec 15, 2023

Using this search with regex on the repo, "org:Terasology language:Java /public void copyFrom(\w* \w*) {\s*}/" (I'm a bit newbie in regexes so it is not very optimised), there seems to be 95 candidates for the refactoring

@spookynutz
Copy link

Hello,

I have opened the following PRs for this issue :

DynamicCities : Terasology/DynamicCities#113
MasterOfOreon : Terasology/MasterOfOreon#105
MetalRenegades : Terasology/MetalRenegades#187
Sample : Terasology/Sample#130
GooeysQuests : Terasology/GooeysQuests#73
Equipment : Terasology/Equipment#140
Machines : Terasology/Machines#57
Inferno : Terasology/Inferno#30
Minesweeper : Terasology/Minesweeper#29
WeatherManager : Terasology/WeatherManager#31
Economy : Terasology/Economy#28
GooeyDefence : Terasology/GooeyDefence#76
ManualLabor : Terasology/ManualLabor#63
InGameHelp : Terasology/InGameHelp#15
FunnyBlocks : Terasology/FunnyBlocks#30
IRLCorp : Terasology/IRLCorp#44
WildAnimalsGenome : Terasology/WildAnimalsGenome#19
Exoplanet : Terasology/Exoplanet#26
EdibleSubstance : Terasology/EdibleSubstance#9
WorkstationInGameHelp : Terasology/WorkstationInGameHelp#13
Portals : Terasology/Portals#7
Scenario : Terasology/Scenario#64

@jdrueckert
Copy link
Member Author

@spookynutz thank you for the contributions, highly appreciated! 💚
I merged all linked module PRs. I'll check later, if there's any remaining occurrences. If there are not, we can close this issue.

@5archoufa
Copy link

Hi, I'm trying to help solve this issue, are there still any remaining occurrences? @jdrueckert

@jdrueckert
Copy link
Member Author

@5archoufa thanks for the reminder! had a quick check and didn't find any remaining occurrences, so I think we can close this issue. If we do find something after all, we can always reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Test/QA Requests, Issues and Changes targeting tests and quality assurance Good First Issue Good for learners that are new to Terasology Size: M Medium-sized effort likely requiring some research and work in multiple areas Topic: Stabilization Requests, Issues and Changes related to improving stablity and reducing flakyness Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity
Projects
Status: Backlog
Development

No branches or pull requests

3 participants