loodakrawa / SpriterDotNet

A pure C# Spriter implementation
zlib License
220 stars 75 forks source link

General codestyle issues. #116

Closed Martenfur closed 4 years ago

Martenfur commented 4 years ago

I have a really big issue with this.

if (asset == null || !CharMapValues.ContainsKey(asset)) return new KeyValuePair<int, int>(folderId, fileId);
return CharMapValues[asset];

This is very bad way to write code. At a glance it looks like a second return is actually the statement body, but it's not. A proper, readable way would look like this:

if (asset == null || !CharMapValues.ContainsKey(asset)) 
{
  return new KeyValuePair<int, int>(folderId, fileId);
}

return CharMapValues[asset];

If you don't like braces, at the very least move an if body to a new line and put an empty line after it.

if (asset == null || !CharMapValues.ContainsKey(asset)) 
  return new KeyValuePair<int, int>(folderId, fileId);

return CharMapValues[asset];

Don't trick people into misunderstanding what you wrote.

You also have a lot of { get; set; } properties without additional modifiers. Don't do that, there is no reason for them to be properties. Convert them to fields instead.

MBeijer commented 4 years ago

If you haven't noticed already, this repository hasn't been touched in 3 years.

MBeijer commented 4 years ago

Also, if you cared to read the readme:

Project Status I'm working on version 2.0 inspired with the development philosophy inspired by the fantastic Handmade Hero. I'll try to implement all the current open issues in version 2.0.

Martenfur commented 4 years ago

And now read the last sentence of your own quote.

loodakrawa commented 4 years ago

@Martenfur - Not a proper way of writing code? I didn't know there's a "proper" way - I was more under the impression that code style and readability is more individual. But if you point me to a document that describes a proper way of writing code, backed by real scientific evidence I'll consider switching styles.

Martenfur commented 4 years ago

@Martenfur - Not a proper way of writing code? I didn't know there's a "proper" way - I was more under the impression that code style and readability is more individual. But if you point me to a document that describes a proper way of writing code, backed by real scientific evidence I'll consider switching styles.

Again. You glance at the code and you see

if somethingSomething
somethingElse

Unless you read the whole line, you can assume second line is the body - which is false. You don't need to do scientific research to figure this out. If you're that stubborn and want to keep all your code in a single line - at the very least put an empty line after the if so signify that something is up.

As for { get; set; } -- this is not even a style preference. It's just redundant. Getter and setter are essentially method calls, so you wrap your fields into methods without any reason to do so. This impacts performance -- not by a lot, but in a library like yours it is important. Additionally, if you have a struct wrapped into { get; set; }, you cannot modify its fields without doing this:

var temp = obj.StructProperty;
temp.Field = modifiedField;
obj.StructProperty = temp;
loodakrawa commented 4 years ago

I really do like one-liners. I guess the main problem in this particular instance is that the statements are sequential. Adding a space between should make it crystal clear. Like so:

if (asset == null || !CharMapValues.ContainsKey(asset)) return new KeyValuePair<int, int>(folderId, fileId);

return CharMapValues[asset];

As for the properties - the performance difference is completely irrelevant since these props are not something that's set every frame. And the struct argument is really an edge case. I'd much rather keep them as props for future proofing since this gives flexibility to add logic/validation without adding breaking changes.