Recent searches
Search options
As a Rails dev, when you see code like this - what are your thoughts? 
@solnic I don’t hate it. I think the final exception handle should be done on a controller level, instead of an action level, depending on what you want the end user experience to be
@Ryanbigg thanks, this is helpful! So, to quickly explain the reasoning - I don't want controllers to be coupled to low-level database errors, also: db exceptions are *expected* to happen and I prefer if it doesn't leak to the controller layer. User experience should be the same regardless of the failure type - get a notice that something went wrong (with useful details how to proceed, if possible).
@solnic @Ryanbigg I like that you're experimenting in this space!
What I'd worry about here is that every single controller action now probably needs to include the exception cases in their pattern matching over the operation's result? This would be a bunch of repetition for something that you might otherwise just want to handle in the same way across all controllers and actions?
@solnic as a very average developer, with strong bias towards convention, and away from fancy-fancy code, I don't understand it at a glance
@emma that's understandable, thanks :) if this code looks strange, it's probably because of the usage of Pattern Matching 
@solnic On the other-other hand, I have been running a software company as a working developer for 15 years so if I don't understand it, maybe don't use it in teams with a range of skills.
@emma sure, there’s a lot of value in well known conventions that most people are familiar with
@solnic Yeah. If you're getting likes from superstar-tier developers like RyanB / JoelD / NoahG etc then there's your answer about what sort of audience it's good for currently. I'll stick with my ExtremeConvention approach and selfishly leave it to you to make the fancy-new things palatable enough to become the new convention in the future.
@solnic Without knowing how it works, it seems like the api is really opaque. What's going on if Failure can take such different args? (I don't want it explained, though -- just saying what my impression is!)
@solnic Seems fine, particularly if CreateUser is large or complicated. It's keeping the controller *just* in charge of routing URLs, which is a pretty good thing for it to do.
If CreateUser is very small or simple then it's a lot of surface area for a very small amount of logic.
@codefolio thanks! The command handles params processing, validating and persisting data via a simple `User.create!(safe_validated_data)` method call, so that's quite a lot
@solnic beautiful 
@solnic at first glance I didn’t think it was Ruby. I understand it but don’t like it. The use of the Failure and Success classes(?) are just weird to me
@jamie hah yeah, people need to catch up with Pattern Matching, it's regular Ruby code in 2024, but it's still not popular. Trying to change that 
@solnic no, I got the pattern matching it’s the DRY style I dislike 
@jamie what's DRY style?
@solnic I think you’re more familiar with dry-rb (sorry, forgot the correct name) than I am
@jamie ah you're talking about the usage of a stateless object with `call` method I suppose :)
@emma @jamie it really depends on how this is used exactly. Over the years Rails devs have been trying to use such objects with more or less success (or complete lack of success). I wouldn't throw this idea away completely though. YMMV but there’s a lot of power and freedom in objects that don't have to be mutated and can be composed more easily.
@solnic I like it. Currently I'm using something doing with sunny/actor gem but with if/else:
result = Users::Create.result(attributes: user_params)
if result.success?
...
else
...
end
But result is just a hash, so I think it could be done with pattern matching.
@solnic seems a little fancy for the sake of it but I can get the gist at a glance.
What's `Success(User => user)` for though? If `user` is a bound variable it doesn't seem to be used. Did you mean to do something like:
redirect_to user_url(user)
Also why is the success API `User => user` but failure is `user: user`? The inconsistency adds cognitive load
@sfcgeorge ahh great feedback, thanks! `user`is actually used in the original code, I just missed this fact in the example (I tried to make it shorter oh well). Really good point about `User => user` vs `user: user` though. It's something I should unify. Really appreciate your reply!
@solnic hey I like this quite a bit! I’ve been using result objects to represent the results of operations successfully! nice that there’s pattern matching available now! does it work?
@eljojo yes it works very well!
@solnic that’s really nice! is it public somewhere? would love to play! in the past i’ve used this for result objects, but had to throw out in favour of something that works well with Sorbet typing https://github.com/github/github-ds/blob/master/lib/github/result.rb
@solnic for these cases of failure, what works well for me is to create exception classes that store the context about what went wrong, so the controller/caller can better see. I avoid returning runtime error or generic exceptions, and try to store as much content within as possible.
1- you don’t need command pattern in Rails (this is what BG jobs are for). user_creator.create(user_params) is more clear to me
2- the pattern matching is fine but I wouldn’t use it, personally
3- Success vs Failure is too general - I prefer specifically-named outcomes. So Created instead of Success
4- validation errors and exceptions should not be conflated - there is no reason to. Plus you cannot coerce all exceptions into a monad so you end up needing to catch exceptions anyway
@solnic any time I see `type` I reflexively think that should be a new class. It signals that it’s a _kind of_ Failure. I’m not sure if that is at odds with conventions of pattern matching
@soulcutter yes you could introduce a dedicated type of failure but for me it would be unnecessary. All that matters is that the operation either succeeded or not and different types of failures should have different handling.
@solnic I think these are contradictory:
> All that matters is that the operation either succeeded or not
> different types of failures should have different handling.
@soulcutter it's not because you pattern match however you like, that's the beauty of it. You don't need to add dedicated failure classes.
@solnic This may be a reflection of it being an example meant to illustrate principles, then?
I am imagining that a more-realistic version would use a non-specific case to catch failures that it has no specific handling for:
```
in Failure
redirect_to users_index_url, notice: "Something went wrong."
```
with no type / reason.
@soulcutter so I actually want unhandled exceptions to blow up the app and invoke generic exception handling. That's why, under the hood, command objects only catch very specific exception types and return Failure, while every other exception just blows up processing and you'd get 500 and I'd get an exception notification from an exception tracking system.
@solnic I imagine that traditional exception handling would work for that
@solnic I might be bogged down in the details - I'm not sure if that was the level of feedback you wanted to solicit. In general I like this kind of experimentation and also think pattern matching has a lot of potential.
@soulcutter @solnic I agree with Peter, you usually don't need an actual class for this in most cases, although you can make one if it makes sense.
I write this using the tuple shorthand as a sort of sexp with the failure name as the node type
@soulcutter @solnic The most important thing, the part that *really* matters, is that you name every failure case.
This was a revelation. The problem with exceptions is that there is simply no way to look at code and tell how many it might throw. Don't get me started about `nil`.
I think Go was entirely on the right track with "errors are just values", they get shit for it because there is no pattern-matching so it becomes tedious.
@alassek @solnic Oooh, you mentioned something I don't agree with:
> The problem with exceptions is that there is simply no way to look at code and tell how many it might throw.
What you say is true, but I don't see it inherently as a problem. I see a connection between that mode of thought and static typing. I've been happy without it since I left Java many years ago (which does not solve THAT problem, but I digress).
Go's approach is ehh, ok. Agreed about its tedium w/o pattern matching
@soulcutter @solnic When your goal is to exhaustively handle all failure cases, exceptions hurt you more than they help due to their non-locality. It's just easier to work with locally-returned values.
I make a distinction between normal errors and exceptions. An unrecoverable situation like the DB connection being down is an exception. The user input failing validation is an error. I don't want to deal with them the same way.
My business logic follows "Functional Core, Imperative Shell"
@alassek @soulcutter yes you put it very well!
@solnic Frankly speaking my first thought is that it's so Elixir-like code
But that's probably because I am lucky that I spend lot of time lately in Elixir beside Ruby.
@bosko yes exactly that's why I like it so much hah!
@solnic I just wrote some code like this yesterday, so I guess I think “ooh, it’d be useful if my failure patterns distinguish exception from other errors”
@solnic why are you writing Java as if it’s Elixir in your Ruby?
@Sci_Phi the java argument was really boring in 2010 already
@solnic I’m not slagging Java, I’m saying User.new is already there, User.special_factory is readily doable, so SomeOtherClass.foo that returns a User instance is not Ruby-ish to me
@solnic This is how I want all controllers to look.
They exist to validate and map inputs to HTTP output
@solnic it’s easy to understand because I am used to this in Scala. I actually tend to write ruby this way too. But it baffles my colleagues. They do not think in pattern matching having only experience in ruby or golang
@solnic “Ah, it’s one of those apps that constantly fights Rails. But I like how this piece over here looks like.”
@solnic I love it
@solnic My first reaction thoughts are:
1. Classes named VerbNoun cause those files to be sorted by verb, not noun (CreateApple, CreateOrange, CreatePear…) which I find unhelpful
2. I would expect User.new to serve the purpose of what CreatUser does. If it’s big and complicated, it’d be User’s responsibility to extract to methods or a class. IMO
3. The case line is doing a couple things, I’d expect/prefer it to be a named variable on a separate line (result = CreateU…; case result)
4. Can render be passed locals: like that? Or do views need @ivars?
5. Is users_index_url on purpose or a typo? I’d expect just users_url
6. Nowhere is `user` explicitly set in this code, so I am/I’d be confused and surprised if those lines worked 
4. Yes they can, I really don't like mixing ivars and locals in views and prefer locals
5. It does work, it was generated by Rails originally
6. This is from Pattern Matching in Ruby. By matching you declare a local variable, it's returned in the result.
...and thanks for feedback!
1. I just followed Rails conventions where you group stuff top-level ie `apps/commands`. Personally, I group by concepts in my real apps.
2. The whole purpose is to not use AR for this stuff and use data as data (and treat AR objects as just data as a consequence)
3. Sure, that's a minor formatting/style improvement
* Commands::CreateUser.new and subsequent .call seem redundant since the initializer doesn't take any arguments. Why isn't there a self.call method?
* Why are result objects being used to represent an exception? A rescue clause should be used to handle exceptions. Better yet, why not let the global exception handler deal with any unexpected DB exceptions?
* There seems to be a lot of repetition with passing local variables to views. Maybe use the new Hash syntax `{user: }`?
@solnic also if the local variable hash keys will always match the local variable names, why not use instance variables? If you don't want to pollute the context with ivars, maybe make a helper method which calls `render :new, locals: {...}` but takes positional arguments?
@postmodern yes normally I'd have a helper method for rendering