Apply suggestions from code review

Co-authored-by: Joel Fischer <joeljfischer@gmail.com>

6 files changed, 24 insertions, 23 deletions

diff --git a/SmartDeviceLink/SDLScreenManager.h b/SmartDeviceLink/SDLScreenManager.h
index 78349ef58..a08d3b40d 100644
--- a/SmartDeviceLink/SDLScreenManager.h
+++ b/SmartDeviceLink/SDLScreenManager.h
@@ -272,15 +272,15 @@ If set to `SDLDynamicMenuUpdatesModeForceOff`, menu updates will work the legacy
 ///
 /// 1. A selector with no parameters. The observer will be notified when a button press occurs (it will not know if a short or long press has occurred).
 ///
-/// 2. A selector with one parameter, (SDLButtonName). The observer will be notified when a button press occurs (it will not know if a short or long press has occurred).
+/// 2. A selector with one parameter: (SDLButtonName). The observer will be notified when a button press occurs (both a short and long press will trigger the selector, but it will not be able to distinguish between them). It will not be notified of button events.
 ///
-/// 3. A selector with two parameters, (SDLButtonName, NSError). The observer will be notified when a button press occurs (it will not know if a short or long press has occurred).
+/// 3. A selector with two parameters: (SDLButtonName, NSError). The observer will be notified when a button press occurs (both a short and long press will trigger the selector, but it will not be able to distinguish between them). It will not be notified of button events.
 ///
-/// 4. A selector with three parameters, (SDLButtonName, NSError, SDLOnButtonPress). The observer will be notified when a long or short button press occurs, but not a button event.
+/// 4. A selector with three parameters: (SDLButtonName, NSError, SDLOnButtonPress). The observer will be notified when a long or short button press occurs (and can distinguish between a short or long press), but will not be notified of individual button events.
 ///
-/// 5. A selector with four parameters, (SDLButtonName, NSError, SDLOnButtonPress, SDLOnButtonEvent). The observer will be notified when any button press or any button event occurs.
+/// 5. A selector with four parameters: (SDLButtonName, NSError, SDLOnButtonPress, SDLOnButtonEvent). The observer will be notified when any button press or any button event occurs (and can distinguish between them).
 ///
-/// To unsubscribe to the hard button, please call `unsubscribeButton:withObserver:withCompletionHandler:`.
+/// To unsubscribe from the hard button, call `unsubscribeButton:withObserver:withCompletionHandler:`.
 ///
 /// @param buttonName The name of the hard button to subscribe to
 /// @param observer The object that will have `selector` called whenever the button has been selected
diff --git a/SmartDeviceLink/SDLSubscribeButtonManager.m b/SmartDeviceLink/SDLSubscribeButtonManager.m
index 74721b107..5e8688b5e 100644
--- a/SmartDeviceLink/SDLSubscribeButtonManager.m
+++ b/SmartDeviceLink/SDLSubscribeButtonManager.m
@@ -83,7 +83,7 @@ NS_ASSUME_NONNULL_BEGIN
 }
 
 - (void)subscribeButton:(SDLButtonName)buttonName withObserver:(id<NSObject>)observer selector:(SEL)selector; {
-    SDLLogV(@"Subscribing to subscribe button with name: %@, with observer: %@, selector: %@", buttonName, observer, NSStringFromSelector(selector));
+    SDLLogD(@"Subscribing to subscribe button with name: %@, with observer: %@, selector: %@", buttonName, observer, NSStringFromSelector(selector));
 
     NSUInteger numberOfParametersInSelector = [NSStringFromSelector(selector) componentsSeparatedByString:@":"].count - 1;
     if (numberOfParametersInSelector > 4) {
@@ -106,7 +106,6 @@ NS_ASSUME_NONNULL_BEGIN
 }
 
 - (void)unsubscribeButton:(SDLButtonName)buttonName withObserver:(id<NSObject>)observer withCompletionHandler:(nullable SDLSubscribeButtonUpdateCompletionHandler)completionHandler {
-
     if (self.subscribeButtonObservers[buttonName] == nil || ![self sdl_isSubscribedObserver:observer forButtonName:buttonName]) {
         SDLLogE(@"Attempting to unsubscribe to the %@ subscribe button which is not currently subscribed", buttonName);
         return completionHandler(nil);
@@ -121,11 +120,11 @@ NS_ASSUME_NONNULL_BEGIN
     __weak typeof(self) weakSelf = self;
     [self.connectionManager sendConnectionRequest:unsubscribeButton withResponseHandler:^(__kindof SDLRPCRequest * _Nullable request, __kindof SDLRPCResponse * _Nullable response, NSError * _Nullable error) {
         if (response.success.boolValue == NO) {
-            SDLLogE(@"Attempt to unsubscribe to subscribe button named %@ failed", buttonName);
+            SDLLogE(@"Attempt to unsubscribe from subscribe button %@ failed", buttonName);
             return completionHandler(error);
         }
 
-        SDLLogD(@"Successfully unsubscribed to subscribe button named %@", buttonName);
+        SDLLogD(@"Successfully unsubscribed from subscribe button named %@", buttonName);
         __strong typeof(weakSelf) strongSelf = weakSelf;
         [strongSelf sdl_removeSubscribedObserver:observer forButtonName:buttonName];
         return completionHandler(error);
@@ -154,9 +153,10 @@ NS_ASSUME_NONNULL_BEGIN
 /// @param buttonName The name of the button
 - (void)sdl_removeSubscribedObserver:(id<NSObject>)observer forButtonName:(SDLButtonName)buttonName {
     NSMutableArray *subscribedObservers = self.subscribeButtonObservers[buttonName];
-    for (uint i = 0; i < subscribedObservers.count; i += 1) {
+    for (NSUInteger i = 0; i < subscribedObservers.count; i++) {
         SDLSubscribeButtonObserver *subscribedObserver = (SDLSubscribeButtonObserver *)subscribedObservers[i];
-        if (subscribedObserver.observer != observer ) { continue; }
+        if (subscribedObserver.observer != observer) { continue; }
+        // Okay to mutate because we will break immediately afterward
         [subscribedObservers removeObjectAtIndex:i];
         break;
     }
@@ -177,7 +177,7 @@ NS_ASSUME_NONNULL_BEGIN
         __strong typeof(weakSelf) strongSelf = weakSelf;
         if (strongSelf.subscribeButtonObservers[buttonName] == nil) {
             SDLLogV(@"Adding first subscriber for button: %@", buttonName);
-            strongSelf.subscribeButtonObservers[buttonName] = [NSMutableArray arrayWithArray:@[subscribedObserver]];
+            strongSelf.subscribeButtonObservers[buttonName] = [NSMutableArray arrayWithObject:subscribedObserver];
         } else {
             SDLLogV(@"Adding another subscriber for button: %@", buttonName);
             [strongSelf.subscribeButtonObservers[buttonName] addObject:subscribedObserver];
@@ -191,10 +191,11 @@ NS_ASSUME_NONNULL_BEGIN
 - (void)sdl_subscribeToButtonNamed:(SDLButtonName)buttonName withObserverObject:(SDLSubscribeButtonObserver *)observerObject {
     SDLSubscribeButton *subscribeButton = [[SDLSubscribeButton alloc] initWithButtonName:buttonName handler:nil];
     __weak typeof(self) weakSelf = self;
+
     [self.connectionManager sendConnectionRequest:subscribeButton withResponseHandler:^(__kindof SDLRPCRequest * _Nullable request, __kindof SDLRPCResponse * _Nullable response, NSError * _Nullable error) {
         __strong typeof(weakSelf) strongSelf = weakSelf;
         // Check if the request was successful. If a subscription request got sent for a button that is already subscribed then the module will respond with `IGNORED` so just act as if subscription succeeded.
-        if (response.success.boolValue == NO && ![response.resultCode isEqualToEnum:SDLResultIgnored]) {
+        if (!response.success.boolValue && ![response.resultCode isEqualToEnum:SDLResultIgnored]) {
             [strongSelf sdl_invokeObserver:observerObject withButtonName:buttonName buttonPress:nil buttonEvent:nil error:error];
             return;
         }
diff --git a/SmartDeviceLinkTests/SDLSubscribeButtonManagerSpec.m b/SmartDeviceLinkTests/SDLSubscribeButtonManagerSpec.m
index c54be9a61..c1bc51bb6 100644
--- a/SmartDeviceLinkTests/SDLSubscribeButtonManagerSpec.m
+++ b/SmartDeviceLinkTests/SDLSubscribeButtonManagerSpec.m
@@ -87,7 +87,7 @@ describe(@"subscribe button manager", ^{
             [testManager start];
         });
 
-        describe(@"should subscribe with an update handler", ^{
+        describe(@"should subscribe with an update block handler", ^{
             __block SDLButtonName testButtonName = nil;
             __block SDLSubscribeButtonUpdateHandler testUpdateHandler1 = nil;
             __block BOOL testHandler1Called = NO;
@@ -223,14 +223,14 @@ describe(@"subscribe button manager", ^{
                 expect([testConnectionManager.receivedRequests[0] isKindOfClass:SDLSubscribeButton.class]);
             });
 
-            it(@"should not notify the observer when a success response is recieved for the subscribe button request", ^{
+            it(@"should not notify the observer when a success response is received for the subscribe button request", ^{
                 [testManager subscribeButton:testButtonName withObserver:testSubscribeButtonObserver1 selector:testSelector1];
                 [testConnectionManager respondToLastRequestWithResponse:testSubscribeSuccessResponse];
 
                 expect(testSubscribeButtonObserver1.selectorCalledCount).to(equal(0));
             });
 
-            it(@"should notify the observer with the error and remove the observer when a failure response is recieved for the subscribe button request", ^{
+            it(@"should notify the observer with the error and remove the observer when a failure response is received for the subscribe button request", ^{
                 [testManager subscribeButton:testButtonName withObserver:testSubscribeButtonObserver1 selector:testSelector2];
                 NSError *testError = [NSError errorWithDomain:@"errorDomain" code:9 userInfo:@{@"subscribe button error":@"error 2"}];
                 [testConnectionManager respondToLastRequestWithResponse:testSubscribeFailureResponse error:testError];
@@ -322,7 +322,7 @@ describe(@"subscribe button manager", ^{
                 [testManager subscribeButton:testButtonName withObserver:testObserver2 selector:testSelector2];
             });
 
-            it(@"should notify all observers when a button event notification is recieved", ^{
+            it(@"should notify all observers when a button event notification is received", ^{
                 [[NSNotificationCenter defaultCenter] postNotification:buttonEventNotification];
 
                 expect(testHandler1Called).toEventually(beTrue());
diff --git a/SmartDeviceLink_Example/SubscribeButtonManager.h b/SmartDeviceLink_Example/SubscribeButtonManager.h
index ee93fc94f..ed4e7f23e 100644
--- a/SmartDeviceLink_Example/SubscribeButtonManager.h
+++ b/SmartDeviceLink_Example/SubscribeButtonManager.h
@@ -18,10 +18,10 @@ NS_ASSUME_NONNULL_BEGIN
 - (instancetype)initWithManager:(SDLManager *)manager;
 
 /// Subscribe to all available preset buttons. An alert will be shown when a preset button is short pressed or long pressed.
-- (void)subscribeToPresetButtons;
+- (void)subscribeToAllPresetButtons;
 
 /// Unsubscribes to all subscribed preset buttons
-- (void)unsubscribeToPresetButtons;
+- (void)unsubscribeFromAllPresetButtons;
 
 @end
 
diff --git a/SmartDeviceLink_Example/SubscribeButtonManager.m b/SmartDeviceLink_Example/SubscribeButtonManager.m
index 788ae7fce..80f8354a2 100644
--- a/SmartDeviceLink_Example/SubscribeButtonManager.m
+++ b/SmartDeviceLink_Example/SubscribeButtonManager.m
@@ -91,7 +91,7 @@ NS_ASSUME_NONNULL_BEGIN
     }
 }
 
-+ (NSArray<SDLButtonName> *)presetButtons {
++ (NSArray<SDLButtonName> *)sdlex_allPresetButtons {
     return @[SDLButtonNamePreset0, SDLButtonNamePreset1, SDLButtonNamePreset2, SDLButtonNamePreset3, SDLButtonNamePreset4, SDLButtonNamePreset5, SDLButtonNamePreset6, SDLButtonNamePreset7];
 }
 
diff --git a/SmartDeviceLink_Example/SubscribeButtonManager.swift b/SmartDeviceLink_Example/SubscribeButtonManager.swift
index 552050cb6..308ff939a 100644
--- a/SmartDeviceLink_Example/SubscribeButtonManager.swift
+++ b/SmartDeviceLink_Example/SubscribeButtonManager.swift
@@ -11,9 +11,9 @@ import SmartDeviceLink
 import SmartDeviceLinkSwift
 
 class SubscribeButtonManager {
-    fileprivate let sdlManager: SDLManager!
-    let presetButtons: [SDLButtonName] = [.preset0, .preset1, .preset2, .preset3, .preset4, .preset5, .preset6, .preset7]
-    fileprivate var presetButtonSubscriptionIDs = [SDLButtonName: Any]()
+    private let sdlManager: SDLManager!
+    private let presetButtons: [SDLButtonName] = [.preset0, .preset1, .preset2, .preset3, .preset4, .preset5, .preset6, .preset7]
+    private var presetButtonSubscriptionIDs = [SDLButtonName: Any]()
 
     init(sdlManager: SDLManager) {
         self.sdlManager = sdlManager