10
\$\begingroup\$

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<Image> images;
    private List<ImageDetails> imageInformation = new List<ImageDetails>();

    private Size finalCollageSize;

    private int collageLength;
    private Orientation collageOrientation;

    private int borderWidth;
    private Color borderColor;

    private Random random;


    public Collage(List<String> 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()
{
    InitializeFullBinaryTree();
    SetNodeSplits();
    SetImagesToLeafNodes();
    SetAspectRatios();
    SetFinalCollageSize();
    SetNewImageSizes();
    SetImageCoordinates();
    GetImageDetailsFromTree();
}

I feel a lot of repetition occurs within these methods. For example, take a look at SetNodeSplits(), SetImagesToLeafNodes(), SetAspectRatios(), SetNewImageSizes(), and SetImageCoordinates(). They all simply parse the tree and perform some action on either an inner node or leaf node. Lots of repetition going on here. I was thinking I could parse the tree only once and call the proper methods once I'm at the right node, but that would obviously violate the Single Responsibility Principle (SRP):

/// <summary>
/// Construct collage tree. It needs to be a full binary tree
/// so add 2 nodes for every one image. Also subtact one from the image
/// count because the root node has already been created.
/// </summary>
private void InitializeFullBinaryTree()
{
    for (int i = 0; i < images.Count - 1; i++)
    {
        root.addNode();
        root.addNode();
    }
}  


/// <summary>
/// Assign inner nodes a 'Vertical' or 'Horizontal' split at random (50/50 chance)
/// </summary>
private void SetNodeSplits()
{
    var currentNode = root;
    var nodeQueue = new Queue<BinaryNode>();

    while (currentNode != null)
    {
        if (currentNode.leftChild != null)
        {
            nodeQueue.Enqueue(currentNode.leftChild);
            nodeQueue.Enqueue(currentNode.rightChild);

            if (currentNode.assignedSplit == Split.None)
            {
                currentNode.assignedSplit = GetRandomSplit();
            }
        }

        currentNode = nodeQueue.Count > 0 ? nodeQueue.Dequeue() : null;
    }
}


/// <summary>
/// Assign images to all leaf nodes
/// </summary>
private void SetImagesToLeafNodes()
{
    var currentNode = root;
    var nodeQueue = new Queue<BinaryNode>();
    var imageIndex = 0;

    while (currentNode != null)
    {
        if (currentNode.leftChild != null)
        {
            nodeQueue.Enqueue(currentNode.leftChild);
            nodeQueue.Enqueue(currentNode.rightChild);
        }
        else
        {
            // It's a leaf node, so assign an image to it.
            if (imageIndex < images.Count)
            {
                Image image = images[imageIndex];
                currentNode.image = image;
                currentNode.aspectRatio = (float)image.Width / (float)image.Height;
                imageIndex++;
            }
        }

        currentNode = nodeQueue.Count > 0 ? nodeQueue.Dequeue() : null;
    }
}


/// <summary>
/// Set aspect ratios of all nodes in the tree
/// </summary>
private void SetAspectRatios()
{
    var currentNode = root;
    var nodeQueue = new Queue<BinaryNode>();

    while (currentNode != null)
    {
        if (currentNode.leftChild != null)
        {
            nodeQueue.Enqueue(currentNode.leftChild);
            nodeQueue.Enqueue(currentNode.rightChild);
        }

        currentNode.aspectRatio = CalculateAspectRatio(currentNode);
        currentNode = nodeQueue.Count > 0 ? nodeQueue.Dequeue() : null;
    }

}


/// <summary>
/// Set image sizes for all nodes in the tre
/// </summary>
private void SetNewImageSizes()
{
    var currentNode = root;
    var nodeQueue = new Queue<BinaryNode>();

    while (currentNode != null)
    {
        if (currentNode.leftChild != null)
        {
            nodeQueue.Enqueue(currentNode.leftChild);
            nodeQueue.Enqueue(currentNode.rightChild);
        }

        currentNode.size = CalculateNewImageSize(currentNode);
        currentNode = nodeQueue.Count > 0 ? nodeQueue.Dequeue() : null;
    }
}


/// <summary>
/// Set coordinates for all nodes in the tree.
/// </summary>
private void SetImageCoordinates()
{
    var currentNode = root;
    var nodeQueue = new Queue<BinaryNode>();

    // breadth-first
    while (currentNode != null)
    {
        if (currentNode.leftChild != null)
        {
            nodeQueue.Enqueue(currentNode.leftChild);
            nodeQueue.Enqueue(currentNode.rightChild);
        }

        currentNode.coordinates = CalculateImageCoordinates(currentNode);
        currentNode = nodeQueue.Count > 0 ? nodeQueue.Dequeue() : null;
    }
}

Please help me properly reduce all of this repetition and construct a better design. I'm hoping this is enough information / context. If it's not, I can provide any other code necessary. Also, this is my first C# project, so if there are any conventions or idioms I've violated, please let me know. Thanks a bunch in advance! I truly appreciate any feedback given.

\$\endgroup\$
3
\$\begingroup\$

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

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;
    }
}

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.

\$\endgroup\$
  • \$\begingroup\$ I was able to successfully implement this into my code and have to say it really cleaned it up nicely. I was unaware of actions and inexperienced with lambdas so I'm glad I got to work with them and learn a bit more. Thanks a lot. However, would you mind explaining the Action name "revivor". Is it a common convention to name Actions this way? I renamed it to "processNode" as it seemed more fitting, but I'm not sure if I'm breaking a taboo. Thanks again. \$\endgroup\$ – BenN Aug 10 '16 at 11:13
  • 1
    \$\begingroup\$ @BenN - no special reason. It stems from when I read Douglas Crockford's "JavaScript: The good parts". When walking the DOM he always called the function parameter a reviver function - I've used the same thing in C#. Thanks for pointing out that I've spelt it wrong here though! \$\endgroup\$ – RobH Aug 10 '16 at 13:54
1
\$\begingroup\$

You can greatly simplify the code and remove a lot of repetitions if you move the nodes enumeration into a separate method:

private IEnumerable<BinaryNode> Nodes()
{
    var currentNode = root;
    var nodeQueue = new Queue<BinaryNode>();
    while (currentNode != null)
    {
        if (currentNode.leftChild != null)
        {
            nodeQueue.Enqueue(currentNode.leftChild);
            nodeQueue.Enqueue(currentNode.rightChild);          
        }

        yield return currentNode;

        currentNode = nodeQueue.Any() ? nodeQueue.Dequeue() : null;
    }
}

Then in each other method you can use a simple foreach:

private void SetNodeSplits()
{
    foeach(var node = Nodes())
    {
        if (node.assignedSplit == Split.None)
        {
            node.assignedSplit = GetRandomSplit();
        }
    }       
}
\$\endgroup\$
  • \$\begingroup\$ This would reduce the amount of lines that are repeated, but I would still have to use a foreach in each method which doesn't quite satisfy the DRY principle. Thanks for the response. Also, I did pick up on your use of nodeQueue.Any(), so thanks for that as well. \$\endgroup\$ – BenN Aug 10 '16 at 0:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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