Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I know that this will be a difficult question to answer without context, but hopefully there are at least some good guidelines to share on this. The questions are at the bottom if you want to skip the details. Most are about OOP in general.

Begin context. I am a jr dev on a PHP application, and in general the devs I work with consider themselves to use many more OO concepts than most PHP devs. Still, in my research on clean code I have read about so many ways of using OO features to make code flexible, powerful, expressive, testable, etc. that is just plain not in use here.

The current strongly OO API that I've proposed is being called too complex, even though it is trivial to implement. The problem I'm solving is that our permission checks are done via a message object (my API, they wanted to use arrays of constants) and the message object does not hold the validation object accountable for checking all provided data. Metaphorically, if your perm containing 'allowable' and 'rare but disallowed' is sent into a validator, the validator may not know to look for 'rare but disallowed', but approve 'allowable', which will actually approve the whole perm check. We have like 11 validators, too many to easily track at such minute detail.

So I proposed an AtomicPermission class. To fix the previous example, the perm would instead contain two atomic permissions, one wrapping 'allowable' and the other wrapping 'rare but disallowed'. Where previously the validator would say 'the check is OK because it contains allowable,' now it would instead say '"allowable" is ok', at which point the check ends...and the check fails, because 'rare but disallowed' was not specifically okay-ed.

The implementation is just 4 trivial objects, and rewriting a 10 line function into a 15 line function.

abstract class PermissionAtom {
public function allow(); // maybe deny() as well
public function wasAllowed();
}

class PermissionField extends PermissionAtom {
public function getName();
public function getValue();
}

class PermissionIdentifier extends PermissionAtom {
public function getIdentifier();
}

class PermissionAction extends PermissionAtom {
public function getType();
}

They say that this is 'not going to get us anything important' and it is 'too complex' and 'will be difficult for new developers to pick up.' I respectfully disagree, and there I end my context to begin the broader questions.

So the question is about my OOP, are there any guidelines I should know:

  1. is this too complicated/too much OOP? Not that I expect to get more than 'it depends, I'd have to see if...'
  2. when is OO abstraction too much?
  3. when is OO abstraction too little?
  4. how can I determine when I am overthinking a problem vs fixing one?
  5. how can I determine when I am adding bad code to a bad project?
  6. how can I pitch these APIs? I feel the other devs would just rather say 'its too complicated' than ask 'can you explain it?' whenever I suggest a new class.
share|improve this question
    
It seems to me that you are using inheritance where you should be using composition. But I don't seem to understand the question. Where is the AtomicPermission class ? What does it mean that something "wraps 'rare but disallowed'" ? You also don't explain why you use four classes. –  user61852 Nov 22 '12 at 16:02
    
Sure, I can clarify. AtomicPermission is a miswording. The class is PermissionAtom, therefore you can consider any children to be an atomic permission. We're not using 5.4 so its inheritance or interface, I chose inheritance. I didn't go into detail on what information is passed in the perm message object, suffice it to say that we use fields, actions, identifiers, which are all very different things. By wrapping 'rare but disallowed', I mean to say the validators will use an object with an accessor to that value, so that through OOP you can do more than just access the data. –  Mike Fairhurst Nov 23 '12 at 23:54
    
Don't understand yet. What is "5.4" ? Don't get the "rare but disallowed" thing. Maybe it's just me.... I don't seek to know all the answers, but to understand the questions. –  user61852 Nov 24 '12 at 0:42
    
PHP 5.4 added traits for mixin composition, previously only interfaces could do mixin composition. For 'rare but disallowed,' its just a metaphor. The permission is an object that holds different types of data, and the validator must properly check all of it. Instead of describing the data that's being sent, why it is so varied, and what determines its validity, I pretended that the data being provided are strings, because strings are widely varied and can describe their own validity. Think of 'rare but disallowed' as a metaphor for any rare but disallowed info being sent into the validator. –  Mike Fairhurst Nov 24 '12 at 2:47
add comment

2 Answers

up vote 8 down vote accepted

I quite like this question, but I don't think it is really about OO or APIs. It's about being a junior or a new member of a team and having input (either ideas or past experience) that the other team members aren't agreeing with.

I'll answer your questions:

  1. Doesn't appear to be
  2. When the problem doesn't warrant any more abstraction
  3. When the problem does warrant more abstraction
  4. Experience
  5. Experience
  6. Explain it, demonstrate it, describe the risks and benefits of it.

That is all very deliberately vague. Abstractions add complexity and remove complexity at the same time - just different types of complexity. The correct application depends on your business, product, team, and so on.

Your teammates may not support your idea for a number of reasons:

  • the idea won't work, is incorrect or bad practice, or is just rubbish (your competence)
  • there are business or system reasons why the idea won't work (your experience)
  • they don't see the benefits of the idea or the risks of not adopting the idea (their experience)
  • they refuse to or cannot understand the idea (their competence)
  • they are asserting their seniority or position (political)

A political or competence problem falls outside the scope of any technical discussion of the idea, is a management issue, and I hope that's not the problem. It's very likely that this is an experience issue. This doesn't mean that you don't have enough experience, or they don't have enough experience, but there's a disconnect somewhere which causes you to see this as a good idea but others to see it as a bad idea.

What do you do in this situation? You discuss the idea. If they aren't willing to discuss the idea with you (and you should give them the opportunity to dissect it, destroy it for you so you can learn from them) then see above (political or competence reasons). Have a frank and open discussion about the alternatives, particularly with respect to implementation difficulty and future flexibility. It's not your idea vs their idea; there are two approaches on the table and the team must decide between them.

share|improve this answer
    
And a good answer! Thank you. I've surprised everyone (even myself) at how solid a developer I've become, and how quickly. Internally I think I'm all the better a dev for learning quickly, while other devs think I'm less because I still have experience gaps. This answer reminded me of that dynamic very cleanly. I just can't expect to easily lead the team into more anything-oriented programming yet, for the five points above. –  Mike Fairhurst Nov 22 '12 at 7:26
add comment

I like OOP so I can reuse and hide implementation details. The key to that is actually reuse and not programming. So, before thinking about creating something new, that requires time for other to learn, have you done a little search if there's already some implementation exists.

For example, in php, maybe you can use Zend/Permissions classes from Zend Framework. You might be able to refactor your existing code and use Zend classes. You will also have documentation for the Zend classes you use, instead of being responsible to write them.

share|improve this answer
    
Could you give any examples of which zend classes might do the same thing he's delineating in this object model, or how you might approach this problem with the zend classes? –  Jimmy Hoffa Nov 22 '12 at 3:51
    
I like Zend's OOP approaches, but my team does not. I will definitely look at their permission libraries, at the very least to compare what I came up with vs how Zend handled it. Maybe get some good takeaways. Thanks! –  Mike Fairhurst Nov 22 '12 at 7:27
    
@JimmyHoffa while it's not very clear to me what the intention of his classes, it sounds like he could use Zend/Permissions/Acl and allow() method would be isAllowed(). –  imel96 Nov 23 '12 at 1:32
    
@MikeFairhurst I'd put personal feelings aside and guessing what people think and reduce it to is there any class available and can it be used. It's hard to understand how can someone doesn't like Zend because you don't have to use the whole thing, only the classes you need. Instead of concerning how much experience you have, show how much knowledge you know about existing solutions or classes. I should say experienced programmer write less code. –  imel96 Nov 23 '12 at 1:38
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.