[suggestion] Code style - improvement readability

Question

[suggestion] Code style - improvement readability

akupiec opened this issue 8 years ago · 5 comments

akupiec commented 8 years ago

For fun I taken a peek on source code and I have few suggestions which would improve readability.

This mod quite massive already, there is only few tests, architecture is quite confusing in few places and can easy be simplified.

There are few quick win situations in this code ex. :

changing code style to place brackets like this:

if (true) {
    ...
} else {
    ...
}

this simply change reduce number of lines to "read" it may be quite inconvenient on beginning but it make code more compact. If you prefer numbers here you have: in file EntityCitizen.java this change reduce number of lines by 100+

quite big number of comments - I learned that well written code don't need almost any comments.

   /**
     * Get the CitizenData that this Job belongs to.
     *
     * @return CitizenData that owns this Job.
     */
    public CitizenData getCitizen()
    {
        return citizen;
    }

it's clearly not needed here - even if you tell me it can be hidden in IDE and collapsed to 1 line its still one redundant line.
There are others examples of comments in methods that can be easy replaced by extracting method or variable.

Way to much abstractions :) but its Java - so I will not push it to far :) (ex. try to fallow usage of AbstractJob.java)
File sizes ^_^ try creating more smaller composed well named files.
And the most eye stinging thing is the tabbing in variables declarations O_o
It's looked cool in vim in XX century but now its just not worth it.
This is a screen from your own code:

Raycoms · Answer 1 · 2016-12-14T22:14:25.000Z

I know that: if (true) { ... } else { ... } Would reduce it by a lot but It's just plain ugly and imo bad style. Brackets should be always aligned. In some cases that may cause more lines but in other cases it actually causes a huge amount of extra visibility. I agree that our getter and setters shouldn't require that much of documentation. But it's nice to know if something is really only a setter/getter (when you're trying to call it) or really a method which does different things. And yeah the tabbing of the variables gets ugly sometimes. Should run a formatter about some of them. 2016-12-14 20:08 GMT-02:00 Artur <[email protected]>:

…

For fun I taken a peek on source code and I have few suggestions which would improve readability. This mod quite massive already, there is only few tests, architecture is quite confusing in few places and can easy be simplified. There are few quick win situations in this code ex. : 1. changing code style to place brackets like this: if (true) { ... } else { ... } this simply change reduce number of lines to "read" it may be quite inconvenient on beginning but it make code more compact. If you prefer numbers here you have: in file EntityCitizen.java this change reduce number of lines by 100+ 1. quite big number of comments - I learned that well written code don't need almost any comments. /** * Get the CitizenData that this Job belongs to. * * @return CitizenData that owns this Job. */ public CitizenData getCitizen() { return citizen; } it's clearly not needed here - even if you tell me it can be hidden in IDE and collapsed to 1 line its still one redundant line. There are others examples of comments in methods that can be easy replaced by extracting method or variable. 1. Way to much abstractions :) but its Java - so I will not push it to far :) (ex. try to fallow usage of AbstractJob.java) 2. File sizes ^_^ try creating more smaller composed well named files. 3. And the most eye stinging thing is the tabbing in variables declarations O_o It's looked cool in vim in XX century but now its just not worth it. This is a screen from your own code: [image: image] <https://cloud.githubusercontent.com/assets/556199/21203259/017696c2-c252-11e6-996f-52d9fa2721ee.png> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#348>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGI9yyD-Z3BHHfUHgXGr4_ahSU56PN5cks5rIGjNgaJpZM4LNeQL> .

akupiec · Answer 2 · 2016-12-14T23:27:28.000Z

Ab 1. I'm pointing it out because I changed to it recently (about 2 year ago by cause of JetBrains lovers) I would also recommend trying it out :)

Ab 2. I was talking about all comments not just documentation - simple one lines comments in functions are usually unnecessary (they are just additional text with little to none content)
If you want I can point few examples. Of course it make sens only if you planing to maintain this code for long period of time or taking break's in codding :)

Ab 5. I would recommend removing them all ^_^ for few reasons.

it can be done automatically for whole project (its not anything "big" and other team members will understand - it's for greater good ^_^) so it can be done in one go.
if you remove only some of them it will crate more inconsistency.
it's to minor change to play with progressive changing

It would be all from me in this topic. Have fun coding 😆

akupiec · Answer 3 · 2016-12-15T14:54:25.000Z

I created this topic mainly because of points 2 and 5.
Like I sad before I mean mainly comments in body functions. ex:

protected final boolean mineBlock(...)
{
//no need to mine block...
//we have to wait for delay
//calculate fortune enchantment
//get all item drops
//Break the block
//add the drops to the citizen
}

Documentation comments could be just simpler :) that's all.

About pull request's, i'm still analyzing the code, number of extended classes relay do not help in this case.

You may close this topic because it's unproductive.

marvin-bitterlich · Answer 4 · 2016-12-15T13:35:59.000Z

I was the same as you on braces style at first @akupiac but we decided on it and will now keep it this way. I think that braces style is religious and both are fine enough that it is easier to just keep one form. Every new coder will want to change it the way he is used to it.

On comments. Having everything documented at least forces people to document and think about it. If we ever have an api and release javadoc this will help a lot.

Abstraction is important for minecraft. They change stuff seemingly random.

Big files are something we currently work on and I gladly accept your help if you want to improve it.

Tabs should not be used but sometimes someone fucks up xD

Raycoms · Answer 5 · 2016-12-15T19:58:42.000Z

We rarely have any comments in the body functions, in some of the older classes I sometimes find them but also often remove them because they point out something "obvious enough".
I only use them if there are a lot of ifs or some java stream which might be hard to interpret.

Yeah we have a lot of abstract classes but that's mainly also just to reduce duplicated code.
Since minecraft changes a lot from update to update, less duplicated code causes less problems and less work when upgrading as well.
But, I agree in some cases we could, should find a better solution. Time is the main problem here =D.
Almost all of the 60 something issues are completely free for work =D

Share to