shahriarhossain
Project Clean Up
Have removed the unused directives. If you see any typo or other such issue feel free to PR.
There is some good progress going on I would really recommend / like to see the following to clean up the code. I would recommend we start a new Branch for version 2.0 for these to take place as they would cause breaking changes. Open to a conversation on these items of course.
- Switch from 4 space tab stops to 2 space. This gives more code on the page to read as indention grows.
- A lot if not all methods seem to have 'Async' at the end. This seems unnecessary. Maybe the ones that are synchronous should be defined and declare that all methods are Asynchronous unless noted.
- Do you use ReSharper? It has a lot of good tips on cleaning up unused Directives, Naming Convention issues. It can automate some of these items.
- A lot of the methods seem like they could be static. For instance why do we need to create an MCList object to delete a list when we are calling DeleteList and passing in a the list_id to delete? Then to delete a list we would only need
MCList.DeleteList(xxx);instead ofMCList mcList= new MCList(); mcList.DeleteList(xxx); - Review all the method Names to makes sure they are consistent across the wrapper. Some other inconsistencies with MC & MailChimp used in class names.
- Of course creating some tests for all the methods at least basic ones to start.
- Make sure each class has the appropriate CRUD methods.
- Review enumerations. I added the code to convert then to string during serialization so that should be helpful instead of have properties as text we can use their enumeration to make sure they are properly set to expected values.
@enkodellc : The name 'MC' and 'MailChimp' used for purpose. Class name that contains 'MailChimp' are the classes that client will invoke. These are public class. Class name that contains 'MC' are internal class, client will never know about these class. We, developer will use that class to extend functionality.
That being said, you(as a user of this client) will never able to do MCList mcList= new MCList(); Check out the class name convention wiki.
The project follows SRP. Each class does only does one thing. For say, MCListsAbuseReports class only manage abuse complaints for a specific list. While the MCListsGrowthHistory class only do summary related work.
As you can see, as soon I have started following SRP in class level, the number of class grows rapidly. Now, client don't have to know about this. So, then FACADE came on the scene. MailChimpList is the only class that client will instantiate and they can do every list related operation from that class. Make sense?
@enkodellc : So, drop the idea of static class. Its not feasible in our case. Again, just keep in mind, client of this wrapper will only see the class that prefixed with the word MailChimp. So, if they want to do list related work, they only have to create an object for MailChimpList class and they will find every list related operation/methods in intellisense.
I do agree about other points, at some point we will go for all those for sure. :+1:
I created a new Branch called CodeRefactor since my next PR will contain many breaking changes this will be the best place to put them and refactor the code for next version.
I will let you review here. The only thing you might request that will take a lot of time is to add [Obsolete tags] to all renamed methods. I suggest we forgo this and just push this to a new version when we are ready for the CodeRefactor branch. Let me know what you think. This should cover your initial request and a bunch of what I suggested. It does not do anything with the line items you gave feedback on.
https://github.com/enkodellc/MailChimp.Api.Net/commit/21993a709cd893b983b04b5d917b26b70bfee3f4
I didn't submit a PR yet as I want to test a bit more. Right now the code compiles / builds.
One issue I have seen while renaming the methods is that I think we should consistently use "Insert" instead of the "Add" or "Create" Your thoughts?
@enkodellc : are you done with your refactoring? If so then send me a PR, will push it for prod.
I am done but 1 commit behind. I will sync it up tomorrow and submit a PR.
@enkodellc
Actually just after I posted I noticed that as a comment - 'Re-add Async to method names' on one of the commits for your Pull Request - hence the deleted comment ;-)
(I see some people trying without Async suffix, but most revert - especially public APIs)
