Refactor `Component`s without attributes to `EmptyComponent`

jdrueckert commented 11 months ago

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> {
}

spookynutz commented 11 months ago

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

spookynutz commented 11 months ago

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 commented 11 months ago

Hello,

I have opened the following PRs for this issue :

jdrueckert commented 10 months ago

@spookynutz thank you for the contributions, highly appreciated! :green_heart: 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 commented 9 months ago

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

jdrueckert commented 9 months ago

@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.

MovingBlocks / Terasology