Partner mating game

Question

I say this is right because X might happen and Y modification to the code is possible in the future. More senior guy says the sorta similar thing, except along the lines "I'm right, you're wrong." Then I back off, b/c he is more senior.

Here I implement a method playMatingGameA(String). My more senior coworker provides alternative implementation playMatingGameB(String). Please, ignore how the main object fits into the grand scheme of things. The point is coding up a single method.

Problem description: an object of class Partner has to play a mating game. The input is a mating call. S/He takes a taxi, goes to a Bar or a Club or whatever, emits a mating call, flirts and mates. The playMatingGame() method isn't supposed to throw an exception or return a value. A string status variable is set instead.

String matingCall must be nonempty. The establishment (Bar) must find it appropriate. A few method calls can throw exceptions/return nulls.

I'll conclude with my own thoughts, but comments from other engineers would be much appreciated.

interface MeatMarket
{
    public Partner findPartner(String matingCall);

    public boolean isMatingCallAllower(String call)
           throws PartnerRejectionException;
} // e.g., Bar, Club, Church, etc.

class TaxiMeatMarketFactory
{
    public static MeatMarket getMeSomePlaceFun()
    throws RuntimeException; // I know, no need 2 declare
}




public abstract class Partner
{
    String name;
    // {get; set; justkiddingthisisjava;}

    private String morningAfterFeeling;

    // here B constructorz [skip]

    // . . . and the methodz
    public void flirt(Partner p)
           throws PartnerRejectionException, ParnterNotSoberException;

    public abstract void mate(Partner p)
           throws PartnerWrongSpeciesException;



    // The goal: with your given mating call, take taxi to a meatmarket
    // find a partner, flirt then mate.
    // No throwing up! No Exceptions!
    //
    // MUST return void and record morningAfterFeeling as a status.
    public void playMatingGameA(String matingCall)
    {
        morningAfterFeeling = "Great";

        if (matingCall == null || matingCall.equals(""))
        {
            morningAfterFeeling = "No voice";
            log.error(morningAfterFeeling);
            return;
        }

        MeatMarket mm = null;

        try
        {
            // might still return null. 
           mm = TaxiMeatMarketFactory.getMeSomePlaceFun();
        }
        catch(RuntimeException e)
        {
            log.error(e);
        }

        if (mm == null)
        {
            // taxi's fault either way.
            log.error("Bad taxi");
            morningAfterFeeling = "Bad taxi";
            return;
        }


        try
        {
            if (!mm.isMatingCallAllowed(matingCall))
            {
                morningAfterFeeling = "Arse kicked & thrown out";
                return;
            }
        }
        catch(PartnerRejectionException e)
        {
            morningAfterFeeling = "Bad meatmarket";
            log.error(e);
            return;
        }


        try
        {
            Partner p = mm.findPartner(matingCall);
            flirt(p);
            mate(p);
        }
        catch(PartnerRejectionException e)
        {
            morningAfterFeeling = "partner not responsive";
            log.error(e);
        }
        catch(PartnerNotSoberException e)
        {
            // here and above the partner's choices are very unwise.
            morningAfterFeeling = "partner not responsive";
            log.error(e);        
        }
        catch(PartnerWrongSpeciesException e)
        {
            // and here it's partner's physique, out of their control
            morningAfterFeeling = "Could not find partner";
            log.error(e);        
        }
        finally
        {
            if (!gotHome())
            {
                morningAfterFeeling += "Slept on street";
            }
        }

        if (!"Good".equals(morningAfterFeeling))
        {
            log.error("Terrible night out");
        }
    }



    public void playMatingGameB(String matingCall)
    {
        try
        {
            morningAfterFeeling = "Great";
            if (matingCall == null || matingCall.equals(""))
            {
                morningAfterFeeling = "No voice";
                log.error(morningAfterFeeling);
                return;
            }
            else
            {
                MeatMarket mm = TaxiMeatMarketFactory.getMeSomePlaceFun();
                if (!mm.isMatingCallAllowed(matingCall))
                {
                    morningAfterFeeling = "Ass kicked & thrown out";
                    return;
                }               
                else
                {
                    Partner p = mm.findPartner(matingCall);
                    flirt(p);
                    mate(p);
                }
            }
        }
        catch(PartnerRejectionException e)
        {
            morningAfterFeeling = "Exception: " + e.getMessage();
            log.error(e);
        }
        catch(PartnerNotSoberException e)
        {
            // here and above the partner's choices are very unwise.
            morningAfterFeeling = "Exception: " + e.getMessage();
            log.error(e);        
        }
        catch(PartnerWrongSpeciesException e)
        {
            // and here it's partner's physique, out of their control
            morningAfterFeeling = "Exception: " + e.getMessage();
            log.error(e);        
        }
        catch (Exception e)
        {
            morningAfterFeeling = "Exception: " + e..getMessage();
            log.error(e);
        }
        finally
        {
            if (!makeItHome())
            {
                morningAfterFeeling += "Slept on street";
            }

            // this clause moved here for no particular reason
            if (!"Good".equals(morningAfterFeeling))
            {
                log.error("Terrible night out");
            }   
        }
    }
}

Method A provides more detailed info about what goes wrong. And it stays away from null references. I'd rather touch a living rattle snake than a null. I've learned to do away with bad (null, negative, etc) inputs in the very beginning. Then move on to the "meat". which is exactly 3 lines long: findPartner(), flirt(), mate().

His method is definitely shorter and the whole logic fits into one paragraph, followed by catches. Where I check for null reference, he catches the NullPointerException and gains a few lines of code this way. His "meat" includes some error checking logic and nicely fits on a page. But is way longer than 3 lines, and it involves nested if-elses. Also notice how the line morningAfterFeeling="Good" in his method. Its correctness depends on the overall catch(Exception e).

He says that all my returns are jumps in logic. Not a problem to me, I think. If this is bad, return. It's over. If not, assume it's good & move on. By the time you get to the meat, you KNOW what you shouldn't worry about.

P.S. I've changed the method/object names, of course, I would't just copy-paste the proprietary code. Recasting the real thing into the mating game setting was an interesting abstraction exercise.

Answer 1

I agree explicitly checking for error conditions (instead of a general catching of Exception) and failing fast or returning early are usually good practices, as you have done in your method. Some people don't like the break in logic flow, but there's no reason to continue on and it avoids nesting of conditionals and/or other code blocks. Catching generic Exception is always a red flag for me -- sometime you have to do it, but usually there's a better way.

To simplify the catch blocks, you might want to consider making a superclass (say AbstractRejectionException) for all your custom exceptions that has an abstract getMorningAfterFeeling() method to allow the subclasses to return their own specific feelings. This would simplify many of your catch blocks into just this:

} catch (AbstractRejectionException e) {
    morningAfterFeeling = e.getMorningAfterFeeling();
    log.error(e);
}

Also, you are initially setting morningAfterFeeling to "Great", but checking for Good".equals(morningAfterFeeling. I'm assuming that was a typo.

Answer 2

You and your senior handling the NullPointerException with same logic

if ( matingCall == null || matingCall.equals("") )

But it gives me a ughhhhh feeling. However you both are right but I would modify your code

if ( matingCall == null || "".equals(matingCall) ) {
    // do what you do
}

This is a guarantee that NPE will never occur.

---

Now playMatingGameA(String) has a good work-flow logic. As you said "If this is bad, return. It's over. If not, assume it's good & move on.". I also believe in this as the code is good to handle. I always sacrifice LOC to readability and maintainability.

Your explicit exception handling makes it easier to debug the code than playMatingGameA(String) method. Now in case of multiple catch codeblocks, you can easily optimize that.

Onwards Java7 you can catch multiple exception in same catch block. Catching multiple exception FTW right?

Now your code or the senior's code looses like, what 8-9 lines of code. Right? So Enjoy.