Open ia3andy opened 1 year ago
redirect(Todos::todos);
If validation fails, that makes sense to redirect at the condition that you redirect to perhaps another page as here redirect
is used no matter if it succeeds or fails --> redirect(Todos::todos);
Mind that with the suggested syntax, there is no need for comments anymore, it's self explainatory.
redirect(Todos::todos);
If validation fails, that makes sense to redirect at the condition that you redirect to perhaps another page as here
redirect
is used no matter if it succeeds or fails -->redirect(Todos::todos);
I don't think that the page you redirect to is changing the issue. Whether you redirect to the same page or not, I am saying it should show expressly that the flow is stopped with a return
.
You are right though on the fact that if the validation failed, you might want to add some error information. So maybe a throw would be more relevant in that case, something like:
@POST
public void add(@RestForm @NotBlank String title, @RestForm @NotBlank String note) {
if(validationFailed()) {
throw new RedirectException(Todos::todos, getValidationErrors());
}
Note todo = new Note();
todo.task = task;
todo.persist();
redirect(Todos::todos);
}
Mind that with the suggested syntax, there is no need for comments anymore, it's self explainatory.
For sure but I suggest then something like this as example ;-)
@POST
public void add(@RestForm @NotBlank String title, @RestForm @NotBlank String note) {
if(validationFailed()) {
redirect(Todos::wrongFormTodo);
return;
}
Note todo = new Note();
todo.task = task;
todo.persist();
redirect(Todos::todos);
}
This syntax is even better
if(validationFailed()) {
throw new RedirectException(Todos::todos, getValidationErrors());
}
Question:
{errors}
?throw new RedirectException(Todos::todos, getValidationErrors());
is what: todos()
does, but with lots more boilerplate ;)
I understand your initial reaction about this, but give the current redirect feature a try, I swear it will grow on you to the point where you never want to use anything else ;)
Hm, and this one?
@POST
public void add(@RestForm @NotBlank String title, @RestForm @NotBlank String note) {
// If validation fails, redirect to the todos page (with errors propagated)
if (!validationFailed()) {
// save the new Todo
Note todo = new Note();
todo.task = task;
todo.persist();
}
// redirect to the todos page
todos();
}
That's fine too, but naive in most scenarios, because quite often, there's more calls to validationFailed()
as validation rules are added as new conditions. Also, it doesn't actually solve their issue of showing that todos()
is a throwing method.
Hm, and this one?
@POST public void add(@RestForm @NotBlank String title, @RestForm @NotBlank String note) { // If validation fails, redirect to the todos page (with errors propagated) if (!validationFailed()) { // save the new Todo Note todo = new Note(); todo.task = task; todo.persist(); } // redirect to the todos page todos(); }
when you have a problem, the solution is to remove the problem! clever boy π
Still this is a different coding style and I think allowing the if at the top way of doing things is important.
is what: todos() does, but with lots more boilerplate ;)
I understand your initial reaction about this, but give the current redirect feature a try, I swear it will grow on you to the > point where you never want to use anything else ;)
@FroMage TBH I am fine with the syntax and could live happily ever after with it. Still I think a lot of people might turn around because of this kind of thing. So better give them a way in :)
@fromage FYI @mcruzdev had the same feeling I had on this one
Totally agree with @ia3andy Another strong argument beside that a Java programmer can't understand this without reading documentation or go deep into the framework. Using
if (validationFailed()) {
editBlogEntry(id);
// Redirect to this blog entry (no need to return, this will stop here because of an internal exception)
}
always leads to a redirect. In my application and many others you don't do a redirect in case of an error, but instead just render the page (editBlogEntry) without a redirect. A redirect leads to bad usability. You loose you form-data after a refresh (flash is just holding it for the first redirect).
The rule of thumb is: Do a redirect in case of success and just render the page in case of an error
I disagree with that statement, especially in Renarde, we always redirect back to the form, preserving the values, in case of errors. That's the most useful thing to do in most cases. Why would you say it's bad usability?
If a user submits a form with invalid data and an error occurs, redirecting to the same form may cause all the input fields to be cleared. This forces the user to re-enter their data, which can be frustrating, especially for complex or long forms. Even though techniques like using flash storage can temporarily hold data, it is only available for the first redirect and can be lost if the user refreshes the page, leading to potential data loss. A better approach in many cases is to render the page with the form again, showing the validation errors while preserving the entered data. This allows users to correct their input without having to re-enter everything, providing a smoother and less frustrating experience.
Best practice: The best practice regarding handling POST requests is to redirect only after a successful submission but to re-render the page with validation errors if the form fails. This approach is commonly used to prevent data loss and improve user experience.
This pattern of redirecting on success but rendering the form again on failure ensures users donβt lose their input and reduces frustration in cases of validation errors.
Remember also: Some application may choose not to use the web-session at all (because of some architecture decisions like scaling issues; function as a service) They can't even use the flash-context. Meaning the use of Renarde wouldn't work at all.
I didn't know about it, but what @mr-mos proposed seems like a better approach
This would also solve #192
Currently, it works like this:
Some could argue that it is not obvious in a java world that
todos();
is stopping the flow of the method. It would be nice to offer an alternate more declarative way of doing it. Something like π would make me happy :) :