piwik-dotnet-tracker
piwik-dotnet-tracker copied to clipboard
Split sendRequest into sendRequest and buildRequest
This allows for modifications of the request object via overriding the buildRequest method in a customized PiwikTracker class and calling base.buildRequest() first.
This is a very basic change which can make a big improvement. Right now, if someone has to change the smallest setting on the request (i.e. enable windows authentication credentials, basic auth credentials, or anything else), he cannot use the complete implementation of the PiwikTracker.
A even more basic change would be to atleast make the sendRequest protected (as it is in the PHP implementation), but this would than require to re-implement the complete sendRequest for such small request object changes.
With the attached pull request, the following would be possible:
public class CustomPiwikTracker : PiwikTracker
{
public CustomPiwikTracker(int idSite, string apiUrl = "") : base(idSite, apiUrl)
{
}
protected override HttpWebRequest buildRequest(string url, string method = "GET", string data = null, bool force = false)
{
var request = base.buildRequest(url, method, data, force);
// change request
return request;
}
}
Hi Philipp, thanks for your contribution. Sorry for taking this long to review it.
We originally tried to make the C# version look as much as possible like the PHP version.
With a hindsight, it does not make a lot of sense.
I hope we will get more contributions like yours to make the code more modular and testable.
I am afraid if we merge your code as is that someday we'll forget for which use case this method was split.
Because we have yet to implement automated tests, see #7, would you mind adding a sample of request customization to the current set of samples?
That way, if someday we refactor the code, the samples won't compile any more and it will remind us why the method was split.
Would you also mind creating an issue? This way, when we'll release the next version, the changelog would be generated with your user story, see #22.