Code Behind to manage AD users for a CRM system

Question

I have big class in ASP.NET WebForms (legacy). I use VS 2012.

I want to evaluate Conditions and execute Actions - Commands like:

Create in AD

Delete in AD

Enable in CRM

DesEnable in CRM (Delete in CRM)

Delete Licenses CRM

Send Email Warning 1

Send Email Warning 2

ShowNotification

TODO code: (without error handling)

if (ActionTypePage == ActionType.Modification || ActionTypePage == ActionType.MyDataMod)
 if (user.Enabled && isEmployer)
 {
 // 1a) (Employer) NOT exists in AD ==> Error
 if (!existsinAD)
 {
 ShowNotification(msg, CommunicationType.Error, true);
 return;
 }
 // 1b) If Employer exists in AD ==> Enable in CRM (If false, New Request to CRM)
 // Enable in CRM (If false, New Request CRM)
 }
 if (user.Enabled && !isEmployer)
 {
 // 2a) If NOTEmployer NOT exists in AD ==> Create user in AD y Enable in CRM (If false, New Request CRM)
 if (!existeinAD)
 {
 // Create in AD
 // Enable in CRM (If false, New RequestCRM)
 }
 // 2b) If NOTEmployer exists in AD ==> Warning (Email) y Enable in CRM (If false, New RequestCRM)
 if (existeinAD)
 {
 // Send Email Warning
 // Enable in CRM (If false, New RequestCRM)
 }
 }
 // DES-Enable
 // 1a) If (Employer) NOT exists in AD ==> Error
 // 1b) If (Employer) exists in AD ==> DesEnable in CRM (Delete in CRM) - Delete Licenses CRM 
 if (!user.Enabled && isEmployer)
 {
 if (!existsinAD)
 {
 ShowNotification(msg, CommunicationType.Error, true);
 return;
 }
 // DesEnable in CRM (Delete in CRM) 
 // Delete Licenses CRM 
 }
 if (!user.Enabled && !isEmployer)
 {
 // 2a) If NOTEmployer NOT exists in AD ==> Warning (Email) y DesEnable in CRM (Delete in CRM) - Delete Licenses CRM 
 // Delete Licenses CRM 
 if (!existeinAD)
 {
 // Send Email Warning
 // DesEnable in CRM (Delete in CRM) 
 // Delete Licenses CRM 
 }
 // 2b) If NOTEmployer exists in AD ==> Eliminar de AD y DesEnable in CRM (Delete in CRM) - Delete Licenses CRM 
 if (!existeinAD)
 {
 // Delete in AD
 // DesEnable in CRM (Delete in CRM) 
 // Delete Licenses CRM 
 }
 }

Full code:

 if (ActionTypePagina == ActionType.Modificacion || ActionTypePagina == ActionType.MisDataM)
 {
 var bAccionOk = false;
 var sPassword = DataPersonales.GenerarPassword();
 var dpDataPersonales = fillDataPersonales(sPassword);
 var userPortal = fillUserPortal();
 var isEmployer = userPortal.IsEmployerRolAorRolB();
 var existsEnAD = ADOperations.ExistsUserEnActiveDirectory(dpDataPersonales.sUser);
 //var userConsultado = setUserConsultado();
 if (userPortal.bIs_Enabled.Value && isEmployer)
 {
 // 1a) Si RolA, RolB (employer) NO exists en AD ==> Error
 // 1b) Si RolA, RolB (employer) exists en AD ==> Enable en CRM (si false, Request New CRM)
 if (!existsEnAD)
 {
 var msg = String.Format("El employer {0:-10} no exists en ActiveDirectory", dpDataPersonales.sUser);
 var log = String.Format("User: {0, -15} - Error: {1:-100}", dpDataPersonales.sUser, msg);
 LoggerAD.Trace("[AdminDataUsers] - Modificación ERROR. ExistsUserEnActiveDirectory. " + log);
 ((NsiMaster)Master.Master.Master).ShowNotification(msg, CommunicationType.Error, true);
 return;
 }
 // Enable en CRM (si false, Request New CRM)
 var okEnable = AdminManager.EnableUserEnCrm(dpDataPersonales.sUser);
 if (!okEnable)
 {
 var msg = "No es posible habilitar al usuario en CRM. Realice una Request New CRM";
 ((NsiMaster)Master.Master.Master).ShowNotification(msg, CommunicationType.Error, true);
 return;
 }
 }
 if (userPortal.bIs_Enabled.Value && !isEmployer)
 {
 // 2a) Si NO RolA, RolB (mediador) NO exists en AD ==> Alta en AD y Enable en CRM (si false, Request New CRM)
 if (!existsEnAD)
 {
 var permisoOK_AltaAD = (UserLogado.EsMediadorMA() || UserLogado.IsEmployerRolAorRolB())
 && !userPortal.IsEmployerRolAorRolB();
 var okAD = false;
 if (permisoOK_AltaAD)
 {
 var msgEstadoAD = "";
 var NombreCompleto = dpDataPersonales.sNombre + " " + dpDataPersonales.sApellido1 + " " + dpDataPersonales.sApellido2;
 okAD = ADOperations.AddUserEnActiveDirectory(dpDataPersonales.sUser, dpDataPersonales.sPassword,
 dpDataPersonales.sNombre,
 dpDataPersonales.sApellido1 + " " + dpDataPersonales.sApellido2
 , NombreCompleto
 , isEmployer, UserConsultado.sUser_Aire, UserConsultado.sCartera
 , dpDataPersonales.sEmail, dpDataPersonales.sTelefonoFijo, dpDataPersonales.sTelefonoMovil
 , out msgEstadoAD);
 if (!okAD)
 {
 var log = String.Format("User: {0, -15} - Error: {1:-100}", dpDataPersonales.sUser, msgEstadoAD);
 LoggerAD.Trace("[AdminDataUsers] - Modificación ERROR. AddUserEnActiveDirectory. " + log);
 }
 if (okAD)
 {
 var log = String.Format("User: {0, -15} - Password: {1:-10}", dpDataPersonales.sUser, PassManager.ShowPassword(dpDataPersonales.sPassword));
 LoggerAD.Trace("[AdminDataUsers] - Modificación. AddUserEnActiveDirectory. " + log);
 }
 }
 // Enable en CRM (si false, Request New CRM)
 var okEnable = AdminManager.EnableUserEnCrm(dpDataPersonales.sUser);
 if (!okEnable)
 {
 var msg = "No es posible habilitar al usuario en CRM. Realice una Request New CRM";
 ((NsiMaster)Master.Master.Master).ShowNotification(msg, CommunicationType.Error, true);
 return;
 }
 }
 // 2b) Si NO RolA, RolB (mediador) exists en AD ==> Warning (Email) y Enable en CRM (si false, Request New CRM)
 if (existsEnAD)
 {
 // Warning (Email)
 AdminManager.EnviarEmailWarning(dpDataPersonales.sUser);
 // Enable en CRM (si false, Request New CRM)
 var okEnable = AdminManager.EnableUserEnCrm(dpDataPersonales.sUser);
 if (!okEnable)
 {
 var msg = "No es posible habilitar al usuario en CRM. Realice una Request New CRM";
 ((NsiMaster)Master.Master.Master).ShowNotification(msg, CommunicationType.Error, true);
 return;
 }
 }
 }
 // DES-Enable
 // 1a) Si RolA, RolB (employer) NO exists en AD ==> Error
 // 1b) Si RolA, RolB (employer) exists en AD ==> DesEnable en CRM (Delete en CRM) - Delete de Licencias CRM 
 if (!userPortal.bIs_Enabled.Value && isEmployer)
 {
 if (!existsEnAD)
 {
 var msg = String.Format("El employer {0:-10} no exists en ActiveDirectory", dpDataPersonales.sUser);
 var log = String.Format("User: {0, -15} - Error: {1:-100}", dpDataPersonales.sUser, msg);
 LoggerAD.Trace("[AdminDataUsers] - Modificación ERROR. ExistsUserEnActiveDirectory. " + log);
 ((NsiMaster)Master.Master.Master).ShowNotification(msg, CommunicationType.Error, true);
 return;
 }
 // DesEnable en CRM (Delete en CRM)
 AdminManager.DeshabilitarUserEnCrm(dpDataPersonales.sUser);
 // Delete de Licencias CRM 
 AdminManager.DarDeDeleteComoUserCrmActivo(dpDataPersonales.sUser);
 }
 if (!userPortal.bIs_Enabled.Value && !isEmployer)
 {
 // 2a) Si NO RolA, RolB (mediador) NO exists en AD ==> Warning (Email) y DesEnable en CRM (Delete en CRM) - Delete de Licencias CRM 
 if (!existsEnAD)
 {
 // Warning (Email)
 AdminManager.EnviarEmailWarning(dpDataPersonales.sUser);
 // DesEnable en CRM (Delete en CRM)
 AdminManager.DeshabilitarUserEnCrm(dpDataPersonales.sUser);
 // Delete de Licencias CRM 
 AdminManager.DarDeDeleteComoUserCrmActivo(dpDataPersonales.sUser);
 }
 // 2b) Si NO RolA, RolB (mediador) exists en AD ==> Delete de AD y DesEnable en CRM (Delete en CRM) - Delete de Licencias CRM 
 if (!existsEnAD)
 {
 // Delete de AD
 var res = ADOperations.DeleteUserEnActiveDirectory(dpDataPersonales.sUser);
 // DesEnable en CRM (Delete en CRM)
 AdminManager.DeshabilitarUserEnCrm(dpDataPersonales.sUser);
 // Delete de Licencias CRM 
 AdminManager.DarDeDeleteComoUserCrmActivo(dpDataPersonales.sUser);
 }
 }

Any suggestions for refactoring, not code-smell, maybe with error handling too?

• BCdotWEB: Please do not update your code after you've received answers. "Do not change the code in the question after receiving an answer. Incorporating advice from an answer into the question violates the question-and-answer nature of this site."
• Kiquenet: update code here: pastebin.com/eEbRKt4C
• Pimgd: Your last edit was rather borked; it introduced several thousand non-characters. I have decided to roll it back, lest some poor browser chokes on a 8000-character line for some reason.

Answer 1

Do NOT use Hungarian notation: bAccionOk, sPassword,...

Do NOT use non-alphanumeric characters: bIs_Enabled, permisoOK_AltaAD,...

---

Concatenating strings quickly becomes unwieldy. This is already difficult, for instance: var NombreCompleto = dpDataPersonales.sNombre + " " + dpDataPersonales.sApellido1 + " " + dpDataPersonales.sApellido2;. Moreover, considering that this concatenates three properties of dpDataPersonales, why isn't NombreCompleto a property of dpDataPersonales?

(Also, NombreCompleto should be camelCase if it is a mere variable.)

---

ADOperations.AddUserEnActiveDirectory( seems to take a dozen(!) or so parameters. Instead, pass a single class where each of these is a property; that way it is easier to read and there's less chance of making an error. Also, considering that much of these parameters are part of dpDataPersonales, why not simply pass that class instead of copying over its values?

---

What is LoggerAD? To me this doesn't look like something that should be in your WebForms code behind, but you haven't provided us with enough information.

---

You code is partly in English, party in Spanish (I think). Same for your comments. This is confusing; ideally I'd advise you to stick to English.

---

Try to move your code from code-behind to specific classes. Specifically, split you UI from your business logic.

I get the impression much of the logic from your second code sample can and should be in a business logic class (or multiple), with communication between UI being limited to a parameter class being used to pass parameters from the UI to the back-end, while another class returns the results (success/error) and various messages.

Look at the variables you define on top: bAccionOk isn't used anywhere I think, and sPassword, dpDataPersonales, userPortal etc. are only used in code that looks to be back-end code. The only things that look to be relevant to the UI are calls like ((NsiMaster)Master.Master.Master).ShowNotification(msg, CommunicationType.Error, true);, and its various parameters can all come from the "response class" I talked about earlier.

• Kiquenet: Sorry, I have updated my code. It was in Spanish, now translate full to English. LoggerAD class for logging Ent.Library to file. Much of the logic should be in a business logic class (or multiple), maybe using any good pattern and practices.

Answer 2

Any suggestions for refactoring, not code-smell...

If you want to refactor your code you should start with code-smell. Currently it looks like at least four or five developers each with different coding-conventions have written this:

bIs_Enabled // with an underscore
dpDataPersonales // hungarian-notation
isEmployer // wow, this one is correct ;-)
existsinAD // with 's'
existeinAD // without 's' 
existsEnAD // ...

No wonder you have difficulties to refactor it. It's nearly impossible to read it.

• Kiquenet: Sorry, I have updated my code. It was in Spanish, now translate full to English. Like says @BCdotWEb Much of the logic should be in a business logic class (or multiple), maybe using any good pattern and practices.
• Kiquenet: update code here: pastebin.com/eEbRKt4C