2
\$\begingroup\$

First, I've extended the String class to detect uppercase and lowercase characters:

class String

    def is_uppercase?
         self == self.upcase    
    end

    def is_lowercase?
        self == self.downcase
    end

end

I then use these instance methods to fulfil my main goal of transferring the format of an old string to a new string:

def maintain_format(instring, outstring)         
      instring_array = instring.split(//)
      outstring_array = outstring.split(//)

      both_arrays = instring_array.zip(outstring_array)

      both_arrays.each do |character|
        if character[0] == nil || character[0].is_lowercase?
          character[1].downcase! if character[1]
        elsif character[0].is_uppercase?
          character[1].upcase! if character[1]
        end
     end

     outstring_array.join("")
end

It works well and does exactly what I want:

2.0.0p247 :002 > maintain_format('jimmy','Jam')
 => "jam" 
2.0.0p247 :003 > maintain_format('Jimmy','xam')
 => "Xam" 
2.0.0p247 :004 > maintain_format('realLylongname','shorter')
 => "shorTer" 
2.0.0p247 :005 > maintain_format('rSeHorteR','muchmuchmuchlongerfoos')
 => "mUcHmuchMuchlongerfoos" 

However, what I love about ruby is how it is sort self-documenting if you use sensible variable and method names and hash keys. With this code it is a bit hard to follow regarding the 1character[0]character[1]` stuff. I attempting it with hashes but got in a bit of a mess. How would you tackle this problem?

Also interested in any ruby tricks to make this take up less lines.

Note this is part of a rails project, so all rails methods are available to me.

\$\endgroup\$
2
\$\begingroup\$

My common advise is to avoid accumulator variables.

def maintain_format template, string
  string.chars.zip(template.chars).map{ |orig, char|
    char ? char == char.upcase ? orig.upcase : orig.downcase : orig
  }.join
end

p maintain_format "qWERty", "asDFGhJj"
>> "aSDFghJj"

Note char ? ... : orig since zip gives nils instead of second array element if it's not available (if template is too short).

\$\endgroup\$
  • \$\begingroup\$ +1 for avoiding the accumulator. But that double ternary is kinda hairy. It's not super complex or anything, but it's unnecessarily terse and harder to read. \$\endgroup\$ – Flambino Apr 17 '14 at 14:49
3
\$\begingroup\$

First off, I'd rename those two String methods. You obviously had a plan in mind when you added them (i.e. that the strings you'd be checking would be 1 character long), but from a more general perspective, it's fraught with ambiguity.

For instance, is the string "Hello, world" uppercase? Or is it lowercase? Is it sentence-case? Titlecase? So your methods, simple as they are, are perhaps too simple to make sense when applied outside the (narrow) usage you've tailored them for. As such, they probably shouldn't be global String methods. Certainly, you don't need to extend a core class like String just for a simple == comparison.

Also, Ruby convention is to not use an is_ prefix on interrogatory methods like those. The nil? method, for instance, isn't called is_nil?. Same for one?, any?, empty?, include? and so on.

As for your maintain_format method, here's what came to mind:

  • There's a String#chars method, which'll give you an array of chars - no need for split
  • Instead of zipping, use a loop and use an index - characters in a string can be accessed just like elements in an array.
  • And if you're going to loop, no need to loop further than the shortest of the two strings. Or, if you zip the arrays like you do right now, make sure you break the loop when one string or the other "runs out"
  • You only need to check for uppercase. Everything else gets downcased by default. A character can only be upper or lowercase, so if it's not uppercase, you already know that it's lowercase; no need for extra ifs (but see the update below)

I'd suggest something like

def maintain_format(template, string)
  string = string.downcase              # do this right away
  template = template[0..string.length] # shorten the template, if necessary
  template.chars.each_with_index do |char, index|
    next unless char == char.upcase     # skip lowercase chars
    string[index] = string[index].upcase
  end
  string
end

Perhaps there's a smarter way to detect upper-/lowercase chars, but I couldn't quite think of one that'd be as simple and handle unicode etc.

Lastly: Naming (again). maintain_format is a bit off to me; you're applying a format, copying a format, or something along those lines. (The title of your question also calls it "transfer" rather than "maintain". But transfer would suggest that one string loses its format, when it's applied to the other, so it's not quite right either.)

Ironically, it might make more sense to add this method to String, since it's somewhat more generally applicable. Besides, a call like template.assimilate(other_string) (besides sounding ominous, would make it much clearer which string is the template, and which string will be changed. Right now, it can quickly become confusing which argument is which. But again, better naming might alleviate that.


Update: As 200_success rightly points out in the comments, it's surprising that a string that's longer than the template string gets downcased rather than simply keep whatever case it had. And code shouldn't surprise.

Hence it might be better to do this:

def maintain_format(template, string)
  string = string.dup  # still want a duplicate to work on
  template = template[0..string.length]
  template.chars.each_with_index do |char, i|
    string[i] = char == char.upcase ? string[i].upcase : string[i].downcase
  end
  string
end

You can still achieve the original behavior by simply downcasing the string yourself before passing it to the method.

\$\endgroup\$
  • \$\begingroup\$ I'd expect the part of the string that extends beyond the end of template to have its case preserved, not forcibly downcased. \$\endgroup\$ – 200_success Apr 16 '14 at 9:56
  • \$\begingroup\$ @200_success As would I. But as far as I can tell from OP's code, case isn't preserved for the "leftover" string... Regardless, you're right; it's unexpected. I'll add a note - thanks \$\endgroup\$ – Flambino Apr 16 '14 at 11:22
  • \$\begingroup\$ @Flambino could you explain the 5th line please? ` string[i] = char == char.upcase ? string[i].upcase : string[i].downcase` Literally all of it. I've got a good idea but I'd want some validation :) \$\endgroup\$ – Starkers Apr 16 '14 at 11:39
  • 1
    \$\begingroup\$ @Starkers It's just an assignment with a ternary, i.e. if...else written in one line. You can spell it out in Ruby as string[i] = (if char == char.upcase then string[i].upcase else string[i].downcase end). \$\endgroup\$ – Flambino Apr 16 '14 at 12:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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