6
\$\begingroup\$

I am creating a fluent interface to build user interfaces in the Unity game engine.

As a subcomponent of this library, I have a set of objects called Components (e.g. Panel, Button, etc) that I want to map to new instances of View objects that are created by cloning an original "prefab" object. This is converts the objects created in the fluent interface into the game-facing UI objects that Unity works with for UI purposes.

The PrefabFactory clones prefab view objects using the Unity Engine's built-in Instantiate method:

public class PrefabFactory<TComponent, TView> : IComponentFactory<TComponent> 
        where TComponent : Components.Component 
        where TView : UIView<TComponent>
    {
        readonly TView prefab;
        public PrefabFactory(TView prefab)
        {
            this.prefab = prefab;
        }

        public GameObject BuildComponent(TComponent component, UIFactory uiFactory)
        {
            var view = Object.Instantiate(prefab);
            view.Bind(component, uiFactory);
            return view.gameObject;
        }

        public GameObject BuildComponent(TComponent component, UIFactory uiFactory, RectTransform parent)
        {
            var view = Object.Instantiate(prefab, parent);
            view.Bind(component, uiFactory);
            return view.gameObject;
        }

        public GameObject BuildComponent(Components.Component component, UIFactory uiFactory)
        {
            return BuildComponent(component as TComponent, uiFactory);
        }

        public GameObject BuildComponent(Components.Component component, UIFactory uiFactory, RectTransform parent)
        {
            return BuildComponent(component as TComponent, uiFactory, parent);
        }
    }

Based on the IComponentFactory interface:

public interface IComponentFactory
    {
        GameObject BuildComponent(UIBuilder.Components.Component component, UIFactory uiFactory);

        GameObject BuildComponent(UIBuilder.Components.Component component, UIFactory uiFactory, RectTransform parent);
    }

    public interface IComponentFactory<in T> : IComponentFactory where T: UIBuilder.Components.Component
    {
        GameObject BuildComponent(T component, UIFactory uiFactory);

        GameObject BuildComponent(T component, UIFactory uiFactory, RectTransform parent);
    }

Where UIBuilder.Components.Component is a model component. Components can be any type of UI object such as buttons, labels, etc. It's defined as follows:

public class Component
    {
        public Positioning Positioning { get; set; }
        public Margin Margin { get; set; }
    }

But this is subclassed for Panels, Buttons, etc. For example, Button:

public class Button : Component
    {
        public Component Content { get; set; }

        public Action ClickAction { get; set; }

        public static ButtonBuilder Builder => new ButtonBuilder();
    }

And the View scripts (a script attached to each cloned prefab for controlling binding and functionality) inherit from this base class:

public abstract class UIView<TModel> : MonoBehaviour where TModel : Components.Component
    {

        private void SetMargin(Margin margin, RectTransform rectTransform)
        {
            rectTransform.offsetMax = new Vector2(-margin.Right, -margin.Top);
            rectTransform.offsetMin = new Vector2(margin.Left, margin.Bottom);
        }

        private void SetPosition(Positioning positioning, RectTransform rectTransform)
        {
            rectTransform.anchorMax = positioning.Anchors.TopRight;
            rectTransform.anchorMax = positioning.Anchors.BottomLeft;

            rectTransform.SetSizeWithCurrentAnchors(RectTransform.Axis.Horizontal, positioning.Width);
            rectTransform.SetSizeWithCurrentAnchors(RectTransform.Axis.Vertical, positioning.Height);
        }

        public void Bind(TModel model, UIFactory uiFactory)
        {
            var rectTransform = GetComponent<RectTransform>();

            if (model.Positioning != null)
            {
                SetPosition(model.Positioning, rectTransform);
            }

            if (model.Margin != null)
            {
                SetMargin(model.Margin, rectTransform);
            }

            BindModel(model, uiFactory);
        }

        protected abstract void BindModel(TModel model, UIFactory uiFactory);
    }

With one typical implementation being:

public class ButtonView : UIView<Components.Button>
    {
        [SerializeField]
        UnityEngine.UI.Button button;

        [SerializeField]
        RectTransform body;

        protected override void BindModel(Components.Button model, UIFactory uiFactory)
        {
            button.onClick.AddListener(() => model.ClickAction());

            uiFactory.BuildUI(model.Content as dynamic, body);
        }
    }

To tie this all together, I have a UIFactory class that takes a dictionary which maps Component types to component factories (it's possible to use a ComponentFactory that doesn't rely on the prefab system, although the typical use case would be the PrefabFactory) and simply uses the correct factory for the type passed in.

public class UIFactory
{
    readonly IDictionary<Type, IComponentFactory> componentFactories;
    public UIFactory(IDictionary<Type, IComponentFactory> componentFactories)
    {
        this.componentFactories = componentFactories;
    }

    public UIFactory() : this(new Dictionary<Type, IComponentFactory>())
    {

    }

    public void RegisterPrefabFactory<TComponent, TView>(TView prefab)
        where TView : UIView<TComponent>
        where TComponent : Components.Component
    {
        componentFactories.Add(typeof(TComponent), new PrefabFactory<TComponent, TView>(prefab));
    }

    public GameObject BuildUI<TComponent>(TComponent component) where TComponent : Components.Component
    {
        var factory = componentFactories[typeof(TComponent)];

        return factory.BuildComponent(component, this);
    }

    public GameObject BuildUI<TComponent>(TComponent component, RectTransform parent) where TComponent : Components.Component
    {
        var factory = componentFactories[typeof(TComponent)];

        return factory.BuildComponent(component, this, parent);
    }
}

Here's a typical usage scenario showing the full API. It sets up the prefab factory by registering prefabs to different component types, and then builds a small panel with a vertical stack of buttons on it. Finally, it passes this stack of buttons into the UIBuilder instance, which instantiates a number of prefabs for the panel, stack content holder and the buttons.

[SerializeField]
    PanelView panelPrefab;

    [SerializeField]
    ButtonView buttonPrefab;

    [SerializeField]
    StackContentView stackPrefab;

    // Use this for initialization
    void Start () {
        var builder = new UIBuilder.Views.Factories.UIFactory();
        builder.RegisterPrefabFactory<Panel, PanelView>(panelPrefab);
        builder.RegisterPrefabFactory<Button, ButtonView>(buttonPrefab);
        builder.RegisterPrefabFactory<StackContent, StackContentView>(stackPrefab);

        builder.BuildUI(CreatePanel(), GetComponent<RectTransform>());
    }

(where CreatePanel() returns a PanelComponent containing a stack of buttons)

This is all working fine and well, but I wonder if there's a better to implement RegisterPrefabFactory to reduce the need to explicitly specify the generics.

Additionally, I'd love a general review of the code shown here to see if there's room for improvement.

Please note that I am restricted to C# 6 for this project and cannot use Nuget packages. This is because the Unity engine requires specific subsets of the .NET framework that no Nuget packages support.

\$\endgroup\$
  • \$\begingroup\$ I see this is your second bounty on this question already ;-) but I find it's very hard to review your code because you didn't tell us what each class is doing exactly :-( There are some general ideas but details are missing. I also don't really know what your goal is or what problem this code solves. So maybe you culd write something more about why you are doing what you are doing? I am also not sure what you mean by Prefab. Is this some pattern or an abbreviation for something? It looks like prefab stands for some template but as I've said... not sure of that either. More details please;-) \$\endgroup\$ – t3chb0t Jan 28 '18 at 17:25
  • \$\begingroup\$ I'd also like to know why you cannot use Nuget packages? Do you mean you cannot access internet or you cannot use any external libraries... because you know... you can extract a package and use a dll anyway or download it and install from a local source. \$\endgroup\$ – t3chb0t Jan 28 '18 at 17:26
  • \$\begingroup\$ @t3chb0t will do, thank you. I can't use NuGet packages because Unity uses a special .NET subset (i.e similarly to how Silverlight is not the same as vanilla .NET 3.5) that no NuGet packages support. \$\endgroup\$ – Nick Udell Jan 29 '18 at 10:03
  • \$\begingroup\$ So components are your own 'logical' UI elements, and views are their (Unity3D-specific) visual representation? \$\endgroup\$ – Pieter Witvoet Jan 30 '18 at 16:50
  • \$\begingroup\$ Yes that's correct. I handle the Unity "sack of scripts" that each view object is made up of by keeping a reference to a "view" script which I consider to be the canonical reference for that object. \$\endgroup\$ – Nick Udell Jan 31 '18 at 10:14
3
+100
\$\begingroup\$

Your code mixes a factory and a builder by not using a consistent vocabulary which makes it confusing because one is not sure what it actually is.

The main class is called PrefabFactory but instead of having such methods as CreatePrefab it uses BuildComponent. As a matter of fact it should be a GameObjectFactory because it creates GameObjects and not Prefabs.

Prefab is a template so call it GameObjectTemplate. Naming might look like a minor issue but it's one of the most important things. Using the right names makes the code self-documenting and you don't have write lenghty explanations. It also influences how you think about the class and lets you better focus on its actuall purpose.

When we take a look at its methods, we see that each of its BuildComponent methods requires a UIFactory. To me it looks like it should be specified only once via the constructor and be one of the dependencies.

Of the four BuildComponent methods only two of them use the generic argument. The other two work with a pure Components.Component. It doesn't look like a good idea. Why would one create a concrete factory with an exact generic argument and then allow other types anyway without even checking if they are of the requried type?

Another thing that I find very confusing is that the UIFactory is a dependency of the PrefabFactory and vice versa because the UIFactory depends on it here where it creates the PrefabFactory.

public void RegisterPrefabFactory<TComponent, TView>(TView prefab)
  where TView : UIView<TComponent>
  where TComponent : Components.Component
{
  componentFactories.Add(typeof(TComponent), new PrefabFactory<TComponent, TView>(prefab));
}

But I prefer to avoid such circular dependencies without a layer of abstraction between them because it makes testing much more difficult.

On the other hand if a UIFactory already knows the PrefabFactory<..,..> it's a little bit strange that the BuildComponent methods still requires a UIFactory as a parameter. There could be a property on the interface for accessing or you could already pass this to the new instance of PrefabFactory and remove it from its methods.


Due to many confusing names (builder or factory?) and no single clear dependency structure I suggest cleaning this up first (probably by completely rewriting it).

\$\endgroup\$
  • \$\begingroup\$ Thanks for taking a detailed look at the API! The Prefab term was meant to refer to the fact it uses prefabs but you're right, PrefabFactory implies it makes prefabs when actually it uses prefabs, so that definitely needs renaming. I also see your point about the circular reference, I'd added RegisterPrefabFactory as a convenience method, but perhaps it would be better done as an extension method, it really does confuse matters and probably doesn't belong in the canonical UIFactory object. \$\endgroup\$ – Nick Udell Feb 5 '18 at 9:05
  • \$\begingroup\$ As for the four BuildComponent methods, that's a very good question. I think I was having difficulties coercing the code into accepting a call when an object was stored as type Component but I knew explicitly at that point that it was a specific TComponent that matched the builder. Perhaps instead I should re-examine why I don't store the explicit type at that point. \$\endgroup\$ – Nick Udell Feb 5 '18 at 9:07
2
\$\begingroup\$

UIView<TModel>

  • Because the methods SetMargin() and SetPosition() are stateless, meaning they don't manipulate any class level variables directly (well, there aren't any), you could make them static.

  • in the SetPosition() method there seems to be either a typo or the line rectTransform.anchorMax = positioning.Anchors.TopRight; could be removed because you are overwriting rectTransform.anchorMax on the next line.

  • in the Bind() method you could discard the var rectTransform = GetComponent<RectTransform>(); if both model.Positioning and model.Margin are null but that only makes sense if GetComponent<RectTransform>() would be expensive.

    Because this is a public method you should have some parameter validation. Well, at least a null check.

UIFactory

  • If one forgets to call RegisterPrefabFactory<TComponent, TView>() before a call to BuildUI() a nasty KeyNotFoundException will be thrown.

Other than the mentioned points you have done well.

  • The code itself is easy to read
  • You use readonly when possible
  • You name your stuff well
  • You are using braces everywhere
  • The methods are quite small.

Just like I like to see it.

\$\endgroup\$
  • \$\begingroup\$ For the typo, that is actually a bug I later fixed, but must have forgotten to update the code here (oops!). Also GetComponent is typically expensive, you're right, so reducing calls to that that is a good idea. \$\endgroup\$ – Nick Udell Feb 5 '18 at 8:58

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.