RFC regarding code structure

The RPTools applications are written in Java. If you're interested in contributing to any project here by submitting patches to the source code, this is the forum to ask questions about how to do so. Please put the two-letter tool name abbreviation in your thread Title. To enter this group, go to the Usergroups page of your User Control Panel and join the Java Developer group.

Moderators: dorpond, trevor, Azhrei

Post Reply
User avatar
Azhrei
Site Admin
Posts: 12086
Joined: Mon Jun 12, 2006 1:20 pm
Location: Tampa, FL

RFC regarding code structure

Post by Azhrei »

I was in one of the code files a few minutes ago fixing a NPE and I came across something that I wanted to change/reorganize. But while I know what I want the end result to be, I'm not sure what the proper level of abstraction should be so I thought I'd ask folks here to see if they had an opinion. :)

Here's a sample of the type of code I'm referring to:

Code: Select all

    public boolean hasLightSourceType(LightSource.Type lightType) {
        if (lightSourceList != null) {
            for (ListIterator<AttachedLightSource> i = lightSourceList.listIterator(); i.hasNext();) {
                AttachedLightSource als = i.next();
                LightSource lightSource = MapTool.getCampaign().getLightSource(als.getLightSourceId());
                if (lightSource != null && lightSource.getType() == lightType)
                    return true;
            }
        }
        return false;
    } 
Very clean and easy to follow. However, there are three or four other methods with almost the exact same code in them. Generally speaking (there are exceptions) it's best to remove redundant code if that code can be adequately parameterized. In this case the Template pattern fits pretty well:

Code: Select all

class LightSourceTypeManipulator {
    protected boolean boolResult(LightSource ls) {
        return (lightSource != null && lightSource.getType() == lightType);
    }
    public boolean boolProcess(Token t, LightSource.Type lightType) {
        if (t.getLightSourceList() != null) {
            for (ListIterator<AttachedLightSource> i = t.getLightSourceList().listIterator(); i.hasNext();) {
                AttachedLightSource als = i.next();
                LightSource lightSource = MapTool.getCampaign().getLightSource(als.getLightSourceId());
                if (boolResult(lightSource))
                    return true;
            }
        }
        return false;
    }
} 
As you can see, the class completely encapsulates the algorithm, but it defers the content of the loop to a separate method. This allows that method to be overridden if necessary to implement something differently. So the original code becomes:

Code: Select all

public boolean hasLightSourceType(LightSource.Type lightType) {
    LightSourceTypeManipulator lstm = new LightSourceTypeManipulator();
    return lstm.boolProcess(this, lightType);
} 
All we've done is moved the code around a little bit. What's the big deal?

Now the other -- very similar -- methods can be rewritten to be much simpler:

Code: Select all

public boolean hasGMAuras() {
    LightSourceTypeManipulator lstm = new LightSourceTypeManipulator() {
        protected boolean boolResult(LightSource ls) {
            if (ls != null) {
                if (ls.getLightList() != null) {
                    for (Light light : ls.getLightList())
                        if (light.isGM())
                            return true;
            }
            return false;
        }
    };
    return lstm.boolProcess(this, lightType);
} 
This extends to methods like "hasOwnerOnlyAuras()", "hasLightSource(LightSource ls)", and a couple others. In all of these, the algorithm (the loop) is essentially identical. Factoring out the loop means that chunk of code will be executed more often, meaning the JIT compiler is more likely to convert it to native code (often-executed methods are JIT compiled), which means the application runs faster. And if a bug in the loop is detected (like a NPE) it can be detected and fixed once. I don't think there's any question that this is the right way to do it, but...

Who should be creating the anonymous class object? In the last two code blocks above, I had hasLightSourceType() and hasGMAuras() create the LightSourceTypeManipulator and override the boolResult() method, but perhaps the code that is calling the hasLightSourceType() method should be creating the object and passing it in as a parameter? Then the method would simply invoke the boolProcess() method and not be stuck with the object creation process.

If the same object were used over and over it would obviously be more memory efficient to do the allocation outside and pass in a reference. But it complicates the outside code because it has to know what the algorithm is that it wants executed. And this algorithm seems to me to be tied to overall object (Token, in this case) so I think it should be hidden.

Well, I think I've just talked myself into the right way to handle this: put the object creation inside the method, not in the caller. ;)

I'm going ahead and posting this anyway. Who knows, maybe it will generate some discussion? If not, it's simply the ramblings of one of the devs and becomes lost amongst the noise of the interwebs. 8)

User avatar
jfrazierjr
Deity
Posts: 5176
Joined: Tue Sep 11, 2007 7:31 pm

Re: RFC regarding code structure

Post by jfrazierjr »

Yea, I guess I am the guilty party. I maintained Trevors original design where I could have done a bit more refactoring. I vaguely seem to recall(It's been years since I looked at it), but I believe functors would be an approach for this type of thing.

Also, you may want to take a look at Zone.Filter and several of the Zone.getTokensFiltered which implements pretty much the same pattern.
I save all my Campaign Files to DropBox. Not only can I access a campaign file from pretty much any OS that will run Maptool(Win,OSX, linux), but each file is versioned, so if something goes crazy wild, I can always roll back to a previous version of the same file.

Get your Dropbox 2GB via my referral link, and as a bonus, I get an extra 250 MB of space. Even if you don't don't use my link, I still enthusiastically recommend Dropbox..

User avatar
Azhrei
Site Admin
Posts: 12086
Joined: Mon Jun 12, 2006 1:20 pm
Location: Tampa, FL

Re: RFC regarding code structure

Post by Azhrei »

Heh, I haven't decided to actually do it yet!

It doesn't really contribute to 1.3 in the form of a bug fix and we have a feature freeze on so I'm reluctant to touch anything that isn't being modified for some other reason.

And I wasn't trying to assign blame, Joe. I'm just noticing that there are other spots in the code like that. For example, the huge overhaul I did to the persistence layer a few months back would likely benefit from the same approach. But I've held off as "it works" and you know what they say about, "if it works..."

No, I was really just musing aloud and when I got the end realized that I had answered my own question. But I didn't have the heart to hit Cancel and completely delete my delirious writings!

User avatar
jfrazierjr
Deity
Posts: 5176
Joined: Tue Sep 11, 2007 7:31 pm

Re: RFC regarding code structure

Post by jfrazierjr »

Azhrei wrote:Heh, I haven't decided to actually do it yet!

It doesn't really contribute to 1.3 in the form of a bug fix and we have a feature freeze on so I'm reluctant to touch anything that isn't being modified for some other reason.

And I wasn't trying to assign blame, Joe. I'm just noticing that there are other spots in the code like that. For example, the huge overhaul I did to the persistence layer a few months back would likely benefit from the same approach. But I've held off as "it works" and you know what they say about, "if it works..."

No, I was really just musing aloud and when I got the end realized that I had answered my own question. But I didn't have the heart to hit Cancel and completely delete my delirious writings!

Oh.. I did not think you were trying to assign blame. Most of the time, I do the quick way to see if I can get it working (when working on something new I don't know much about) and THEN go back and refactor. Sometimes..well... I forget the refactor part...

I have the same issue right now with the exposeArea, hideArea, etc in Zone. Each of them have loop over tokens and then modifies the area in some way inside the loop if condition X is true, so I need to refactor that to get a list of applicable tokens first from another method and make that loop smaller.
I save all my Campaign Files to DropBox. Not only can I access a campaign file from pretty much any OS that will run Maptool(Win,OSX, linux), but each file is versioned, so if something goes crazy wild, I can always roll back to a previous version of the same file.

Get your Dropbox 2GB via my referral link, and as a bonus, I get an extra 250 MB of space. Even if you don't don't use my link, I still enthusiastically recommend Dropbox..

Post Reply

Return to “Java Programming Info”