Kurinku-Studios / FNF-Forward-Launcher

A mod launcher for FNF: Forward!
GNU General Public License v3.0
11 stars 9 forks source link

Update bad code #6

Closed mosemister closed 3 years ago

mosemister commented 3 years ago

Mod

ModId

In your code, you are able to modify the Mod ID in the client. A ID should always be final and therefore should have no setters. https://github.com/ColonelKai/FNF-Forward-Launcher/blob/f7675dece2f3020001d7e73dc5f1b60c26ee36aa/src/main/java/org/colonelkai/mod/Mod.java#L28

Mod methods

In the method of getModId, getModName, getModDescription are all contained within the Mod class and it is not a abstract class, therefore the "Mod" in the function name isn't needed as it can be assumed that when your interacting with a mod object to get the mod id, its safe to assume that getId in the mod object means the id of the mod.

https://github.com/ColonelKai/FNF-Forward-Launcher/blob/f7675dece2f3020001d7e73dc5f1b60c26ee36aa/src/main/java/org/colonelkai/mod/Mod.java#L25

Lacking constructor

While there is a design pattern called Builder which does not contain a constuctor but instead has everything as getters and setters, the builder design pattern requies the getters and setters class to be named "ModBuilder" with a method called "build" which creates a "Mod" class.

By only providing the builder class and not the main class, it means that unless your willing to check every time that the value you have gotten from the object is 1) not null and 2) valid. Using a constructor removes this issue as your able to check in the constructor that all the values are valid.

https://github.com/ColonelKai/FNF-Forward-Launcher/blob/f7675dece2f3020001d7e73dc5f1b60c26ee36aa/src/main/java/org/colonelkai/mod/Mod.java#L23

Is Installed .... maybe

Due to the fact you have "setInstalled(boolean)" your program needs to manually set if it is installed for the "isInstalled" to be correct. I would make it so "isInstalled" checks to see if the mod file is present, if it is then it is installed then return true, if not return false. This way you don't need to manually tick that box to say its up to date.

Or just remove the method completely....

https://github.com/ColonelKai/FNF-Forward-Launcher/blob/f7675dece2f3020001d7e73dc5f1b60c26ee36aa/src/main/java/org/colonelkai/mod/Mod.java#L54

we have Optional as of Java 8 .... we use it for a reason

Getting a cached image should return optional. What happens if the cached image isn't there. If it should return just the path to it, then the name of the method should reflect that. Aka "getCachedIcon()" to be sounds like it should return a InputStream of the cached image, not the path to the cached image, if I wanted the path to it, then I would type "getCachedIconPath" or "getCachedIconDirectory"

https://github.com/ColonelKai/FNF-Forward-Launcher/blob/f7675dece2f3020001d7e73dc5f1b60c26ee36aa/src/main/java/org/colonelkai/mod/Mod.java#L88

I shorten the word for your conviniounce ... what do you mean you don't understand the method "d" it makes a circle

One of the biggest problems with Open Source code is people who want to understand your program will read your code meaning that you should be as fully formed as possible. While Dir is common for directory and will get away with it, don't get used to shortening your words.

https://github.com/ColonelKai/FNF-Forward-Launcher/blob/f7675dece2f3020001d7e73dc5f1b60c26ee36aa/src/main/java/org/colonelkai/mod/Mod.java#L102

It says its not cached ... but I can see the cached file

Like what you had with isInstalled you may want to consider checking the location of the cached image and seeing if its there or not. I personally would still keep the field check but I would say, if that fails then check the cached location

Import all the things

I have found that you hack a import that isn't used. This slows down the program (not by any noticable amount but will add up) and could be a security issue.

I would recommend to create the example macro on Intellij (format and save) as this will optimise your code when you save as well as make it readable (not that you have a issue with readablilty) https://www.jetbrains.com/help/idea/using-macros-in-the-editor.html

Loading Stage Handler

Used long sword ... it wasn't effective

Imagine a method which is 10,000 lines long and you have a bug in it. It turns into a game of finding a needle in the haystack and you aint got a magnet.

The init method is 61 lines long which does multiple smaller things. This should be broken down into smaller private methods with the init method calling those private methods such as the following

https://github.com/ColonelKai/FNF-Forward-Launcher/blob/f7675dece2f3020001d7e73dc5f1b60c26ee36aa/src/main/java/org/colonelkai/stages/LoadingStageHandler.java#L22

Mod Downloader

Repeating code == bad

How many times did you write new File(the entire path + a tiny extra), we use methods and fields to prevent repeate code.

Wonderful world of OOP .... lets use static

Static methods in OOP are designed to be used sparingly. Not sure what possessed you to use static methods for every method