High level access

Developer
Oct 19, 2014 at 12:42 PM
Edited Oct 19, 2014 at 12:43 PM
Continued discussion from other work items (please read these first):
If we're ever going to pick a release that qualifies for version 1.0.0.0, it better comes with a damn well designed and user-friendly public API. This is simply not the case in any of our previous releases.

Basically, my question is this: how do you want to use our APIs?
  1. What should the code that you write look like?
  2. How do you want to be notified about errors?
  3. ... ???
Developer
Oct 21, 2014 at 7:45 PM
Alright, I'll go first.

Here's how I believe that code that you write should look like.

Image
Developer
Oct 22, 2014 at 10:43 PM
High Level Access
For me personally, I definitely like the idea of having access to the API via static classes. The API itself is a static entity. There is only 1 access point, so I don't see much value in having to instantiate a class to gain access to that API.

On the other hand, and this might be a minor point, having a static entry point reduces the testability of anything using the library. I don't know of any way to mock a static class for testing. This is something to consider, but I wouldn't say you shouldn't have a static class just for testability reasons.

Localization
TBH I haven't had to implement localization in a .NET app, so I'm kind of a noob in this area, but what you have proposed seems reasonable, although in reality I don't see myself making use of the ["<two-letter culture>"] interface. I would probably just always use the "ForCurrentUICulture" mentioned in the Localizable strings work item and then set the CurrentUICulture for my application as needed. Having options makes a lot of sense though.

Transport Errors
My thoughts on errors are as such: Only use exceptions for exceptional cases. You would probably then ask, what is an exceptional case? In my opinion, an exceptional case is any unexpected action that seldom occurs. Another justification for exceptions that I've seen is that exceptions should be thrown if a method cannot fulfill it's contract.

In the case of the GW2 API, I think it's reasonable for an exception to occur if something goes wrong accessing the API or if the API itself is messed up. Those seem like exceptional cases, or cases in which a method of the library is unable to fulfill it's contract.

When raising exceptions, the more detail available and passed with the exception the better, even if that means creating custom exception classes. Also, whatever exceptions might be thrown by a function of the library should be clearly documented with that library. I personally always check the xml documentation of the library I am using for exception details (<exception> tag for xml documentation).


Hope this helps. I won't pretend to be a master of these areas, but these are my thoughts right now.
Developer
Oct 23, 2014 at 2:01 PM
Very useful, thanks! One more thing: would you use a database solution that you did not design yourself? That is, if we provide the SQL scripts and code needed to persist API data to a local SQL Express/LocalDB instance, would you use it?

Somewhat related: my reasoning behind the indexed endpoint is so that you could easily do this:
foreach (var lang in new[] { "de", "en", "es", "fr" })
{
    var keys = new[] { 1, 2, 3 };
    foreach (var item in GW2.V2.Items[lang].FindAll(keys ))
    {
        MyDatabase.Save(item);
    }
}
I do agree that the interface is mostly useless for GUI applications that only use live data.
Developer
Oct 24, 2014 at 9:48 AM
shurne wrote:
When raising exceptions, the more detail available and passed with the exception the better, even if that means creating custom exception classes. Also, whatever exceptions might be thrown by a function of the library should be clearly documented with that library. I personally always check the xml documentation of the library I am using for exception details (<exception> tag for xml documentation).
I think we have 4 error conditions that we need to design for (not including errors caused by programming errors).
  1. Service errors at the API level (400 Bad Request due to invalid parameters)
  2. Service errors at the HTTP level (404 Not Found, 414 Request URI Too Long, ...)
  3. Service errors at the TCP level (Timeout)
  4. Client errors due to JSON mismatches (typically when the API is updated)
Right now, we only throw custom exceptions for the first and the second error condition. The code does not handle the other two cases, typically resulting in one of the following exceptions:
  • System.Net.WebException
  • System.Runtime.Serialization.SerializationException
  • Newtonsoft.Json.JsonSerializationException
The actual exception depends on which implementations of IServiceClient and ISerializer were injected. I realize now that this is a leaky abstraction. The code has to be updated to wrap exceptions in a custom exception for all of these error conditions.
Developer
Oct 26, 2014 at 12:55 PM
StevenLiekens wrote:
Very useful, thanks! One more thing: would you use a database solution that you did not design yourself? That is, if we provide the SQL scripts and code needed to persist API data to a local SQL Express/LocalDB instance, would you use it?
Maybe. Probably no, but if it was simple to use, then maybe. I would certainly consider it, but it's hard for me to say whether I would end up using it in the end.
Coordinator
Oct 26, 2014 at 3:37 PM
Personally I don't like accessing the library via static classes. I'm not a fan of singletons and I try to avoid them where I can. Is creating a new instance really that of a problem? Personally I don't think so.
On the other hand, and this might be a minor point, having a static entry point reduces the testability of anything using the library.
This is the other reason why I am against static classes. While you can test them, it is much more difficult.

I think The repository pattern is the way to go for us and we should persuade it. See the apge for more details, but here is a good summary:
  • You want to maximize the amount of code that can be tested with automation and to isolate the data layer to support unit testing.
  • You access the data source from many locations and want to apply centrally managed, consistent access rules and logic.
  • You want to implement and centralize a caching strategy for the data source.
  • You want to improve the code's maintainability and readability by separating business logic from data or service access logic.
  • You want to use business entities that are strongly typed so that you can identify problems at compile time instead of at run time.
  • You want to associate a behavior with the related data. For example, you want to calculate fields or enforce complex relationships or business rules between the data elements within an entity.
  • You want to apply a domain model to simplify complex business logic.
All of them (except maybe the last) apply to us in a certain way. Currently we have almost nil unit tests and this is making the development harder. With a repository pattern we could automate much of the testing process.
We also gain the data from a centralized resource -- the API. I don't think that the logic "The API itself is a static entity." applies. Think of the API as a database you access via web. You wouldn't access a DB via static classes in your code, wouldn't you? For me it is the same.
Additionally If we implement a (standard) caching solution (which would most likely be based on a DB solution) later on the query should be the same, but are we really going to do it via static classes?
How do you want to be notified about errors?
In my eyes it should be exceptions. And lot's of them. An exception in my eye is not simply for exceptional cases. For me an exception is the development of error codes. Exceptions give so much more than a simple error code. So why not use them.
I think of an exception more of a strongly typed fault code in the program and not an error, which is going to be raised in case of a C2D or similar problems.
Therefore in my opinion we should go with the Best Practices for Exceptions MS gave us.
TBH I haven't had to implement localization in a .NET app, so I'm kind of a noob in this area, but what you have proposed seems reasonable.
In my eyes it depends on the app you are developing. In ConsoleApps and WinForms it may be ok, but WPF gives us strongly typed localization options, so we have to include both ways. Also: magic strings are baaad! ;)
Developer
Oct 26, 2014 at 3:56 PM
Don't worry, the static GW2 class is just a thin wrapper that creates and configures instances of other classes. It has no significant logic of its own. The underlying code remains unit testable.

(TODO: actually start writing unit tests)
Developer
Oct 26, 2014 at 4:17 PM
Aren't magic strings only magic if they were chosen arbitrarily? Two-letter language codes are standardized by the ISO (see: ISO 639-1). The framework will throw an exception if you use any string other than the two-letter codes on that list.

Something like this will always throw an exception:
var items = GW2.V2.Items["qwewqrewqterttyu"].FindAll();
System.Globalization.CultureNotFoundException was unhandled
HResult=-2147024809
Message=Culture is not supported.
Parameter name: name
qwewqrewqterttyu is an invalid culture identifier.
Source=mscorlib
ParamName=name
InvalidCultureName=qwewqrewqterttyu
 
If you'd like, I can put in a code contracts rule that enforces valid input at compile-time (yay code contracts!).
Coordinator
Oct 26, 2014 at 7:25 PM
Aren't magic strings only magic if they were chosen arbitrarily?
Yes you are right on that point. However even if they are standardized by ISO, you can still mistype them. That's why I am such an advocate for strongly typed variables. I'm not saying we should throw it out of the window, but we should reduce strings wherever we can.
Developer
Oct 26, 2014 at 8:23 PM
Edited Oct 26, 2014 at 8:31 PM
It would have made more sense if they designed the lang parameter as a path segment.
  • /v1/{lang}/item_details.json
  • /v2/{lang}/items
  • ...
That way, the string value becomes part of the service contract in the same way that the "v1" and "v2" strings are part of the service contract and not just some magic strings. It wouldn't even matter if you passed a non-ISO value, because the service would return 404 instead of defaulting to English, thus triggering our error mechanism.
  • /v1/qwewqrewqterttyu/item_details.json
  • /v2/qwewqrewqterttyu/items
Backing up a bit... a properly designed API would actually catch invalid path segment and return 400 Bad Request instead of 404 Not Found. But I'm starting to think that Anet doesn't deal in proper designs (judging by their forum reactions anyway).
Coordinator
Oct 27, 2014 at 5:45 PM
Sadly this is probably the case. I guess we have to life with that and try to make our best of it. However we can deal in prober design and give the users what they expect per standard.

E.g. If they pass the wrong language parameter we return the proper exception the user expects, even if we have to write our own code for that. This is what I'd expect from a proper library.
Developer
Oct 31, 2014 at 9:55 AM
Edited Oct 31, 2014 at 9:57 AM
Quick progress update: I had a burst of inspiration that led me to standardize on the IRepository interface across all endpoints. We were already using repository classes for /v2, but /v1 implementations were entirely based on custom code for each endpoint. In the next release, most endpoints for both /v1 and /v2 will be based an IRepository. Exceptions to that rule include the /v1/build.json and /v2/commerce/exchange endpoints, because they provide computed instead of static data.

What, how?? /v1 does not support /v2 request syntax!!!

Yes, /v1 does not support all of the same capabilities, so an exception of type NotSupportedException is thrown for specific cases where the request is not supported. This behavior is documented on the IRepository interface. Your code should always handle NotSupportedException when you write code against the interface.

IRepository.cs: https://gw2dotnet.codeplex.com/SourceControl/latest#GW2.NET/Code/GW2NET.Core/Common/IRepository.cs

The goal of all this is?
  1. To provide a common public API
  2. To promote proper internal design that is needed to make (1) possible
  3. To get rid of leaky abstractions: each implementation (should) behave as documented on the interface. Most importantly, an implementation is not allowed to throw exceptions that are not documented on the interface.
  4. Testability
Right now, the only class that adheres to these new standards is the WorldRepository.

WorldRepository.cs (sorry about the regions... damn you ReSharper!): https://gw2dotnet.codeplex.com/SourceControl/latest#GW2.NET/Code/GW2NET.Core/V2/Worlds/WorldRepository.cs

I haven't had much time due to work. Today is Friday though. Expect more to follow soon.
Coordinator
Nov 1, 2014 at 10:09 PM
Yep, that's precisely what I meant to happen.
Developer
Nov 1, 2014 at 10:37 PM
Edited Nov 2, 2014 at 8:29 AM
I'm working on a FloorRepository now. The map_floor.json endpoint is particularly interesting, because it has 2 required parameters in addition to the optional lang parameter. How do you go about implementing a generic repository for that? It's easier if you think of the endpoint as a collection of 8 repositories: 4 repositories for each continent, 1 for each language.

FloorRepository[]
  • [0] Tyria, DE
  • [1] Tyria, EN
  • [2] Tyria, ES
  • [3] Tyria, FR
  • [4] Mists, DE
  • [5] Mists, EN
  • [6] Mists, ES
  • [7] Mists, FR
// Get floor details for all languages
foreach (string lang in new[] { "de", "en", "es", "fr" })
{
    // Get a continents repo for the current language
    IRepository<int, Continent> continents = GW2.V1.Continents[lang];

    // Find all continents and their localized names
    foreach (Continent continent in continents.FindAll().Values)
    {
        // Get a floors repo for the current language
        IRepository<int, Floor> floors = GW2.V1.Floors[continent.ContinentId, lang];

        // Find all floors and their localized details for the current continent
        foreach (var floorId in continent.FloorIds)
        {
            Floor floor = floors.Find(floorId);
        }
    }
}
This indexed property thing with multiple parameters is a bit unusual, but hopefully IntelliSense will make IntelliSense of it. (see what I did there?)
Developer
Nov 2, 2014 at 11:37 AM
Edited Nov 2, 2014 at 11:38 AM
Here's a fun little code sample that effortlessly downloads the entire items database in parallel. It runs in just over 30 seconds on my machine. Who said that multithreading is hard?
// Get some contextual information first (TODO: keep pushing Pat Cavit for HTTP HEAD support)
var context = GW2.V2.Items.Default.FindPage(pageSize: 200, pageIndex: 0);
var pageSize = context.PageSize;
var pageCount = context.PageCount;
var resultCount = context.TotalCount;

// Create buffers of the optimal size
var items = new List<Item>(resultCount);
var tasks = new List<Task<ICollectionPage<Item>>>(pageCount);

// Start a bunch of tasks
foreach (var lang in new[] { "de", "en", "es", "fr" })
{
    var itemRepository = GW2.V2.Items[lang];
    tasks.AddRange(itemRepository.FindAllPagesAsync(pageSize, pageCount));
}

//  While there are any tasks left, handle each task as soon as it completes
while (tasks.Any())
{
    Task.WhenAny(tasks).ContinueWith(
        task =>
        {
            var asyncResult = task.Result;

            // Remove the task from the tasks buffer
            tasks.Remove(asyncResult);

            // Add the task results to the items buffer
            items.AddRange(asyncResult.Result);
        }).Wait();
}

// Print each item name to the console (because why not)
foreach (var item in items)
{
    Console.WriteLine(item.Name);
}
Developer
Nov 3, 2014 at 6:28 PM
I was refactoring the ItemRepository, and I think that I accidentally came up with a design that could allow us to add new item types without having to re-compile everything. We already had a fallback UnknownItem type for unknown item type strings (duh), but there wasn't actually any way to tell the library which class to use for a particular type string. There's no reason why it should be like that, but I'm not sure if this is worth investigating further. New item types are only added when the game is updated, so it makes sense to force a library update as well.
Coordinator
Dec 2, 2014 at 2:39 PM
Another note on high level access design. While reading some LINQ articles again, something came to my mind. Why shouldn't we design our library like the HttpWebrequest class, so the user could do the following:
using (GW2ApiRequest request = new GWApiRequest.Create("items"))
{
    using (GW2ApiResponse response = request.GetResponse())
    {
        var items = request.Value;
        
        // Do some additional thingy's with the response
    }
}
This is a very simple example, which I took almost 1:1 from the HttpWebrequser/HttpWebresponse classes, but I think this is a good design which only allocates ressources when they are needed and uses the GC more efficiently through the use of using statements and an implementation of IDisposable.
Developer
Dec 2, 2014 at 9:44 PM
It's funny that you mention HttpWebRequest, because I'm working on my own version of that.
https://github.com/StevenLiekens/http-client

The System.Net implementation of the HTTP protocol is actually a really, really ugly implementation. I know that now, because I'm reading the protocol's specification, starting with the same RFC 2616 that was used for the .NET framework (RFC 2616 has been superseded in 2014).

I've gotten to a point where it seems to me as if Microsoft had an intern create the WebRequest API on a lazy Sunday, while he was probably watching the sports channel.

Note that the newer System.Net.Http.HttpClient is not much better. It is actually a wrapper for the same WebRequest and WebResponse classes that we're stuck with. Additionally, it accesses internal framework APIs that allows it to do super secret things with WebRequest that we can't do in our own code (not without using reflection). As an example, the HttpClient can send a WebRequest that does not trigger a WebException when the response status code is within the 4xx range.

In short, we have to move away from the WebRequest and WebResponse object model, not gravitate towards it. It already has a semi-deprecated status in .NET 4.5, given that many features were not even considered for Windows 8 style apps: http://msdn.microsoft.com/en-us/library/system.net.httpwebrequest(v=vs.110).aspx. Look for the green icon. How many can you count? Not that many.

Moving forward, I really want to further investigate the TPL Dataflow framework, and how we can use it to build a fantastic HTTP client library. TPL Dataflow is the perfect framework for building data pipelines, and HTTP is a protocol that SCREAMS to be pipelined (See: section 8 of RFC 2616). Why nobody has ever tried to combine these two concepts is beyond me.