Solved Any way to shorten nested IFs?

Discussion in 'Plugin Development' started by octoshrimpy, Aug 28, 2014.

Thread Status:
Not open for further replies.
  1. Offline

    TheHandfish

    94.7% sure if you do what I told you to do, you can keep it all in one method, or class for that matter.
     
  2. Offline

    BillyGalbreath

    I try to avoid nested brackets where possible. For your example, since there's no other actions needed after the if/else just return instead.
    Code:java
    1.  
    2. public boolean onCommand(CommandSender s, Command cmd, String label, String[] args) {
    3. if(!cmd.getName().equalsIgnoreCase("f")) {
    4. return true;
    5. }
    6. if(!(s instanceof Player)) {
    7. s.sendMessage("cannot run this specific command from console.");
    8. return true;
    9. }
    10. if(args.length == 0)) {
    11. s.sendMessage("this is a non-descriptive error message because you used arguments");
    12. s.sendMessage(" -O");
    13. return true;
    14. }
    15. //do something
    16. return true;
    17. }
    18.  


    Here's an example of how ugly nested brackets can get:

    Code:java
    1.  
    2. public void method() {
    3. if (foo) {
    4. if (bar) {
    5. if (anotherVariable) {
    6. if (isalive) {
    7. if (daytime) {
    8. if (idk) {
    9. // do something
    10. return true;
    11. }
    12. }
    13. }
    14. }
    15. }
    16. }
    17. return false;
    18. }
    19.  


    Here's how I would do the exact same thing, which is much more readable and easier to maintain:

    Code:java
    1.  
    2. public boolean method() {
    3. if (!foo)
    4. return false;
    5. if (!bar)
    6. return false;
    7. if (!anotherVariable)
    8. return false;
    9. if (!isalive)
    10. return false;
    11. if (!daytime)
    12. return false;
    13. if (!idk)
    14. return false;
    15. // do something
    16. return true;
    17. }
    18.  


    ;)
     
    TheOddPuff and AoH_Ruthless like this.
  3. Offline

    octoshrimpy

    TheHandfish if I do what you told me, whilst using the Executor, it won't register commands other than the one I registered on the onEnable (if I understand properly, which I believe I did). What you say has to be done without the executor, and is what I am doing already, hence why I am mostly ignoring your side of the argument, since I am looking for new ways to do things, and trying to hear as many opinions as possible. Also, 94% isn't a very good number IMO.
    ^that. I already do what you're suggesting.
     
  4. Offline

    BillyGalbreath

    Why would you want to keep everything in the same class anyways? That just starts to clutter things up and make maintainability a nightmare..
     
    octoshrimpy likes this.
  5. Offline

    octoshrimpy

    Exactly. OOP FTW. My uncle (who works for Intel) always told me "Many methods make light code". This is what he was meaning. :D

    Code:java
    1.  
    2. public boolean onCommand(CommandSender s, Command cmd, String label, String[] args){
    3. if(!cmd.getName().equalsIgnoreCase("generic command")){
    4. return true;
    5. }if(args.length==0){
    6. s.sendMessage("generic help info message");
    7. return true;
    8. }if(args.length==1){
    9. if(args[0].equalsIgnoreCase("this"))
    10. //do something
    11. return true;
    12. if(args[0].equalsIgnoreCase("foo"))
    13. //do something
    14. return true;
    15. if(args[0].equalsIgnoreCase("bar"))
    16. //do something
    17. return true;
    18. }if(args.length==2){
    19. //repeat above setup
    20. }


    TheHandfish and BillyGalbreath is this cleaner and better, then? doesn't use the Executor, but allows for multiple commands, and can be stuck in its own class where I can just pass it the arguments onCommand().

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: Jun 10, 2016
  6. Offline

    BillyGalbreath

    Are you attempting to organize subcommands? Theres a better way..
     
  7. Offline

    _Filip

  8. Offline

    BillyGalbreath

  9. Offline

    AoH_Ruthless

    octoshrimpy
    Other basic "good practices" you should follow that don't affect your code too too much:
    • Use @Override annotation in onCommand. This avoids accidental parameter / spelling errors in your method and ensures that you will be notified if the API breaks (if Bukkit changes method to something like onCmd in future, which likely won't happen).
    • You should use if / else if ... / else wherever possible; using just 'if' otherwise is slower on compile because the compiler doesn't recognize that it is mutually exclusive from a previous condition check.
    • The command can't be "generic command". I know this is an example only, but it cannot have spaces.
    • There is something wrong with this:
    Code:java
    1. if (args[0].equalsIgnoreCase("this"))
    2. // code
    3. return true;
    4. if (args[0].equalsIgnoreCase("foo"))
    5. // code
    6. return true;

      • When not using brackets for if statements, only the line of code directly after the condition is recognized. For example:
    Code:java
    1. if (a)
    2. b; // NOTHING WILL BE RECOGNIZED AFTER... INCLUDING return statements.
    3.  
    4. if (c) // this WILL not work. return true; will not be recognized. As such, use brackets on if statements... Anyways, it's better practice.
    5. // code
    6. return true;
    7.  
    8. if (d) { // MUCH BETTER
    9. // code
    10. return true;
    11. }

    • Lastly, when comparing multiple sub-commands, there are better ways. As BillyGalbreath is implying (I think), you should include sub-commands in different classes. This is always very useful in plugins with one/two baseline commands. However, in your case in one-method, you should use switch statements. They are more efficient than a bunch of if/else if/else condition checks. http://docs.oracle.com/javase/tutorial/java/nutsandbolts/switch.html
     
  10. Offline

    octoshrimpy

    _Filip but with that, it weeds out console for ALL commands.... which I've read (And agree) that is a bad practice... ._. Granted, I could have the instanceof Player check inside each commandMap.

    BillyGalbreath Which way is that? :eek:

    ----EDIT---
    Ninja'd
     
  11. Offline

    PandazNWafflez

    TheHandfish Keeping your commands in one class is a terrible idea - it could be hundreds of lines instead of easy to navigate 50 line classes.
     
  12. Offline

    BillyGalbreath

    See my link above. I highly recommend it. Not sure who wrote it originally, but Metalhedd included it in a past project as a PR and I love it ^_^

    It has the ability for commands to be player exclusive (extend PlayerCommand) or allow players AND console (extend BaseCommand).
     
  13. Offline

    Syd

    TheHandfish
    Who wants to keep all commands in one class? The point of my suggestion was to actually NOT have all commands in one class, but each in a seperate class.
    This is much cleaner, easier to maintain and also object oriented.

    octoshrimpy
    3 classes in a file (or even in a class) are a valid concept in Java. The only limitation is one public class per file (with the same name as the class).

    And yes, you need to do this for every top level command. You COULD also use the same Executor for different commands, but that would defeat the purpose. ;)

    For subcommands I recommend to either use an already existing command framework, or developing an own one.
    Using one has saved me many hours of work. ;)

    Btw. to understand such stuff and find out new things how to do stuff, I'd recommend you to take a look in the source of some well written open source plugins. You can learn a lot of how they do things and may improve your own code.
    Most popular (and function rich) plugins are well written from my experience.
     
    octoshrimpy likes this.
  14. Offline

    octoshrimpy

    Thank you all so much, ladies and gents! :D I got my reading for the week, and know I'll be rewriting 10's of lines of code this weekend. Now, to understand hashmaps..
     
  15. Offline

    TheHandfish

    Syd:
    There's nothing wrong with keeping your commands in one class. There are instances when it's more efficient not to. I personally do not do this very often. In fact, the only time I ever do is when I code for another server and they are not going to dig around in my source.

    EDIT: I never said you SHOULD keep your commands in one classes. For public plugins, especially. I simply said that if you want to do this, you can. I'm not about to critique people about the way they code. If it works for them, there is no reason for me to judge them.
     
  16. Offline

    Syd

    TheHandfish
    There are a few principles of programming that make the difference between good and bad code and it makes sense to tell these principles to unexperiences programmers.
    Because if you only know bad code, you'll have some bad habits sticking with you and in some cases people tend to teach their bad habits to others.

    If you actually know the difference between good and bad code and you think, that it's easier to write working bad code than working good code, you're free to do so, but please, NEVER teach others to use bad code.
    Also, from my experience doing good code is in most cases faster and especially more reliable.
     
  17. Offline

    TheHandfish

    Syd: What I taught is not bad code. I mentioned how to keep a few commands all in one class. You don't want to do it? Fine. Everyone else taught you how to do it otherwise.

    You seem to forget that I answered his question. He asked if he would need to make a different executor. I said no, provided you simply did a getName() check. To which I was bashed for providing "bad code" when all I did was say, "no, you don't have to." :p Perhaps you missed the replies?
     
  18. Offline

    octoshrimpy

    So no, you didn't. :p Syd Gave me an explanation, and everyone else pointed me towards code to study, which I appreciate. BillyGalbreath I will definitely have to use that in a bigger plugin, very useful. also, for a simple one-command plugin for a friend his double-tap spacebar rarely works), how does this look?
    Code:java
    1. public boolean onCommand(CommandSender s, Command cmd, String label, String[] args){
    2. if(!cmd.getName().equalsIgnoreCase("ff")){
    3. return true;
    4. }if(!(s instanceof Player)){
    5. s.sendMessage("cannot run this specific command from console.");
    6. return true;
    7. }
    8. Player player = ((Player) s);
    9. if(args.length==0){
    10. Location playerLoc = player.getLocation();
    11. player.setFlying(true);
    12. player.teleport(playerLoc.add(0, 1, 0));
    13. player.sendMessage("You are now flying!");
    14. return true;
    15. }if(args.length>0){
    16. player.sendMessage("just ff");
    17. }
    18. return true;
    19. }


    --EDIT--
    realized I had no return true; after console sending command, which would cause errors. added it in.
     
  19. Offline

    TheHandfish

    Oh, the irony... :p I thought you didn't need to check the getName()?

    You asked if you would need to make a new executor, and I said:

    Thus simply providing the answer. I don't see what's so difficult to understand, lol. :p By the looks of it, it even looks like you can use the same class for multiple commands provided you add another (command).setExecutor(blah blah) statement.

    Anyway, enough of that. On topic. You don't want to use the same method. That's advisable. By the looks of your code it looks like it would technically work. However, instead of checking for the player, seeing it is false and returning, you might consider putting the statement around the entire block of code without a return statement. This would mean flipping the if/else upside down. You can test your current code out and it may work for you (I'm on a mobile device so I can't). But if you want to make it cleaner, which you've made very clear is what you want ( :p ) go ahead and do what I've suggested.
     
  20. Offline

    BillyGalbreath

    Syntax looks good to me ^_^

    I would, though, either remove the return at line 14, or add a return after the message on line 16. Just for consistency.

    Apparently you didnt get the memo about NOT nesting wherever possible, hmm?
     
  21. Offline

    octoshrimpy

    done. :) Thank you for your help!
     
  22. Offline

    TheHandfish

    My mistake, got so off track with the executor thing. :O
     
  23. Offline

    Syd

    TheHandfish
    Don't even mention bad code. Just because things CAN be done in a certain ways, there is absolutly NO need to tell that to new programers. There is no extra knowledge for them, as it's bad code, but it still confuses them.
    It's like "static all the things" or imperative programing: ofc you can do it with Java, and it will work, but you should not do it, because thats no the way, Java was designed for.
     
  24. Offline

    ZodiacTheories

    Copied my sig? :p
     
  25. Offline

    teej107

    Actually I didn't realize I did. It is a really good statement though.
     
    ZodiacTheories likes this.
Thread Status:
Not open for further replies.

Share This Page