Let's go over the basics first!
The indentation is quite a mess. But I suppose that might be related to the SE formatting. Otherwise, you should make your indentation coherent. Stuff like :
public interface IGYDataAccess
{
void GetUserInfo(Action<GYUser, Exception> act, string id);
}
shouldn't be in production code, it looks bad and gives headaches to maintainer (that could be you!)
Why are your classes prefixed of GY
? Maybe there's a good reason, but I'm pretty sure your names would make just as much sense without that prefix. The pattern you're using for caching uses the Repository
design pattern. It abstracts the implementation to access to data. So that pattern name should be found in your naming! IRepository
, LocalDataRepository
, WebDataRepository
. Some might argue that it's useless, but I think it's good to see the pattern's name in the class name, so we know what we're dealing with.
Multi-line lambdas are... okay, I guess. But I think in your case they should be in a method. A lambda is an anonymous function, something that doesn't matter that much in your code. But that code is important, don't let it crawl in the shadows, that code deserves a method. (The lambda in GetUserInfo
)
Don't keep your connection string as a const
in your code, that's a very bad practice. It should be in a configuration file, and you should get it using the ConfigurationManager
class (In System.Configuration
).
private
members should be camelCased
, not PascalCased
, so DataBase -> dataBase
(In GYLocalData
). Also, you should specify the private
accessor, so I know it wasn't forgotten or something like that. See, if it's not there. I don't know your clear intent. Maybe you wanted this field to be public
but you forgot because you were out of coffee. I'll never know because you didn't specify it.
I want to give a little warning that might be unnecessary. The CheckDatabase
method isn't thread safe. That means if your GYLocalData
object can be called from multiple threads at once, you might create the database twice. And that's a costly operation. That's bad.
You should only use var
when you can figure the Type
by reading the code. In the example below, I can't figure it out, so it's harder to know what's happening and even harder to review! :)
var user = DataBaseProfiles.GetInfo(id);
You could use some Dependancy Injection in your GYWebData
class. You want to store everything that is took from the web API in a another storage. So the GYWebdata
class should receive in the constructor a GYLocalData
instance where you would store the information :
public class GYWebData : IGYDataAccess
{
private GYLocalData storage;
public GYWebData(GYLocalData storage)
{
this.storage = storage;
}
public void GetUserInfo(Action<GYUser, Exception> act, string id)
{
GYUserAPI.GetInfo(id, (result, error) =>
{
if (error != null)
{
act.Invoke(null, error);
return;
}
act.Invoke(result.Result, null);
storage.UpdateUserInfo(result.Result);
});
}
}
Now this isn't exactly Dependency Injection because I give a concrete class as parameter. You would need another interface, IDataUpdater
, maybe :
public interface IDataUpdater
{
void UpdateUserInfo(GYUser user);
}
The LocalData
class should implement that interface, and then :
public class GYWebData : IGYDataAccess
{
private IDataUpdater updater;
public GYWebData(IDataUpdater updater)
{
this.updater = updater;
}
public void GetUserInfo(Action<GYUser, Exception> act, string id)
{
GYUserAPI.GetInfo(id, (result, error) =>
{
if (error != null)
{
act.Invoke(null, error);
return;
}
act.Invoke(result.Result, null);
updater.UpdateUserInfo(result.Result);
});
}
}
Boom, you now have zero dependency between the local storage and the web storage.
Finally, looking at your workflow, you could use the Proxy
design pattern to help a bit. The Proxy would be used to get info from the web and cache it in the local storage. The proxy would be the only class you use for storage. I don't want to give it all to you, because you'll learn better by yourself! :) But know that the Proxy
is a good case for this!