client.d: split message handling into separate functions

[mathiascode]

No description provided.

[slook]

This giant list of function definitions doesn't look like it belongs here, this constructors seem to be intended for parameters about a particular User.

It's a bit of a dubious placement because a reference to a user object in another user in the user_list calling from the server module can't be allowed any possibility of executing unrelated methods via an obscure route.

Can't it go anywhere else? Perhaps consider moving process_message() outside of this Class or to another module if necessary.

[slook]

Overall, whilst I am not totally against the idea of refactoring code into smaller functions (clearly that would usually be the "right" way to do things by modern standards, and it does look a lot prettier!), it is a major departure from the monolithic style in which this module was originally designed, so I have serious concerns about he amount of unforeseen work that this might entail for us (well, mainly yourself!) further down the road.

I'm not saying that the end result won't be better, it obviously would be because I am confident that you pull it off as you have done exemplary work in other projects, but basically this is ripping all the guts (and soul?) out of the program and then putting it all back in a different order. Possibly it is too soon to be performing such major surgery.

+504 −437
No description provided.

The complete module re-write you have staged in this PR is a drastic measure, so maybe it would be a good exercise in sanity to enumerate the benefits (easier debugging?) and potential pitfalls (inevitable regressions?) along with the reasoning behind your logic for wanting to put so much effort and time into this, and compare that reasoning to what steps are really needed to achieve the desired outcomes...

There has to be a heck of good reason(s) for wanting to do such a major refactoring as this, but yet I feel like you don't really have any verifiable evidence which shows that the ugliness of the current select ... case block is actually unfit for purpose?

[mathiascode]

To be honest, I didn't expect this PR to be this controversial compared to previous ones. Why I chose to split the proc_message() function:

• Separation of concerns
• Reduced nesting
• Avoid a single huge function if more messages are added (login() was already split before, for example)
• soulcurses did something similar. It seemed like something that would've been adopted in soulfind as well, as the number of messages grows.

Possible pitfalls:

• A message isn't handled properly due to oversights when moving code

The alternative is to do nothing.

I have serious concerns about he amount of unforeseen work that this might entail for us (well, mainly yourself!) further down the road.

Can you elaborate on what kind of work you're concerned about?

Possibly it is too soon to be performing such major surgery.

I'd say it's better to make major changes now, before the project matures.

[slook]

Your reasoning is sound. As I said, I am not against this refactoring because it is a nice progression to make. I suppose I am just being devil's advocate intentionally because (I hope) it helps you to ratify the development cycle more carefully than one would being capable of doing on their own.

Can you elaborate on what kind of work you're concerned about?

Well, as you are well aware working on a project like this entails years of follow up work and maintenance. However, we are both relatively new to this language and the vast array of features and optimization it offers, so it is likely that even better methods will be discovered and you will be keen to implement them, which is great, but I am concerned doing all this might be a waste of effort if the program is operational now, then there is no need to rush into anything.

Also, out of respect for the efforts of the previous developers and give credit to the fact that the internal workings of the client-side handling of the server are they way are, while perhaps not being totally ideal. There is no need to fix something that isn't broken.

Dlang is not a loosely typed scripting language like what most of your coding experience is, so there is a need to be very aware of exactly how functions are declared and the risks of faulty exception handling throwing or nothrowing as you might assume ought to occur, because such luxury's don't seem to be taken for granted unless they are explicitly defined.

I'd say it's better to make major changes now, before the project matures.

Yes, indeed that is correct. If you feel that these types of major changes are inevitable, then you should just go ahead and commit it without any undue delay. There are no real consequences to making mistakes or bad decisions on this project, so we ought to take full advantage of the opportunity for experimental research and development.

I am not going to be putting myself in a position of offering approval nor disapproval to you, but if you want my opinion then this PR looks good to me.

Having said that, it would be nice to fully test how the program performs the way it is structured now. That way, it would be easier to see what are old bugs vs new bugs we introduce ourselves.

[mathiascode]

I'll close this PR for now. There's no immediate need for these changes, but they could be worth revisiting if the proc_message method grows too large in the future.

[mathiascode]

Dlang is not a loosely typed scripting language like what most of your coding experience is, so there is a need to be very aware of exactly how functions are declared and the risks of faulty exception handling throwing or nothrowing as you might assume ought to occur, because such luxury's don't seem to be taken for granted unless they are explicitly defined.

Adding type hints to Nicotine+ is one of those things I really want to do in the near future. It's a bit more work short-term, but helps a lot when refactoring code later (the dreaded AttributeErrors...), and forces you to design your functions well.