Code allowed to test mail invitations by users (from another post) on an instance of Visual studio debugging I am currently working on an application in ASP.Net MVC, with a SQL Server 2016 database and Entity Framework 6.

The application can process specific data and generate forms (in DOC format), and in some cases it sends emails to targeted people.

My problem is with this piece of code (The code set is further down):
                

#region Fix proxy proxy locally on URL
Var currentHost = "localhost: 57061";
Var newhost = $ "{GetIPAddressV4 ()}: 90";
Url = url.Replace (currentHost, newhost);
#endregion


What I learned from the explanation I was given was:


  This code allowed to test mail invitations by users (from another
  post) on an instance of Visual studio debugging.
  
  The purpose of this step is to modify the URL sent in the email so
  that the new URL points to the instance of Debug VS. To use this
  trick, it uses iisexpress-proxy.


I have an opinion on this code but I would like an expert feedback on this.

My questions:


Is this code clean?  
Is it dangerous (reliability, safety)?  
Can we replace it?


The code set is here:

 public class SendMailMgr
    {
        #region MailInvit

        public static void SendMailInvit(BaseViewModels model, string userId, string htmlInvit)
        {
            var usr = PersonalInfoDbMgr.GetByUserId(userId);

            if (usr != null)
            {
                var url = string.Format("{0}://{1}{2}",
               model.Request.Url.Scheme,
               model.Request.Url.Authority,
               model.Url.Action("FirstStep", "Account", new { id = usr.AspNetUser.Id }));

                #region Fix prob de proxy en local sur l'URL

                var currentHost = "localhost:57061";
                var newhost = $"{GetIPAddressV4()}:90";
                url = url.Replace(currentHost, newhost);

                #endregion Fix prob de proxy en local sur l'URL

                var mailT = BuildMailMgr.MailInvit(new Business.Tools.Mail.ViewModels.MailInvit()
                {
                    UrlInvit = url,
                    FirstName = usr.FirstName,
                    LastName = usr.LastName,
                    Profile = usr.Profile,
                    HtmlMessage = htmlInvit,
                });

                mailT.To = usr.Email;
                MailMgr.SendMail(mailT);
            }
            else
            {
                model.Feedback.AddError("SendMailInvit User Not find ", " SendMailInvit User Not find =&gt; UserId : " + userId);
            }
        }

        #endregion MailInvit

        public static string GetIPAddressV4()
        {
            var res = "";
            IPHostEntry ipHostInfo = Dns.GetHostEntry(Dns.GetHostName());

            for (int i = 0; i &lt; ipHostInfo.AddressList.Length; i++)
            {
                IPAddress ipAddress = ipHostInfo.AddressList[i];
                var ip = ipAddress.ToString();
                if (ip.Length &lt;= 12)
                {
                    res = ip;
                }
            }
            return res;
        }
    }
Class names shouldn't be plural except for a few exceptions, and BaseViewModels does not seem to be such a case.



Avoid this kind of logic:

if (usr != null)
{
    // lots to do
}
else
{
    // one thing to do
}


Especially here: you could have done

if (usr == null)
{
    model.Feedback.AddError("SendMailInvit User Not find ", " SendMailInvit User Not find =&gt; UserId : " + userId);
    return;
}


and then the rest of the method could be the long list of other instructions.



I've never worked with ASP.NET MVC, so I don't know whether this is the usual way of doing things, but passing a model and then setting properties to give feedback -- e.g. model.Feedback.AddError("SendMailInvit User Not find ", " SendMailInvit User Not find =&gt; UserId : " + userId); -- looks really odd to me.

Especially since this class doesn't have anything to do with the UI; which means you're mixing up tiers.



Don't use regions.



This seems like a hacky solution:

var currentHost = "localhost:57061";
var newhost = $"{GetIPAddressV4()}:90";
url = url.Replace(currentHost, newhost);


Wouldn't it be much simpler to simply have the base URL as a value in a .config file? Especially considering what's happening in GetIPAddressV4().



If I were you I'd move the whole "compile the URL" logic to a separate method.



Avoid abbreviations like UrlInvit, htmlInvit etc. It only makes you code harder to read.