Posted by marvil07 on June 7, 2009 at 6:37pm
Coding:
- This week I started moving function to methods one by one(versioncontrol_get_accounts and versioncontrol_user_accounts_load)
- Then I decide to make more drastic changes, aborting the plan of keeping oop branch stable. So I made bulk updates of classes: repository, operation, item, label, branch, tag, account(defering the way proposed on the original dia diagram like jpetso suggest me).
- Now I'm working(oop branch) on fix each class methods to get all original functionallity working(I made a specific TODO).
Comments
Good progress, it looks
Good progress, it looks like. Some comments, keeping in mind that at least some of these things are probably just by virtue of the fact that we've got copy/pasted code and you just haven't had time to get to em yet...
Really, I don't think we're getting much with
VersioncontrolAccount::userAccountsLoad(). If it's a menu loader function, just leave it as a function; unless/until the menu system supports doing things like this, then all we're really doing is introducing an extra couple method calls. Given that, at least right now, we're not storing any of that data into static vars inVersioncontrolAccount::getAccounts(). (Unless I missed it).Things like the static $successor_action_priority array in
VersioncontrolItem::getItemHistory()should be declared as class constants.I don't think including the
@accesstag is necessary by drupal coding standards. If it helps your doxygen generation, tho, all good.There are prolly other ones, but one coding convention we have is to get rid of the prepended underscore on function names when they become methods, and just make em protected. f.e.
VersioncontrolItem::_badItemWarning.I would have figured, based on last week's conversation, that it's not just VersioncontrolBackend that's abstract, but also VersioncontrolOperation, VersioncontrolItem, VersioncontrolRepository, etc; that these, too, will be extended by the backends as needed. Although that doesn't require them to be abstract, of course - just that they hold all the shared code that's common to all the vcses.