hoop33 / wry

App.net command-line client for Mac OS X
http://grailbox.com/wry
MIT License
43 stars 6 forks source link

Omits user relationship when none #77

Closed jeremy-w closed 10 years ago

jeremy-w commented 10 years ago

This implements the "show nothing instead of ' -- You'" suggestion discussed in #76.

parkr commented 10 years ago

This is really convoluted code (though thanks for the newlines :). Any way we can separate it out into a separate method and simplify it?

I'm definitely not an Obj-C developer, but here's a clearer idea:

- (NSString* relationship)relationshipText {
    if (!self.youFollow && !self.followsYou) { return @""; }
    if (self.youFollow  && self.followsYou)  { return @"<--> You"; }
    if (self.youFollow  && !self.followsYou) { return @"<-- You"; }
    if (!self.youFollow && self.followsYou)  { return @"--> You"; }
}
jeremy-w commented 10 years ago

@parkr: I had about 3 different revisions of that logic, because it's just gnarly enough to be annoying. This was actually the most straightforward because it's an expression resulting in a single assignment as the result of a series of if-else tests.

Could your sense that it's convoluted simply be due to not seeing the ternary operator ?: used much before?

A rewording of what I have now in statement-form is:

  NSString *relationship;
  if (self.youFollow && self.followsYou) relationship = @" <--> You";
  else if (self.youFollow) relationship = @" <-- You";
  else if (self.followsYou) relationship = @" --> You";
  else relationship = @"";  /* no relationship */

If you squint, this is little more than a textual transformation of the ternary version, but it's far more verbose constantly reiterating "this is an assignment to relationship!" (which is handled in the ?: version by its being an expression returning a value that can then be assigned at the end); there's also a lot more keyword noise.

My first version looked like this:

  NSString *relationship = @"";
  BOOL isRelated = self.youFollow || self.followsYou;
  if (isRelated) {
    char *youFollow = (self.youFollow ? "<" : "");
    char *followsYou = (self.followsYou ? ">" : "");
    relationship = [NSString stringWithFormat:@" %s--%s You",
                    youFollow, followsYou];
  }

And a version that I liked for its directness and lack of branching, but disliked because you had to think a bit to verify the index was always correct, was:

  NSString *relationships[] = {
    @"",
    @" <-- You",
    @" --> You",
    @" <--> You",
  };
  bool youFollow = self.youFollow;
  bool followsYou = self.followsYou;
  NSString *relationship = relationships[youFollow + 2*followsYou];

In the end, I went with the very explicit and unambiguous if-else expression rather than statement. I figured a bunch of hemming-and-hawing commits over how to compute a relationship would be unwanted in a PR.

jeremy-w commented 10 years ago

As far as a separate method: since it really is little more than a switch to pick a string, and is unlikely to be used elsewhere, it didn't seem to warrant one, but I could readily extract it into a stringForRelationshipToYou method.

parkr commented 10 years ago

I tend to avoid the ternary operators, as it just too little syntax to be developer-friendly. Nested ternary operators, as is present there, is like trying to put together a puzzle whose edges are blurred. Parentheses would help, but ultimately I think ternary conditionals should be relegated to one-line statements only. It's a preference, but I think it objectively makes the code easier to read.

I like the first example you give in your code a lot.

jeremy-w commented 10 years ago

It actually makes it harder to read for me, because we've decoupled declaring the variable and storing the value to it. Prior to ARC, I'd have had to have expressly initialized the variable to a known value, or risk an uninitialized variable access should the logic somehow fail to set it.

It's likely I could get the best of both worlds using the statement-expression extension, but I'll just switch to the more standard version I put above.

hoop33 commented 10 years ago

I moved it to a relationship method so it can be used for the colorized version as well.

parkr commented 10 years ago

Excellent, thank you!