last day (17 days later) » 

Jez
Jez
18:32
test
worked
we can use the wpf room np
might be useful for someone
@Jez you have write access now
push away
Jez
Jez
just adding it
I'm curious about the reason for not using a framework/library for di.
Only reason I can think of is that it makes the code a bit more difficult which is a slight smell
Jez
Jez
basically
hmm
it wants a webapi to connect to
Jez
Jez
18:42
??
no my code
oh
Been many years since I used httpclient
    public class AssetSyncService
    {
        private readonly IAssetSyncServiceClient client;

        public AssetSyncService(IAssetSyncServiceClient client)
        {
            this.client = client;
        }
    }
^ is what I meant with passing the client interface
nothing fancy at all, standard ctor injection
makes it very explicit and easy to test
Jez
Jez
ok i pushed some code
with roughly what i'm doing now
the idea is that it would be connecting to a web API and api/ping should return a 204 to indicate a successful ping
^ suggestion, set NamespaceProvider to false for small folders like that
Jez
Jez
what ver of VS are you using
2015
var response = await _client.GetAsync("api/ping").ConfigureAwait(false);
Jez
Jez
18:53
interesting, i dont get "namespace provider"
use ConfigureAwait(false) there
@Jez did you select a project folder?
Has been there since 2010 I think
Jez
Jez
select a project folder?
yes, hold on I'll make a gif
Jez
Jez
why ConfigureAwait(false)? (i find the documentation for this stuff is pretty bad compared to the older .NET framework)
@Jez yes it is a wart in async
the default should be false
Jez
Jez
18:57
@JohanLarsson namespace provider doesnt appeaar for me
@JohanLarsson i'd have thought marshaling back to the original context was a good idea
If you do ConfigureAwait(true) or omit it the calling synchronizationcontext is captured and execution is scheduled back to it after the await
@Jez You should never do it in library methods
You only want to do it if you are touching UI elements after
Jez
Jez
what harm does it do?
Can easily deadlock
Also not good for performance
Jez
Jez
anyway we are touching UI elements after
maybe you do PingServiceAsync() and then update a label
not in the ping method
@Jez yes but then you do:
await srvice.PingServiceAsync().ConfigureAwait(true);
label.Content = "active";
inside the PingServiceAsync method you should have ConfigureAwait(false)
Jez
Jez
19:02
as for easily deadlocking, well, that rather blows out of the water the claim that the new async/await make async programming easier!
Try:
service.PingServiceAsync().Wait();
label.Content = "active";
^ should deadlock without ConfigureAwait(false)inside the PingServiceAsync method
We can skip this you can watch some talks about async, ping me if you find one not telling you to ConfigureAwait(false) inside library methods
Jez
Jez
ok
looking at the factory now
    public static class AssetSyncServiceClientFactory
    {
        private static readonly Lazy<IAssetSyncServiceClient> _instance = new Lazy<IAssetSyncServiceClient>(() => new AssetSyncServiceClient());

        /// <summary>
        /// Gets the AssetSync service client singleton; creates lazily if necessary.
        /// </summary>
        /// <returns>The AssetSync service client singleton.</returns>
        public static IAssetSyncServiceClient GetAssetSyncServiceClient()
        {
            return _instance.Value;
I'd write it like ^
thread safe
Jez
Jez
why isnt mine thread safe
Jez
Jez
19:08
ok well yours has the same principle
but with excessive locking
@Jez It is nice to make the field readonly with lazy
Jez
Jez
yeah
Also very simple overall
Jez
Jez
for some reason i always forget about the readonly keyword
and at the place i work now there is zero code review so no-one reminds me :-)
you could not use readonly with your implementation
@Jez we have nothing either
Jez
Jez
19:10
but the point is our implementations do the same
or the compiler checks my code :)
Jez
Jez
im more interest in the principle of the design. it's no longer a singleton
@Jez yep, was just a suggestion
@Jez which one?
both are singletons
Jez
Jez
AssetSyncServiceClient
both with lazy and your implementation
oh, yeah
Jez
Jez
19:11
I made the Exception specifically so that a different implementation of AssetSyncServiceClient could use it
because its errorType is an int instead of an enum so another implementation could add new values with its own enum
i've always thought Exception should have a better way of passing specific error codes back to calling code; i've often seen calling code parsing the Message string which is crazy
what if the error message changes a bit or changes language?
so i tend to put in custom error code functionality
@Jez yeah, smelly
you can subclass exception for that
Jez
Jez
of course i guess you could extend the class 1000 times for 1000 different things but that seems messy
public class JezException : Exception
{
}
Jez
Jez
i'd prefer an error code so yeah maybe i should've added a field rather than use the Data collection
yeah, I almost never do that
oh, sry saw your exception now
don't mind me
#region Private vars
private const string _defaultMsg = "There was a miscellaneous error with the AssetSync service client.";
private const int _assetSyncServiceClientErrorTypeOther = 0;
#endregion
Jez
Jez
19:15
i think i will change the error code to its own field
^ don't use regions there imo
Jez
Jez
why not
kills ctrl + m +o
Jez
Jez
i habitually put private fields and methods in a region
one of the nicest commands in vs, collapse all
Jez
Jez
19:16
what does that do, collapse everything
yeah, awesome for checking what a class does
Jez
Jez
i guess but it would seem to rule out any regions
maybe there should be a "collapse everything but regions" command
yeah, style choice not very important about regions
Jez
Jez
huh?
but I hate them with a passion
@Jez my nag about regions is not important
What is the reason you want the client as a singleton btw?
Jez
Jez
19:19
i want it to persist throughout the lifetime of the application
and for the same client to be used each call
yeah, it is a reasonable from perf caching point of view I think
Jez
Jez
and if the client has authenticated with a cookie it will presumably keep sending the cookie
@Jez is there a reason for this?
ah, +1
I suck at web
Jez
Jez
but also HttpClient is just designed to be persisted. it's not recommended to keep tearing it down as there's an overhead
yeah, gleaned that from an so answer
    private HttpClient _client {
        get {
            lock (_lock) {
                if (!_initialized) { throw new AssetSyncServiceClientException("HTTP client has not been initialized."); }
                return _instanceClient;
            }
        }
    }
^ looks strange
why not initialize it instead of throwing?
Jez
Jez
19:23
well that was one thing i was going to ask about your proposed solution
ok, firstly
oh, yeah we can't initialize there as we don't know the url
Jez
Jez
your solution takes settings in the ctor, meaning that the code creating the class has to know the settings
so im not sure how that works with a single instance
secondly having an Initialize method means you can reinitialize too, to set those to new settings
and thirdly, it throws because of the need for initialization before use
hold on I'll write a branch
Jez
Jez
(that's actually another annoyance; Microsoft don't like you throwing exceptions inside getters which i think is stupid. weirdly when i changed the exception i threw to my own exception, it stopped giving me a code style warning)
but MS say that properties are "smart fields" and shouldn't throw because the sort of "look like fields". bullshit. they look like properties and they are methods.
so according to MS i should technically Java-ize the code and have a GetInstance method
or GetClient
because throwing there is totally appropriate
19:45
@Jez sry about the delay I would write it like this
yes using IoC as it simplifies things so much
It felt like settings with url should should be in app.settings or somewhere
so you don't have to recompile if an url changes
kneejerked that one
Jez
Jez
oh Ninject was already in there
using IoC simplifies things so much there imo
I'll make a proper setting now
Jez
Jez
cant you just use a factory
pushed setting as proper setting
@Jez Yeah I guess but it requires more and complicated code
both for application and for testing
If you do ctor injection passing in a mock is trivial
moving back to master and writing a factory now
Jez
Jez
20:02
that factory is like 10 lines
that is more complicated than introducing a DI framework?
fair enough
but I don't see di framework as something to avoid, the opposite in fact
suggestion: default to sealed for types that implement IDisposable
Jez
Jez
I hate sealed
i would eliminate that keyword if i could
20:20
@Jez I pushed to master, a suggestion for the factory way
Move settings to a central place, not nice to hardcode urls where it is used
Nasty to have to fix in many places
Made the class private nested so only the factory can create it
Jez
Jez
argh
those "normalize line endings" boxes
i so desperately wish MS had just standardized on \n when they released windows NT
also, why the hell are they still defaulting to \r\n? literally the only text editor that can't deal with \n is notepad and that isn't even installed in win10
yeah, dunno why this repo has that issue
I dumbed something, dunno what
Jez
Jez
it shouldn't be a issue at all
NOBODY should EVER use \r\n
agreed
did you pull master?
Jez
Jez
yeah
20:33
perhaps a bit cleaner idk
Jez
Jez
i think i'd leave AssetSyncServiceClientErrorType in the same file as the client
removes the initialize and passing in url
@Jez ok, i always do type per file in c# but no biggie
Jez
Jez
i still don't really get how this works
i mean, the settings are hardcoded
and you only initialize it once in the ctor
in my application, the settings can be set in the UI and/or read from a file, and you may need to reinitialize each time settings are updated
oh, that requires different handling
Jez
Jez
also, what do you think about having PingServiceAsync in the same class as the HttpClient?
i'm thinking it's ok but i bet some would separate that (and all the other methods for various web api calls) into another class that calls the HttpClient wrapper
20:42
yeah, depends on how messy it gets
probably nice to have public class ApiService and ApiClient separate
moving things like that around is so nice if you use ioc
then you just change in constructors and get the new graph
Jez
Jez
i don't know why people still call it IoC
as i describe in this question
-3
Q: Should we still be calling it "inversion of control"?

JezIt seems to me that these days, good OOP design is basically predicated on inversion of control. Pretty much the first question you should be asking for any design pattern is "is the control inverted?" The thing should implement an interface, it should be instantiated elsewhere, it should have ...

@Jez programmers is an annoying site, closing all the things
Jez
Jez
it's a bit like "atheism"
yeah i know
nothing wrong with your question imo
Jez
Jez
virtually everything is offtopic there
20:47
I don't go there often
Jez
Jez
but IoC describes a design pattern as if it's somehow abnormal
"I'm inverting normal control"
I see what you mean
Jez
Jez
when 99% of your code is written under an IoC pattern, the "normal" flow of control is for code to be called by something else
i'd call it something like "controlled code", or use "dependency injection"
I don't have a better word so I use it as people recognize it
Jez
Jez
the term IoC is kind of fuzzy to me now, it means a lot of things
i mean, unless it's a console program that runs line-by-line it's basically got some inversion of control
virtually everything has it
services get called back
web APIs get called
20:49
upvoted your question for balance
Jez
Jez
IoC classes get called
GUI events get called
this is all "inversion of control"
it's hard to think of times when control isn't "inverted"
The framework part of IoC framework is also a bit of a misnomer imo
more library than framework imo
where library is much nicer than framework
Jez
Jez
yeah
framework is more wide-ranging
so it's a DI library
not an IoC framework
Jez
Jez
i'd be screwed without the .NET framework though...
Jez
Jez
anyway with the new requirements for the client
how would you change your design?
the way i see it, there needs to be an Initialize method, either in the client itself or the factory
but maybe i'm missing something
@Jez yeah, getting there now
there is no default that would be read from a setting?
Jez
Jez
no, the default is hardcoded
ok, I was lazy, starting to write something for dynamic url now
Jez
Jez
you mean the ability to reinitialize the BaseUrl of the httpclient
20:58
yeah
Jez
Jez
frankyl i'd just create a new HttpClient at the same time
(after closing)
.... which my original code does :-)
yeah sounds right
@Jez problem is that somewhere in the code someone may have cached a client
that will then be stale
this is where Service wrapping the client can be nice
Jez
Jez
cached a client?
there's only one client
say in mainwindow we do:
private IAssetSyncServiceClient _client = AssetSyncServiceClientFactory.Client;
then it would be dead after an url change
not a nice api as it is easy to get it wrong
Jez
Jez
no it wouldnt
it would use the new HttpClient
21:04
oh, yeah you are right
21:23
@Jez pushd something to master
not happy with it
SerialDisposable is nice though
not rock solid at all just proof of conecept
no validation on url for example
I still prefer using a di library
having the default in a setting is nicer than hardcoded
for some reason I picked _fieldName for this, my R# settings is fieldName and this.fieldName
probably missed in some places
Jez
Jez
trying to concentrate on this and sargon of akkad interview at the same time :-)
I've been fragmented aswell, didn't feel like coding tonight
Jez
Jez
then a belated Mother's Day tomorrow as it's the first time my mum and brother were free
21:41
coding on laptop sucks
so small screen
been writing in bed all night code probably looks like shit if viewed on a proper screen
Jez
Jez
heh

  last day (17 days later) »