Sergix / JTerm

A terminal written in Java for cross-platform compatibility and usage.
https://sergix.github.io/projects/jterm
GNU General Public License v3.0
52 stars 32 forks source link

Changed command classes to implement all the same `Command` interfaces #67

Closed ojles closed 7 years ago

ojles commented 7 years ago

The biggest thing is making the command classes to implement Command. It's better because you don't need to use Java Reflection, so you don't have your methods and classes to match the command name.

If some exception occurs I'm throwing a CommandException.

I also removed some help info, because you don't really need help for clear or exit. But we can implement a man command, that will print some extra info.

Before merging I suggest you to test the code. I'll do it by my self, but need your help to.

Sergix commented 7 years ago

The issue with using a hashmap, as I have already discussed in #53, is that it's hardcoded. A current way of reimplementing command input while still using reflection is currently being implemented.

I will still review the code once I get a chance to test it.

Sergix commented 7 years ago

After looking in your code, I saw a couple things. Currently, I'm actually working on away to implement a command class, as you made, into auto generating a hashmap on startup. So I can still use some of your code, but reflection will still be used, and the hashmap won't be hardcoded.

Also, I think it would still be good to have a -h for clear and exit, especially if we eventually add command options for them.

ojles commented 7 years ago

Can you explain me more about what you mean by 'hardcoded'? Even if you use reflection, you have to recompile the code to add/remove features.

ojles commented 7 years ago

Or at least we need to try to load the command classes at once, not every time we execute the command

Sergix commented 7 years ago

"Hardcoded" by writing down every single command value into the hashmap.

Sergix commented 7 years ago

And I mean loading all the classes on program startup.

ojles commented 7 years ago

Hum, maybe this solution will work? https://stackoverflow.com/a/12021972

We could annotate command classes and load the into a map or what ever

nanoandrew4 commented 7 years ago

@ojles Effectively both approaches are hardcoded, @Sergix suggests one that looks a bit cleaner and is more dynamic, since if you add commands you don't have to bother to add more lines, as it will work the same with 10 commands than with 100 commands. If you were to hard code it by adding all the commands manually to the hashmap, it would take up a lot of lines (as the program grows) + be annoying since each new command has to be added manually.

ojles commented 7 years ago

@Sergix @nanoandrew4 you are right, we can forget about this map so it's better to load them dynamically, although there only 14 of them...

I implemented dynamic loading, take a look if you have time :)

nanoandrew4 commented 7 years ago

Looks good to me! (at least from reading it, haven't had a chance to test it yet).

nanoandrew4 commented 7 years ago

@ojles Just be aware that adding another dependency increases the package size, I think it's about an extra 3MB (don't quote me on it...). Just keep it in mind!

Sergix commented 7 years ago

If the package really is that large, it would be best to just get the command class lost manually instead of using Reflections.

Sergix commented 7 years ago

*list

nanoandrew4 commented 7 years ago

@Sergix Just in case you are interested, dropping apache lang and reflections dropped my package size from 5.1MB to 1.5MB.

To determine system OS without apache lang:

public static void setOS() {
        String os = System.getProperty("os.name").toLowerCase();

        if (os.contains("windows"))
            isWin = true;
        else if ("linux".equals(os) || os.contains("mac") || "sunos".equals(os) || "freebsd".equals(os))
            isUnix = true;
}
Sergix commented 7 years ago

Ok. I'll try and put the manual code in soon.

Sergix commented 7 years ago

@nanoandrew4 So, I added the setOS() function, but I realized the lang3 package can't be removed due to its use by Ps.java.

Sergix commented 7 years ago

@ojles Some manual changes need to be done on your branch due to incoming changes in order to merge.

ojles commented 7 years ago

the lang3 package can't be removed due to its use by Ps.java

@Sergix you only use the OS constant form lang3, but you can use JTerm.IS_WIN or JTerm.IS_UNIX

That's why I added the FIXME: comment, because i was confused

Sergix commented 7 years ago

@ojles Oh! I see. Thank you, I didn't even realize this. 😄

ojles commented 7 years ago

@Sergix I merged the previous changes and fixed merge conflicts, should be fine now.

Sergix commented 7 years ago

Looks great!

One last thing, could you write a changelog entry briefly describing your main changes? After that this PR will merge.

ojles commented 7 years ago

@Sergix not sure about the file format, but I did it. BTW, why do we need a changelog, when you can simply type git log or use annotated tags?

Sergix commented 7 years ago

@ojles Thanks! I keep a manual changelog just because it is a lot more descriptive and greatly helps me when I write the release docs.