current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

I have a navigation controller set up with a user code and password and a log in button. When the user clicks on the Log In button or the return key on the keyboard on the password text box - the program checks if the user code and / or password is blank. It then goes off and calls a Web API running on a server to see if the user and password are valid. If it is, it goes to the next view controller - I created a segue between the two view controllers and named it.

This now works for me but because I'm new to this I'd love if someone could have a quick look at my code and see if I am doing anything I shouldn't be doing. I'm worried about memory problems and is it good practice to disable the screen while the system waits for the web service to return.

Any guidance would be much appreciated:

    // when the user clicks the return key on the user code - the focus goes to the password
- (IBAction)txtUserDidEndOnExit:(id)sender {
    [sender resignFirstResponder];
    [_txtPassword becomeFirstResponder];
}

// when the user clicks the return key on the password - it performs the click on the log in button
- (IBAction)txtPasswordDidEndOnExit:(id)sender {
    [sender resignFirstResponder];
    [_butLogin sendActionsForControlEvents:UIControlEventTouchUpInside];
}

- (IBAction)butLoginClick:(id)sender {
    [self logIn];
}

- (void) logIn {

    // check if the user code is blank - if it is - tell the user and stop the log in
    if ([self.txtUser.text length] == 0)
    {
        UIAlertView *alert = [[UIAlertView alloc] initWithTitle:@"Log In Error" message:@"User Required" delegate:self cancelButtonTitle:@"OK" otherButtonTitles:nil];

       alert.tag = TAG_USER;
       [alert show];
        return;
    }

    // check if the password is blank - if it is - tell the user and stop the log in
    if ([self.txtPassword.text length] == 0)
    {
       UIAlertView *alert = [[UIAlertView alloc] initWithTitle:@"Log In Error" message:@"Password Required" delegate:self cancelButtonTitle:@"OK" otherButtonTitles:nil];

       alert.tag = TAG_PWD;
         //   save = false;
       [alert show];
       return;
    }


    // read the web service url from settings - if its blank tell the user and stop the log in
    NSUserDefaults *userDefaults = [NSUserDefaults standardUserDefaults];
    NSString *webService = [userDefaults stringForKey:@"keyURLWebService"];

    if ([webService length] == 0)
    {
       UIAlertView *alert = [[UIAlertView alloc] initWithTitle:@"Settings Error" message:@"The settings for the Web Service URL is blank" delegate:self cancelButtonTitle:@"OK" otherButtonTitles:nil];
       [alert show];
            return;
    }

    // disable the current view so the user can not enter in another user id / password or click on the log in button
    [self.view setUserInteractionEnabled:NO];
    NSString *usercode = self.txtUser.text;
    NSString *password = self.txtPassword.text;

    // generate the complete url here
    NSString *urllink = [NSString stringWithFormat:@"%@/API/Users/GetValidateUser/?usercode=%@&&password=%@",
                             webService, usercode, password];

    NSURLSession *session = [NSURLSession sharedSession];
    NSURLSessionDataTask *dataTask = [session dataTaskWithURL:[NSURL URLWithString:urllink]
        completionHandler:^(NSData *data, NSURLResponse *response, NSError *error)
    {
        NSString* webresponse = [NSString stringWithUTF8String:[data bytes]];

        dispatch_async(dispatch_get_main_queue(), ^(void){

        if ([webresponse isEqual: @"OK"]) {
             [self.view setUserInteractionEnabled:YES];
             [self performSegueWithIdentifier: @"PMenu" sender: self];
        }
        else {
             [self.view setUserInteractionEnabled:YES];
             UIAlertView *alert = [[UIAlertView alloc] initWithTitle:@"Log In Error" message:webresponse delegate:self cancelButtonTitle:@"OK" otherButtonTitles:nil];
            alert.tag = TAG_USER;
            [alert show];
        }
    });

   }];

  [dataTask resume];

}


- (void)alertView:(UIAlertView *) alertView clickedButtonAtIndex:(NSInteger)buttonIndex{


    if (alertView.tag == TAG_USER)
    {
        [self.txtUser becomeFirstResponder];
    }

    if (alertView.tag == TAG_PWD)
    {
        [self.txtPassword becomeFirstResponder];
    }
}

I've made all the suggested changes - new code below

//
//  ProfileAccountsViewController.m
//  ProfileAccounts
//
//  Created by Profile on 12/03/2014.
//  Copyright (c) 2014 Profile Technology Ltd. All rights reserved.
//

#import "ProfileAccountsViewController.h"

@interface ProfileAccountsViewController ()

@end

@implementation ProfileAccountsViewController

#define TAG_USER 1
#define TAG_PWD 2
#define TAG_SETTINGS 3


- (void)viewDidLoad
{
    [super viewDidLoad];

    self.butLogin.layer.cornerRadius = 7;
    [self registerForKeyboardNotifications];
    [_txtUser becomeFirstResponder];
}

- (void)didReceiveMemoryWarning
{
    [super didReceiveMemoryWarning];
    // Dispose of any resources that can be recreated.
}

-(IBAction)textFieldDidEndOnExit:(id)sender {
    if (sender == _txtUser)
        [_txtPassword becomeFirstResponder];
    else if (sender == _txtPassword) {
        [sender resignFirstResponder];
        [self logIn];
    }
}

- (IBAction)butLoginClick:(id)sender {
    [self logIn];
}

- (void) logIn {

    // check if the user code is blank - if it is - tell the user and stop the log in
    // If the user forgot a field
    int returnval = [self textFieldsAreValid];

    if (returnval != 0) {

        NSString *alertMessage = (returnval == 1) ? @"User required" : @"Password required";

        UIAlertView *alert = [[UIAlertView alloc] initWithTitle:@"Log In Error" message:alertMessage delegate:self cancelButtonTitle:@"OK" otherButtonTitles:nil];

        alert.tag = (_txtUser.text.length == 0) ? TAG_USER : TAG_PWD;
    }
    else {
        // read the web service url from settings - if its blank tell the user and stop the log in
        NSUserDefaults *userDefaults = [NSUserDefaults standardUserDefaults];
        NSString *webService = [userDefaults stringForKey:@"keyURLWebService"];

        if ([webService length] == 0)
        {
            UIAlertView *alert = [[UIAlertView alloc] initWithTitle:@"Settings Error" message:@"The settings for the Web Service URL is blank" delegate:self cancelButtonTitle:@"OK" otherButtonTitles:nil];
            [alert show];
        }
        else {
            [self.activityInd startAnimating];
            // disable the current view so the user can not enter in another user id / password or click on the log in button
            [self.view setUserInteractionEnabled:NO];
            NSString *usercode = self.txtUser.text;
            NSString *password = self.txtPassword.text;
            NSString *urllink = [NSString stringWithFormat:@"%@/API/Users/GetValidateUser/?usercode=%@&&password=%@", webService, usercode, password];

            NSURLSession *session = [NSURLSession sharedSession];
            NSURLSessionDataTask *dataTask = [session dataTaskWithURL:[NSURL URLWithString:urllink]
                                                    completionHandler:^(NSData *data, NSURLResponse *response, NSError *error)
                                          {
                                              NSString* webresponse = [NSString stringWithUTF8String:[data bytes]];

                                              if (error != nil) {
                                                  [self handleError:error];
                                              }

                                              dispatch_async(dispatch_get_main_queue(), ^(void){

                                                  [self.activityInd stopAnimating];
                                                  if ([webresponse isEqualToString: @"OK"]) {
                                                      [self.view setUserInteractionEnabled:YES];
                                                      [self performSegueWithIdentifier: @"ProfileMenu" sender: self];
                                                  }
                                                  else {
                                                      [self.view setUserInteractionEnabled:YES];
                                                      UIAlertView *alert = [[UIAlertView alloc] initWithTitle:@"Log In Error" message:webresponse delegate:self cancelButtonTitle:@"OK" otherButtonTitles:nil];
                                                      alert.tag = TAG_USER;
                                                      [alert show];
                                                  }
                                              });

                                          }];

            [dataTask resume];
        }
    }
}


-(int)textFieldsAreValid {

    int wh = 0;

    if (_txtUser.text.length == 0){
        wh = 1;
    } else {
        if (_txtPassword.text.length == 0) wh = 2;
    }

    return wh;
}


/**
 Handle errors in the download by showing an alert to the user.
 */
- (void)handleError:(NSError *)error {

    [self.activityInd stopAnimating];

    NSString *errorMessage = [error localizedDescription];
    NSString *alertTitle = NSLocalizedString(@"Error", @"Log In Error");
    NSString *okTitle = NSLocalizedString(@"OK ", @"Log In OK");

    UIAlertView *alertView = [[UIAlertView alloc] initWithTitle:alertTitle message:errorMessage delegate:nil cancelButtonTitle:okTitle otherButtonTitles:nil];
    [alertView show];
}

- (void)alertView:(UIAlertView *) alertView clickedButtonAtIndex:(NSInteger)buttonIndex{


    if (alertView.tag == TAG_USER)
    {
        [self.txtUser becomeFirstResponder];
    }

    if (alertView.tag == TAG_PWD)
    {
        [self.txtPassword becomeFirstResponder];
    }
}


#pragma mark - event of keyboard relative methods
- (void)registerForKeyboardNotifications
{
    [[NSNotificationCenter defaultCenter] addObserver:self
                                             selector:@selector(keyboardWillShown:)
                                                 name:UIKeyboardWillShowNotification object:nil];

    [[NSNotificationCenter defaultCenter] addObserver:self
                                             selector:@selector(keyboardWillBeHidden:)
                                                 name:UIKeyboardWillHideNotification object:nil];

}

-(void)unregisterForKeyboardNotifications
{
    [[NSNotificationCenter defaultCenter] removeObserver:self
                                                    name:UIKeyboardWillShowNotification
                                                  object:nil];
    // unregister for keyboard notifications while not visible.
    [[NSNotificationCenter defaultCenter] removeObserver:self
                                                    name:UIKeyboardWillHideNotification
                                                  object:nil];
}


- (void)keyboardWillShown:(NSNotification*)aNotification
{
    NSDictionary* info = [aNotification userInfo];
    CGSize kbSize = [[info objectForKey:UIKeyboardFrameBeginUserInfoKey] CGRectValue].size;

    CGRect frame = _scrollView.frame;

    if (UIInterfaceOrientationIsPortrait(self.interfaceOrientation)) {
        frame.size.height -= kbSize.height;

    }else{
        frame.size.height -= kbSize.width;
    }
    CGPoint fOrigin = _activeField.frame.origin;
    fOrigin.y -= _scrollView.contentOffset.y;
    fOrigin.y += _activeField.frame.size.height;
    if (!CGRectContainsPoint(frame, fOrigin) ) {
        CGPoint scrollPoint = CGPointMake(0.0, _activeField.frame.origin.y + _activeField.frame.size.height - frame.size.height);
        [_scrollView setContentOffset:scrollPoint animated:YES];
    }
}

- (void)keyboardWillBeHidden:(NSNotification*)aNotification
{
    //[_scrollView setContentOffset:CGPointMake(0, _scrollView.contentInset.top) animated:YES];
    //[_scrollView setContentOffset:CGPointZero animated:YES];
}


@end
share|improve this question
add comment

2 Answers

up vote 8 down vote

When you have something like this in your code:

- (void)didReceiveMemoryWarning
{
    [super didReceiveMemoryWarning];
    // Dispose of any resources that can be recreated.
}

You can simply delete all 5 of these lines. The only reason to include the the stub for - (void)didReceiveMemoryWarning is if you're actually going to add code to the method.

int returnval

This variable should be renamed as returnVal.

#define TAG_USER 1
#define TAG_PWD 2
#define TAG_SETTINGS 3

There's no reason why this shouldn't be an enum:

typedef NS_ENUM(NSInteger, UITAGS) {
    TAG_USER      1,
    TAG_PWD       2,
    TAG_SETTINGS  3
};
share|improve this answer
add comment
up vote 7 down vote

Here is what I see in your code :

In -(IBAction)txtUserDidEndOnExit:(id)sender

You don't need to resignFirstResponder before you make another view become first responder. Right now, the keyboard probably quickly hides/shows when you press the return key when the focus is on the first field.

In -(IBAction)txtPasswordDidEndOnExit:(id)sender

Why use sendActionsForControlEvents: ? This will only trigger the method associated with the login button, which is [self logIn];. Simply call this, you will certainly avoid confusion if your code changes.

About the 2 above methods

You could merge those methods into a single one :

-(IBAction)textFieldDidEndOnExit:(id)sender {
    if (sender == _txtUser)
        [_txtPassword becomeFirstResponder];
    else if (sender == _txtPassword) {
        [sender resignFirstResponder];
        [self login];
    }
}

You would of course need to link both your UITextViews to this method instead of a different method for each one. This does not reduce the line count, but it groups your actions together. Note that I didn't test it, so it might not work straightaway.

In -(void)logIn

Text fields checking :

This :

// check if the user code is blank - if it is - tell the user and stop the log in
if ([self.txtUser.text length] == 0)
{
    UIAlertView *alert = [[UIAlertView alloc] initWithTitle:@"Log In Error" message:@"User Required" delegate:self cancelButtonTitle:@"OK" otherButtonTitles:nil];

   alert.tag = TAG_USER;
   [alert show];
    return;
}

// check if the password is blank - if it is - tell the user and stop the log in
if ([self.txtPassword.text length] == 0)
{
   UIAlertView *alert = [[UIAlertView alloc] initWithTitle:@"Log In Error" message:@"Password Required" delegate:self cancelButtonTitle:@"OK" otherButtonTitles:nil];

   alert.tag = TAG_PWD;
     //   save = false;
   [alert show];
   return;
}

should be in a dedicated method, like this :

-(BOOL)textFieldsAreValid {

    if (_txtUser.text.length == 0 || _txtPassword.text length == 0) return NO;

    return YES;
}

so if in the future, you want to add more verifications before the login, it will be easy to do. This would make the beginning of logIn look like :

- (void) logIn {

    // If the user forgot a field
    if (![self textFieldsAreValid]) {

        NSString *alertMessage = (_txtUser.text.length == 0) ? @"User required" : @"Password required";

        UIAlertView *alert = [[UIAlertView alloc] initWithTitle:@"Log In Error" message:alertMessage delegate:self cancelButtonTitle:@"OK" otherButtonTitles:nil];

        alert.tag = (_txtUser.text.length == 0) ? TAG_USER : TAG_PWD;
    }

    [...]

You could also imagine returning an NSInteger from textFieldsAreValid, which could be 0->fields are ok, 1->user empty, 2-> password empty, so you don't have to check _txtUser.text.length == 0 multiple times.

Rest of the logIn method

The rest of the method should be in an else { } clause. It's not a good practice to return from a void method. You should use if-else in place of your if-return. Place the rest of the code in an else, and it won't be executed if a field is empty. Same goes when you test the webservice address length: use an if-else, don't use return.

Calling [self.view setUserInteractionEnabled:NO]; may not be the best option here. You should probably call _textField.enabled = NO; on each of your text fields rather than disabling the whole view.

Also, I don't think you're providing any UI information to the user while the app is validating the credentials. This is not good, you have to let the user know that something is happening. Have a look at iOS' UIActivityIndicator class.

Next, [webresponse isEqual: @"OK"] should be [webresponse isEqualToString: @"OK"]. There is not a big difference here, but the second one is much faster. As it says on this thread :

isEqual: compares a string to an object, and will return NO if the object is not a string. isEqualToString: is faster if you know both objects are strings

This will most certainly make no performance improvement here considering the strings you use, but it's a better practice to use the isEqualTo<Class>: if you know that the two objects you are comparing are from the same class. In the case of isEqualToArray:, you may see a big performance difference.

Happy coding ! And if you have any questions, let me know in the comments ! :]

share|improve this answer
1  
You could also imagine returning an NSInteger from textFieldsAreValid, which could be 0->fields are ok, 1->user empty, 2-> password empty, so you don't have to check _txtUser.text.length == 0 multiple times. How about an enum? Or perhaps better, an NSString, where nil represents okay, and anything else is the string that represents the error. –  nhgrif Mar 14 at 11:40
    
@nhgrif : yes, an enum would be the best here. I didn't have enough time to explain it, that's why I said an NSInteger (enums are just wrappers for this). Not sure about the NSString though, I'd say it's a bit too much for the situation.. –  rdurand Mar 14 at 12:43
    
@rdurand - thank you so much for your very helpful comments. Some of them - like having the one method for the did end on exit - very obvious and something I should have seen. –  user2961575 Mar 14 at 12:55
    
@nhgrif - I made the change you suggested too - hopefully I have it right –  user2961575 Mar 14 at 12:55
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.