5
\$\begingroup\$

I'm writing a service that receives a list of Latitude/Longitude objects, prepares them for transaction, and then calls a web service to write them to the organization's Mainframe. According to the existing application that I'm replacing, the Mainframe can only accept "chunks" of up to 5 Lat/Long values at a time, however the user is able to enter as many as they want.

After getting the list of Lat/Longs from the UI, if it is more than 5 pairs I break it into separate lists of 5, and then call another developers code to prepare them to transmit. This preparation includes setting up the transaction with the Case File's relevant information (FileType/FileNumber, Username, password, ect), calling SetCode() or SetMisc() on each individual Latitude and Longitude string, and then calling the web service. I do this for every chunk of 5 Lat/Longs, and if the last chunk is less than 5, I only call SetCode() or SetMisc() for however many there are left. There may also be fewer than 5 records to begin with, and often times will only be 1 Latitude/Longitude.

While my code works, I feel like it could be improved. Specifically, I don't care for all of the if checks, but I haven't been able to find another way to work it. I also feel like this is inefficient code, however this specific code only runs once per each case, so efficiency isn't absolutely vital.

public void PostLatLong(string fileType, string fileNumber, List<LatLongDEntry>     latLongList,  DateTime? transactionDate = null, string commentText1 = null, string commentText2 = null, string username = null, string password = null)
{
    string transactionCode = "LATLONGS";
    List<List<LatLongEntry>> splitList = new List<List<LatLongEntry>>();

    try
    {
        while (latLongList.Any())
        {
            splitList.Add(latLongList.Take(5).ToList());
            latLongList = latLongList.Skip(5).ToList();
        }

        foreach (List<LatLongEntry> list in splitList)
        {
            SetupCaseFileUpdateTransaction(transactionCode, fileType, fileNumber, commentText1, commentText2, transactionDate, username, password);
            SetCode(1, list[0].Latitude);
            SetCode(2, list[0].Longitude);
            if (list.Count > 1)
            {
                SetCode(4, list[1].Latitude);
                SetCode(5, list[1].Longitude);
            }
            if (list.Count > 2)
            {
                SetCode(6, list[2].Latitude);
                SetCode(7, list[2].Longitude);
            }
            if (list.Count > 3)
            {
                SetMisc(1, list[3].Latitude);
                SetMisc(2, list[3].Longitude);
            }
            if (list.Count > 4)
            {
                SetMisc(3, list[4].Latitude);
                SetMisc(4, list[4].Longitude);
            }
            CallWebService();
        }
    }
    catch (Exception e)
    {
        LogTransactionException(transactionCode, e);
        throw;
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ What's the logic behind when to call SetCode and SetMisc and their parameter values? \$\endgroup\$ Oct 24, 2016 at 7:26
  • \$\begingroup\$ This is not very efficient latLongList = latLongList.Skip(5).ToList(); \$\endgroup\$
    – paparazzo
    Oct 24, 2016 at 8:30
  • \$\begingroup\$ Why don't you SetCode(3, ...? You skip 3 for seemingly no reason. \$\endgroup\$
    – RobH
    Oct 24, 2016 at 11:01
  • \$\begingroup\$ @GertArnold Unfortunately I don't have an answer to that. The existing system sets the Code and Misc values in that fashion, so that is what I did. I don't have access to the inner workings of the Mainframe side of things. \$\endgroup\$ Oct 25, 2016 at 2:28
  • \$\begingroup\$ @RobH replied in a comment as well, but similar to above, the existing system skips it and so I did as well. Unfortunately I don't have any documentation to work off of. \$\endgroup\$ Oct 25, 2016 at 2:29

2 Answers 2

5
\$\begingroup\$

Branch Complexity

While my code works, I feel like it could be improved. Specifically, I don't care for all of the if checks, but I haven't been able to find another way to work it.

If you could come up with a smart pattern to reduce the number of if-statements, be my guest. I think some sub patterns can be found, but there would be too many exceptions to the rule. The complexity of mapping the LatLongEntry items has to be stored somewhere.

Command Pattern

Wouldn't you rather call..

foreach (List<LatLongEntry> list in splitList)
{
    SetupCaseFileUpdateTransaction(transactionCode, fileType, fileNumber,
        commentText1, commentText2, transactionDate, username, password);

    for (int i = 0; i < list.Count; i++)
    {
        commands[i].Execute(this, list[i]);
    }

    CallWebService();
}

than..

foreach (List<LatLongEntry> list in splitList)
{
    SetupCaseFileUpdateTransaction(transactionCode, fileType, fileNumber, 
        commentText1, commentText2, transactionDate, username, password);
    SetCode(1, list[0].Latitude);
    SetCode(2, list[0].Longitude);
    if (list.Count > 1)
    {
        SetCode(4, list[1].Latitude);
        SetCode(5, list[1].Longitude);
    }
    if (list.Count > 2)
    {
        SetCode(6, list[2].Latitude);
        SetCode(7, list[2].Longitude);
    }
    if (list.Count > 3)
    {
        SetMisc(1, list[3].Latitude);
        SetMisc(2, list[3].Longitude);
    }
    if (list.Count > 4)
    {
        SetMisc(3, list[4].Latitude);
        SetMisc(4, list[4].Longitude);
    }
    CallWebService();
}

Let's assert your class provides public methods SetCode and SetMisc.

// I don't know the class/interface of your container, but it doesn't really matter
interface IContainer
{
    void SetMisc(int index, double coordinate);
    void SetCode(int index, double coordinate);
}

We should provide a command pattern. A LatLongEntryCommand contains the indices for both the Latitude and Longitude and operations for posting updates.

class LatLongEntryCommand
{
    public int LatitudeIndex { get; }
    public int LongitudeIndex { get; }
    public Action<IContainer, int, double> LatitudeOperation { get; }
    public Action<IContainer, int, double> LongitudeOperation { get; }

    public LatLongEntryCommand(
        int latitudeIndex, 
        int longitudeIndex,
        Action<IContainer, int, double> latitudeOperation,
        Action<IContainer, int, double> longitudeOperation)
    {
        LatitudeIndex = latitudeIndex;
        LongitudeIndex = longitudeIndex;
        LatitudeOperation = latitudeOperation;
        LongitudeOperation = longitudeOperation;
    }

    public void Execute(IContainer container, LatLongEntry entry)
    {
        LatitudeOperation(container, LatitudeIndex, entry.Latitude);
        LongitudeOperation(container, LongitudeIndex, entry.Longitude);
    }
}

Your container class should implement a lookup map of commands once at initialisation. The complexity of the original if-statements is moved to this place. I do feel this is a somewhat cleaner implementation than the original code. But is it really worth it?.. up to you to decide.

// instantiate some local instance variable 'commands' at init

var setCode = new Action<IContainer, int, double>((c, i, l) => c.SetCode(i, l));
var setMisc = new Action<IContainer, int, double>((c, i, l) => c.SetMisc(i, l));

commands = new Dictionary<int, LatLongEntryCommand>
{
    { 0,  new LatLongEntryCommand(1, 2, setCode, setCode)},
    { 1,  new LatLongEntryCommand(4, 5, setCode, setCode)},
    { 2,  new LatLongEntryCommand(6, 7, setCode, setCode)},
    { 3,  new LatLongEntryCommand(1, 2, setMisc, setMisc)},
    { 4,  new LatLongEntryCommand(3, 4, setMisc, setMisc)}
};
\$\endgroup\$
6
\$\begingroup\$

Separate the mechanics and the semantics of the code. Here's my very well used Batch<T> extension method on IEnumerable<T>:

public static class EnumerableExtensions
{
    public static IEnumerable<IEnumerable<T>> Batch<T>(this IEnumerable<T> enumerable, int batchSize)
    {
        if (enumerable == null) throw new ArgumentNullException(nameof(enumerable));
        if (batchSize <= 0) throw new ArgumentOutOfRangeException(nameof(batchSize));
        return enumerable.BatchCore(batchSize);
    }

    private static IEnumerable<IEnumerable<T>> BatchCore<T>(this IEnumerable<T> enumerable, int batchSize)
    {
        var c = 0;
        var batch = new List<T>();
        foreach (var item in enumerable)
        {
            batch.Add(item);
            if (++c % batchSize == 0)
            {
                yield return batch;
                batch = new List<T>();
            }
        }
        if (batch.Count != 0)
        {
            yield return batch;
        }
    }
}

Now that's the batching taken care of, we need to sort out the loop. What would we like it to look like?

foreach (var batch in latLongList.Batch(5))
{
    SetupCaseFileUpdateTransaction(transactionCode, fileType, fileNumber, commentText1, commentText2, transactionDate, username, password);
    SetWebServiceBatch(batch.ToList());
    CallWebService();
}

Great! The main method is now much shorter and easier to reason about. However, we've just moved the problem elsewhere. So let's look at the SetWebServiceBatch piece:

// Typed directly into CR so may not compile/be 100% right.
private void SetWebServiceBatch(List<LatLongEntry> latLongs)
{
    if(latLongs.Count > 5) throw new ArgumentException(nameof(latLongs));
    for (var i = 0; i < latLongs.Count; i++)
    {
        if (i < 3)
        {
            SetCode(1 + i * 2, latLongs[i].Latitude);
            SetCode(2 + i * 2, latLongs[i].Longitude);
        }
        else
        {
            SetMisc(1 + (i - 3) * 2, latLongs[i].Latitude);
            SetMisc(2 + (i - 3) * 2, latLongs[i].Longitude);
        }
    }
}

Note that I've assumed that skipping SetCode(3 was a mistake and that you want to set code 1,2,3,4,5,6 and set misc 1,2,3,4. If that's not the case you'll need to slightly modify the for loop to take into account this discontinuity.

\$\endgroup\$
3
  • \$\begingroup\$ Excellent! Skipping SetCode (3) was just because the existing application does the same. Unfortunately, the mainframe side is sort of a black box to me so the only "documentation" is the existing code. \$\endgroup\$ Oct 24, 2016 at 16:26
  • \$\begingroup\$ @alaskanloops - that's annoying but all too common :( Do you want me to update the for loop to skip 3 or are you okay to do that? \$\endgroup\$
    – RobH
    Oct 25, 2016 at 7:38
  • \$\begingroup\$ Well it's been years and I'm just now seeing your comment. If I remember correctly I figured it out! \$\endgroup\$ Jul 20, 2023 at 22:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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