This project is read-only.

Object model

May 24, 2013 at 2:08 PM
What do you think about reworking the object model a bit? While the singleton approach solved the caching issue, it's a little cumbersome to use.

I actually wrote a simple wrapper around the API that implements caching and dropped it on GitHub (https://github.com/Amonn/GW2API). In that code I just hung everything off a top-level class, GW2APICaller. It instantiates the objects and holds onto them, so singletons aren't needed. Also it makes usage very concise, because everything is exposed from a single object (https://github.com/Amonn/GW2API/wiki/Examples).

I'd love it if we could do something similar in this project to make it a little easier to use. Thoughts?

Also, regarding the event name dictionary, we can't get rid of it without hurting performance. We could get rid of the EventName class and just directly populate the dictionary with Guid/String pairs that we deserialize from the json response somehow. However, that would mean that someone using our object model can't get an EventName object if they want one.

This brings up another question about the object model. The API provides a way for the caller to list the names and IDs of all existing events. Do we intend to hide that functionality in our object model, and only allow the caller to see event objects that include the state of the event as well?

Also, what about map names and world names? They come back from the same type of call - we have event_names.json, map_names.json, and world_names.json, and each returns an ID/string pair. If we eliminate EventName but keep MapName and WorldName (actually it's called Map and World in the current code), that seems a little inconsistent. For that matter, maybe we should rename Map and World to MapName and WorldName for consistency.
May 24, 2013 at 4:59 PM
Edited May 24, 2013 at 6:27 PM
Ok, let's start at the bottom of the post and work upwards.
  1. It is called map and world because GwEvent was just named Event at the beginning, this resulted in some problems with the .Net Event class, thus the name change. For consistency Map and World should be called GwMap and GwWorld respectively.
  2. I threw the map and world together because they are closely tied together. Yes events are at least as closely tied to a map as the map is to a world, so it may be a good solution to refactor the maps out of the WorldData class.
  3. I don't see any implementation where only the event names and Id's would be required. Take this thought from an MVVm approach. An user has a application there he selects the world and then the map. Then the application spits him a list of all events, so far so good if we would stop here the the EventNames model would still be required. However most users don't want to stop here, they probably want to display additional informations about the event, meaning either we have to make an additional call to the API or we have the data already in our cache. The first would still require the EventNames struct, the second not. I strongly prefer the second method because it drops one Model for an additional bit of code elsewhere and it has one call less to the API.
    Also in MVVm (which is the de-facto standard in c#/.Net UI-coding) makes is totally easy to bind to the full model and then retrieve only parts of it.
    So if we would write the code so that the user has to make only one call and then gets a list of events with every available information, this would be the user friendliest approach.
    tl;dr; Remove the EventName class for public use and return a list of events with all possible informations. This is imho the best solution for the end user.
  4. I looked at your code and if I understood it correctly you create an instance of the GW2APICaller class which then creates and manages the instances of all other classes. The end user cannot create an instance of the different "sub-classes" (such as the Events class) so the caching is efficient.
    I like it that way, so I propose the following structure:
    1. We have three different "top-level" classes. One for the WorldData (events, worlds and maps), one for WvW and one for the items.
    2. These top level classes create and manage the sub level classes, e.g. The WorldData class creates an instance of the EventManager class which in return provides methods to call the api and return the events.
    3. The top level classes have public properties which are used to access the cached content.
    4. The user cannot access the sub-classes directly, everything is routed though the top-level class.
How does that sound?



/edit: To make it more clearly I have written a quick example (not tested if it works, but it should get the idea I have)
using System.Collections.Generic;

using GW2DotNET.V1.World.Models;

namespace GW2DotNET.V1.World
{
    public enum Language
    {
        en,
        fr,
        de,
        es
    }

    public class WorldManager
    {
        private static readonly WorldManager instance = new WorldManager();

        private WorldData worldData;

        private IList<GwWorld> worlds;

        /// <summary>
        /// Prevents a default instance of the <see cref="WorldManager"/> class from being created. 
        /// </summary>
        private WorldManager()
        {
            this.worldData = new WorldData();
        }

        public static WorldManager Instance
        {
            get
            {
                return WorldManager.instance;
            }
        }

        public IList<GwWorld> Worlds
        {
            get
            {
                if (this.worlds == null)
                {
                    worldData.GetWorlds(this.Language.ToString());
                }

                return this.worlds;
            }
        }

        public Language Language
        {
            get;
            set;
        }
    }
}
using System.Collections.Generic;

using GW2DotNET.V1.Infrastructure;
using GW2DotNET.V1.World.Models;

namespace GW2DotNET.V1.World
{
    internal class WorldData
    {
        internal IList<GwWorld> GetWorlds(string language)
        {
            var arguments = new List<KeyValuePair<string, object>>
            {
                new KeyValuePair<string, object>("lang", language)
            };

            return ApiCall.CallApi<List<GwWorld>>("world_names.json", arguments);
        }
    }
}
Usage scenario User creates an instance of the WorldManager Class and wants to get the Maps. Therefore he calls the Maps property (which is null at this point). The Property then calls the GetWorlds method on the WorldData class which returns a list of worlds.
May 24, 2013 at 6:17 PM
Yep you understood my code perfectly. The approach you describe sounds pretty good. However, with two separate top-level classes for WorldData and WvW, we'll have to cache the world_names.json result in both. WvW has a dependency on world_names because wvw/matches.json describes each match in terms of world_id.

Of course, the world names list is small, so calling and caching it twice isn't a huge deal. And that will only happen in the scenario where the user actually uses both top-level classes.

Thanks for the other answers. That helps me understand where you want to go with this project. The intent is definitely to abstract away the GW2 API itself, creating an object model that may look a bit different than the raw API.
May 24, 2013 at 7:52 PM
Edited May 24, 2013 at 7:55 PM
Yes going away a little bit was my idea. I personally are a UI coder first (I taught it myself that way) and have the UI usability in mind. And for that the API is sometimes a little cumbersome. (e.g. separating event_names and the event list itself blargh)

I did some initial coding in a different namespace (Gw2DotNet.V1 namespace) of the project. I'll move some types over and then upload the content so you get the code. I'll also start to write some usage information in the documentation page on this project.
Moving the implementation in another namespace also makes updating more easy. Should ANet release a v2 of their API we can code in another namespace and declare the v1 classes as obsolete, thus giving the user the info that he should update and not forcibly break his code.
May 24, 2013 at 8:07 PM
Wait, I spy a singleton!
   private static readonly WorldManager instance = new WorldManager();
...
    public static WorldManager Instance
    {
        get
        {
            return WorldManager.instance;
        }
    }
I'm trying to get rid of the singletons, which I admit I introduced. :)

I think we should basically rely on the caller to instantiate a single WorldManager and keep using it. As long as the caller does that, the data is cached. If the caller keeps instantiating a new WorldManager every time they want to do something, then yeah, we do tons of unnecessary API hits, but really that's a problem with the caller, not our object model.

In other words, I'm proposing something more like:
namespace GW2DotNET.V1.World
{
    public enum Language
    {
        en,
        fr,
        de,
        es
    }

    private Language language;

    public class WorldManager
    {
        public class WorldManager(Language language)
        {
            this.language = language;
        }

        private IList<GwWorld> worlds;

        public IList<GwWorld> Worlds
        {
            get
            {
                if (this.worlds == null)
                {
                    worldData.GetWorlds(this.Language.ToString());
                }

                return this.worlds;
            }
        }
    }
}
We should simply document our intent that the caller hangs onto his WorldManager instead of repeatedly instantiating new ones. Also, we should prevent changing the language since that invalidates the cache. If they want a different language, they can instantiate a new WorldManager with that language.
May 24, 2013 at 9:00 PM
OK, I'll remove the singleton from the code. However I will leave the user to change the language (and the world for that matter) and null the cache fields. From an UI point of view this is the better solution as I can bind the e.g. ComboBox.SelectedItem property via TwoWay binding to the Language and if the user changes the language the cache is emptied and as soon as the user requests the data again it is again filled.
May 25, 2013 at 1:01 AM
Ok, done for the most part. The event_names.json return game me a mild headache but I think I have it solve reasonably (of course I will look into it in a later stage and try to optimize it).
Now moving to finish the rest as you currently can change the language and the world but the cache is not nulled.
May 25, 2013 at 4:37 AM
There are some issues with the new V1 object model.

First, WorldManager is caching GwEvent objects for a particular world. However, it doesn't actually make sense to do this. A GwEvent contains event status information, which will change constantly, so caching is not what we want to do here. The caller will likely be calling this repeatedly to see if anything has changed. To address this, I removed the Events property as well as the World property from WorldManager, and the associated test.

What we DO want to cache is the event_names response that EventData is retrieving. As it's coded right now, every time the caller calls GetEvents to see the status of the events, we're going to get the entire list of event names again so we can map the names all over again. To address this I added an internal property that handles the event names list and moved the code that retrieves it into that property getter.
May 25, 2013 at 5:01 AM
Also, rather than having WorldManager call the Data class (such as MapData) and then cache the result in WorldManager as an IList<T>, I'm changing the Data classes to implement the IList<T> interface. This way, all WorldManager has to do is return the class instance to the caller, and we can keep the caching implementation within the class itself, keeping the top-level class nice and clean. This way, we can also implement all method calls in the class that will actually handle them.

The caller still has to go through WorldManager to get to the class, but all the implementation and caching stays in the class. I'll be checking in these changes shortly.
May 25, 2013 at 6:31 AM
OK, I'm pretty happy with how the object model is looking now. I think things are getting much more intuitive:
        GW2DotNET.V1.World.WorldManager wm = new GW2DotNET.V1.World.WorldManager();
        // Find my server by name
        var world = wm.Worlds["Tarnished Coast"];
        // Get the events for my server
        var eventsForMyWorld = wm.Events.GetEvents(world);
How cool is that?

One thing is bugging me. Do we need to return Worlds and Maps as an IList, or would an IEnumerable be good enough? There are two main reasons I'd like to simplify to IEnumerable. 1) It means less implementation in the Data classes. 2) It means I could replace the int indexer with an ID-based lookup, just like the name lookup you see in the example above. I think that would be much more useful than all this IList stuff. The caller can shove the IEnumerable into a List if they really want to anyway.
May 25, 2013 at 12:04 PM
Edited May 25, 2013 at 2:06 PM
At first everything returned an IEnumerable<T>, then I changed it to IList<T> because of some hickupI got while coding. Look here and here for reasons.

Still I'll go over them and see if it wouldn't be better to return an IEnumerable<T>. The thing is, with LINQ there are practically no differences in handling those two.

For your second post. The thing I want to do on the long term is to replace the int based id fields with references to the map and world model so that the event model looks more like this:
public struct GwEvent
{
    public GwWorld World
    {
        get;
        private set;
    }
}
instead of this:
public struct GwEvent
{
    public int World
    {
        get;
        private set;
    }
}
But this is on the long bench not something I'll do in the next few days.


Still we have a little problem here:
I can't request a list of maps, I have to pass a map id to the function and I'll get one map in return. This is not suitable for a ui application which uses data binding.

In an application which uses databinding the coder binds the control to a property which implements INotifyPropertyChanged (the latter is of no concern to us right now). However If I have a ListBox which displays a list of items and not – say a text box which only displays one item – I need a property which returns a collection of items.

So I'm going to revert or change some of the things you did to accomplish that.
May 26, 2013 at 1:08 AM
Edited May 26, 2013 at 1:08 AM
Actually, you CAN request a list of Maps.
var maps = wm.Maps;
This is because MapData itself implements IEnumerable, plus it implements two-way lookups. So right now both Maps and Worlds supports all three of these operations:
var allWorlds = wm.Worlds;
var oneWorld = wm.Worlds["Tarnished Coast"];
var otherWorld = wm.Worlds[1017];
At least, that's how I have it set up right now. I think this covers all possible scenarios, while keeping the code extremely concise.
May 26, 2013 at 1:26 AM
After reading those two links, I'm pretty sure we should be returning an IEnumerable<> rather than an IList<>, because we do NOT want to allow the caller to insert and remove things from the list. There's no need to expose those operations.