#correctness of model/controller logic in an http server?

1 messages · Page 1 of 1 (latest)

honest meteor
#

got into a heated debate about the correctness of this logic that i wrote

#[post("")]
async fn sign_up(
    db: Data<mongodb::Database>,
    http_client: Data<reqwest::Client>,
    payload: Json<SignUpPayload>,
) -> Response {
    let SignUpPayload {
        username,
        password,
        email,
        captcha_tokens,
    } = &payload.0;

    if !captcha::are_tokens_valid(&http_client, captcha_tokens).await? {
        unauthorized!();
    }

    if User::exists_with_username(&db, username).await? {
        forbidden!("username is being used");
    }

    if User::exists_with_email(&db, email).await? {
        forbidden!("email is being used");
    }

    QueuedRegistration::builder()
        .username(username)
        .hashed_password(&hash::apply(password)?)
        .email(email)
        .build(&db)
        .await?;

    ok!();
}

User & QueuedRegistration being database models, and the post request handler being the controller

according to the guy i was arguing with, he claimed that the logic that i'm handling inside of the controller shouldn't be in the controller since it makes it "unmaintainable" and i was left pretty dumbfounded.

it may just be me, but this seems pretty damn maintainable?

just looking for some second opinions here

full star
#

It's perfectly fine for me, idk

#

The only thing I see is that theoretically someone might be able to create 2 accounts at the same time, thus bypassing some checks

#

That'd probably be caught by unique key, but yeah

shy barn
#

is this one of those cases where some web developer uses an ancient full stack framework and every other line of code is in a different file because apparently that's "clean code"

full star
#

M8's using React for auth

placid stratus
#

he probably meant the actual business registration logic should be abstracted away from the route itself - which makes sense if there could be multiple things calling that.

full star
placid stratus
#

but at the same time it's so easy to abstract out later...

shy barn
#

i don't see why there would be more than one endpoint that needs to register

full star
#

Maybe OAuth2 autoregistration 🤷

placid stratus
#

depends on scale

shy barn
#

I don't think having more classes is necessarily more maintainable

#

sounds like OOP propaganda to me

honest meteor
shy barn
#

I wouldn't even have a QueuedRegistration class tbh

honest meteor
shy barn
#

You could abstract away sending emails but i don't see a reason to abstract away anything else tbh

full star
#

Tbf people can just create a huge ton of email accounts 🤷

#

Not really an issue for them

misty mason
#

You use whatever works best for your project. MVC just happens to be a good structure for CRUD apps with views, but its not the only way.

shy barn
#

that doesn't mean you need to abstract away every single CPU instruction inside each controller

honest meteor
misty mason
#

I frequently find a lot of my projects becoming MVC 😔

honest meteor
#

i thought the point of the controller was to handle validation

shy barn
#

imo

misty mason
#

Eh. The point of a controller is mostly to route stuff and normalize data.

honest meteor
#

well i guess not the whole point, but its one of the tasks the controller is meant to handle

shy barn
#

it's a pretty dumb system if you can't do anything in a controller

honest meteor
#

the model is just supposed to be an interface between the database and the controller

honest meteor
shy barn
#

i just write my code however i like

#

in my most recent projects, my API models are in a separate library crate so i can access them nicely from the frontend

keen forge
#

if you are using this authentication a lot some middleware would be beneficial....

misty mason
#

I was given some really good advice recently - write your code as if you have infinite memory, then work your way down from there.

honest meteor
keen forge
#

yeah so the dude is being dumb then

misty mason
#

Looking at your code more closely now - I like this pattern. Its actually what the linux kernel uses I think. Gating is really powerful.

Although - I've found myself putting that logic into the models, since I like the idea of my model telling me if I've created my thing wrong. It helps the controller simply be create_new_user()

#

There isn't anything wrong with it being here though

#

especially if you need to be able to make users without being blocked by these requirements.

olive spear
#

I don't have all the context, but I'll just say that following these in order of highest to lowest priority is good:

  • KISS (Keep It Stupidly Simple)
  • WET (Write Every Time)
  • DRY (Don't Repeat Yourself)

Because Duplication is cheaper than a wrong abstraction

shy barn
#

you're not making the code any better

#

you're just moving it around

#

which arguably is worse for readability because now you need 5 files open instead of just 1

misty mason
#

in a situation like this where you're likely not going to be reusing the model creation code everywhere, it really doesn't matter. What I'm talking about is important if you have like five different places calling to make a User.

shy barn
#

also

#

my models to the database aren't going to be the same as to my endpoint api

#

so i need to do some conversions somewhere

keen forge
misty mason
#

isn't the idea of a model to give you your own standardization that you can use in the rest of your code, to allow all data cares to stay in one location?

full star
shy barn
misty mason
#

not sure I understand.

honest meteor
#

i find this to be the funniest part of the debate

#

basically

#

"i'm right, you're wrong"

shy barn
#

yes thats exactly what i was talking about lmao

honest meteor
#

🤣

keen forge
#

actual lunacy

#

I hate that C# code so much

#

in C#, using async like that is a performance COST

#

your methods don't cost nothing, the compiler will inline them

misty mason
#

I have written wrappers around stuff literally just cause I didn't like the wording they're using.

keen forge
#

but in C# Tasks cost shit to make

#

so him doing await on a SINGLE TASK only to make ANOTHER TASK is a WASTE OF TASKS!!!

#

if you remove the async and await from that code it literally will be more efficient

honest meteor
#

nah he's right and we're all automatically wrong

keen forge
#

massive tangent rant but god

#

pet peeve of mine

keen forge
#

because in rust futures are built like iterators which means the type systems handles this for you

#

in other langs it has to be on the heap and need their state machine to be generated at runtime

full star
#

Just like this is the most efficient sum

const counter = await createAsyncCounter(0);
for await (const x of await createAsyncArrayIterator(nums)) {
  const x$1 = await x();
  await counter.add(await x$1.getValue());
}
return await counter.getValue();
keen forge
#

I wanted to assume the best for this individual

#

but the fact they don't care about at least minimizing this one thing is egregious

#

wrappers in rust don't matter really, they just MIGHT make your code messy