13
\$\begingroup\$

I recently interviewed with a company that needed a C/C++ programmer to work on the iOS side of the products. The job description indicated they needed someone with 4 years of Objective-C and iOS programming and I was surprised that they wanted to interview me.

Prior to this coding challenge I have never worked in Xcode, or programmed in iOS, Objective-C or swift. I am an absolute beginner in these areas. I still don't think I know these programming environments but I am learning.

Environment

  • OSX - El Capitan
  • Xcode - Version 8.2 (8C38) // Swift 3
  • Running in the iPhone 7 simulator
  • Late 2010 17 inch MacBook Pro

The following section is the extraction of the email the hiring manager sent me:

Programming Challenge:

Create a static library or iOS Framework using Objective-C that performs the following 3 functions:

  • Collects the GPS location (latitude and longitude) of the user at a point in time
  • Collects the battery state and returns whether or not the device is plugged in and what percentage of life is left
  • Accesses any publicly available, free API to collect the data of your choice and returns it (this should be a network call)

Build a simple application with 3 buttons and a label where text can be displayed. Each button should call into the three functions of the library described above and output the response to the label. Your application should consist of two tabs, one written in Objective-C and one written in Swift. Both tabs should call into the same Objective-C library and perform the same function.

Only use Apple frameworks to complete this task. Fully comment your code explaining your logic and choices where multiple choices are available. For example, Apple provides numerous ways to retrieve a network resource, document why you choose the solution you did.

This question has been divided into 2 parts based on the size of the code to be reviewed. One part contains the Objective-C static library and the other part contains the simple application. This question contains the Application written in Objective-C and Swift the static library can be found at this Question.

The source code and project files for both questions can be found at this GitHub repository in case you are interested in building it and running it.

This application was built in 3 pieces. First, the Objective-C application was written as a stand alone project, then the Swift application was written as a stand alone project and finally the tab bar was added to the Objective-C stand alone project and the Swift file was merged into the project. Story Boards and Interface Builder were not used in this project.

The application took approximately 40 hours to create. 12 hours in build the first app in Objective-C and integrate the static library. 12 hours to build the swift version of the application and integrate the library. 16 hours to add the tab bar and merge the swift and objective-c portions of the program.

Only the code I wrote is included in the question, the main.m file and the portions of AppDelegate.m that I did not code are excluded.

What I desire from a review:

Since this is the first time I've programmed for iOS in both Objective-C and Swift I'd like to know:

  • Are there any memory leaks?
  • What iOS, Objective-C or Swift programming conventions have I missed or used incorrectly?
  • What are the obvious things that I should know that I don't?
  • What error checking should I have included that I didn't?
  • How could I have written this with less code?

AppDelegate.h

//
//  AppDelegate.h
//  DevAndNetInfo2
//
//  Created by Paul Chernick on 4/18/17.
//

#import <UIKit/UIKit.h>
#import <CoreData/CoreData.h>
#import "PCI7DataModelLibrary/PCI7DataModelLibrary/PCI7DataModelLibrary.h"


@interface AppDelegate : UIResponder <UIApplicationDelegate>

@property (strong, nonatomic) UIWindow *window;

@property (readonly, strong) NSPersistentContainer *persistentContainer;

@property (strong) UITabBarController *tabBarController;

// Create the library as a property so that consumer objects can use a getter to receive a pointer to it.
// Prevent objects that use the library from changing it.
@property (readonly, strong) PCI7DataModelLibrary *dataModelLibrary;

- (void)saveContext;


@end

AppDelegate.m

//
//  AppDelegate.m
//  TabbedOCandSwift
//
//  Created by Paul Chernick on 4/18/17.
//

#import "AppDelegate.h"
#import "ObjectiveCViewController.h"
#import "TabbedOCandSwift-Swift.h"

@interface AppDelegate ()

@end

@implementation AppDelegate

@synthesize dataModelLibrary = _dataModelLibrary;

#pragma mark - The following functions were added or modified to implement a tabbed application implemented in swift and objective-c

- (BOOL)dataModelLibraryInitialize {
    BOOL libraryCreatedAndInitialized = YES;

    if (!self.dataModelLibrary || !_dataModelLibrary) {
        PCI7DataModelLibrary *tempLibPtr = [[PCI7DataModelLibrary alloc] init];
        if (!tempLibPtr) {
            // If the library can't be allocated or initialized none of the buttons will work in any view controller.
            NSLog(@"application didFinishLaunchingWithOptions: Unable to alloc or init the PCI7DataModelLibrary object");
            return NO;
        }
        _dataModelLibrary = tempLibPtr;
    }

    return libraryCreatedAndInitialized;
}

- (BOOL)createTabBarAndViewControllers {
    ObjectivCViewController* objectiveVC = nil;
    SwiftViewController* swiftVC = nil;

    self.tabBarController = [[UITabBarController alloc] init];

    objectiveVC = [[ObjectivCViewController alloc] init];
    if (objectiveVC) {
        [objectiveVC setTitle:@"Objective-C"];
    }
    else {
        NSLog(@"In AppDelegate.createTabBarAndViewControllers: Unable to create objectiveVC");
        return NO;
    }

    swiftVC = [[SwiftViewController alloc] init];
    if (swiftVC) {
        [swiftVC setTitle:@"Swift"];
    }
    else {
        NSLog(@"In AppDelegate.createTabBarAndViewControllers: Unable to create swiftVC");
        return NO;
    }

    NSArray* controllers = [NSArray arrayWithObjects:objectiveVC, swiftVC,  nil];
    self.tabBarController.viewControllers = controllers;
    self.tabBarController.selectedIndex = 0;

    self.window = [[UIWindow alloc] initWithFrame:[[UIScreen mainScreen] bounds]];
    self.window.rootViewController = self.tabBarController;
    [self.window makeKeyAndVisible];

    return YES;
}

- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions {
    // Create the library object if it hasn't already been created
    // There should only be one copy of the library.
    //      - Reduce memory usage, devices have limitied memory, no app should use more than it needs and only one library is necessary.
    //      - There is a runtime cost to starting up the library, if it is done once early in the life of the application
    //          the user will notice it less than when a viewcontroller starts or resumes.
    //      - There is asyncronous code that runs in the library in a serial manner to reduce possible interactions and prevent deadlock only one library should exist.
    if (![self dataModelLibraryInitialize]) {
        // If the library can't be allocated or initialized none of the buttons will work in any view controller.
        NSLog(@"application didFinishLaunchingWithOptions: Unable to alloc or init the PCI7DataModelLibrary object");
        return NO;
    }
    if (![self createTabBarAndViewControllers]) {
        return NO;
    }

    return YES;
}

@end

ObjectiveCViewController.h

//
//  ObjectivCViewController.h
//  TabbedOCandSwift
//
//  Created by Paul Chernick on 4/18/17.
//

#import <UIKit/UIKit.h>

@interface ObjectivCViewController : UIViewController

- (id)init;


@end

ObjectiveCViewController.m

//
//  ObjectivCViewController.m
//      - Implements the Objective-C view controller specified in the coding challenge
//  TabbedOCandSwift
//
//  Created by Paul Chernick on 4/18/17.

#import "ObjectiveCViewController.h"
#import "AppDelegate.h"

@interface ObjectivCViewController ()

- (UIButton *)makeAButton: (CGFloat) yButtonStart title: (NSString *) buttonTitle underSubview: (nullable UIView *) previousSiblingView;
- (UILabel *)makeALablel: (CGFloat) yLabelStart height: (CGFloat) height underSubview: (nullable UIView *) previousSiblingView;
- (void)addButtonAndLabels;
- (void)setSubViewSizeVariablesBasedOnViewBounds;
- (void)setLabelWithGPSLatitudeAndLongitudeWithTimeStampData;
- (void)setLabelWithBatteryLevelAndState;
- (void)setLabelActionNetwork;

@end

@implementation ObjectivCViewController
// Instance variables.
{
    UIButton *getGPSLongitudeAndLatitudeWithTimeStamp;
    UIButton *getBatteryLevelAndState;
    UIButton *getNetworkData;
    UILabel *displayButtonAction;
    UILabel *screenTitle;
    PCI7DataModelLibrary *displayDataModel;     // Imported with AppDelegate
    BOOL isFirstGpsClick;

// The following variables are used in multiple functions. They are constant during the display of the super view
// and control the size of the subviews
    CGFloat selfWidth;
    CGFloat verticalSpaceAvailableToUse;
    CGFloat buttonHeight;
    CGFloat viewElementWidth;
    CGFloat viewElementVerticalSpace;
    CGFloat buttonYCenterOffset;
    CGFloat buttonXCenter;
    CGFloat buttonXInit;
    CGFloat startingVerticalLocation;
    CGFloat displayLabelHeight;
    CGFloat tabBarHeight;
}

#pragma mark - Set up subviews (buttons and labels)

// Set the button and label sizes, preferrable based on device orientation and size
- (void)setSubViewSizeVariablesBasedOnViewBounds {
    selfWidth = self.view.bounds.size.width;
    verticalSpaceAvailableToUse = self.view.bounds.size.height - tabBarHeight;
    viewElementVerticalSpace = verticalSpaceAvailableToUse / 14.0;
    viewElementWidth = 0.85 * selfWidth;
    buttonHeight = viewElementVerticalSpace * 0.65;
    buttonYCenterOffset = buttonHeight / 2.0;
    buttonXCenter = selfWidth / 2.0;
    buttonXInit = (selfWidth - viewElementWidth) / 2.0;
    startingVerticalLocation = 110.0;           // Chosen based on experimentation in the simulator
    displayLabelHeight = 3.0 * viewElementVerticalSpace;
}

- (UILabel *)makeALablel: (CGFloat) yLabelStart height: (CGFloat) height underSubview: (nullable UIView *) previousSiblingView
{
    UILabel* thisLabel;

    thisLabel = [[UILabel alloc] initWithFrame:CGRectMake(buttonXInit, yLabelStart, viewElementWidth, height)];
    [thisLabel setFont:[UIFont systemFontOfSize:12]];     // Reduce the size of the text so that more output fits on a single line
    thisLabel.lineBreakMode = NSLineBreakByWordWrapping;
    thisLabel.numberOfLines = 0;                          // Allow the label to grow as necessary
    thisLabel.textAlignment = NSTextAlignmentCenter;
    thisLabel.textColor = [UIColor blackColor];

    if (previousSiblingView) {
        [self.view insertSubview:thisLabel belowSubview:previousSiblingView];
    }
    else {
        [self.view addSubview:thisLabel];
    }

    return thisLabel;
}

// Create a button with the starting y coordinate, the button title, and the sibling view to appear under the first item passed in
// may not have a sibling subview, but all following buttons should have a previous sibling view.
// The target action is not set in this function, I haven't found documentation on how to pass a function into a function.
- (UIButton *)makeAButton: (CGFloat) yButtonStart title: (NSString *) buttonTitle underSubview: (nullable UIView *) previousSiblingView
{
    UIButton *thisButton = [UIButton buttonWithType:UIButtonTypeRoundedRect];
    [thisButton setFrame:CGRectMake(buttonXInit, yButtonStart, viewElementWidth, buttonHeight)];
    [thisButton setCenter:CGPointMake(buttonXCenter, yButtonStart + buttonYCenterOffset)];
    [thisButton setTitle:buttonTitle forState:UIControlStateNormal];
    [thisButton setBackgroundColor:[UIColor yellowColor]];
    [thisButton setTitleColor:[UIColor blackColor]  forState:UIControlStateNormal];

    if (previousSiblingView) {
        [self.view insertSubview:thisButton belowSubview:previousSiblingView];
    }
    else {
        [self.view addSubview:thisButton];
    }

    return thisButton;
}

// Add the necessary sub-view buttons and labels to the application
// Assumptions here, all instance variables for buttons and labels have been set
// to null by the calling function.
// TODO: Put the titles, previous subviews and, object types and action selectors into a table and loop through the table.

- (void)addButtonAndLabels {
    if (selfWidth < 1.0) {
        return;
    }

    CGFloat viewElementVerticalLocation = startingVerticalLocation;

    screenTitle = [self makeALablel:viewElementVerticalLocation height:buttonHeight underSubview:nil];
    NSString* titleText = @"Objective-C Implementation";
    [screenTitle setText:titleText];
    UIFont* titleFont = [UIFont boldSystemFontOfSize:16];
    [screenTitle setFont:titleFont];     // This is the title of the screen make it the larges text
    viewElementVerticalLocation += viewElementVerticalSpace;

    getGPSLongitudeAndLatitudeWithTimeStamp = [self makeAButton: viewElementVerticalLocation  title: @"Get GPS Location with TimeStamp" underSubview: screenTitle];
    // The target action is not set in makeAButton function, I haven't researched how to pass a selector/function into a function.
    [getGPSLongitudeAndLatitudeWithTimeStamp addTarget:self action: @selector(setLabelWithGPSLatitudeAndLongitudeWithTimeStampData) forControlEvents: UIControlEventTouchUpInside];
    viewElementVerticalLocation += viewElementVerticalSpace;

    getBatteryLevelAndState = [self makeAButton: viewElementVerticalLocation  title: @"Get Battery Level and State" underSubview: getGPSLongitudeAndLatitudeWithTimeStamp];
    [getBatteryLevelAndState addTarget:self action: @selector(setLabelWithBatteryLevelAndState) forControlEvents: UIControlEventTouchUpInside];
    viewElementVerticalLocation += viewElementVerticalSpace;

    getNetworkData = [self makeAButton: viewElementVerticalLocation  title: @"Get the American Express Stock Price" underSubview: getBatteryLevelAndState];
    [getNetworkData addTarget:self action: @selector(setLabelActionNetwork) forControlEvents: UIControlEventTouchUpInside];
    viewElementVerticalLocation += viewElementVerticalSpace;

    displayButtonAction = [self makeALablel:viewElementVerticalLocation height:displayLabelHeight underSubview:getNetworkData];
}

/*
 * The view is changing, either being displayed or rotated.
 * Set the button and label variables to nil to decrease any resource counters.
 * Redraw the buttons.
 */
- (void)viewWillLayoutSubviews {
    getGPSLongitudeAndLatitudeWithTimeStamp = nil;
    getBatteryLevelAndState = nil;
    getNetworkData = nil;
    displayButtonAction = nil;
    screenTitle = nil;
    /*
     * Get the tab bar height from the Application Delegate so that the total vertical space
     * can be calculated.
     */
    AppDelegate* appDelegate = (AppDelegate*)[[UIApplication sharedApplication]delegate];
    if (appDelegate) {
        UITabBarController *TempTabBar = appDelegate.tabBarController;
        if (TempTabBar) {
            // Tab Bar Height is larger than myDelegate.tabBarController.tabBar.frame.size.height indicates
            tabBarHeight = TempTabBar.tabBar.frame.size.height * 2.5;
        }
    }

    [self setSubViewSizeVariablesBasedOnViewBounds];
    [self addButtonAndLabels];
    self.view.backgroundColor = [UIColor whiteColor];
}

#pragma mark - Button Action functions

- (void)setLabelWithGPSLatitudeAndLongitudeWithTimeStampData {
    NSString *actionString = nil;

    if (displayDataModel) {
        if (isFirstGpsClick) {
            // Call to the DataModel library that receives a pointer UIAlertView object from the GPS library implementation
            // If the UIAlertView pointer is nil proceed with the displaying the latitude, longitude and timestamp.
            // If the UIAlertView has a value show the alert, the alert should contain a function to update data in the GPS model.
            // This will enable the user to approve of using WiFi or Radio triangulation when the GPS is not available.
            assert([NSThread isMainThread]);
            isFirstGpsClick = NO;
            UIAlertController* gpsAlert = [displayDataModel provideGPSAlerters];
            if (gpsAlert) {
                [self presentViewController:gpsAlert animated:NO completion:nil];
            }
        }
        actionString = [displayDataModel provideGPSLocationData];
    }
    else {
        actionString = @"GPS Button Action Failure: Data Model not created";
    }

    [displayButtonAction setText:actionString];
}

- (void)setLabelWithBatteryLevelAndState {
    NSString *actionString = nil;

    if (displayDataModel) {
        actionString = [displayDataModel provideBatteryLevelAndState];
    }
    else {
        actionString = @"Battery Button Action Failure: Data Model not created";
    }

    [displayButtonAction setText:actionString];

}

- (void)setLabelActionNetwork {
    NSString *actionString = nil;

    if (displayDataModel) {
        actionString = [displayDataModel provideNetworkAccessData];
    }
    else {
        actionString = @"Network Button Action Failure: Data Model not created";
    }

    [displayButtonAction setText:actionString];

}

#pragma mark - Memory management and class initialization

- (void)viewDidLoad {
    [super viewDidLoad];

    if (self.displayModelLibraryInitialization) {
        NSLog(@"In Objective-C Implementation viewDidLoad - unable to initialize displayModelLibrary");
    }
}

- (BOOL)displayModelLibraryInitialization {
    // If the data model library is nil then get the pointer to the data library from the application delegate
    // The data model library should be created only once in the application delegate while the application launches.

    if (!displayDataModel) {
        AppDelegate* appDelegate = (AppDelegate*)[[UIApplication sharedApplication]delegate];
        displayDataModel = appDelegate.dataModelLibrary;
    }
    return (displayDataModel == nil);
}

- (void)didReceiveMemoryWarning {
    [super didReceiveMemoryWarning];
}

- (id)init {
    self = [super init];
    isFirstGpsClick = YES;
    getGPSLongitudeAndLatitudeWithTimeStamp = nil;
    getBatteryLevelAndState = nil;
    getNetworkData = nil;
    displayButtonAction = nil;
    screenTitle = nil;
    displayDataModel = nil;     // Imported with AppDelegate
    if (self.displayModelLibraryInitialization) {
        NSLog(@"In Objective-C Implementation viewDidLoad - unable to initialize displayModelLibrary");
        return nil;
    }

    return self;
}

@end

TabbedOCandSwift-Bridging-Header.h

//
//  Use this file to import your target's public headers that you would like to expose to Swift.
//

#import "AppDelegate.h"

SwiftViewController.swift

//
//  SwiftViewController.swift
//  SwiftTabForApp
//

import UIKit

@objc class SwiftViewController: UIViewController {
    var getGPSLongitudeAndLatitudeWithTimeStamp : UIButton?
    var getBatteryLevelAndState : UIButton?
    var getNextorkImplementation : UIButton?
    var displayButtonAction : UILabel?
    var screenTitle : UILabel?
    var displayDataModel : PCI7DataModelLibrary?
    var isFirstGpsClick : Bool = true

    // The following variables are used in multiple functions. They are constant during the display of the super view
    // and control the size of the subviews. They should change when the orientation changes
    var selfWidth : CGFloat = 0.0
    var verticalSpaceAvailableToUse: CGFloat = 0.0
    var buttonHeight : CGFloat = 0.0
    var viewElementWidth : CGFloat = 0.0
    var viewElementVerticalSpace : CGFloat = 0.0
    var buttonXInit : CGFloat = 0.0
    var startingVerticalLocation : CGFloat = 0.0
    var displayLabelHeight: CGFloat = 0.0
    var selfUsableHeight: CGFloat = 0.0
    var tabBarHeight: CGFloat = 0.0

    // Size the buttons and labels based on the available width and height.
    func initFramingValuesOfMyDisplay() {
        selfWidth = self.view.bounds.width
        verticalSpaceAvailableToUse = self.view.bounds.height - tabBarHeight
        viewElementWidth = 0.85 * selfWidth;
        viewElementVerticalSpace = verticalSpaceAvailableToUse / 14.0
        buttonHeight = viewElementVerticalSpace * 0.65
        buttonXInit = (selfWidth - viewElementWidth) / 2.0;
        startingVerticalLocation = 110.0  // chosen based on experimentation in the simulator
        displayLabelHeight = viewElementVerticalSpace * 3.0
    }

    // This function is called when the getGPSLongitudeAndLatitudeWithTimeStamp button is receives the touchUpInside event.
    func setLabelWithGPSLatitudeAndLongitudeWithTimeStampData()
    {
        var actionString : String = "Testing Label Text"

        if (self.displayDataModel != nil) {
            if (self.isFirstGpsClick) {
                // Call to the DataModel library that receives a pointer UIAlertView object from the GPS library implementation
                // If the UIAlertView pointer is nil proceed with the displaying the latitude, longitude and timestamp.
                // If the UIAlertView has a value show the alert, the alert should contain a function to update data in the GPS model.
                // This will enable the user to approve of using WiFi or Radio triangulation when the GPS is not available.
                self.isFirstGpsClick = false;
                let gpsAlert : UIAlertController? = self.displayDataModel!.provideGPSAlerters();
                if (gpsAlert != nil) {
                    self.present(gpsAlert!, animated:false, completion:nil);
                }
            }
            actionString = (self.displayDataModel?.provideGPSLocationData())!
        }
        else {
            actionString = "GPS Button Action Failure: Data Model not created"
        }

        DispatchQueue.main.async {
            self.displayButtonAction?.text = nil
            self.displayButtonAction?.text = actionString
        }
    }

    // This function is called when the getBatteryLevelAndState button is receives the touchUpInside event.
    func setLabelWithBatteryLevelAndState() {
        var actionString : String = "Get Battery Level and State";

        if (self.displayDataModel != nil) {
            actionString = (self.displayDataModel?.provideBatteryLevelAndState())!
        }
        else {
            actionString = "Battery Button Action Failure: Data Model not created"
        }

        DispatchQueue.main.async {
            self.displayButtonAction?.text = nil
            self.displayButtonAction?.text = actionString
        }
    }

    // This function is called when the getNextorkImplementation button is receives the touchUpInside event.
    func setLabelActionNetwork() {
        var actionString :String = "Fake Button set to American Express Stock Price"

        if (self.displayDataModel != nil) {
            actionString = (self.displayDataModel?.provideNetworkAccessData())!
        }
        else {
            actionString = "Network Button Action Failure: Data Model not created"
        }

        DispatchQueue.main.async {
            self.displayButtonAction?.text = nil
            self.displayButtonAction?.text = actionString
        }
    }

    func makeAButton(yButtonStart : CGFloat, buttonTitle: String, underSubview: UIView?) -> UIButton
    {
        let thisButton = UIButton.init(type: .system)
        thisButton.frame = CGRect(x: buttonXInit, y: yButtonStart, width: viewElementWidth, height: buttonHeight)
        thisButton.setTitle(buttonTitle, for:UIControlState.normal)
        thisButton.backgroundColor = UIColor.yellow
        thisButton.setTitleColor(UIColor.black, for: UIControlState.normal)

        if ((underSubview) == nil) {
            self.view.addSubview(thisButton)
        }
        else {
            self.view.insertSubview(thisButton, belowSubview:underSubview!)
        }

        return thisButton;
    }

    func makeALabel(yLabelStart : CGFloat, height: CGFloat, underSubview: UIView?) -> UILabel
    {
        let thisLabel = UILabel.init()
        thisLabel.frame = CGRect(x: buttonXInit, y: yLabelStart, width: viewElementWidth, height: height)
        thisLabel.font = thisLabel.font.withSize(12)     // Reduce the size of the text so that more output fits on a single line
        thisLabel.lineBreakMode = .byWordWrapping;
        thisLabel.numberOfLines = 0;                          // Allow the label to grow as necessary
        thisLabel.textAlignment = NSTextAlignment.center;
        thisLabel.textColor = UIColor.black;

        if ((underSubview) == nil) {
            self.view.addSubview(thisLabel)
        }
        else {
            self.view.insertSubview(thisLabel, belowSubview:underSubview!)
        }

        return thisLabel;
    }

    override func viewDidLoad() {
        super.viewDidLoad()
        // rather than assume a particular background color, set the background color so that everything can be seen.
        self.view.backgroundColor = UIColor.white
    }

    func addButtonAndLabels() -> Void {
        // If the width of the screen hasn't been used as a base for the size of the sub-views then
        // this function is not ready to generate the sub-views.
        if (selfWidth < 1.0) {
            return;
        }
        var viewElementVerticalLocation: CGFloat = startingVerticalLocation;

        self.screenTitle = makeALabel(yLabelStart: viewElementVerticalLocation, height: buttonHeight, underSubview: nil)
        self.screenTitle?.text = "Swift Implementation"
        self.screenTitle?.font = self.screenTitle?.font.withSize(16)
        viewElementVerticalLocation += viewElementVerticalSpace

        self.getGPSLongitudeAndLatitudeWithTimeStamp = makeAButton(yButtonStart: viewElementVerticalLocation, buttonTitle: "Get GPS Location with TimeStamp", underSubview: nil)
        self.getGPSLongitudeAndLatitudeWithTimeStamp?.addTarget(self, action: #selector(setLabelWithGPSLatitudeAndLongitudeWithTimeStampData), for:  .touchUpInside)
        viewElementVerticalLocation += viewElementVerticalSpace

        self.getBatteryLevelAndState = makeAButton(yButtonStart: viewElementVerticalLocation, buttonTitle: "Get Battery Level and State", underSubview: getGPSLongitudeAndLatitudeWithTimeStamp)
        self.getBatteryLevelAndState?.addTarget(self, action: #selector(setLabelWithBatteryLevelAndState), for:  .touchUpInside)
        viewElementVerticalLocation += viewElementVerticalSpace

        self.getNextorkImplementation = makeAButton(yButtonStart: viewElementVerticalLocation, buttonTitle: "Get the American Express Stock Price", underSubview: getBatteryLevelAndState)
        self.getNextorkImplementation?.addTarget(self, action: #selector(setLabelActionNetwork), for:  .touchUpInside)
        viewElementVerticalLocation += viewElementVerticalSpace


        self.displayButtonAction = makeALabel(yLabelStart: viewElementVerticalLocation, height: displayLabelHeight, underSubview: getNextorkImplementation)
    }

    func displayModelLibraryInitialization() -> CBool {
        if (self.displayDataModel == nil) {
            if let myDelegate = UIApplication.shared.delegate as? AppDelegate {
                self.displayDataModel = myDelegate.dataModelLibrary;
            }
        }
        return (self.displayDataModel == nil)
    }

    /*
     * The view is changing, either being displayed or rotated.
     * Set the button and label variables to nil to decrease any resource counters.
     * Redraw the buttons.
     */
    override func viewWillLayoutSubviews()
    {
        self.view.backgroundColor = UIColor.white
        self.getGPSLongitudeAndLatitudeWithTimeStamp = nil
        self.getGPSLongitudeAndLatitudeWithTimeStamp = nil
        self.getNextorkImplementation = nil
        self.screenTitle = nil
        self.displayButtonAction = nil
        /*
         * Get the tab bar height from the Application Delegate so that the total vertical space
         * can be calculated.
         */
        if let myDelegate = UIApplication.shared.delegate as? AppDelegate {
            // Tab Bar Height is larger than myDelegate.tabBarController.tabBar.frame.size.height indicates
            tabBarHeight = myDelegate.tabBarController.tabBar.frame.size.height * 2.5;
        }

        initFramingValuesOfMyDisplay()
    }

    func displayModelLibraryInitialization() -> CBool {
        if (self.displayDataModel == nil) {
            if let myDelegate = UIApplication.shared.delegate as? AppDelegate {
                self.displayDataModel = myDelegate.dataModelLibrary;
            }
        }
        return (self.displayDataModel == nil)
    }

    /*
     * The view is changing, either being displayed or rotated.
     * Set the button and label variables to nil to decrease any resource counters.
     * Redraw the buttons.
     */
    override func viewWillLayoutSubviews()
    {
        self.view.backgroundColor = UIColor.white
        self.getGPSLongitudeAndLatitudeWithTimeStamp = nil
        self.getGPSLongitudeAndLatitudeWithTimeStamp = nil
        self.getNextorkImplementation = nil
        self.screenTitle = nil
        self.displayButtonAction = nil
        /*
         * Get the tab bar height from the Application Delegate so that the total vertical space
         * can be calculated.
         */
        if let myDelegate = UIApplication.shared.delegate as? AppDelegate {
            // Tab Bar Height is larger than myDelegate.tabBarController.tabBar.frame.size.height indicates
            tabBarHeight = myDelegate.tabBarController.tabBar.frame.size.height * 2.5;
        }

        initFramingValuesOfMyDisplay()
        addButtonAndLabels()

    }

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()
        // Dispose of any resources that can be recreated.
    }

    public required init(coder aDecoder: NSCoder) {
        super.init(nibName: nil, bundle: nil)
        if (self.displayDataModel == nil) {
            if (self.displayModelLibraryInitialization()) {
                abort()
            }
            self.getGPSLongitudeAndLatitudeWithTimeStamp = nil
            self.getGPSLongitudeAndLatitudeWithTimeStamp = nil
            self.getNextorkImplementation = nil
            self.screenTitle = nil
            self.displayButtonAction = nil
        }
        initFramingValuesOfMyDisplay()
    }

    public init() {
        super.init(nibName: nil, bundle: nil)
        if (self.displayDataModel == nil) {
            if (self.displayModelLibraryInitialization()) {
                abort()
            }
            self.getGPSLongitudeAndLatitudeWithTimeStamp = nil
            self.getGPSLongitudeAndLatitudeWithTimeStamp = nil
            self.getNextorkImplementation = nil
            self.screenTitle = nil
            self.displayButtonAction = nil
        }
        initFramingValuesOfMyDisplay()
    }
}
\$\endgroup\$

1 Answer 1

2
+100
\$\begingroup\$

(The question is more than two years old, and both Xcode and Swift have been developed substantially in that time. The following review is written with the current Xcode 10.3 and Swift 5 in mind.)

Creating the user interface

Both the Objective-C and the Swift implementation create the user interface purely in code, which is laborious and error-prone. There are also some problems with your implementation.

First, the buttons and labels are created in viewWillLayoutSubviews, which can be called multiple times per view. Using the "Debug View Hierarchy" feature of Xcode you can see that all labels and buttons are created twice right after the application start:

enter image description here

Next, the layout is not updated correctly on an orientation change:

enter image description here

If you really want to create the user interface programmatically then the viewDidLoad methods is a better place. This method is called exactly once.

However, this is much easier done in the Xcode Interface Builder where you can design the views in a Storyboard, connect the button actions to code, etc.

And a better method to place the user interface elements correctly is Auto Layout. With Auto Layout you define a set of constraints between all UI elements. This can be done in a way that the views automatically adapt to various screen sizes and orientation changes.

The layout constraints can be defined in the Xcode interface builder or in code (although defining them in code is more work).

The action methods

This

let gpsAlert : UIAlertController? = self.displayDataModel!.provideGPSAlerters();
if (gpsAlert != nil) {
    self.present(gpsAlert!, animated:false, completion:nil);
}

looks like a design error to me: The model should not create an UI element and interact with with the user.

Here

@objc func setLabelWithBatteryLevelAndState() {
    var actionString : String = "Get Battery Level and State";

    if (self.displayDataModel != nil) {
        actionString = (self.displayDataModel?.provideBatteryLevelAndState())!
    }
    else {
        actionString = "Battery Button Action Failure: Data Model not created"
    }

    DispatchQueue.main.async {
        self.displayButtonAction?.text = nil
        self.displayButtonAction?.text = actionString
    }
}

several things can be improved:

  • Setting self.displayButtonAction?.text to nil before setting it to the actual message is not necessary.
  • Action methods are always called on the main thread, the DispatchQueue.main.async is not needed.
  • You can define actionString as a constant (with let) if it is initialized exactly once before used, the dummy initialization to "Get Battery Level and State" is not needed.
  • Optional binding is almost always preferred over explicit testing against nil and forced-unwrapping, see for example When should I compare an optional value to nil? on Stack Overflow.

With these suggestions, the action method could look like this:

@objc func setLabelWithBatteryLevelAndState() {
    let actionString : String
    if let displayModel = self.displayDataModel {
        actionString = displayModel.provideBatteryLevelAndState()
    } else {
        actionString = "Battery Button Action Failure: Data Model not created"
    }
    self.displayButtonAction?.text = actionString
}

However, this does not provide the best user experience: If the data model could not be created (for whatever reason) then the user is notified of that fact only after pressing the button.

The better approach is to enable or disable a button depending on whether its functionality is available or not.

The same remarks apply to the other action method setLabelActionNetwork().

Initializing the data model

Initializing the dataModelLibrary exactly once is easier done with a custom getter method and dispatch_once(), which is thread-safe:

- (PCI7DataModelLibrary *)dataModelLibrary {
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        _dataModelLibrary =  [[PCI7DataModelLibrary alloc] init];
    });
    return _dataModelLibrary;
}

(For Swift, see for example Whither dispatch_once in Swift 3? on Stack Overflow.)

Now you can simply test

if (self.dataModelLibrary) { ... }

to check if the model is available or not. However, this

if (![self dataModelLibraryInitialize]) {
    // If the library can't be allocated or initialized none of the buttons will work in any view controller.
    NSLog(@"application didFinishLaunchingWithOptions: Unable to alloc or init the PCI7DataModelLibrary object");
    return NO;
}

has the only effect that the user is presented with a black screen if the model is not available. (The return value from the application:didFinishLaunchingWithOptions: method is only relevant if the app is started to handle a URL resource or continue a user activity, but otherwise ignored.)

As mentioned above, the better approach is to enable or disable UI elements accordingly.

Further remarks

  • There is a Core Data model and corresponding methods in AppDelegate.m which are apparently unused.

  • The array initialization

    NSArray* controllers = [NSArray arrayWithObjects:objectiveVC, swiftVC,  nil];
    

    can be simplified using “Objective-C array literals”:

    NSArray* controllers = @[objectiveVC, swiftVC];
    
  • The declaration of

    - (id)init;
    

    in ObjectiveCViewController.h is not needed because that method is inherited from NSObject.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Thank you, in two days this will probably be the accepted answer. Thanks for killing a zombie. \$\endgroup\$
    – pacmaninbw
    Jul 28, 2019 at 16:30
  • \$\begingroup\$ Are any of the changes you have mentioned newer than 2 years ago when I wrote the question? (Should I have coded this in the suggested way 2 years ago?). I think this review explains some of the memory leaks I thought I was seeing in the debugger. \$\endgroup\$
    – pacmaninbw
    Jul 28, 2019 at 16:40
  • 1
    \$\begingroup\$ @pacmaninbw: I mentioned that because I had to change the Swift code a bit in order to make it compile in Xcode 10.3, and because I was not sure if features like "Debug View Hierarchy" existed in Xcode 8. Everything should apply independent of the Xcode/Swift version. – I did not observe memory leaks when running the app in the Simulator. \$\endgroup\$
    – Martin R
    Jul 28, 2019 at 16:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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