Terasology: Fix up LGTM errors

[syntaxi]

So we've recently just enabled LGTM to this project and there is a number of alerts coming popping up.
We should probably try and fix most of these, barring false positives of course.

I've tried to group them into different types and detail my thoughts on them.

Errors

| Error Type | LGTM rule | Details | Fixed? |
| --- | --- | --- | --- |
| Command line is built with string concatenation. | rule | Seems to be an easy fix, and a worthwhile security flaw to patch. Just need to follow details in the rules | ☑️ #3512 |
| Array index out of bounds | rule | Easily fixed by adding a length check to the line if (c == '&') { just before the errors | ☑️ #3514 |
| Container contents are never accessed | rule | IntelliJ seems to agree with this. I'd recommend removing and with thorough tests to ensure there are no side effects or that it isn't accessed in an odd manner elsewhere | ☑️ #3512 |
| Container contents are never initialized | rule | Again, like above I think remove and test. | ☑️ #3512 |
| Inconsistent equals and hashCode | rule | I think this falls under the label of 'over zealous checking'. As far as I can see this class is never used in such a way that hashcode is needed. Don't think we should fix this, as it's just not worth it | ☑️ #3518 |
| Missing format argument | rule | Cervator pointed out that this one is likely a false positive. Don't think we should 'fix' | ☑️ #3520 |
| Type mismatch on container modification | rule | Possibly another false positive. Take a look at the code and see if it makes sense to remove it but only if it is actually causing an error | ☑️|

Warnings

| Warning Type | LGTM rule | Details | Fixed? |
| --- | --- | --- | --- |
| Container size compared to zero | rule | Should look at the code and test it to see if it is an issue. If it is, perhaps change to a better hasName. | ☑️ |
| Dereferenced variable may be null | rule | Directly below the warning line there is a null check, so I think we should add change the conditional to something like newItem == null || newItem.equals(EntityRef.NULL) | ☑️ |
| Implicit narrowing conversion in compound assignment | rule | There are quite a few of these. I think it's best to go through them, if they can be fixed with a small change then do so. Otherwise I doubt it's worth it | ☑️ |
| Potential input resource leak | rule | We should probably see if we can change these to a try-with-resource style. If we can then that is a safer option. Again, on a per warning basis | ☑️ |
| Result of multiplication cast to wider type | rule | Some of them are easy fixes, some are not. I think we should fix the simple ones and leave the others. Almost all of these are not capable of overflowing in the given uses of the game. | ☑️ |
| Unused format argument | rule | Some, possibly all, of these are going to be false positives. Carefully evaluate each one to see what can be fixed and what doesn't need to be | ☑️ #3520 |
| Useless comparison test | rule | Similar to the first warning type. Some of these may be correct, so carefully work out if they need to be changed and an appropriate change to make. | ☑️ |

If an Error or Warning hasn't been fixed, then please feel free to fix it. If you fix one please tick the checkbox or comment below so others know not to try and solve what has already been patched

[mwvdev]

I'll give this a crack.

[Cervator]

Nice write-up @jellysnake - thanks!

On the "Unused format argument" what stuck out to me was when it is a logger.error instance since I think that method has a variant that takes an argument to pass an error into, while logger.info indeed should really match between the number of {} bits present vs arg count. That part does look super useful with some log statements out of whack :-)

[mwvdev]

I'm going to start work on "Array index out of bounds".

[mwvdev]

Fixes have currently been submitted for the following errors:

• Command line is built with string concatenation.
• Array index out of bounds
• Container contents are never accessed
• Container contents are never initialized

Everything else is still up for grabs.

[mwvdev]

I'm going to start work on "Inconsistent equals and hashCode". Even though it might seem a bit overzealous, I think it's a worthwhile investment to reduce the noise in the alerts from LGTM.

[mwvdev]

I'll start working on the logger errors and warnings:

• Missing format argument
• Unused format argument

[mwvdev]

I'm taking a look at "Type mismatch on container modification".

[mwvdev]

Going to take a look at the "Potential input resource leak" warnings next.

[mwvdev]

Digging into "Useless comparison test".

[bpas247]

What else should be done for this issue? I just checked and looks like we still have '46' alerts, are we aiming to try and get this down to 0?

[syntaxi]

Over the long term yes, I should go through and edit the issue to better reflect the current status. The LGTM errors are also good jumping in points for new contributors so even though we don't _need_ them fixed at all they're a good start

[bpas247]

Okay cool, I think I'll open a PR to fix some of the alerts :+1:

[Cervator]

Going to close this as we're just about out of alerts, down to just 14 right now :-) Down from a peak of ... a lot.

The rest can probably just be poked at organically over time, some that remain may be controversial.