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

Rewrite API to idiomatic C# #2

Open
tucaz opened this issue Mar 27, 2015 · 9 comments
Open

Rewrite API to idiomatic C# #2

tucaz opened this issue Mar 27, 2015 · 9 comments

Comments

@tucaz
Copy link
Contributor

tucaz commented Mar 27, 2015

Hi,

I just create a new pull request with some improvements and fixes regarding culture handler and while doing it I noticed that the C# used in this library is not very idiomatic.

Would you guys be willing to accept pull requests moving the code to a more idiomatic C#?

Thanks

@erwan
Copy link
Contributor

erwan commented Mar 30, 2015

Hi,

It's important for us to have idiomatic code in each langage. That said there are times where we balance that with consistency with other kits. In the case of the C# kit, it's not the langage I'm the most proficient in so there are certainly areas that can be improved in terms of code style.

To prevent you from taking time to make big pull requests that we may not be able to integrate for API consistency reasons, could you first let me know what are the points that would need to be changed to be more idiomatic C#?

@tucaz
Copy link
Contributor Author

tucaz commented Aug 22, 2015

I'm sorry for the long time it took for me to reply.

A few changes that I would do are:

  • Put each Fragment class (and other types) in their own file
  • Use aliases for types instead of class names (String vs string, Boolean vs bool)
  • Use String.Concat, StringBuilder and String.Format instead of just + for concatenating string whenever suited in order to improve performance and readability
  • Use appropriate conversion methods instead of casting/boxing/unboxing (Convert.ToInt32 vs (int)"2")
  • Use implicit variable types instead of declaring the type (var something = 0 vs int something = 0)
  • Capitalize methods properly (CleanUp vs cleanup, Fetch vs fetch)

@erwan
Copy link
Contributor

erwan commented Sep 4, 2015

Hi,

  • OK for the Fragment class in their own file, they can even go to a separate folder to correspond to the namespace
  • OK for the type alias as soon as they don't break compatibility
  • String.Format for readability yes, StringBuilder I'm not so sure it's needed: http://blog.codinghorror.com/the-sad-tragedy-of-micro-optimization-theater/
  • OK for conversion methods
  • Explicit types don't matter that much, I'm not sure what we would gain from it. There are cases where explicit types make it easier to read and catch errors early.
  • OK for method capitalization as soon as we can keep backward compatibility

If you want to do pull requests that's great. Just make sure to make separate PR for each issue to keep them small and quick to review. Otherwise I'll probably do it eventually.

@ghost
Copy link

ghost commented Jan 17, 2016

Whatever happened to this effort? I agree, the code could be made more idiomatic without losing any functionality and maintaining readability.

@erwan
Copy link
Contributor

erwan commented Jan 17, 2016

As I said it's something I'll probably do eventually, but it's not very high priority on my task list.

@ghost
Copy link

ghost commented Jan 17, 2016

It sounds like tucaz is willing to do the work (as per the first message).
Would accepting contributions from the community make it more feasible?

@erwan
Copy link
Contributor

erwan commented Jan 18, 2016

Hi @PeteK68 ,

Yes, I will accept contributions for points I noted above as soon as they don't break compatibility with the current version (deprecating existing methods if necessary).

Ideally it would be in separate pull requests so they are easier to review and can be integrated more quickly.

@Grepsy
Copy link

Grepsy commented Mar 8, 2018

We're evaluating this product and the .NET API not being idiomatic is a bit of a turn-down. Especially since it hasn't been fixed for over two years.

@AlexFursenkoOd
Copy link

AlexFursenkoOd commented Jun 18, 2018

Could you modify method Query(params IPredicate[] predicates) in Form.cs to use iterator of type IPredicate instead of Predicate. Until you do so there is no way to create my own predicate. There is no sense to inherit from IPredicate because method Query uses iterator of type Predicate, so if I pass my predicate of type IPredicate method will try to convert it to instance of type Predicate which will through an Exception.

Or perhaps mark method Query of type Predicate as virtual. This way we can inherit from type Predicate. It will allow us to override existing implementation of Predicate class
Anyway, for now there is no real way to add not supported in this library predicates (like "has")

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

No branches or pull requests

4 participants