Skip to content

Conversation

@tfamula
Copy link

@tfamula tfamula commented Aug 8, 2019

This PR provides some refactoring for the Skill Management. It includes:

  • Move all persistence to new repository classes (ilBasicSkillLevelDBRepository.php, ilBasicSkillUserLevelDBRepository.php, ilBasicSkillTreeDBRepository.php)
  • Define interfaces to the repository classes
  • Instantiate the repositories in the contructor of ilBasicSkill
  • Use a new ilSkillObjectAdapter when calling static ilObject methods
  • Add type hints
  • Add return type declarations
  • Add or correct PHPDoc
  • Remove includes

To do:

  • Just pass ilDB as a parameter to the repositories, remove the other ones
  • PhpUnit tests

@tfamula tfamula requested review from alex40724 and xus August 8, 2019 14:42
@xus
Copy link

xus commented Aug 9, 2019

Hi @tfamula
After a quick read, this PR looks good to me. Imo, You moved properly the SQL stuff to the repositories with the minimum changes. But, for the next steps, I would like to suggest a few small and easy refactorings/changes or at least discuss them with @alex40724

  • Repository constructors:
    In terms of repositories I think it's better to send ids to methods instead of the constructors.
    Having an identifier in the constructor can force the consumer code to instantiate more than one repository in the logic business layout. Also, the developers will have to search and remember how the repository was instantiated.
    Some methods rely on the main configuration of the repository. For example, if ilBasicSkillLevelDBRepository is instantiated without skill id, "deleteLevelsOfSkill()" will delete all the levels in the database with skill_id null. Scary, in fact, the method does not do what it says it does because we don't have any skill.
    Another method called deleteLevelsWithoutAssignedSkill should be created if we need such action.
    Also, it will keep consistency over the methods.

  • Avoid innecessary method variables

$ilDB = $this->db;
$id = $this->id;
  • ilBasicSkillLevelDBRepository->addLevel :
    extract the nr+1 of the insert statement. Increment this value before and also can be nice to have a method for this increment "incrementMaxLevelNr" to avoid this +N in different places.

  • Too much responsability and conditionals. Concrete methods can help with the semantics and preventing bugs:
    Example to get rid of conditionals:
    Split getLevelData in two methods: getLevelData() and getLevelDataById(int $a_id). No more conditionals ;)
    Example of too much responsability:
    fixLevelNumbering -> can be nice to have a method for the query and another for the update.
    Most of these possible refactorings are in ilBasicSkillUserLevelDBRepository.php where we have large methods doing too much.

@xus
Copy link

xus commented Aug 9, 2019

db->quotes missing in ilBasicSkillLevelDBRepository->addLevel

@tfamula
Copy link
Author

tfamula commented Apr 15, 2020

Follow-up: #24

@tfamula tfamula closed this Apr 15, 2020
@tfamula tfamula deleted the 6_0_skill_repository_pattern branch February 23, 2023 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants