CC: Tweaked

CC: Tweaked

42M Downloads

A grand-ol' cleanup

SquidDev opened this issue ยท 4 comments

commented

A continuation of #91 for the modern age. Can't believe we're 550 issues later...

  • Go through all times we request a block update, and see if we can narrow the scope. We don't actually need to mark the chunk as dirty a lot of the times.

    This was, in part at least, responsible for #643 (turtles shouldn't be doing block updates when reading NBT!).

  • Clean up "on destroyed" hooks for blocks (i.e. modems). I feel there's a couple of places we could get away with using removed. Likewise, could we rebuild the state when validated?

  • Remove all the m_ prefixes. So petty I know, and going to cause merge conflicts. But currently we're horrifically inconsistent.

  • Investigate using Mojang mappings. Should make the job of Fabric people easier, but a) need to check legal terms and b) see how well FG copes with it.

  • Reformat everything to use single-line braces and no spaces around parens. Undecided on this, but would mean IDEA can cope with switch expressions in a sensible way.

  • Look into what network packets block updates send. I'm fairly sure most blocks (peripherals especially) don't need to send as much now.

commented

Hey, thanks for your interest in CC: Tweaked!

I'm afraid the answer here is a bit of an awkward "it depends". I'm definitely happy to merge refactoring PRs, though it's going to depend on what they touch. There's definitely some more fragile and complicated subsystems I wouldn't want to be touching without being very careful1.

TextBuffer is definitely fair-game though - a lot of the terminal code is a little worse for wear.

lots of duplicated code in constructors and elsewhere. Instead we can use this() or create new functions maybe, assuming the constructor overloads are actually being used

Looking at this, I think a fair number of those constructors aren't used. We probably only need TextBuffer( char c, int length ) and TextBuffer( String text ).

redundant code:

I don't think this one is correct - end is reused within the next Math.min call, so we do use the intermediate value. It looks like this ensures end is less than the buffer's width and the text's length.

That said, I'm not 100% sure how many of the write overrides are used - might be possible to inline this into write( String text, int start ) and simplify the code anyway.

[1]: Things like wired networks, ComputerThread/ComputerExecutor. Basically anything with a copious amount of locks or comments.

commented

I don't think this one is correct - end is reused within the next Math.min call, so we do use the intermediate value. It looks like this ensures end is less than the buffer's width and the text's length.

Oops. That one just looked weird to me at a glance.

Would writing tests be a better way of getting familiar to the project? At least that carries no risk of breaking anything :) Going off topic we can always create a new issue if necessary.

commented

I wouldn't worry too much about breaking things too much. Most of the time I (or tests) catch things - it's just a few complex bits which can break in very subtle ways...

That said, definitely not going to complain about more tests. We do track coverage, which should give an idea of what code isn't run (not the same as what is tested :p).

There's a bit of info in CONTRIBUTING.md about how things are tested, though if you're sticking to non-Forge related code you probably don't need to worry too much.

commented

Hey, would you accept PRs just refactoring random parts of the codebase against this issue? I want to contribute to this project somehow but I'm reluctant about learning all about forge modding because it looks like such a complicated ecosystem.

Just to pick a random example, in TextBuffer.java, we have:

  • lots of duplicated code in constructors and elsewhere. Instead we can use this() or create new functions maybe, assuming the constructor overloads are actually being used
  • redundant code definitely not redundant ๐Ÿ™ˆ:
end = Math.min( end, pos + text.length() );
end = Math.min( end, this.text.length ); // overwrites the prev line with no side effects, right?

Hope I'm right about those, I'm quite rusty with Java at this point.