Converting SDLNames Defines to Constants #446

asm09fsu · 2016-09-21T23:44:11Z

Fixes #3

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

These changes have 100% passing tests.

Summary

This PR changes all #defines in SDLNames.h to use a typedef of NSString* called SDLName. Also, SDLFunctionID now uses a static NSDictionary for function names/ids to improve performance, and only one should ever need to be allocated.

Changelog

Enchancements

Updated all #defines in SDLNames.h to use constants.
Updated SDLFunctionID to use a static dictionary.

…so modified SDLFunctionID to use a static dictionary.

joeljfischer · 2016-09-22T13:26:51Z

SmartDeviceLink-iOS.xcodeproj/project.pbxproj

@@ -505,7 +505,7 @@
 		5D61FCF91A84238C00846EE7 /* SDLMenuParams.m in Sources */ = {isa = PBXBuildFile; fileRef = 5D61FB0C1A84238A00846EE7 /* SDLMenuParams.m */; };
 		5D61FCFA1A84238C00846EE7 /* SDLMyKey.h in Headers */ = {isa = PBXBuildFile; fileRef = 5D61FB0D1A84238A00846EE7 /* SDLMyKey.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		5D61FCFB1A84238C00846EE7 /* SDLMyKey.m in Sources */ = {isa = PBXBuildFile; fileRef = 5D61FB0E1A84238A00846EE7 /* SDLMyKey.m */; };
-		5D61FCFC1A84238C00846EE7 /* SDLNames.h in Headers */ = {isa = PBXBuildFile; fileRef = 5D61FB0F1A84238A00846EE7 /* SDLNames.h */; };
+		5D61FCFC1A84238C00846EE7 /* SDLNames.h in Headers */ = {isa = PBXBuildFile; fileRef = 5D61FB0F1A84238A00846EE7 /* SDLNames.h */; settings = {ATTRIBUTES = (Public, ); }; };


Does this have to be public now?

joeljfischer · 2016-09-22T13:28:18Z

SmartDeviceLink/SDLChangeRegistration.m

    } else {
-        [parameters removeObjectForKey:NAMES_ttsName];
+        [parameters removeObjectForKey:SDLNameTtsName];


Shouldn't this be SDLNameTTSName?

joeljfischer · 2016-09-22T14:56:15Z

SmartDeviceLink/SDLFunctionID.m

+                            @98305: SDLNameOnSyncPData
+                            };
+        }
+        self.functionIDs = functionIDs;


Would this be better as a singleton? Kind of weird to have to create a new object every time.

joeljfischer · 2016-09-22T14:58:28Z

SmartDeviceLink/SDLFunctionID.m

- (NSString *)getFunctionName:(int)functionID {
-    return [functionIDs objectForKey:[NSString stringWithFormat:@"%d", functionID]];
+- (SDLName)getFunctionName:(int)functionID {
+    return [self.functionIDs objectForKey:@(functionID)];


This can use shortened syntax: self.functionIDs[@(functionID)]

joeljfischer · 2016-09-22T15:00:50Z

SmartDeviceLink/SDLFunctionID.m

+
+@property (nonatomic, strong, nonnull) NSDictionary* functionIDs;
+
+@end

 @implementation SDLFunctionID


This class can be private

joeljfischer · 2016-09-22T15:03:49Z

SmartDeviceLink/SDLFunctionID.m

    }
    return self;
 }

- (NSString *)getFunctionName:(int)functionID {
-    return [functionIDs objectForKey:[NSString stringWithFormat:@"%d", functionID]];
+- (SDLName)getFunctionName:(int)functionID {


The get function naming is very Java. Should probably be more like functionNameForId:

joeljfischer · 2016-09-22T15:05:46Z

SmartDeviceLink/SDLGPSData.m

 }

 - (void)setUtcYear:(NSNumber *)utcYear {
    if (utcYear != nil) {
-        [store setObject:utcYear forKey:NAMES_utcYear];
+        [store setObject:utcYear forKey:SDLNameUtcYear];


Same as above comment regarding capitalization?

asm09fsu · 2016-09-22T19:38:16Z

@joeljfischer I think this is ready, can you validate?

…into feature/issue_3_constant_names * 'develop' of https://github.com/smartdevicelink/sdl_ios: Update public imports for organization and to remove a duplicate A few more updates to travis scripts Try better Travis CI build formatting

codecov-io · 2016-09-23T16:54:03Z

Current coverage is 67.39% (diff: 69.55%)

Merging #446 into develop will increase coverage by 39.71%

@@            develop       #446   diff @@
==========================================
  Files           334        334           
  Lines         10451      10515     +64   
  Methods        3059       3061      +2   
  Messages          0          0           
  Branches        799        799           
==========================================
+ Hits           2893       7087   +4194   
+ Misses         7500       3036   -4464   
- Partials         58        392    +334

Powered by Codecov. Last update 30f9b83...7246055

joeljfischer · 2016-09-23T19:22:43Z

SmartDeviceLink-iOS.xcodeproj/project.pbxproj

@@ -427,7 +427,7 @@
 		5D61FCAA1A84238C00846EE7 /* SDLFileType.m in Sources */ = {isa = PBXBuildFile; fileRef = 5D61FABD1A84238A00846EE7 /* SDLFileType.m */; };
 		5D61FCAB1A84238C00846EE7 /* SDLFuelCutoffStatus.h in Headers */ = {isa = PBXBuildFile; fileRef = 5D61FABE1A84238A00846EE7 /* SDLFuelCutoffStatus.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		5D61FCAC1A84238C00846EE7 /* SDLFuelCutoffStatus.m in Sources */ = {isa = PBXBuildFile; fileRef = 5D61FABF1A84238A00846EE7 /* SDLFuelCutoffStatus.m */; };
-		5D61FCAD1A84238C00846EE7 /* SDLFunctionID.h in Headers */ = {isa = PBXBuildFile; fileRef = 5D61FAC01A84238A00846EE7 /* SDLFunctionID.h */; };
+		5D61FCAD1A84238C00846EE7 /* SDLFunctionID.h in Headers */ = {isa = PBXBuildFile; fileRef = 5D61FAC01A84238A00846EE7 /* SDLFunctionID.h */; settings = {ATTRIBUTES = (Private, ); }; };


Note, we don't actually want 'private', we want 'project'.

joeljfischer

Well that was a needle in haystack hunt...

joeljfischer · 2016-09-27T15:06:45Z

SmartDeviceLink/SDLProxy.h

@@ -42,7 +42,7 @@ __deprecated_msg("Use SDLManager instead")
 - (void)sendRPCRequest:(SDLRPCMessage *)msg __deprecated_msg("use -sendRPC: instead");

 - (void)handleRPCDictionary:(NSDictionary *)dictionary;
- (void)handleRpcMessage:(NSDictionary *)msg __deprecated_msg("use -handleRPCDictionary: instead");
+- (void)handleRPCMessage:(NSDictionary *)msg __deprecated_msg("use -handleRPCDictionary: instead");


This cannot be changed

This was a mis-commit when using regex to modify the 1,000,000 files involved in this PR.

joeljfischer · 2016-09-27T15:12:33Z

SmartDeviceLink/SDLProtocol.m

@@ -203,7 +203,7 @@ - (BOOL)sendRPC:(SDLRPCMessage *)message encrypted:(BOOL)encryption error:(NSErr
            // Build a binary header
            // Serialize the RPC data into an NSData
            SDLRPCPayload *rpcPayload = [[SDLRPCPayload alloc] init];
-            rpcPayload.functionID = [[[[SDLFunctionID alloc] init] getFunctionID:[message getFunctionName]] intValue];
+            rpcPayload.functionID = [[[SDLFunctionID sharedInstance] functionIdForName:[message functionNameForId]] intValue];


A few things about this seem odd.

Why are we getting the name for the id, then the id for the name? Seems circular?

Wouldn't the message's functionNameForId method just be functionName if we're not passing anything? Can't find if or where that changed in this PR though.

seems we need a way to retrieve the id for a function name (as a response perhaps?) and the function name for the id (to send a request?). This is just a slight modification/improvement over how it's already been done. A full revamp can be done for 5.0.

I think i was just confused by the change in [message functionNameForId]. It's actually getting the function name not the id.

joeljfischer · 2016-09-27T15:14:26Z

SmartDeviceLink/SDLRPCMessage.h

@@ -13,7 +13,7 @@

 - (instancetype)initWithName:(NSString *)name;
 - (instancetype)initWithDictionary:(NSMutableDictionary *)dict;
- (NSString *)getFunctionName;
+- (NSString *)functionNameForId;


Oh, here it is, guessing a copy-paste error?

So to be clear, this has to stay the same to preserve the impact of this PR.

Replaced all defines with constant strings of the SDLName typedef. Al…

dd05e50

…so modified SDLFunctionID to use a static dictionary.

asm09fsu added enhancement best practice Not a defect but something that should be improved anyway labels Sep 21, 2016

asm09fsu changed the base branch from master to develop September 21, 2016 23:44

joeljfischer reviewed Sep 22, 2016

View reviewed changes

asm09fsu added 7 commits September 22, 2016 10:25

Reverted SDLNames to Project instead of Public.

3590d69

Removed extensibility from SDLName

9ea32a7

Updated naming of some constant.

ea5beb1

Fixed issue with tests not passing

90c51bf

Updated SDLFunctionID to be a singleton.

8676a8a

Fixed remaining failing tests.

a194d52

Updated SDLFunctionID to be private.

56dd436

joeljfischer and others added 2 commits September 23, 2016 09:24

Merge branch 'develop' into feature/issue_3_constant_names

aac4fe7

joeljfischer reviewed Sep 23, 2016

View reviewed changes

Updated SDLFunctionID to project instead of private.

120b92f

joeljfischer suggested changes Sep 27, 2016

View reviewed changes

asm09fsu added 2 commits September 27, 2016 09:15

Fixed accidental function renaming.

7246055

reverting function renaming in SDLRPCMessage.

2f587e1

joeljfischer approved these changes Sep 27, 2016

View reviewed changes

joeljfischer merged commit 2f587e1 into develop Sep 27, 2016

joeljfischer deleted the feature/issue_3_constant_names branch September 27, 2016 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converting SDLNames Defines to Constants #446

Converting SDLNames Defines to Constants #446

asm09fsu commented Sep 21, 2016

joeljfischer Sep 22, 2016

joeljfischer Sep 22, 2016

joeljfischer Sep 22, 2016

joeljfischer Sep 22, 2016

joeljfischer Sep 22, 2016

joeljfischer Sep 22, 2016

joeljfischer Sep 22, 2016

asm09fsu commented Sep 22, 2016

codecov-io commented Sep 23, 2016 •

edited

Loading

joeljfischer Sep 23, 2016

joeljfischer left a comment

joeljfischer Sep 27, 2016

asm09fsu Sep 27, 2016

joeljfischer Sep 27, 2016

asm09fsu Sep 27, 2016

joeljfischer Sep 27, 2016

joeljfischer Sep 27, 2016

joeljfischer Sep 27, 2016

Converting SDLNames Defines to Constants #446

Converting SDLNames Defines to Constants #446

Conversation

asm09fsu commented Sep 21, 2016

Risk

Testing Plan

Summary

Changelog

Enchancements

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asm09fsu commented Sep 22, 2016

codecov-io commented Sep 23, 2016 • edited Loading

Current coverage is 67.39% (diff: 69.55%)

Choose a reason for hiding this comment

joeljfischer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 23, 2016 •

edited

Loading