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

Discuss Term and Fingerprint class structure #73

Closed
thiemowmde opened this issue Apr 24, 2014 · 2 comments
Closed

Discuss Term and Fingerprint class structure #73

thiemowmde opened this issue Apr 24, 2014 · 2 comments

Comments

@thiemowmde
Copy link
Contributor

Personally I'm not absolutely happy with how the Term and Fingerprint class structure is build currently. Examples:

$entity->getFingerprint()->setAliasGroup(
    new AliasGroup( 'en', array( 'ohi' ) )
);
// vs.
$entity->getFingerprint()->setAliasGroup(
    'en', array( 'ohi' )
);
$entity->getFingerprint()->setAliasGroups(
    new AliasGroupList( array( /* ... */ ) )
);
// vs.
$entity->getFingerprint()->setAliasGroups(
    array( /* ... */ )
);

Similar for most of the new ...List classes (I hope you don't misunderstood me when I said that these classes feel to me like "reimplementing PHP arrays"). I'm aware this is a very vague discussion at this point. I think we all need to actually work with this code to form personal opinions if it's good as it is or should be simpler.

This ticket is a reminder for that later discussion.

@JeroenDeDauw
Copy link
Contributor

I've been thinking that adding shortcut methods as follows might be nice:

$fingerprint->setDescriptionText( 'en', 'some english stuff' );
$fingerprint->setAliasGroupTexts( 'en', array( ... ) );

Do keep in mind that patterns such as this should happen rarely, since they "reach through objects":

$entity->getFingerprint()->stuff()

In almost all cases, code doing things with labels and such should just get a Fingerprint rather than a whole Entity.

@thiemowmde
Copy link
Contributor Author

Here is an other example I found in #62:

$this->fingerprint->setDescriptions( new TermList( array() ) );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants