TaledensInvManagerUpdated icon indicating copy to clipboard operation
TaledensInvManagerUpdated copied to clipboard

Code cleanup

Open LordMike opened this issue 6 years ago • 5 comments

I made a checkout of the code and my Resharper is basically screaming at me. There are quite a few instances of:

  • Code not used at all, e.x. string ScreenFormatter.Format(double value, int width, out int unused)
  • Lots of this references that aren't needed (though I'm sure the Build/rewriter removes those, so they're not costing space)
  • Logic that's always true or false, e.x.: if (tagRegex == null || updateTagRegex) is always true in void ProcessScriptArgs()
  • .ToString()'s that aren't needed, typically on string types (this specific thing could have been a type or another primitive before it was a string - hence the ToString being leftovers from before)
  • In quite a few places, there are redundant type specifiers, f.ex. in array initialization (char[] SPACE = new char[] { ' ', '\t', '\u00AD' } => char[] SPACE = new[] { ' ', '\t', '\u00AD' }) or in other places where the types can be inferred by C#
  • It seems the two build configurations (Release / Debug) have different language versions (woops)
  • If possible, string interpolation would save a few bytes - also, it's rumored to be faster than string concatenation; especially true for string.Format(..., ...) cases
  • If possible, the final script should use var everywhere, as it's usually shorter than f.ex. HashSet<bK> - dunno if the builder can do this
  • I'm certain parts of this code could need refactoring
  • I'm also certain parts of this code could be faster or done in shorter code .. :)

Should I make some PR's?

I could also probably work out a R# configuration file that wouldn't conflict with what SE accepts. I could imagine they don't work with all C# features..

LordMike avatar Mar 07 '19 21:03 LordMike

I know there are a lot of issues with this code that make it difficult to maintain (it's part of the reason I lost motivation to do anything with it for a while). There are some parts that are nearly unreadable (and I still don't know what they do exactly), so I do think there are a lot of improvements that can be done, however I feel like I'd prefer to rewrite TIM from the ground-up (using this source as reference) so it can be easier to maintain, more performant, and more space efficient.

I also want to move to MDK, as it provides a much better build system than I could write (or have the time to). It also exposes the ability to split the project down into multiple files, meaning I can write the whole project utilising C# to its fullest (which the current codebase really doesn't).

If you want to make improvements, I'll gladly look over them and bring them in, but it works for now (mostly), so I don't think we need to do anything too drastic.

On the note of rewriting, I've been debating whether I should replace this project with the rewrite, or create a new one and leave this one to bug-fixes only. The same applies with the workshop item, I'm not sure if I want to replace the workshop release with the rewritten version, or release it as a new script that is meant to be a 'spiritual successor' (I have no idea what I'd name it, though). Would you have any thoughts on this?

luvies avatar Mar 07 '19 23:03 luvies

Since this is a side project, and motivation or available time at times can be lacking, I would go with patching on the current code in incremental steps. I think any efforts for rewrites will die off before they’re complete, making it wasted time.

But tweaking or rewriting parts of the current script in incremental steps, fex dedicating time to identify what these odd places of code do, would be better spent.

It will take longer. But it will succeed.

I will try making a few changes. It should make the readability and maintainability better.

Splitting into multiple files would be wonderful. Can the current build thing merge them back - or will this “MDK” so it?

Mike.


From: Luke [email protected] Sent: Friday, March 8, 2019 12:39 AM To: luvies/TaledensInvManagerUpdated Cc: Michael Bisbjerg; Author Subject: Re: [luvies/TaledensInvManagerUpdated] Code cleanup (#34)

I know there are a lot of issues with this code that make it difficult to maintain (it's part of the reason I lost motivation to do anything with it for a while). There are some parts that are nearly unreadable (and I still don't know what they do exactly), so I do think there are a lot of improvements that can be done, however I feel like I'd prefer to rewrite TIM from the ground-up (using this source as reference) so it can be easier to maintain, more performant, and more space efficient.

I also want to move to MDK, as it provides a much better build system than I could write (or have the time to). It also exposes the ability to split the project down into multiple files, meaning I can write the whole project utilising C# to its fullest (which the current codebase really doesn't).

If you want to make improvements, I'll gladly look over them and bring them in, but it works for now (mostly), so I don't think we need to do anything too drastic.

On the note of rewriting, I've been debating whether I should replace this project with the rewrite, or create a new one and leave this one to bug-fixes only. The same applies with the workshop item, I'm not sure if I want to replace the workshop release with the rewritten version, or release it as a new script that is meant to be a 'spiritual successor' (I have no idea what I'd name it, though). Would you have any thoughts on this?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/luvies/TaledensInvManagerUpdated/issues/34#issuecomment-470745794, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AA-sJwkOy22mPrf8i0bLEyOh9eD8EjFUks5vUaNEgaJpZM4bkMgG.

LordMike avatar Mar 08 '19 11:03 LordMike

I know that a rewrite would take a while, but I don't mean to start from nothing. It would take a decent amount of logic this script already uses in its own (since this script does work). However, these are the changes I'd want to make:

  • Split out project into multiple files
  • Move scanning to a full grid & diff system
    • It would build up a data structure of found blocks, and on subsequent scans, only alter the data structure where it no longer matches
  • Rework the inventory movement to a node-edge one that it clearer to use (usage on classes, etc. since that isn't done right now and makes it overly complicated)
  • Rework item distribution so that it fills up containers 1 by 1, instead of distributing items evenly
    • This is more performant, but the old behaviour should be available via opt-in
  • Attempt to improve refinery & assembler management
    • Things like trying ores to see if they move in and seeing what they make (supports modded ores)
    • Component disassembling once over quota
    • The current system is nearly impossible to read, so would require a complete rewrite anyway so that anyone could work on it

Since there are so many changes I'd want to make, I think it would be better to start on a fresh base and bring things in as and when they're needed. Good examples would be the arguments processing & LCD panel writing; the current argument processing is a rewrite already, so should be fine being copied in directly (with small changes I'd imagine), and the LCD panel writing is complicated as hell, so at most I'd only want to rework it, rather than rewrite from scratch.

The current build system is one I wrote from scratch, so what it does now is bascially the limit of it, but MDK does support it (I've been writing a small airlock script to test it, and it's very good). This is also another reason why I'd want to move to a new repo, since the entire project would have to be re-generated (I'd prefer to leave this one intact as it's a good historical thing)

luvies avatar Mar 08 '19 19:03 luvies

Oh, this issue also references #10 kinda

luvies avatar Mar 08 '19 19:03 luvies

I just tried out MDK with TIM, and it seems to work great. Slow minifier, but that can probably be fixed.

Only issue I see are the user-specific values in MDK.options. I've created this issue to try and find a solution there.

LordMike avatar Mar 09 '19 16:03 LordMike