current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite
1

I have a piece of code that I am not feeling comfortable.

  • I don't want to block the UI
  • Read operation works synchronously (reading may broke 50 ms rule)
  • Save operation supports async (returns Task)
  • I have to await SaveChanges (so operation could be completed before Dispose)

public Task AddToMyProject(string directory) {
    return Task.Run(async () => {
        if (!Directory.Exists(directory)) return;

        using (var context = _contextFactory.CreateContext()) {
            var existing = context.WatchDirectories.FirstOrDefault(wd => wd.Directory == directory);
            if (existing == null) {
                var childWatchers = context.WatchDirectories.Where(wd => wd.Directory.StartsWith(directory));
                foreach (var watchDirectory in childWatchers) {
                    context.Delete(watchDirectory);
                    RemoveDirectoryWatcher(watchDirectory.Directory);
                }

                context.Insert(new WatchDirectory { Directory = directory });
                await context.SaveChanges()
                    .ContinueWith(t => AddDirectoryWatcher(directory));
            }
        }
    });
}

I just want to know if there is a better approach for this.

share|improve this question
    
Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. –  Heslacher Feb 9 at 15:28

2 Answers 2

up vote 0 down vote 
await context.SaveChanges()
    .ContinueWith(t => AddDirectoryWatcher(directory));

This means that when SaveChanges() throws an exception, you're going to silently ignore it. Generally speaking, you shouldn't use ContinueWith() when you can use await:

await context.SaveChanges();
AddDirectoryWatcher(directory);
share|improve this answer
up vote 2 down vote

Instead of WatchDirectories.FirstOrDefault you can make use of Any() which is more meaningful.

Also if you are checking Any() you can return early and save horizontal spacing. 

public Task AddDirectoryWatcher(string directory) {
    return Task.Run(async () => {
        if (!Directory.Exists(directory)) return;

        using (var context = _contextFactory.CreateContext()) {
            if (context.WatchDirectories.Any(wd => wd.Directory == directory)) { return; };

            var childWatchers = context.WatchDirectories.Where(wd => wd.Directory.StartsWith(directory));

This AddDirectoryWatcher() is using a context to delete and insert items. Doesn't the RemoveDirectoryWatcher() method already uses a context too ? If yes, the call to context.Delete(watchDirectory); is redundant.

Instead of checking if the passed directory exists and silently returning , I would check this before calling the method and throw a DirectoryNotFoundException.

share|improve this answer
    
My real concerns were pitfalls of async lambda and Task.Run Etiquette and Proper Usage. But you are right about .Any usage, there used to be an UpdateTime property which is updated every time an object saved, I removed it but forgot to change the code, thanks. By the way, AddDirectoryWatcher and RemoveDirectoryWatcher are not using context, they just create/remove FileSystemWatcher objects. –  Umut Ozel Feb 9 at 11:54
    
I realized my code looks recursive, my mistake. I changed the method's name in the post, because it contained top secret information :) I fixed it now. –  Umut Ozel Feb 9 at 12:13
    
Hi @UmutOzel, if you liked this answer then maybe you can reward the poster with an upvote + accept –  janos♦ Feb 15 at 11:52
    
Hi @janos, sorry but this answer does not answer my questions about async/await usage. –  Umut Ozel Feb 15 at 12:43
    
@UmutOzel Fair enough, how about an upvote then? It seems it helped you. In any case, you don't have to, just a thought, feel free to ignore. –  janos♦ Feb 15 at 13:02

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.