Problem
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?
Solution
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.
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.