Rules should Run



  • Our software has a custom rules-based thingy built into it. I personally believe it (the implementation) is icky, but ... whatever ...

    So, when a piece of data flows through the system, it is checked against the stored rules, here's the logic used:

    if ( rules.checkShouldRun(data) ) {
      // do nothing
    } else
      processingQueue.add(data);
    }

    Yep, that's right ... when checkShouldRun() returns true, the data should NOT be run through the system. In other words, the data matches a rule in the system that implies a 'do not run' condition. Wouldn't a person who cares for their (and others') sanity name the function checkShouldNotRun()?



  • Surely change the checkShouldRun() function to return true if it should be added to the processing queue. Then ditch the empty if block.



  • Why is there an if() { //do nothing }?  Why not if(!rules.checkShouldRun()) { // do important stuff }

    BTW, the naming is a wtf.  What does the documentation, if any, say about it?  Personally, I would've named it rules.interrogateThisBitch(data).



  • @C-Octothorpe said:

    Why is there an if() { //do nothing }?  Why not if(!rules.checkShouldRun()) { // do important stuff }

    This is actually the way it is coded ... I was just forcing everying to see the backwardness of the method name.

    BTW, the naming is a wtf.  What does the documentation, if any, say about it?  Personally, I would've named it rules.interrogateThisBitch(data).

    The method documentation doesn't mention anything about the 'reverse logic' ... There are a couple places in the source where it is referenced which mentions the reverse logic; but there are also a few more places in the source where it is referenced but no mention of the reverse logic of the name.



  • @zelmak said:

    Wouldn't a person who cares for their (and others') sanity name the function checkShouldNotRun()?
    No, they wouldn't. Functions shouldn't have negative semantics.

    What would you consider easier to read (specially if in a hurry during crunch time):

     

    if( !checkShouldNotRun(data) ){
        processingQueue.add(data);
    }

     

    or

     

    if( checkShouldRun(data) ){
        processingQueue.add(data);
    }

     

    ?

    Just fix the return value. And, if as I suspect, it is an int/enum rather than a boolean, write the comparison explicitly.



  • I think it is the "optionally do the job" design pattern (there is a real name, but i don't care about it).

    Each function is called until one function says that it took care of the job. It is perfectly normal to use a boolean to say "I did it, stop running stuff".



  • @Zecc said:

    @zelmak said:

    Wouldn't a person who cares for their (and others') sanity name the function checkShouldNotRun()?
    No, they wouldn't. Functions shouldn't have negative semantics.

    What would you consider easier to read (specially if in a hurry during crunch time):

     

    if( !checkShouldNotRun(data) ){
        processingQueue.add(data);
    }

     

    or

     

    if( checkShouldRun(data) ){
        processingQueue.add(data);
    }

     

    ?

    Just fix the return value. And, if as I suspect, it is an int/enum rather than a boolean, write the comparison explicitly.

    "Fixing" in this case would likely break more than fix...


  • @henke37 said:

    I think it is the "optionally do the job" design pattern (there is a real name, but i don't care about it). Each function is called until one function says that it took care of the job. It is perfectly normal to use a boolean to say "I did it, stop running stuff".

    Chain of Responsibility?

    TRWTF is the caching proxy draconian access control measure that hangs on every fourth HTTP 'GET' ... makes AJAX ... tricky ...

    Anyhow, I recalled the code incorrectly, it actually looks like:

    if (rules.checkShouldRun(data) == false) {
      // do something important with data
    }
    


  • ... is it not a valid business case to do something with the data which the rules rejected..?



  • @Xyro said:

    ... is it not a valid business case to do something with the data which the rules rejected..?

    They're already stored in the database for 'trend analysis' ... its just that nothing further is done with them.



  • @Zecc said:

    Functions shouldn't have negative semantics.
     

    Your sig has part of one. isNaN() confuses me during crunch time. :-/



  • @Zecc said:

    Functions shouldn't have negative semantics.
    I can't fully agree on this tautological statement, I think there are valid exceptions.

    For example, I'm sure most of us have created methods like

    public boolean notNullOrEmpty(string str) {
        return str != null ? str.Length > 0 : false;
    }

    to ward against NullReferenceExceptions. And, at least personally, I consider if (notNullOrEmpty(str)) {...} a bit more concise than if (!isNullOrEmpty(str)) {...}



  • @Anonymouse said:

    @Zecc said:

    Functions shouldn't have negative semantics.
    I can't fully agree on this tautological statement, I think there are valid exceptions.

    For example, I'm sure most of us have created methods like

    public boolean notNullOrEmpty(string str) {
        return str != null ? str.Length > 0 : false;
    }

    to ward against NullReferenceExceptions. And, at least personally, I consider if (notNullOrEmpty(str)) {...} a bit more concise than if (!isNullOrEmpty(str)) {...}

    Yes, I don't fully agree with it myself. It was a generalization, and generalizations are generally false. I should have said instead: "it's generally best to avoid functions with a negative connotation".

    What I meant is, in this case it would be better if the function could be named such that the condition would be written has <font face="courier new,courier">( needsToBeRun(data) )</font>, or at least <font face="courier new,courier">( !skipsRunning() )</font>.

    Have said that, <font face="courier new,courier">checkShouldNotRun()</font> isn't that bad a name. I was just being picky.

    @Zemm said:

    Your sig has part of one. isNaN() confuses me during crunch time. :-/
    But NaN is a well-known value. It just happens to be called that way. This function checks for "positive not-a-numberness" rather than "negative numberness", if you know what I mean. Throw it a string and it will return false, because a string is not a NaN. **

    Bottom line is: I like when "negative" → return false, "positive" → return true.

    **EDIT: ok, I just checked and isNaN() returns true if you pass it a string that fails to convert to a number. But not if it is an empty string. Okay, that's just nasty!



  • @Anonymouse said:

    For example, I'm sure most of us have created methods like
    public boolean notNullOrEmpty(string str) {
        return str != null ? str.Length > 0 : false;
    }
    I haven't. .Net has that function built in.


  • @Sutherlands said:

    @Anonymouse said:

    For example, I'm sure most of us have created methods like
    public boolean notNullOrEmpty(string str) {
        return str != null ? str.Length > 0 : false;
    }
    I haven't. .Net has that function built in.
    Not negated like that though: string.IsNullOrEmpty


  • @C-Octothorpe said:

    @Sutherlands said:

    @Anonymouse said:

    For example, I'm sure most of us have created methods like

    public boolean notNullOrEmpty(string str) {
    return str != null ? str.Length > 0 : false;
    }
    I haven't. .Net has that function built in.
    Not negated like that though: string.IsNullOrEmpty

    Once again, positive method names. And the fact that !string.IsNullOrEmpty is 2 characters less than string.IsNotNullOrEmpty.



  • @The_Assimilator said:

    Once again, positive method names. And the fact that !string.IsNullOrEmpty is 2 characters less than string.IsNotNullOrEmpty.

    In fact, this is a good example of why positive method names are a good thing. Does "IsNotNullOrEmpty" mean "Is Not (Null Or Empty)" or "Is (Not Null) Or Empty"? Granted, in this specific case you can probably guess what's intended, but more generally "IsNotPropertyAOrPropertyB" is ambiguous.



  • I think "checkShouldRun" is describing the action taken.  The method checks if anything should run.  It has a non-intuitive return value of false (which is a WTF) which indicates that the data should be processed.  Ideally, the method would use the "get" convention, e.g "getShouldRun".  So this method is even more WTF-y at closer inspection.


  • ♿ (Parody)

    @frits said:

    I think "checkShouldRun" is describing the action taken.  The method checks if anything should run.  It has a non-intuitive return value of false (which is a WTF) which indicates that the data should be processed.  Ideally, the method would use the "get" convention, e.g "getShouldRun".  So this method is even more WTF-y at closer inspection.


    But isn't doing a lot of processing inside a getter generally a WTF? This is clearly not a getter, but changing it to getShouldRun makes it look like that, and therefore more of a WTF than the current name.



  • @boomzilla said:

    @frits said:

    I think "checkShouldRun" is describing the action taken.  The method checks if anything should run.  It has a non-intuitive return value of false (which is a WTF) which indicates that the data should be processed.  Ideally, the method would use the "get" convention, e.g "getShouldRun".  So this method is even more WTF-y at closer inspection.

    But isn't doing a lot of processing inside a getter generally a WTF? This is clearly not a getter, but changing it to getShouldRun makes it look like that, and therefore more of a WTF than the current name.

    OK, get is probably the wrong prefix, but at least it's a conventional one.  Really, "shouldRun" would be perfectly acceptable and follow convention.



  • How about "isValidFor()"? That's a nice fluent name.

    Rules rules = givenRules.isValidFor(data)? givenRules : DEFAULT_RULES.getInstance();
    log.info("Using rules "+rules+" to process "+data+".");
    rules.run(data);

  • ♿ (Parody)

    @Xyro said:

    How about "isValidFor()"? That's a nice fluent name.

    Rules rules = givenRules.isValidFor(data)? givenRules : DEFAULT_RULES.getInstance();
    log.info("Using rules "+rules+" to process "+data+".");
    rules.run(data);

    I find that confusing. Frankly, I don't see anything wrong with the original name, just the return value. isValidFor what, exactly? It makes it sound like you've already run the data through the rules and got a positive return.



  • @boomzilla said:

    Frankly, I don't see anything wrong with the original name

     

    The original name implies that you are commanding the object to take an action, even though you really want to check a condition.  That's what's wrong with it.


  • ♿ (Parody)

    @frits said:

    @boomzilla said:
    Frankly, I don't see anything wrong with the original name

    The original name implies that you are commanding the object to take an action, even though you really want to check a condition.  That's what's wrong with it.

    Maybe it does have to take some action in order to do the checking. Basically, my problem was that isValidFor is just too vague. shouldRun is fine, I think meaning wise, and not much different, except less verbose than the original checkShouldRun. The description is, "checked against the stored rules," so it definitely sounds like there is something more significant than returning a cached boolean going on.



  • @Xyro said:

    How about "isValidFor()"? That's a nice fluent name.

    Rules rules = givenRules.isValidFor(data)? givenRules : DEFAULT_RULES.getInstance();
    log.info("Using rules "+rules+" to process "+data+".");
    rules.run(data);

    If you want to go that way, better use a proper Chain-of-Responsability pattern:

     

    class Rules
    {
    	Rules next;
    
    void setNext(Rules next) { this.next = next; }
    
    bool runIfApplicable(Data data) { return false; };
    
    bool run(Data data)
    {
    	if( runIfApplicable() ){
    		return true;
    	}
    	else if( next ){
    		return next.run();
    	}
    	return false;
    }
    
    // Constructor at the end because I'm feeling adventurous
    Rules()
    {
    	next = new DefaultRules();
    }
    

    }

    class DefaultRules: Rules
    {
    bool runIfApplicable(Data data)
    {
    Log.info('Running default behaviour');
    return true;
    }
    }

    class TheseParticularRules: Rules
    {
    bool checkShouldRun(Data data)
    {
    return Dice.roll() == 4;
    }

    bool runIfApplicable(Data data)
    {
    	// Look at that, we only hid the exact same condition
    	// inside a bunch of classes
    	if( checkShouldRun(data) ){
    		Log.info('Doing stuff with stuff');
    		return true;
    	}
    	return false;
    }
    

    };

    // ... Over-engineered much?

    // Some people would cringe when seeing this one liner. Screw them!
    new TheseParticularRules().run(data);

     

    @frits said:
    The original name implies that you are commanding the object to take an action, even though you really want to check a condition.  That's what's
    wrong with it.

     

    checkShouldRun
    implies commanding an object to take action??

     



  • @Zecc said:

    checkShouldRun

    implies commanding an object to take action??

    Yes.  In Java (and OO in general) boolean methods conventionally have prefixes like "is" or "has" or sometimes "should".  "Check" is a verb.  Generally, if a method name starts with a verb, you are explicitly telling the object to perform an action.  In this case, the consumer may be checking something, but that is of no concern to the object.  In kind, the object may be doing a large amount of processing, but the implementation is of no concern to the consumer.



  • @frits said:

    Yes.  In Java (and OO in general) boolean methods conventionally have prefixes like "is" or "has" or sometimes "should".  "Check" is a verb.  Generally, if a method name starts with a verb, you are explicitly telling the object to perform an action.  In this case, the consumer may be checking something, but that is of no concern to the object.  In kind, the object may be doing a large amount of processing, but the implementation is of no concern to the consumer.
    Oh, ok. I was confusing "taking action" with "mutating data somewhere".


  • Discourse touched me in a no-no place

    @frits said:

    Yes. In Java (and OO in general) boolean methods conventionally have prefixes like "is" or "has" or sometimes "should". "Check" is a verb. Generally, if a method name starts with a verb, you are explicitly telling the object to perform an action
    I know you said 'generally' but from what you wrote, you give the impression you think 'is', 'has', and 'should' aren't verbs.



  • @PJH said:

    @frits said:
    Yes. In Java (and OO in general) boolean methods conventionally have prefixes like "is" or "has" or sometimes "should". "Check" is a verb. Generally, if a method name starts with a verb, you are explicitly telling the object to perform an action
    I know you said 'generally' but from what you wrote, you give the impression you think 'is', 'has', and 'should' aren't verbs.

     

    I meant "action" verbs.



  • @frits said:

    @Zecc said:

    checkShouldRun

    implies commanding an object to take action??

    Yes.  In Java (and OO in general) boolean methods conventionally have prefixes like "is" or "has" or sometimes "should".  "Check" is a verb.  Generally, if a method name starts with a verb, you are explicitly telling the object to perform an action.  In this case, the consumer may be checking something, but that is of no concern to the object.  In kind, the object may be doing a large amount of processing, but the implementation is of no concern to the consumer.

    In addition to the name, I also follow the convention that a method performs some action, or has some side-effects.  I hate the property getters that have 74 lines of code hitting four services and three DB calls...  If there is processing that will be done, then it's a method, otherwise a property should (ideally, but not always) just return the underlying private field.  Naming enhances this convention, IMO.

    myObj.IsValid vs myObj.IsValid()

    To me the second implies that there will be side-effects, or some significant processing.



  • Since the original checkShouldRun() takes in data as a parameter, I'd say it's safe to assume that some processing is going on. (Not that it matters, though, since a field-like getter wouldn't be applicable with a parameter.)

    Nevertheless, there shouldn't be side-effects one way or another if all we're doing is checking the validity of the rules on the data. Unless you count processing time as a side-effect.



  • Not processing time, but processing steps or complexity.  It's hard to draw a clean line, but I have in the past refactored a read-only property into a method when it's complexity started to get more than just "return _field;".  I think it all depends on the coding standards and that it's consistent across all the team members' work.



  • Actually not that unusual of a pattern. Success is 0, and non-zero is specific errors. Generally made much cleaner by having a const (or equiv) of SUCCESS so it i s

    if (checkRule(data) == success) ....

    So the WTF *may* not be in the implementation but rather in the usage.


Log in to reply