https://kotlinlang.org logo
Title
a

Andrew Gazelka

01/23/2019, 5:50 PM
for constructors one must initially reference
this(...)
. However, sometimes it requires a bit of code to transform parameters to
...
. Should I... 1) have a separate function which acts as a static constructor 2) have the constructor of the class reference a function
a

Alan Evans

01/23/2019, 6:11 PM
are you talking about when using multiple constructors?
When dealing with multiple constructors, instead of telescoping them, I think that many of the benefits of, well named, static factory methods still exist in KT https://blog.kotlin-academy.com/effective-java-in-kotlin-item-1-consider-static-factory-methods-instead-of-constructors-8d0d7b5814b2
d

Dico

01/24/2019, 3:44 AM
Constructors can be used in super calls, factory functions cannot. I assume that's outside the scope of the question though.
a

Alan Evans

01/24/2019, 12:37 PM
That is covered by Effective Java item 16, Favor composition over inheritance, which by making every class final by default, is Kotlin's default position on this.
a

Andrew Gazelka

01/27/2019, 4:20 PM
@Alan Evans how would you recommend me having multiple constructors (i.e. a string constructor which has a default base path) for this then?
abstract class BasicConfig(private val path: Path) {

    fun reload(){
        val configuration = YamlConfiguration.loadConfiguration(path.toFile())
        process(configuration)
    }

    protected abstract fun process(config: ConfigurationSection)
}
just use a regular constructor or restructure my code?
a

Alan Evans

01/27/2019, 4:22 PM
I actually wouldn't recommend
abstract
classes in any situation.
Imagine:
interface ConfigurationSectionLoader {
    fun load(): ConfigurationSection
}

class YamlConfigurationLoader(val path: Path) : ConfigurationSectionLoader {
    override fun load(): ConfigurationSection {
        return YamlConfiguration.loadConfiguration(path.toFile())
    }
}
class YourCustomClass(private val loader: ConfigurationSectionLoader) {

    fun reload() {
        process(loader.load())
    }

    private fun process(load: ConfigurationSection) {
        TODO("not implemented")
    }
}
Now your custom class just depends on something rather than descending from something.
ConfigurationSectionLoader
interface is optional. Depends how you want to couple these.
As for default path, that's just:``` class YamlConfigurationLoader(val path: Path = File("some/default/path.yml").toPath()) : ConfigurationSectionLoader { ```
Note how
YourCustomClass
does not need to know the path in any situation. Where as with
abstract
it will always need to provide that option in it's constructor.
a

Andrew Gazelka

01/28/2019, 2:19 AM
alright, so I've applied this pattern to a lot of code. Think it is a good idea to separate all implementations into separate classes? If I do this it seems to get pretty messy... maybe it is better to use anonymous classes instead for very specific uses?
object PlayerSubCommand : SimpleCommand by SubCommandBase(
        1,
        CommandAlias("player", "p"),
        PlayerTabAutocomplete,
        object : SubCommandProcessor {
            override fun on(sender: CommandSender, command: Command, alias: String, args: Array<out String>): SubCommandResult {
                val input = args[0]

                val offlinePlayer = try {
                    sp.server.getOfflinePlayer(UUID.fromString(input)) // UUID
                } catch (e: IllegalArgumentException) {
                    sp.server.getOfflinePlayer(input) // name
                }

                return SubCommandResult.Success(
                        setOf(
                                PlayerAddGroupCommand(offlinePlayer),
                                PlayerRemoveGroupCommand(offlinePlayer)
                        )
                )
            }
        }
)
where
class SubCommandBase(
        private val argsCount: Int,
        private val nameable: Nameable,
        private val tabCompleter: TabCompleter,
        private val subCommandProcessor: SubCommandProcessor) :
        SimpleCommand,
        Nameable by nameable {
...
}
a

Alan Evans

01/28/2019, 12:36 PM
I just fail to see what is so complex about a Command that it can't just be an interface. 4 dependencies is a lot things like
TabCompleter
and arg count are SRP smells to me.
would like to see the body of `SubCommandBase to see what makes it need to be an object.
d

Dico

01/28/2019, 1:14 PM
Command handling in minecraft servers is no trivial task, from experience
I recommend considering https://github.com/Mojang/brigadier
a

Alan Evans

01/28/2019, 2:08 PM
Obviously if you're subject to other people's architecture choices then at their API boundary there's not much you can do. But beyond that boundary you can minimize their API impact on the cleanness of your code through good abstractions, and that's what projects like brigadier are attempting to do.
d

Dico

01/28/2019, 2:32 PM
Well, no, I disagree. I will leave it at that though
a

Andrew Gazelka

01/28/2019, 3:02 PM
a

Alan Evans

01/28/2019, 4:08 PM
It's getting too specific for me, I can only discuss in generalities and a big part of the problem with these snippets is you can't explain the situation to me without me learning about minecraft. If that's the case, then you need to lean on domain experts like Dico.
@Dico I was only saying a library like brigadier (again me without actually learning what minecraft is) is an abstraction layer which makes dealing with minecraft commands more easy. Do you disagree with that?
And that without using a library or writing your own abstraction layer then your code is subject to all the architecture designs of the minecraft API (but also, maybe that's OK).
d

Dico

01/28/2019, 4:39 PM
The first one no, the second yes. Brigadier allows you to build very complex command systems. It adds a lot more features on top of a relatively simple api.
In general, libraries can indeed be designed to make something more simple to use, or more accessible by supporting multiple platforms, but that isn't always the case.
a

Andrew Gazelka

01/28/2019, 5:23 PM
https://kotlinlang.slack.com/archives/C0922A726/p1548678980676400?thread_ts=1548265838.440000&amp;cid=C0922A726
SimpleCommand
is still an interface
SubCommandBase
is just a class which combines dependencies. I don't see how this is bad...
d

Dico

01/28/2019, 5:24 PM
I think what you're doing is good
Mostly different approach than I've seen
a

Alan Evans

01/28/2019, 5:39 PM
Yeah I'm not saying it's bad, I'm just saying I don't have enough of the picture to really appreciate what you're doing or offer any alternatives.
I would say that the use of
by
here is not required:
object PlayerSubCommand : SimpleCommand = SubCommandBase(
Would work just as well wouldn't it?
d

Dico

01/28/2019, 5:42 PM
I wish that were valid syntax actually
Wouldn't make sense though
Have to use val
a

Alan Evans

01/28/2019, 5:45 PM
ah yes,
val playerSubCommand : SimpleCommand = SubCommandBase(
I think it's clearer what's going on there, as with
by
you might also be overriding some of the interface and I would have to look at the bottom of the declaration to find out.
Another syntactical suggestion is to put functions at the end of the list of arguments. Now imagine a function that helps you create a `SimpleCommand`:
var playerSubCommand: SimpleCommand = simpleSubCommand(
    1,
    CommandAlias("player", "p"),
    PlayerTabAutocomplete
) { args -> // none of the other crap, I don't think you should need anything but the command's arguments
    {
        val input = args[0]

        val offlinePlayer = try {
            sp.server.getOfflinePlayer(UUID.fromString(input)) // UUID
        } catch (e: IllegalArgumentException) {
            sp.server.getOfflinePlayer(input) // name
        }
        SubCommandResult.Success(
            setOf(
                PlayerAddGroupCommand(offlinePlayer),
                PlayerRemoveGroupCommand(offlinePlayer)
            )
        )
    }
}
That function could create a
SubCommandBase
you see.
SubCommandBase
should be renamed in any case though, as it is no longer a "Base" class.
Or even multiple functions for 1-n arguments
var playerSubCommand: SimpleCommand = simple2ArgSubCommand(
    CommandAlias("player", "p"),
    PlayerTabAutocomplete
) { arg1, arg2 ->
    {
        val offlinePlayer = try {
            sp.server.getOfflinePlayer(UUID.fromString(arg1)) // UUID
        } catch (e: IllegalArgumentException) {
            sp.server.getOfflinePlayer(input) // name
        }
        SubCommandResult.Success(
            setOf(
                PlayerAddGroupCommand(offlinePlayer),
                PlayerRemoveGroupCommand(offlinePlayer)
            )
        )
    }
}
❤️ 1
d

Dico

01/28/2019, 8:22 PM
Might be worth mentioning that
server.getOfflinePlayer
can return players that don't exist. You should check that the condition
it.hasPlayedBefore() || it.isOnline()
is true on the result.
That is, talking about the UUID version
a

Andrew Gazelka

01/29/2019, 8:21 PM
Thanks. btw @Dico I took inspiration from the link you posted (https://github.com/Mojang/brigadier) and created my own Kotlin version. Using the builder pattern is MUCH NICER than my old layout imo. https://gist.github.com/andrewgazelka/f5721f9bbb36827cae19224eeb426d4d
👍 1
d

Dico

01/29/2019, 9:31 PM
Very nice :) you mean you added some kotlin utilities for brigadier?
a

Andrew Gazelka

01/29/2019, 9:43 PM
nope... found it easier to just remake since there wasn't a whole lot of documentation and and I'll be generating help commands and whatnot which will take me a while to figure out how to do in brigadier
plus I wanted to structure things slightly different than brigadier
kinda a necro post but btw @Dico this is what I created https://github.com/SimplyServers/SimpleCommand
d

Dico

02/14/2019, 2:11 PM
If it works for your use case, that's great. Well done :)
a

Alan Evans

02/14/2019, 3:20 PM
Looks nice! Consider unit tests at some point.