Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Master/detail implementation for #23 #24

Merged
merged 4 commits into from
May 14, 2015

Conversation

petersondrew
Copy link
Contributor

This PR implements a solution for #23 and associated tests.

Please let me know if you'd like any changes to the API, it's pretty basic and I didn't give it a ton of thought.

@petersondrew
Copy link
Contributor Author

Here's a usage example:

// Init multi record file engine
var engine =
    EngineFactory.GetEngine(
        new[]
        {
            typeof (SettlementHeaderRecord),
            typeof (TransactionRecord),
            typeof (TransactionDetailRecord),
            typeof (SettlementTrailerRecord)
        },
        s =>
        {
            if (String.IsNullOrEmpty(s) || s.Length < 1) return null;
            switch (s[0])
            {
                case 'H':
                    return typeof(SettlementHeaderRecord);
                case 'D':
                    return typeof(TransactionRecord);
                case 'P':
                    return typeof(TransactionDetailRecord);
                case 'V':
                    return typeof(SettlementTrailerRecord);
            }
            return null;
        });
// Read file
using (var stream = File.OpenRead(file))
    engine.Read(stream);

var transactions = engine.GetRecords<TransactionRecord>().ToList();

// Get detail records
foreach (var detailRecord in transaction.DetailRecords.OfType<TransactionDetailRecord>())
    detailRecord.TransactionId = transaction.Id;

It's not quite as abstracted as the engine is (e.g. engine.GetRecords<T>), but it leaves less to implement when implementing IMasterRecord if you just use the OfType<T> extension method when accessing the detail records to pull the type you'd like to work with (if you have multiple detail record types).

@petersondrew petersondrew changed the title Master detail implementation for #23 Master/detail implementation for #23 May 14, 2015
@forcewake
Copy link
Owner

@petersondrew, it looks good.
But I have several questions/suggestions:

  • Could you please update the documentation as it will be lost after the merge
  • I'm not sure that I'm OK with the engine's initialization code:
// Init multi record file engine
var engine =
    EngineFactory.GetEngine(
        new[]
        {
            typeof (SettlementHeaderRecord),
            typeof (TransactionRecord),
            typeof (TransactionDetailRecord),
            typeof (SettlementTrailerRecord)
        },
        s =>
        {
            if (String.IsNullOrEmpty(s) || s.Length < 1) return null;
            switch (s[0])
            {
                case 'H':
                    return typeof(SettlementHeaderRecord);
                case 'D':
                    return typeof(TransactionRecord);
                case 'P':
                    return typeof(TransactionDetailRecord);
                case 'V':
                    return typeof(SettlementTrailerRecord);
            }
            return null;
        });

Let's me explain why:

  1. It's rather hard to read
  2. It's rather hard to extend in future
  3. When we will have a lot of engines we will have a lot of copy-paste code

I can merge you changes without any problems, but could you please spend some time on rethinking about the initialization (maybe some kind of the IEngineConfiguration or something else)?

Thank you for you great help.

Pavel.

@petersondrew
Copy link
Contributor Author

Thanks for the feedback @forcewake . Just an FYI - the engine code initialization hasn't changed since it was created 😉

As for coming up with some sort of engine configuration class, I think that's a good idea. Also note that you can pass a named function rather than an anonymous one for better code re-use on the initialization.

For this PR though, nothing has changed with the API for the multi-record engine. Just the support for the IMasterRecord/IDetailRecord stuff. Any thoughts on that part in particular?

@forcewake
Copy link
Owner

Any thoughts on that part in particular?

Yes, I will create another issue. Good idea. 👍

Just an FYI - the engine code initialization hasn't changed since it was created

My fault 😢
Nevertheless, it will be cool if can spend some time on this small issue.

I will merge and close this PR

Thank you for help.

forcewake added a commit that referenced this pull request May 14, 2015
Master/detail implementation for #23
@forcewake forcewake merged commit 0ae4a13 into forcewake:dev May 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants