HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Automated collage tool

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
automatedcollagetool

Problem

I've been working on refactoring a project of mine and need some help. I'm looking to apply some design principles and patterns to make the code cleaner and easier to maintain. It looks like I'm clearly violating the DRY principle as there seems to be quite a bit of repetition. Also, I think there is a design pattern or two that can be implemented, I'm just not sure which ones.

The program implements HP's Blocked Recursive Image Composition (BRIC) algorithm . The algorithm creates and steps through a binary tree multiple times to assign various properties like size, coordinates, and aspect ratios to all of the nodes in the tree.

I have a BinaryNode class set up that has references to its left child, right child, and parent as well as holds various properties like size, aspect ratio, and coordinates.

My Collage class, where the actual collage is constructed from a list of images passed in to the constructor, is set up like so:

class Collage
{
    private BinaryNode root = new BinaryNode();

    private List images;
    private List imageInformation = new List();

    private Size finalCollageSize;

    private int collageLength;
    private Orientation collageOrientation;

    private int borderWidth;
    private Color borderColor;

    private Random random;

    public Collage(List imagePaths, int collageLength, Orientation collageOrientation, int borderWidth, Color borderColor)
    {
        this.images = convertPathsToImages(imagePaths);
        this.collageLength = collageLength;
        this.collageOrientation = collageOrientation;
        this.borderWidth = borderWidth;
        this.borderColor = borderColor;
        random = new Random();
        SetCollageSplit();
    }
    ...


After the Collage object is constructed, I execute collage.CreateCollage(), which immediately hands off the majority of the algorithm to CreateCollageTree():

```
public Bitmap CreateCollage()
{
CreateCollageTree();
...
}

private void CreateCollageTree()
{

Solution

I'll just concentrate on DRYing out your code. Create a general VisitTree method:

public VisitTree(Action reviver)
{
    if (reviver == null)
    {
        throw new ArgumentNullException("reviver");
    } 
    var currentNode = root;
    var nodeQueue = new Queue();

    while (currentNode != null)
    {
        if (currentNode.leftChild != null)
        {
            nodeQueue.Enqueue(currentNode.leftChild);
            nodeQueue.Enqueue(currentNode.rightChild);
        }
        reviver(currentNode);
        currentNode = nodeQueue.Count > 0 ? nodeQueue.Dequeue() : null;
    }
}


Then you just create an Action that does all the stuff you want on your tree - you keep SRP because that method (or lambda) should be broken up into different methods that do different things.

e.g.

VisitTree(node => 
    {
        SetNodeSplit(node);
        SetImageToLeafNode(node);
        SetAspectRatio(node);
        SetFinalCollageSize(node);
        SetNewImageSize(node);
        SetImageCoordinate(node);
    });


I'd also suggest adding a IsLeaf property to your BinaryNode class to make it clearer.

Code Snippets

public VisitTree(Action<BinaryNode> reviver)
{
    if (reviver == null)
    {
        throw new ArgumentNullException("reviver");
    } 
    var currentNode = root;
    var nodeQueue = new Queue<BinaryNode>();

    while (currentNode != null)
    {
        if (currentNode.leftChild != null)
        {
            nodeQueue.Enqueue(currentNode.leftChild);
            nodeQueue.Enqueue(currentNode.rightChild);
        }
        reviver(currentNode);
        currentNode = nodeQueue.Count > 0 ? nodeQueue.Dequeue() : null;
    }
}
VisitTree(node => 
    {
        SetNodeSplit(node);
        SetImageToLeafNode(node);
        SetAspectRatio(node);
        SetFinalCollageSize(node);
        SetNewImageSize(node);
        SetImageCoordinate(node);
    });

Context

StackExchange Code Review Q#138253, answer score: 3

Revisions (0)

No revisions yet.