1
\$\begingroup\$

This code is Express NodeJS code that handles post registration .post action where the user must select a custom username. The username is validated, and if valid, the database is updated. If not, the user is redirected to the page with an error message so he/she can try again.

I would like to know if you would refactor this code with the extract method, that is extracting some functionality into their own extracted method to improve fragmentation of this code and readability.

// This route is only used for newly registered facebook user so they can supply a custom userName.
users_router.route( "/userName" )

    // Process the customUserName recording.
    .post( function( req, res ) {
        // Receive data from the customUserName form field.
        let form = new forms.IncomingForm()
        form.parse( req, function( err, fields, files ) {

            let userModel = new User( fields ).userModel() // Instantiate current user
            let validationResults = new Validation( userModel, fields ).validationResults()

            // There is no error: Update the customUserName field in the database.
            if ( validationResults === g.error.no ) {
                let properties: string = req.session[ "userModel" ].id.content // Session holds copy of model
                let customUserNameField = fields[ "custom_user_name" ]
                let customUserNameData = { "facebookId": properties, "customUserName": customUserNameField }

                customUserNameDbUpdate( customUserNameData, function() {
                    console.log( "Custom user name was added" )
                    res.end( "ok" ) // TODO: redirect
                })
            }

            // There are errors: redirect back to user name selection page with error message.
            if ( validationResults !== g.error.no ) {
                let errorMessage = validationResults[ "custom_user_name" ].errorMessage

                res.render( "apps/users/views/chooseUsername.handlebars", { // TODO: should not redirect to template
                    error: errorMessage
                })
            }
        })
    })
\$\endgroup\$
1
\$\begingroup\$

It's not a good practice to put everything into the request parser. You can put there your formParser, but let an UserController.* handle the rest.

Think about your userActions (UserController.updateUsername) in a more abstract way, for example if you decide to setup an admin backend, where you have some other routes, you would like to reuse your code..

If you consider, to use a seed script, it could directly talk to the UserController, or modify values from a CommandLineTool.

The best scenario would be, if your routes just points to the controllers, so you could define a bunch in a single file, to see all your endpoints, and where they are pointing too.

One more hint: use a middleware to parse the form data.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. Well this is the userController.... I really tried to externalize the heavy duty code for reuse, and this is what I did with userModel, validationResults and customUserNameDbUpdate which can easily be reused. I thought that the simple conditional logic I left would not be bad practice. But I see your point which is certainly better. I need to read more mvc code, because this is as far as I could get with reading books. \$\endgroup\$ – Benj Jun 20 '16 at 8:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.