Identify 'code smell' and refactor.

[v0dro]

trafficstars

Rubex was written in a bit of hurry, so parts of the code base are quite messy. Identify these smelly parts and refactor them with better design. Refer to Martin Fowler's Refactoring Ruby book for reference. Here's a rough list based on basic parameters like a method/class doing too many things or being too long. Feel free to add your own.

• [ ] Statement::Alias#analyse_statement. This method is using an if-else approach for detecting function pointers. Ideally we should use a new class for denoting function pointers for arguments (like this commit: https://github.com/SciRuby/rubex/commit/ff00ff83038664b0fc803c9452f4f7699f66e2e3)

• [x] Expression::MethodCall#analyse_statement

Method is too long and does too many things in 30 lines of code :O

• [x] Expression::Name#analyse_statement

Too long and does too many things.

• [x] analyse_statement in Expression classes

  The analyse_statement method should only analyse statements and make sense of their types and generate symbol table entries etc. Currently methods of this name exist in Expression classes as well. This should go so that there is a clear demarcation between statements and expressions. Don't just rechristen to analyse_expression. Let there be some value to it.

• [x] Separate classes into their own files as per ruby-style-guide.

• [x] Expression::CommandCall#generate_evaluation_code Method is too long and does too many things. Too many conditionals.

• [x] ElementRef#analyse_statement

• [x] ElementRef#generate_evaluation_code

• [ ] Expression::StringLit class Class trying to be both a Ruby string and C string. Segregate.

• [x] Refactor temp allocation mechanism. Current mechanism requires too much work on part of programmer.

• [ ] Refactor code generation. Current mechanism relies on the c_code construct which does not convey meaning properly and does too many things in one method in many cases.

• [ ] Unify Statement::ArgumentList and Expression::ArgList. There should be only one class for dealing with argument lists.

[v0dro]

Refactoring should be done in the refactor branch: https://github.com/SciRuby/rubex/tree/refactor

[shaunakpp]

Do we have a Roadmap or Milestones planned for refactoring? Also, what needs refactoring and what not can be subjective, so can we keep a dedicated channel for discussing this?

[v0dro]

Works. How do you think a channel should be setup? Gitter or something?

I'm currently figuring out things to refactor myself. Will post my list in this thread now.

[v0dro]

I've updated with a rough list.

[shaunakpp]

Gitter or slack would be best. Also, one thing I noticed was that files in lib/rubex/ast/ implement multiple classes in one single file. If we keep separate files for separate classes, it will help in maintaining the project and also reduce the overall code-churn. What do you think?

[v0dro]

So its mainly done that way because all the classes under say expression.rb cater to only expressions, and most of these classes tend to be interdependent so it makes it easier to read and make edits between classes. Also, most of them are quite small too (average of 40 lines, max 100 lines) so I didn't think it would be best to split them into multiple files.

Is having a class per file a best practice, though?

I'll setup Gitter soon. I'll also open a #rubex channel on the sciruby slack.

[shaunakpp]

Is having a class per file a best practice, though?

Yup. Check this But if there are a lot of interdependent classes, we can keep references to files in comments, wherever required.

[v0dro]

OK. I'm sold. I'm updating the above list to reflect this change.

[shaunakpp]

I can start working on separating the classes. If no-one else has already taken it up.

[v0dro]

Sure. Please commit your changes to the 'refactor' branch. And be sure to send regular PRs so that I can pull your latest changes. This approach is just easier than having to merge two conflicting branches into the master later.

[v0dro]

MethodCall#analyse_statement: https://github.com/SciRuby/rubex/commit/0779f5660df554debd3ff0ddb210723c5976762e

[v0dro]

CommandCall#generate_evaluation_code : https://github.com/SciRuby/rubex/commit/012963f63c99ca767dc5567238a72775aaf80dce

[v0dro]

Name#analyse_statement: https://github.com/SciRuby/rubex/commit/a31144b4900da684b26892516289a866447ead88

[v0dro]

ElementRef: https://github.com/SciRuby/rubex/commit/583d85980bc6b9361fe2fa2206e9fc375e48805e

[v0dro]

@shaunakpp any progress on your end?

[shaunakpp]

I'll attempt it this weekend. Been a bit busy with work.

Sent from my Nokia 3310

---

From: Sameer Deshmukh [email protected] Sent: Thursday, January 18, 2018 10:10:45 AM To: SciRuby/rubex Cc: Shaunak Pagnis; Mention Subject: Re: [SciRuby/rubex] Identify 'code smell' and refactor. (#35)

@shaunakpphttps://github.com/shaunakpp any progress on your end?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/SciRuby/rubex/issues/35#issuecomment-358535848, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ADBfHrkBoHEgLVk1b5CyEg3lYbpM6sCPks5tLstNgaJpZM4RaaM5.