Skip to main content
Code Review

Return to Answer

Bounty Awarded with 25 reputation awarded by Community Bot
deleted 69 characters in body
Source Link
janos
  • 112.9k
  • 15
  • 154
  • 396

Extract constant expression to outside of loop

In this code, (decode ? 26 - key : key) is evaluated for each character in text:

 text
 .chars
 .select { |char| ALPHABET.include?(char) }
 .map {|char| shift(char, (decode ? 26 - key : key) )}
 .join

It would be better to move it outside the expression so that it's only evaluated once:

 shift_key = decode ? 26 - key : key
 text
 .chars
 .select { |char| ALPHABET.include?(char) }
 .map {|char| shift(char, shift_key)}
 .join

The same goes for the other cipher method, where you do the same.

Minor formatting issues

The code formatting style is not consistent, for example compare these two lines:

 .select { |char| ALPHABET.include?(char) }
 .map {|char| shift(char, shift_key)}

In the first one, there's a space after { and before }, but in the second there isn't. Both ways seem fine to me, but it would be better to use one of these ways, consistently everywhere.

Similarly here, there's a space after [ but not before ]. For the sake of consistency, it would be better to remove the space after [ or add one before ].

 ALPHABET[ (ALPHABET.index(letter) + key) % ALPHABET.length]

Overall

This looks nice, a pleasure to read, and I don't even know Ruby ;-)

Extract constant expression to outside of loop

In this code, (decode ? 26 - key : key) is evaluated for each character in text:

 text
 .chars
 .select { |char| ALPHABET.include?(char) }
 .map {|char| shift(char, (decode ? 26 - key : key) )}
 .join

It would be better to move it outside the expression so that it's only evaluated once:

 shift_key = decode ? 26 - key : key
 text
 .chars
 .select { |char| ALPHABET.include?(char) }
 .map {|char| shift(char, shift_key)}
 .join

The same goes for the other cipher method, where you do the same.

Minor formatting issues

The code formatting style is not consistent, for example compare these two lines:

 .select { |char| ALPHABET.include?(char) }
 .map {|char| shift(char, shift_key)}

In the first one, there's a space after { and before }, but in the second there isn't. Both ways seem fine to me, but it would be better to use one of these ways, consistently everywhere.

Similarly here, there's a space after [ but not before ]. For the sake of consistency, it would be better to remove the space after [ or add one before ].

 ALPHABET[ (ALPHABET.index(letter) + key) % ALPHABET.length]

Overall

This looks nice, a pleasure to read, and I don't even know Ruby ;-)

Extract constant expression to outside of loop

In this code, (decode ? 26 - key : key) is evaluated for each character in text:

 text
 .chars
 .select { |char| ALPHABET.include?(char) }
 .map {|char| shift(char, (decode ? 26 - key : key) )}
 .join

It would be better to move it outside the expression so that it's only evaluated once:

 shift_key = decode ? 26 - key : key
 text
 .chars
 .select { |char| ALPHABET.include?(char) }
 .map {|char| shift(char, shift_key)}
 .join

Minor formatting issues

The code formatting style is not consistent, for example compare these two lines:

 .select { |char| ALPHABET.include?(char) }
 .map {|char| shift(char, shift_key)}

In the first one, there's a space after { and before }, but in the second there isn't. Both ways seem fine to me, but it would be better to use one of these ways, consistently everywhere.

Similarly here, there's a space after [ but not before ]. For the sake of consistency, it would be better to remove the space after [ or add one before ].

 ALPHABET[ (ALPHABET.index(letter) + key) % ALPHABET.length]

Overall

This looks nice, a pleasure to read, and I don't even know Ruby ;-)

Source Link
janos
  • 112.9k
  • 15
  • 154
  • 396

Extract constant expression to outside of loop

In this code, (decode ? 26 - key : key) is evaluated for each character in text:

 text
 .chars
 .select { |char| ALPHABET.include?(char) }
 .map {|char| shift(char, (decode ? 26 - key : key) )}
 .join

It would be better to move it outside the expression so that it's only evaluated once:

 shift_key = decode ? 26 - key : key
 text
 .chars
 .select { |char| ALPHABET.include?(char) }
 .map {|char| shift(char, shift_key)}
 .join

The same goes for the other cipher method, where you do the same.

Minor formatting issues

The code formatting style is not consistent, for example compare these two lines:

 .select { |char| ALPHABET.include?(char) }
 .map {|char| shift(char, shift_key)}

In the first one, there's a space after { and before }, but in the second there isn't. Both ways seem fine to me, but it would be better to use one of these ways, consistently everywhere.

Similarly here, there's a space after [ but not before ]. For the sake of consistency, it would be better to remove the space after [ or add one before ].

 ALPHABET[ (ALPHABET.index(letter) + key) % ALPHABET.length]

Overall

This looks nice, a pleasure to read, and I don't even know Ruby ;-)

lang-rb

AltStyle によって変換されたページ (->オリジナル) /