3
\$\begingroup\$

Please assist and review the following task.
There are many geographic data to be found using the Bing Map server. It is necessary to collect data, parse bing response and store it in one of two files, depending on the result. I need to make it parallel as it could a lot of data. I do not like how I parallel the code, so I would be particularly grateful to the comments on the improvements. Below is code

using Newtonsoft.Json;
using System;  
using System.IO; 
using System.Collections.Generic;
using System.Linq;
using System.Net.Http;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using System.Collections;
using System.Diagnostics;

namespace BingGeoLocations
{
class Program
{
    private static string _folder = @"D:\TempFolder\";
    private static object _consoleLock = new object();
    private static object _successfileLock = new object();
    private static object _failurefileLock = new object();
    static void Main(string[] args)
    {
        var successGeoLocationCount = 0;
        var failedGeoLocationCount = 0;

        var allLocations = GetDocumentLocations().ToList();
        var allLocationsCount = allLocations.Count();

        var timer = new Stopwatch();
        timer.Start();

        var reportLocker = new object();
        Parallel.For(0, allLocations.Count,
            (i) =>
            {
                try
                {
                    var bingLocations = GetBingLocations(allLocations[i]);
                    Interlocked.Increment(ref successGeoLocationCount);
                    StoreSuccessResults(allLocations[i], bingLocations);
                }
                catch (Exception ex)
                {
                    Interlocked.Increment(ref failedGeoLocationCount);
                    StoreFailure(allLocations[i], ex.Message);
                }

                lock (reportLocker)
                {
                    Console.SetCursorPosition(0, Console.CursorTop);
                    Console.Write($"Processed {successGeoLocationCount + failedGeoLocationCount} locations out of { allLocationsCount}. Successful - {successGeoLocationCount}. Failed - {failedGeoLocationCount}");
                }
            });

        timer.Stop();

        Console.WriteLine();
        Console.WriteLine($"Total execution time - {timer.ElapsedMilliseconds}.");
        Console.ReadLine();
    }

    private static void StoreFailure(string docLocation, string failureDescription)
    {
        var failureFileName = "geography_failed.txt";
        if (docLocation != null)
        {
            var newInfo = new StringBuilder();
            newInfo.AppendLine(String.Join(";", new string[] { docLocation, failureDescription }));

            lock (_failurefileLock)
            {
                using (StreamWriter writer = File.AppendText(_folder + failureFileName))
                {
                    writer.Write(newInfo.ToString());
                }
            }
        }
    }

    private static void StoreSuccessResults(string docLocation, IEnumerable<BingLocation> bingLocations)
    {
        var successFileName = "geography_success.txt";
        if (docLocation != null && bingLocations != null && bingLocations.Count() > 0)
        {
            var newInfo = new StringBuilder();

            foreach (BingLocation bingLoc in bingLocations)
            {
                newInfo.AppendLine(String.Join(";", new string[] {
                    docLocation, bingLoc.CountryRegion, bingLoc.AdminDistrict, bingLoc.AdminDistrict2 }));
            }

            lock (_successfileLock)
            {
                using (StreamWriter writer = File.AppendText(_folder + successFileName))
                {
                    writer.Write(newInfo.ToString());
                }
            }
        }
    }

    static IEnumerable<string> GetDocumentLocations()
    {
        var fileName = "geography.txt";
        return File.ReadAllLines(fileName).Where(s => !String.IsNullOrWhiteSpace(s));
    }

    static IEnumerable<BingLocation> GetBingLocations(string docLocation)
    {
        var result = new List<BingLocation>();

        var bingKey = "MySecretBingKey";

        using (HttpClient client = new HttpClient())
        {
            var response = client.GetStringAsync("http://dev.virtualearth.net/REST/v1/Locations?q=" + Uri.EscapeDataString(docLocation) + "&c=en-US&maxResults=10&key=" + bingKey).Result;

            dynamic responseObject = JsonConvert.DeserializeObject(response);
            var statusCode = responseObject.statusCode;
            if (statusCode != "200")
            {
                throw new Exception("Status code is not 200.");
            }

            var highConfidenceResources = ((IEnumerable)responseObject.resourceSets[0].resources).Cast<dynamic>().Where(p => p.confidence.ToString().ToUpper() == "HIGH").ToList();
            if (highConfidenceResources.Count == 0)
            {
                throw new Exception("There are not High Confident results.");
            }

            foreach (dynamic res in highConfidenceResources)
            {
                var bingLocation = new BingLocation();
                bingLocation.AdminDistrict = res.address.adminDistrict;
                bingLocation.CountryRegion = res.address.countryRegion;

                if (res.address.adminDistrict2 != null)
                {
                    bingLocation.AdminDistrict2 = res.address.adminDistrict2;
                }
                else
                {
                    bingLocation.AdminDistrict2 = res.address.locality;
                }

                result.Add(bingLocation);
            }
        }

        return result;
    }

}
}

public class BingLocation
{
    public string CountryRegion;
    public string AdminDistrict;
    public string AdminDistrict2;
}
\$\endgroup\$
3
\$\begingroup\$

I would suggest using the Bing Maps JSON Data Contract classes to deserialize the response:

private static BingMapsRESTService.Common.JSON.Response DeserializeResponse(string json)
{
    using (var tempStream = new MemoryStream(Encoding.UTF8.GetBytes(json)))
    {
        var serializer = new DataContractJsonSerializer(typeof(BingMapsRESTService.Common.JSON.Response));
        var response = (BingMapsRESTService.Common.JSON.Response)serializer.ReadObject(tempStream);
        return response;
    }
}

I would also suggest using TPL Dataflow as it can increase concurrency by running steps in parallel with a few simple building blocks. Dataflow does include a bit of boilerplate setup code but it's easy implement and reorganize blocks.

Here's an dataflow implementation of your program:

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Net.Http;
using System.Runtime.Serialization.Json;
using System.Text;
using System.Threading.Tasks;
using System.Threading.Tasks.Dataflow;

namespace BingGeoLocations
{
    class Program
    {
        public static async Task Main(string[] args)
        {
            var locationsFileName = "geography.txt";
            var outputFolder = @"D:\TempFolder\";
            var successFileName = Path.Combine(outputFolder, "geography_success.txt");
            var failureFileName = Path.Combine(outputFolder, "geography_failed.txt");

            var bingLocationQueryOptions = new ExecutionDataflowBlockOptions
            {
                MaxDegreeOfParallelism = DataflowBlockOptions.Unbounded,
                BoundedCapacity = 1000, // Limits the memory usage
            };

            var queryBingLocationBlock = new TransformBlock<string, BingLocationQuery>(
                async location => await GetBingLocationsAsync(location),
                bingLocationQueryOptions
            );

            var processResultOptions = new ExecutionDataflowBlockOptions
            {
                MaxDegreeOfParallelism = 1, // Serialize so we can write to file/console
                BoundedCapacity = 500, // Limits the memory usage
            };

            var processSucceededQueryBlock = new TransformBlock<BingLocationQuery, BingLocationQuery>(
            result =>
            {
                File.AppendAllText(successFileName, result.ToResultString());
                return result;
            },
            processResultOptions
            );

            var processFailedQueryBlock = new TransformBlock<BingLocationQuery, BingLocationQuery>(
            result =>
            {
                File.AppendAllText(failureFileName, result.ToResultString());
                return result;
            },
            processResultOptions
            );

            var propagateCompletionOptions = new DataflowLinkOptions
            {
                PropagateCompletion = true,
            };

            queryBingLocationBlock.LinkTo(processSucceededQueryBlock, propagateCompletionOptions, query => query.Succeeded);
            queryBingLocationBlock.LinkTo(processFailedQueryBlock, propagateCompletionOptions, query => !query.Succeeded);

            var locations = File.ReadLines(locationsFileName)
                                .Where(l => !String.IsNullOrWhiteSpace(l))
                                .ToList();

            var totalQueries = locations.Count;
            var failedQueriesCount = 0;
            var succeededQueriesCount = 0;

            var outputQueryProgressBlock = new ActionBlock<BingLocationQuery>(
            result =>
            {
                if (result.Succeeded)
                {
                    succeededQueriesCount += 1;
                }
                else
                {
                    failedQueriesCount += 1;
                }

                Console.SetCursorPosition(Console.CursorLeft, Console.CursorTop);
                Console.Write($"Processed {succeededQueriesCount + failedQueriesCount} locations out of { totalQueries}. Successful - {succeededQueriesCount}. Failed - {failedQueriesCount}.");
            },
            processResultOptions
            );

            processSucceededQueryBlock.LinkTo(outputQueryProgressBlock);
            processFailedQueryBlock.LinkTo(outputQueryProgressBlock);

            // When completion goes from multiple block to a single block
            // we can't use link propagation and have to do it ourself.
            Task.WhenAll(processSucceededQueryBlock.Completion, processFailedQueryBlock.Completion).ContinueWith(_ => outputQueryProgressBlock.Complete());

            var timer = Stopwatch.StartNew();

            foreach (var location in locations)
            {
                await queryBingLocationBlock.SendAsync(location);
            }

            queryBingLocationBlock.Complete();

            await outputQueryProgressBlock.Completion;

            Console.WriteLine();
            Console.WriteLine($"Total execution time - {timer.ElapsedMilliseconds}.");
            Console.ReadLine();
        }

        internal sealed class BingLocation
        {
            public string CountryRegion { get; set; }
            public string AdminDistrict { get; set; }
            public string AdminDistrict2 { get; set; }
        }

        internal sealed class BingLocationException : Exception
        {
            public BingLocationException(string message)
                : base(message)
            { }
        }

        internal abstract class BingLocationQuery
        {
            public string Location { get; }
            public bool Succeeded { get; }

            protected BingLocationQuery(string location, bool succeeded)
            {
                if (string.IsNullOrWhiteSpace(location))
                {
                    throw new ArgumentException(nameof(location) + " cannot be null or white space.", nameof(location));
                }

                Location = location;
                Succeeded = succeeded;
            }

            public abstract string ToResultString();
        }

        internal sealed class BingLocationQuerySuccess : BingLocationQuery
        {
            public List<BingLocation> BingLocations { get; }

            public BingLocationQuerySuccess(string location, List<BingLocation> bingLocations)
                : base(location, succeeded: true)
            {
                BingLocations = bingLocations;
            }

            public override string ToResultString()
            {
                var resultStringBuilder = new StringBuilder();

                foreach (var bingLoc in BingLocations)
                {
                    resultStringBuilder
                        .Append(Location)
                        .Append(';')
                        .Append(bingLoc.CountryRegion)
                        .Append(';')
                        .Append(bingLoc.AdminDistrict)
                        .Append(';')
                        .Append(bingLoc.AdminDistrict2)
                        .AppendLine();
                }

                return resultStringBuilder.ToString();
            }
        }

        internal sealed class BingLocationQueryFailure : BingLocationQuery
        {
            public string FailureDescription { get; }

            public BingLocationQueryFailure(string location, string failureDescription)
                : base(location, succeeded: false)
            {
                FailureDescription = failureDescription;
            }

            public override string ToResultString()
            {
                var resultStringBuilder = new StringBuilder();

                resultStringBuilder
                    .Append(Location)
                    .Append(';')
                    .Append(FailureDescription);

                return resultStringBuilder.ToString();
            }
        }

        private static async Task<BingLocationQuery> GetBingLocationsAsync(string docLocation)
        {
            const string bingKey = "MySecretBingKey";
            using (var client = new HttpClient())
            {
                var jsonResponse = await client.GetStringAsync("https://dev.virtualearth.net/REST/v1/Locations?q=" + Uri.EscapeDataString(docLocation) + "&c=en-US&maxResults=10&key=" + bingKey);

                var response = DeserializeResponse(jsonResponse);

                var statusCode = response.StatusCode;
                if (statusCode != 200)
                {
                    return new BingLocationQueryFailure(docLocation, "Status code is not 200.");
                }

                var highConfidenceResources =
                    response.ResourceSets[0]
                            .Resources
                            .OfType<BingMapsRESTService.Common.JSON.Location>()
                            .Where(p => string.Equals(p.Confidence.ToString(), "HIGH", StringComparison.OrdinalIgnoreCase))
                            .ToList();

                if (highConfidenceResources.Count == 0)
                {
                    return new BingLocationQueryFailure(docLocation, "There are not High Confident results.");
                }

                var bingLocations = highConfidenceResources
                    .Select(res => new BingLocation
                    {
                        AdminDistrict = res.Address.AdminDistrict,
                        CountryRegion = res.Address.CountryRegion,
                        AdminDistrict2 = res.Address.AdminDistrict2 ?? res.Address.Locality,
                    })
                    .ToList();

                return new BingLocationQuerySuccess(docLocation, bingLocations);
            }
        }

        private static BingMapsRESTService.Common.JSON.Response DeserializeResponse(string json)
        {
            using (var tempStream = new MemoryStream(Encoding.UTF8.GetBytes(json)))
            {
                var serializer = new DataContractJsonSerializer(typeof(BingMapsRESTService.Common.JSON.Response));
                var response = (BingMapsRESTService.Common.JSON.Response)serializer.ReadObject(tempStream);
                return response;
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ also you suggestion about Bing Maps JSON Data Contract was useful ! Thank you again. \$\endgroup\$ – Disappointed May 10 '18 at 10:17
  • \$\begingroup\$ By the way, this version Console.SetCursorPosition(0, Console.CursorTop); is probably better as it does not pollute consol with a huge amount of records \$\endgroup\$ – Disappointed May 10 '18 at 10:35
  • \$\begingroup\$ Unfortunately there is an issue with your version, as not all data is processed. Like "Processed 59 locations out of 294. Successful - 54. Failed - 5." \$\endgroup\$ – Disappointed May 10 '18 at 11:14
3
\$\begingroup\$

You forgot to remove the _consoleLock. It's not used anymore. I assume it has been replaced by the reportLocker which has a smaller scope. A good thing of course. But the name doesn't match the convention of the other Locks.

You use Parallel.For to iterate over allLocations. You could use Parallel.ForEach instead.

You use the generic Exception type. Thus catching every possible exception in your try-catch-block. It would be advisable to create your own BingLocationRequestException. Thus making sure that any other kinds of exceptions bubble up instead of counting up your failedGeoLocationCount.

successGeoLocationCount doesn't match the naming convention of failedGeoLocationCount. successfulGeoLocationCount would be better.

You can use the Stopwatch.StartNew() static function to create a new Stopwatch and start it immediately.

StoreFailure(...) and StoreSuccessResult(...) should fail early. Instead of checking if docLocation is not null and continuing when true, check if it's null and return immediately. Thus reducing the indentation.

failureFileName and successFileName and 'fileName' can be static const.

Consider using File.AppendAllText to immediately append a string to a file referenced by it's path.

GetDocumentLocations() and GetBingLocations(...) are missing their access modifiers.

You are a tiny bit inconsistent with your use of var. In StoreSuccessResults(...) foreach loop for example, you specify the exact type of the elements.

The class BingLocation is not in the BingGeoLocations namespace.

I won't comment on the use of dynamic though. I assume you used a snippet for the HTTP request. Maybe using dynamic here is actually the right way to go. But in my experience, it's best to make damn sure it's really necessary to use it.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your useful remarks. You are right. Can you add something about parallel execution ? I dislike current version a lot. It does not look elegant however task is pretty common. What do you think ? I looked at TPL Dataflow but not sure it is ok for this solution ... \$\endgroup\$ – Disappointed May 8 '18 at 9:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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