#Best practices for invalid param

1 messages · Page 1 of 1 (latest)

blazing garden
#

Honestly, didn't really know what to call this thread but I have a question about best practices. Okay so imagine I have a function getPartyMembers(Party party) that returns an int. The function also validates the input party. This is the important bit: If the input party is invalid, what's the best way to handle that? I was originally gonna throw an error but is that the best way of doing it?

stone latch
#

Why not Party::getPartyMemberAmount

#

Or something

#

getPartyMemberSize

blazing garden
#

Ngl just an example function

#

I'm just asking about functions with validation in general

#

Bad example xD

stone latch
#

Alr, well it looked more like a function that belongs to the data transfer object itself

#

Alright Alfie first of all you have to ask yourself

#

Should the function tolerate null as a valid input

#

Because maybe you just wanna map null to 0

#

But if not

#

Yield an unchecked exception as a means of telling the caller of the function that they provided an erroneous input

#

Yeah

#

Definitely

#

Yea sounds good

#

Anyway Alfie, error handling is harder than it looks like

#

And most people just go on autopilot and return null, or throw an exception without a good reason to do former or latter

blazing garden
#

The intention is provide as much info as possible

#

That's why I consider using errors, to point out specificly what's wrong

stone latch
#

Well the intention is to also with the help of the method name, clearly communicate the contract of the method

#

For example

#

if a method is named
getOrNull(Key key)

#

and it instead throws an exception (as opposed to returning null)

#

You’d say it does not follow its contract

#

Right?

blazing garden
#

Yeah

#

But, if I had a more complicated function that needs to check several conditions before running

#

My question is, is error handling the best way to convey exactly what went wrong?

#

As opposed to, like, setting some error holding variable like in C?

stone latch
#

Yeah so the way I think about it is first of all,
Does it make sense the way im doing this? Like, is this intended, is this expected?
Then, is this useful, is this usable?
Then, is this a good design, will it lead to issues in the future?
Then I also think, what’s the convention of the language? Does the language encourage result objects, or errors?
But the general rule if you don’t wanna be thinking so much is, “if the function cannot purposely fulfill its contract, that means what its intended to do, then it needs to throw an error” I think it was

#

Now that last rule becomes a bit hard if you per say have Map::get(Key)

#

Cuz like, should you return null, or should you throw NoSuchElementException?

#

But now, think about it, is it the programmers fault that the element may not exist in the map? No. Did the function fulfill its contract by returning null? Technically yes, because the entry for the given might have been absent so no retrieval process, and the stored value might be null. Therefore returning null may be the better choice.

#

Hope this helps, good luck.

blazing garden
#

Right thanks

#

I just didn't really wanna use errors since like, I'm used to only throwing them when something avoidable happens

stone latch
#

Yeah

#

Well if the programmer did something wrong, it can often be worth throwing an unchecked exception

#

Because it normally shouldn’t ever be thrown, its just that if the programmer did something wrong

#

If something wrong can happen but its not the programmers fault, then a checked exception is better

#

for instance, the database is down, or the file content is wrongly formatted

#

(But yeah sometimes you wanna avoid throwing and just returning null, or a result, or unchecked exception due to performance simply, or to avoid cluttering every method with checked exception signatures)

blazing garden
#

Oh, I'm more referring to things that aren't really errors, it's just that the function doesn't do what the code is asking it to do. Like, for example, if #Party.addPlayer(Player player) is called, but there's already the maximum number of players in a party

#

I definitely wanna check the player number in this function because otherwise it could be prone to errors

stone latch
#

I’d have another function to get the data whether max players is reached

#

Cause like you’d be running into that the function does more than one thing essentially (which sometimes has to be compromised)

#

But here you do have the choice to just use 2 functions

#

But yeah, the only way 2 functions can be prone to errors is if you do this concurrently

blazing garden
#

So you'd say (in this instance) to return false in the addPlayer() function, and just validate in the main script?

stone latch
#

Na more like

#

Party::isFull

blazing garden
#

Well, to validate in both places but handle it in the main script

stone latch
#

Party::addPlayer

blazing garden
#

So like: ```
class Party
boolean addPlayer() # return true if successful, false if full
boolean isFull()

class CommandHandler
addPlayerTo(Party p) {
if (isFull()) # Error handling
else # Success code```

stone latch
#

(But yeah addPlayer returning a result object, in ur case a boolean isn’t totally out of scope of either, it’s just to me that the function might be doing a little too much)

Yeah that’s nice

#

This way you can check if the party is full even if you don’t have a player with you, but also the party might also be full due to other reasons, where if you combined it with addPlayer that’d just pollute addPlayer

#

There are some advantages and disadvantages (here I’d argue you have more advantages by splitting it up)

blazing garden
#

I suppose

blazing garden
stone latch
#

(Of course in addPlayer you still check if the player can be added)

#

Hmm

#

Well there is one principle

#

Called command and query separation principle

#

But its not always applicable

#

Anyway alfie I should say one other thing

#

having
boolean addPlayer(Player) can make sense if you are writing a high level api

#

Since you often wanna reduce and condense complexity of ur logic

#

But at a low level you want configurability and explicitness

blazing garden
#

The idea behind my current plugin is to act as an API as much as possible

#

So I wanna make it easier for third parties to use

stone latch
#

May I ask have you ever looked at luckperms code?

blazing garden
#

I don't think so

stone latch
#

Alright, because it has a really good way of dealing with its api

blazing garden
#

Alright I'll check it out

stone latch
#

Its hard to tell you, its more like you have to explore it yourself, mainly since I assume you don’t know a lot of design patterns

blazing garden
#

No not really, that's why I've come to ask about best practices

#

I have read through a couple of plugins but mostly to see how they tackle specific problems

stone latch
#

Yeah, understandable, well basically if I were to tell you about it I’d be using terms like adapter, bridge and abstraction layer.

#

Fair enough

#

Anyway try to understand the design of luckperms api, you can ask here if you have any questions, by that I mean api + implementation

blazing garden
#

Thanks!

stone latch
#

:)

stone latch
#

Yeah its good quality altho there are a couple of things I don’t agree with regarding its design but yeah

#

One thing I don’t like is that the design is so centered around passing (iirc LuckPermsPlugin class) around every other class

#

Its almost as if could be turned into a singleton since its pretty much a context object that couples together high level dependencies on a global level.

#

Or if not singleton, I’d probably try to create an intermediate abstraction layer between that class and all the classes that depend on it.

#

Also I’m really not sure if a reentrant lock for the file storage implementation is the best option, I think a normal stamped lock would do just as good since iirc they don’t really take advantage and use the reentrant functionality of a reentrant lock.

#

(Reentrant locks are slow, altho iirc faster than synchronized, but still slow due to being reentrant)

#

Those are good starting points

#

I don’t remember everything since it was a while I read through the implementation.

#

That looks great

#

Yeah if it reduces whatever boilerplate you have to write, then yeah, if you wanna support other data storage types just facade those behind a common interface I’d say

#

I think luckperms does this excellently by having one class that wraps this facade around with promises/completables also, which is a pattern named humble object principle

#

Maybe just with a vararg?

#

Object… params) at the end

#

Aight that’s dope

#

Yeah, well just don’t go with a blob I suppose since that’d defeat the entire point of using sql

#

Yeah maybe that’s fine

#

But like, if possible, take advantage of being able to structure the data

#

There are alot of things to think about when you do sql stuff, some of them being that you wanna be able to do data analysis (which isn’t impossible with no sql, just not as designed for it compared to sql), being able to do complex transactions (ACID), being able to handle relational data effectively. So just blobbing a key | value table would ruin these key features. But sometimes it can be fine to compromise (like with an itemstack)