• Post Reply Bookmark Topic Watch Topic
  • New Topic

Rate my code  RSS feed

 
Greenhorn
Posts: 4
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
How would you rate my code?
Pastebin

Thanks!
 
Marshal
Posts: 56600
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Welcome to the Ranch

Please post the code here with code tags because mny people are reluctant to follow links to find code.
 
Alexandro Rossi
Greenhorn
Posts: 4
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It's pastebin, almost every coder uses it
 
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Pjotr Inno wrote:How would you rate my code?

Well, it's nicely formatted, and you appear to be using proper Java naming conventions; so well done on that score.

However:
1. I'd suggest adding Javadoc headers for all your public methods.

2. This is a personal preference, but I'd use switch statements rather than those huge if...else blocks. These days you can use them for Strings as well as numeric values.
Either way, having lots of them suggests that you're doing a lot of "dispatching", so it might be worth considering subtyping/polymorphism, or possibly enums, to hide some of that logic, or make it more readable.

3. Your handleBanCommands() method looks very "monolithic", so I'd try to break it up a bit ... actually, a LOT. 
My general rule of thumb is that methods should rarely be longer than a "viewable page", or about 20 lines of code - and 10 is even better.

HIH

Winston
 
Saloon Keeper
Posts: 7993
143
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Pjotr Inno wrote:It's pastebin, almost every coder uses it

I really don't think so.
 
Stephan van Hulst
Saloon Keeper
Posts: 7993
143
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You have a lot of hard-coded text. I would move these to resource bundles.
 
Sheriff
Posts: 11494
180
Android Debian Eclipse IDE IntelliJ IDE Java Linux Mac Spring Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Methods are too long and incoherent. One in particular, handleBanCommands(), is horrific and obscenely long. In some places, writing code like this can actually be cause for termination. Honestly.

BTW, I agree with Stephan about PasteBin. A lot of programmers I know don't use it, myself included.
 
author
Sheriff
Posts: 23295
125
C++ Chrome Eclipse IDE Firefox Browser Java jQuery Linux VI Editor Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Stephan van Hulst wrote:
Pjotr Inno wrote:It's pastebin, almost every coder uses it

I really don't think so.


Agree with Stephan. Most developers use a source code control system as a repository for code. And even then, it runs the gamut. The reason for that is source code control is not a developer's decision, but a corporate decision. The repository is used by everyone on the team, perhaps, even across teams (company wide), and in many cases, integrated with the build system and the bug tracking system.

Henry
 
Alexandro Rossi
Greenhorn
Posts: 4
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you so much for the comments!

And to the pastebin comment: Yes, I use GitHub myself.
I just mean to share quick snippets, pastebin is widely used and there's nothing "unsafe" about the site that's what I meant
 
Alexandro Rossi
Greenhorn
Posts: 4
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Winston Gutkowski wrote:
Pjotr Inno wrote:How would you rate my code?

Well, it's nicely formatted, and you appear to be using proper Java naming conventions; so well done on that score.

However:
1. I'd suggest adding Javadoc headers for all your public methods.

2. This is a personal preference, but I'd use switch statements rather than those huge if...else blocks. These days you can use them for Strings as well as numeric values.
Either way, having lots of them suggests that you're doing a lot of "dispatching", so it might be worth considering subtyping/polymorphism, or possibly enums, to hide some of that logic, or make it more readable.

3. Your handleBanCommands() method looks very "monolithic", so I'd try to break it up a bit ... actually, a LOT. 
My general rule of thumb is that methods should rarely be longer than a "viewable page", or about 20 lines of code - and 10 is even better.

HIH

Winston

Thanks
I'll definitely improve the handleBanCommands method, since it's massive
 
lowercase baba
Bartender
Posts: 12565
49
Chrome Java Linux
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Pjotr Inno wrote:It's pastebin, almost every coder uses it

I always figure, if I'm asking someone else for a favor, it's my job to make it as easy as possible for them to do so. And which is easier:

Asking someone to read something on the screen they are already looking at and reply right there

- or -

Asking them to go somewhere else, find whatever it is you are talking about, maybe cut-n-paste bits and move that back to the original website...

just something to consider.
 
Winston Gutkowski
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Pjotr Inno wrote:I'll definitely improve the handleBanCommands method, since it's massive

One of the chaps around here is fond of saying that good code should tell a story; and I completely agree with him. But to do that, you have to think about what your code is doing, rather than just how it's going to do it.

For example, if I was writing a chess program, I might start with a method like this:The above is purely for illustration and there are probably many better ways to do it; but I'd say that it describes a game of chess pretty well.

And most importantly, I don't have to know anything about how those methods work (and they could be very complex indeed), or exactly what a Board or Player or Result object looks like; I'm simply describing the process of a game of chess in "Java-ese".

I hope you can also see that choosing good, descriptive names for your methods and variables can really help other people to understand your code; but keeping methods short is probably the most important thing you can do to aid readability.

HIH

Winston
 
Marshal
Posts: 4052
239
Clojure IntelliJ IDE Java
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Pjotr Inno wrote:It's pastebin, almost every coder uses it

As a purely practical consideration, I am unable to participate in this discussion because pastebin is not accessible from my office network. Therefore I cannot see your code at all.

Also, think about the long term usefulness of this topic. There has been, and may continue to be, lots of really good advice about general programming techniques and syntax specific preferences. But, where is the context? At best the future reader has to navigate off to some other site to see your code which is an inconvenience. At worst, the code being discussed is gone forever because you decided to delete it from pastebin, or maybe even pastebin closed down, in which case this topic is now useless.

My recommendation for posting questions about code on CodeRanch is to always post the code you want to discuss directly on CodeRanch. You preserve the usefulness of the thread for future generations, and you make it much easier for the reader to engage with your question and thus are more likely receive the help you seek.
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!