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

refactor WcfLogReceiverClientFacade, fix backwardscomp #784

Closed
304NotModified opened this issue Jul 1, 2015 · 3 comments
Closed

refactor WcfLogReceiverClientFacade, fix backwardscomp #784

304NotModified opened this issue Jul 1, 2015 · 3 comments
Labels
bug Bug report / Bug fix enhancement Improvement on existing feature must have
Milestone

Comments

@304NotModified
Copy link
Member

Fix backwardscompatiblity with 3.2

The change in NLog 4.0 was unwanted breaking. We need to fix this:

The problem:

  • WcfLogReceiverClientFacade won't inherit from ClientBase<...>. But in 3.2.0 WcfLogReceiverClient was returned, and this class did implement ClientBase<...> (which contains 'EndpointandClientCredentailsproperties). Thus: WcfLogReceiverClientFacade should inherit fromClientBase<..>`. But this is difficult to fix. We have to find a good solution for this.

See: #783 and #561

Refactor or remove WcfLogReceiverClientFacade

The class WcfLogReceiverClientFacade introduced in 4.0 (#565), contains unwanted stuff:

  • there is a lot of code duplication in the constructors
  • every property is implemented with the same if-else constructions
  • there are two properties, m_twoWayClient and m_oneWayClient and there is always only one used, hence, it should be one property.
  • there are some Hungarian names. e.g. m_oneWayClient should be without the prefix m_.
  • ILogReceiverClient and ILogReceiverOneWayClient are almost identical
  • WcfLogReceiverClient and WcfLogReceiverOneWayClient are code duplication?

Solutions:

  • We should clean this up and create one factory method with do the if (useOneWay) check. It should return an interface and only that interface should be used in the rest of the class.
  • Clean up code duplication as much as possible.

code: https://github.com/NLog/NLog/blob/master/src/NLog/LogReceiverService/WcfLogReceiverClientFacade.cs

@304NotModified 304NotModified added the enhancement Improvement on existing feature label Jul 1, 2015
@304NotModified 304NotModified changed the title WcfLogReceiverClientFacade; a lot of code smells. refactor WcfLogReceiverClientFacade, fix backwardscomp Jul 17, 2015
@304NotModified 304NotModified added the bug Bug report / Bug fix label Jul 17, 2015
@304NotModified 304NotModified added this to the 4.1 milestone Jul 17, 2015
@MartinTherriault
Copy link
Contributor

I just finished doing it in my fork. Can you take a look? I'm not sure how things should be named. I just used "V2" but I'll change it to something else if you want to.

I restarted from the original code and added the method to the client interface. For the server interface, I inherited the original interface and added the new method. The target kept its parameter to indicate which method to use.

There is however one possibility in which the changes could break old code, it is if someone was inheriting directly the client interface, they would have to implement the new method for the code to compile. I tried to make it work with two client interfaces, but I get errors with the .config file that I have not managed to fix yet.

@304NotModified
Copy link
Member Author

Great!! Just make a (new) PR please. That's easier to discuss.

@304NotModified
Copy link
Member Author

fixed by #874

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report / Bug fix enhancement Improvement on existing feature must have
Projects
None yet
Development

No branches or pull requests

2 participants