Just realised how bad my code was

Discussion in 'Programming' started by Terrobility, May 30, 2016.

  1. https://www.spigotmc.org/resources/goldenapplecontrol.12639/download?version=73835

    Decompile it and see exactly how bad it was. I opened it up today to give all my plugins on Spigot an update and it hit me like a freaking truck. I mainly make quality plugins for ParallaxMC, the network where I work at (mostly for free, unfortunately). I usually make good use of object orientated programming and refraining from using static unless it's suitable to use it. Not to mention, I use MySQL/SQLite in quite a few plugins.

    I imported the GoldenAppleControl project into my IntelliJ and I was blown away. Not blown away with amazement, blown away with cringe. I made this plugin when I didn't know how to code at all. I simply didn't know where to start. Firstly, I migrated my project to Maven and linked it to my Github. I thought to myself, let's try making a better and more dynamic config! So that's what I did. I wanted to dynamically read from it and store the cooldowns for each consumable in an Item object. That Item object would contain CoolDown and ConsumptionControl objects as well. So I managed to set them up and make my item manager convert the stuff in the config into their respective objects (untested, like everything else).

    Right now I'm setting up my listener and everything is so damn long. And I'm tired. Please save me or hire me. The worst bit is that I've still got two more plugins to go through and improve. Is there any mistakes I'm making with this code? Any feature requests? Anything? Please save me.

    I want to know if you've ever had any moments like these. Leave your code cringe stories below!
     
    • Friendly Friendly x 1
  2. A very important step in learning a language is using it first hand. We have all experienced such a thing, and it is great to look back upon it. The problem within the Minecraft community is that we/us/the noobs tend to publish these horrible projects for the public to use. Or maybe that isn't a problem at all.
     
    • Agree Agree x 3
  3. A lot of newer developers don't really understand that you can code with any particular aspect in mind and still end up with garbage code. Following OOP is important, but that does not mean there isn't garbage OOP code out there - there is. Same for just about anything else, static or otherwise.
    Something that you could do, from my viewing of your code, is trying to break the code down into more manageable pieces. In regards to the "use final wherever possible" principle you've employed, that's nice and all but honestly, it just adds to visual bloat. This is not to say that you shouldn't use it wherever you can, it's just a case of when you're setting mutator in a loop as final... you're going overboard. That variable is going to change, that's the foundation of a for each loop, that variable is re-assigned. However it works internally really doesn't matter one bit, it's the reasoning about your code that does.

    At my last work place I did some paired programming with one of the newer interns who was absolutely bent on this and honestly, it literally just cost him time. Setting something final out of force of habit, realizing that the manner in which he desires to run the code requires mutation, removing the final constraint, continuing. My point here is: immutability is great, but there is such a thing as too much of a good thing. I'm also sure that the compiler is probably able to do some guesswork about how that variable works and maybe make your code run 0.00001 seconds faster or whatever (some of the reasons I see people argue for the use of final with) and honestly, ignore that rubbish. Final is a note to the programmer, "This is constant". Anything and everything else is secondary to that fact.

    You're also, in my opinion, commenting in a pretty poor style. (Again, I'm just going over the listener here). Documentation is intended to be a description of what that method does, how it does it if relevant, and a description of what each of the parameters are. IF you really really really want to be crystal clear, you can give some sample inputs and outputs. The comments like //get data are totally 100% irrelevant and are basically just visual bloat. Those are a distraction, if someone can't tell what you're doing there from just reading the code... then what the hell are they doing reading that code in the first place?

    A small note on where a design pattern may come in handy, here: https://gist.github.com/anonymous/46fa5a01a4a6f444c25b6034f9fc8926 Your constructor for some reason calls a public method... (?) which is quite complex. This is fine and all, but a factory here might seem useful. The implication with a factory is that the production of an instance of that object involves complex decision making (more or less) and it's not as simple as injecting dependencies. Use of a factory reduces the visual bloat and allows people to ignore that complexity if they want to.

    As a minor, minor point, you have a thing about setting things to null. Basically, if you're doing <anything> = null, you're doing it wrong. @Rocoty wrote up a pretty good post explaining the Optional type on one of my threads a little while back, but the long and the short of it is null is the representation of nothing and it is up to the programmer to check against that. This is bad... because programmers are typically human... and humans typically forget/make mistakes. If you use Optional, it'll force you to check.

    A final note, referring back to the listener once again, the loop is something that you may want to buff up. I'd suggest maybe learning about streams next, for the easy filtering and terminal operations they provide. Overall your loop is whatever, but my thinking when I look at your code is you're not crystal clear on what the responsibility of each entity should be and I say this because the files I looked at are populated with large blocks of code that have no clear separation. It's not that big of a deal, trust me, it's not. But if you feel as though you can break those big blocks into smaller pieces that fit together nicely, well you'll end up with some pretty elegant code :)

    All in all, you're not as bad as you think, just program a bunch & learn lots :)
     
    • Useful Useful x 2
    • Like Like x 1
  4. Every few months that happens to me :). Check back in a few months at your current code and you'll experience the same thing. It's a gradual part of learning and it just shows how much you've learned in that time.
     
    • Like Like x 1
    • Agree Agree x 1
  5. Thank you. I'll take your advice onboard and my next move will be removing final, using the Optional type (which I didn't know existed), and looking into streams. I haven't annotated my code properly since there's so much to convert that it becomes hard to keep up with the annotations.

    So a little while later I came up with this:
    https://gist.github.com/anonymous/6767f040a004c973fe68bee04a369998
    How can I avoid setting my Objects to null? You said I could use Optional but I can't seem to figure out how. One more thing, is this the correct implementation for a factory manager? I use Optional instead of null checks in my other classes but I can't seem to figure this one out. I tried using filtered streams on my listener in order to check if the item and data type matched, and also to check if the player had the right permission if there was any.
     
  6. The usage for it is when you create a class that maybe handles data IO from the disk or the like, or you explicitly say Object = null and you return that, you instead wrap that in Optional or set it to Optional#empty()
     
    • Useful Useful x 1
  7. https://gist.github.com/anonymous/2c3fb5c0daccec49cc07e8212649bd34
    So something like this? I'm still essentially passing through null to the Item constructor if the Optional is empty though.
     
  8. In most cases, the compiler will make a mutable once assigned variable inside a method automatically final.
     
    • Informative Informative x 1
  9. Right, but that really doesn't change what you would use final for. It's a note to the programmer.
     
  10. I find Kotlin solves many of the problems discussed above. I would say it is worth looking into. :)
     
    • Winner Winner x 1
    • Useful Useful x 1
  11. Glad to see some love for kotlin around the forums.
     
  12. I used a string as a Boolean in config and tons of unneeded checks, I cringe so much looking at my old code
     
  13. Also, if you don't want to do Java related things I would suggest looking into Ruby. Solves most of these problems. (Is interpreted and dynamic though, some people might not like that.)
     
  14. I opened one if my old PHP projects one day. For a start, PHP forces far less consistency on you than Java; that only decreases the codes readability etc. We all get it, as long as you don't try to sell the code you're fine; we all have to start somewhere!
     
  15. In my java class the professor forces us to follow her coding conventions. You want to see some bad code? Try reading this:

    Code (Text):
    import java.util.*;
    public class asciiPatterns
    {static int numBoxes=3;
    static int widthB=4;
    static int heightB=6;
    static int widthH=5;
    static int heightH=6;
    static int numTiles=3;
    static int numRows=4;
    static int widthT=4;
    static int heightT=4;

    public static void main(String[] args)
    {boxes();
    honeycomb();
    tiles();
    }
    //----------------------------------------------------------
    public static void boxes()
    {System.out.println("Number of boxes: "+numBoxes);
    System.out.println("Width of boxes: "+widthB);
    System.out.println("Height of boxes: "+heightB);
    border();
    center();
    border();
    }
    public static void center()
    {int i,j,k;
    for(k=0;k<heightB;k++)
      {
    System.out.print("|");
    for(i=0;i<numBoxes;i++)
         {for(j=1;j<widthB;j++)
             System.out.print(" ");
          System.out.print(" |");
            }
    System.out.println();
       }
    }
    public static void border()
    {int i,j;

    System.out.print("+");
    for(i=0;i<numBoxes;i++)
         {for(j=0;j<widthB;j++)
             System.out.print("-");
            System.out.print("+");
            }
    System.out.println();
    }
    //------------------------------------------------------
    public static void honeycomb()
    {int i,j;
    System.out.println("Height of Honeycomb: "+heightH);
    System.out.println("Width of honeycomb: "+widthH);
    topedge();
    for(i=0;i<heightH;i++)
       {top();
       bottom();
        }
    }
    public static void topedge()
    {int j;
    System.out.print(" ");
    for(j=0;j<widthH;j++)
    System.out.print("_   ");
    System.out.println();
    }
    public static void top()
    {int j;
    for(j=0;j<widthH;j++)
    System.out.print("/ \\_");
    System.out.println();
    }

    That's exactly how she types it out and we lose points for not following her convention.
     
    • Winner Winner x 1
  16. There's still that one fundamental question - what does it actually do?
     
  17. It's a program that outputs boxes (ascii art) using only for loops and class constants. The rest of the progam just prints different ascii art (I cut out the full source for space)

    This is what I handed in compared to her code:
    Code (Text):
    public class asciiArt {
        public static int boxesCount = 7;
        public static int boxesCols = 5;
        public static int boxesRows = 4;
     
     
        public static void main(String[] args)
        {
            boxes();
        }
     
        public static void boxes()
        {
            boxesDetails();
         
            boxesWrapper();
         
            for(int rows = 0; rows < boxesRows; rows++)
            {
                for(int i = 0; i < boxesCount; i++)
                {
                    System.out.print("|");
                 
                    for(int cols = 0; cols < boxesCols - 1; cols++)
                    {
                        System.out.print(" ");
                    }        
                }
             
                System.out.println("|");
            }
         
            boxesWrapper();
        }
     
        /**
         *
         * The top and bottom pattern of the boxes.
         *
         */
        public static void boxesWrapper()
        {
            for(int i = 0; i < boxesCount; i++)
            {
                System.out.print("+");
             
                for(int cols = 0; cols < boxesCols - 1; cols++)
                {
                    System.out.print("-");
                }
            }
         
            System.out.println('+');
        }
     
        /**
         *
         * Print out the details for the current boxes setup.
         *
         */
        public static void boxesDetails()
        {
            System.out.println("The number of boxes: " + boxesCount);
            System.out.println("The width of the boxes: " + boxesCols);
            System.out.println("The height of the boxes: " + boxesRows);
        }
    }
     

    I received -5 points for not following her convention.


    Looks something like:
    Code (Text):

    +--+--+--+--+--+--+--+
    |     |    |    |    |    |    |    |
    |     |    |    |    |    |    |    |
    |     |    |    |    |    |    |    |
    |     |    |    |    |    |    |    |
    +--+--+--+--+--+--+--+
     
     
    #17 davidxd33, Jun 1, 2016
    Last edited: Jun 1, 2016
  18. IntelliJ IDEA for instance can learn coding styles and change your code to follow them. I would suggest coding in your style and then using a tool like that to change it to your teacher's
     
  19. Still would need to suffer in class when were limited to using jGrasp. An editor with no auto braces, auto complete, code hinting, or color coding. We can't install anything on the computers so doing assignments her way is a nuisance.
     
  20. I am a proponent of static typing, and tend to dislike interpreted languages. I like Kotlin a lot because it is designed to run on the JVM and has full Java interop.