Skip to content

Remove duplicate code for sending events. - #234

Open
SvenRosvall wants to merge 1 commit into
MessageBuilderfrom
NoVaArgs
Open

Remove duplicate code for sending events.#234
SvenRosvall wants to merge 1 commit into
MessageBuilderfrom
NoVaArgs

Conversation

@SvenRosvall

Copy link
Copy Markdown
Owner

Removed duplicate code after discussion with Martin.
Also replaced code that uses va_args as this seems to create larger codes.

@MartinDaCosta53 MartinDaCosta53 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A clever idea to determine opc by using the offset in the table, but I think it makes the code more opaque. The more I look at possible implementations of compacting the producer service, the more I think that there is little gain and at the expense of clarity, which is the reverse to what I'd been looking for when I voiced my thoughts on the video call.
Therefore, I think that we should leave this code as it is.

@SvenRosvall

Copy link
Copy Markdown
Owner Author

I agree that the function findEventOpCode() is not clear. Should be documented better and should have unit tests. Or maybe rewritten with nested switches and returning the actual OPC_XXX constants. But that would introduce tedious duplications again. :(

Is it just the findEventOpCode() function you don't like? Unfortunately it is crucial for deduplicating the send functions. I hope you liked the new send functions.

The actual program memory gains were smaller than I hoped. I'll break down these changes to measure their impact better.

@MartinDaCosta53

Copy link
Copy Markdown
Collaborator

Having pulled the file out to look at in isolation rather than one showing ommissions and additions from the previous version, it all becomes a lot less daunting. In fact, having looked at it over a couple of days, I rather like it. Just needs more comments to make it clear how and why it works.. I'd offer to do it but can't before the end of next week as I'm going away.

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