2
\$\begingroup\$

Very simple requirements here:

  1. connect to web service
  2. check if account number exists
  3. if it exists update it
  4. if it does not exist create the account

I have accomplished this with the code below I am wondering if this is efficient and secondly if there is a better way of doing this.

 protected void Button1_Click(object sender, EventArgs e)
 {
    //authentication header
    com.x.w_1.www.WebService ws = new com.x.w_1.www.WebService();
    com.dat_e_baseonline.w_1.www.AuthenticationHeader header = new com.x.w_1.www.AuthenticationHeader();
    header.UserName = "";
    header.Password = "";
    ws.AuthenticationHeaderValue = header;


    string email = TextBox1.Text.ToString();

    //Are they a subscriber?
    XmlNode result = ws.getAccountIDbyEmail(email, "247");

    Label1.Text = result.OuterXml;

    foreach (XmlNode x in result.FirstChild)
    {
        string m = x.InnerText;
        string account = result.LastChild.InnerText;

        switch (m)
        {

            case "100":
                //update the account
                ws.Subscribe("x", "3", account);
                Label1.Text = "account exists";
                break;
            case "200":
                //create an account
                ws.createAccount("x", "5", email, "x", "x", "x", "01-01-1998", "1", "yes");
                Response.Redirect("/thankyou.aspx");
                break;

        }
    }

For reference here is the XML I get back when I send an email to be updated:

<getAccountID:name xmlns:getAccountID="http://x">
<getAccountIDResultCode xmlns="">100</getAccountIDbyEmailResultCode> 
<getAccountIDResultDescr xmlns="">Account found.</getAccountIDResultDescr> 
<accountID xmlns="">12345</accountID> 
</getAccountIDbyEmail:name>
\$\endgroup\$
4
\$\begingroup\$

I would move the operations on the web service out to their own class:

public class AccountRepository
{
    private readonly void _webService;

    public AccountRepository()
    {
        //authentication header

        var header = new AuthenticationHeader
                              {
                                  header.UserName = "";
                                  header.Password = "";
                              }

        _webService = new WebService
                              {
                                   ws.AuthenticationHeaderValue = header;
                              }
    }

    public IEnumerable<AccountResult> SearchByEmail(string email)
    {
         return ConvertXmlToResultsList(_webService.getAccountIDbyEmail(email, "247"));
    }

    private IEnumerable<AccountResult> ConvertXmlToResultsList(XmlDocument xmlDocument)
    {
         // logic to convert xml to account results
    }

    public void UpdateAccount(/* parameters */)
    {
         _webService.Subscribe(/* parameters */);
    }

    public void CreateAccount(/* parameters */)
    {
         _webService.createAccount(/* parameters */
    }
}

Doing this gets rid of most of the housekeeping code out of the event handler.

I would also consider creating two classes: Account, and SearchResult.

Account should be obvious, it contains all the functionality and data that an account requires. This would be instantiated in the Repository on the search, and also in the event handler when you need to create a new Account.

The SearchResult class I would use to return the results of the search. It would contain the Account from the search, and the status of the search. You could also put stuff like error checking, or anything else you see as being relevant. You will also see a ResultCode variable type. ResultCode is an enum that contains all of the possible codes returned from the search.

public enum ResultCode
{
     Error,
     UpdateAccount,
     CreateAccount,
}

public class SearchResult
{
    private readonly Account _account;
    private readonly ResultCode _code;

    public SearchResult(Account account, int resultCode)
    {
         _account = account;
         _code = DetermineCode(resultCode);
    }

    private static ResultCode DetermineCode(int resultCode)
    {
         switch(resultCode)
         {
              case 100:
                  return ResultCode.UpdateAccount;
              case 200:
                  return ResultCode.CreateAccount;
         }

         return ResultCode.Error;
    }
}

The final result of all this is an event handler that looks like:

protected void Button1_Click(object sender, EventArgs e)
{

     var results = _accountRepository.SearchByEmail(TextBox1.Text);    

     foreach (var result in results)
     {
         switch (result.ResultCode)
         {
             case ResultCode.UpdateAccount:
                 result.Account.Update(/* parameters */);
                 _accountRepository.Update(result.Account);

                 Label1.Text = "account exists";
                 break;
             case ResultCode.CreateAccount:
                 var account = new Account("x", "5", email, "x", "x", "x", "01-01-1998", "1", "yes");

                 _accountRepository.Create(account);

                 Response.Redirect("/thankyou.aspx");
                 break;

         }
     }
 }

There are a few more points I'd like to make.

Calling .ToString() on a .Text property of a TextBox is counterproductive. TextBox.Text returns a string, no need to make it a string.

In your code, I would move ResultStatus magic numbers into constants. This will better describe what they represent. Of course, if you take my code suggestions, no need to worry about this.

I don't know who wrote the web service, but the method names follow java naming conventions, not C#. This is just a btw, ignore if the web service is written in Java.

Finally, there is quite a bit more clean-up to do on the code, even what I have posted, but this is a good start. Hopefully you can pick up some tips from it.

Cheers.

\$\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.