0
\$\begingroup\$

I have to handle an Action (Modify) about my web application.

The app working with SMTP Email, ActiveDirectory, External System (Biztalk, CRM, FTP, others WCF Services) and Internal System (WebServices and Databases SqlServer-Oracle).

The main action checks several conditions in several entities. There are duplicate calls to methods: SendEmailForNotify, EnableUserInExternalSystem, DisableUserInExternalSystem, InactiveUserInInternalSystem, ...

I'm looking for two kinds of feedback:

  • Is there a better way of doing what I'm attempting to do, for improvement code and avoid duplicated code?.

  • How improvement error handling?. Is it possible "transactionally" for several actions managing several systems (Email, AD, CRM, Database Sql Server)?

Any crazy idea: using State Machine (maybe Circuit Break?)

For example, a flow:

DeleterUserInAD => DisableUserInExternalSystem => InactiveUserInInternalSystem

There are problems if were errors any method. Not transactionally, how to do "Rollback"?.

Sample: If fails InactiveUserInInternalSystem, then the good rollback it will be CreateUserInAD => EnabledUserInExternalSystem

For me is complex. "Rollback actions" can be fails to.

Code: http://pastebin.com/isf6xB00

    public static bool HandleActionModify(ENT.CustomData dp, ENT.User up, ENT.User upLogged, ENT.User upReaded, out string errorMsg)
    {
        errorMsg = "";
        var isClient = up.IsClientWithRoleA();
        var okInAD = ADOperations.IsUserInAD(dp.UserLogin);

        #region Enable Client
        if (up.IsEnabled && isClient)
        {
            if (!okInAD)
            {
                Logger.Trace("[MyModule] - ERROR. User not in AD");
                errorMsg = "User not in AD";
                return false;
            }

            var ok = EnableUserInExternalSystem(dp.UserLogin);
            if (!ok)
            {
                errorMsg = "Cannot enabled to user in External System";
                return false;
            }

        }
        #endregion

        #region Enable NotClient

        if (up.IsEnabled && !isClient)
        {
            if (!okInAD)
            {
                var userCanAddToAD = (upLogged.IsNotClientWithRoleB() || upLogged.IsClientWithRoleA())
                    && !up.IsClientWithRoleA();

                var okAD = false;
                if (userCanAddToAD)
                {
                    var errorMsgAD= "";
                    okAD = ADOperations.AddUserToAD(dp, isClient, upReaded, out errorMsgAD);

                    if (!okAD)
                    {
                        Logger.Trace("[MyModule] - ERROR. AddUserToAD. " + errorMsgAD);
                        errorMsg = "Cannot create user in AD";
                        return false;
                    }

                    if (okAD)
                    {
                        Logger.Trace("[MyModule] -  OK AddUserToAD. ");
                    }
                }

            var ok = EnableUserInExternalSystem(dp.UserLogin);
                if (!ok)
                { 
                    errorMsg = "Cannot enabled to user in External System";
                    return false;
                }

            }

            if (okInAD)
            {
                var okSend = SendEmailForNotify(dp.UserLogin);
                if (!okSend)
        {
                     Logger.Trace("[MyModule] -  ERROR SendEmailForNotify. ");
                }

            var ok = EnableUserInExternalSystem(dp.UserLogin);
                if (!ok)
                { 
                    errorMsg = "Cannot enabled to user in External System";
                    return false;
                }

            }

        }
        #endregion

        #region Disable Client

        if (!up.IsEnabled && isClient)
        {
            if (!okInAD)
            {
                Logger.Trace("[MyModule] - ERROR. User not in AD");
                errorMsg = "User not in AD";
                return false;
            }

            var ok = DisableUserInExternalSystem(dp.UserLogin);
            if (!ok)
            {
                errorMsg = "Cannot disabled to user in External System";
                return false;
            }

            var okInactive = InactiveUserInInternalSystem(dp.UserLogin);
            if (!okInactive)
            {
                errorMsg = "Cannot inactive user in Internal System";
                return false;
            }
        }
        #endregion

        #region Disable NotClient

        if (!up.IsEnabled && !isClient)
        {
            if (!okInAD)
            {
                var okSend = SendEmailForNotify(dp.UserLogin);
                if (!okSend)
        {
                     Logger.Trace("[MyModule] -  ERROR SendEmailForNotify. ");
                }

                var ok = DisableUserInExternalSystem(dp.UserLogin);
                if (!ok)
                {
                   errorMsg = "Cannot disabled to user in External System";
                   return false;
                }

                var okInactive = InactiveUserInInternalSystem(dp.UserLogin);
                if (!okInactive)
                {
                   errorMsg = "Cannot inactive user in Internal System";
                   return false;
                }

            }

            if (okInAD)
            {
                var okAD = ADOperations.DeleterUserInAD(dp.UserLogin);
                if (!okAD)
                {
                    errorMsg = "Cannot delete user in AD";
                    return false;
                }

                var ok = DisableUserInExternalSystem(dp.UserLogin);
                if (!ok)
                {
                   errorMsg = "Cannot disabled to user in External System";
                   return false;
                }

                var okInactive = InactiveUserInInternalSystem(dp.UserLogin);
                if (!okInactive)
                {
                   errorMsg = "Cannot inactive user in Internal System";
                   return false;
                }
            }
        }

        #endregion

        return true;

    }
\$\endgroup\$
  • 1
    \$\begingroup\$ Avoiding code duplication is one of the many generic common requests around here. Our site standard is to title questions according to the task that the code accomplishes. (See How to Ask.) Maybe something like "Controller method to modify Active Directory and send notification"? \$\endgroup\$ – 200_success Aug 2 '16 at 7:53
1
\$\begingroup\$
  • Having one or multiple regions in one method is a big no go because it is telling you that the method in question is doing too many things, hence violating SRP. One could say each reagion should be extracted to a separate method.

  • naming things is a hard task, but you really could do this better. Names like dp, up, upLogged, upReaded won't tell you anything what they are about. If you come back to this code in a few months to fix an issue or to add a feature you will have a hard time to understand it and the task in hand will take longer than needed.

  • if you use a variable only once, you could just omit it. Like

    if (!EnableUserInExternalSystem(dp.UserLogin))
    {
        errorMsg = "Cannot enabled to user in External System";
        return false;
    }
    
\$\endgroup\$

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.