Skip to main content
Code Review

Return to Answer

edited body
Source Link
alexwlchan
  • 8.7k
  • 1
  • 23
  • 55

Mostly this looks good, but I’ll make a few suggestions:

Boolean arguments in your function signature

Boolean arguments in your function signature

These control whether certain strings/mappings are lowercased first. I think this behaviour should be removed from keymap_replace(). It's scope creep: it’s gone from being string replacements to more complicated string manipulations. I’d leave that sort of manipulation to the caller.

(And you could continue to add options like this: what about uppercasing the string, or reversing it, or another pattern. Rather than muddling up your function with this, I’d just take it out.)

That makes the arguments simpler, and simplifies the function definition as well.

But if you really want those parameters in there, then I’d suggest making them keyword-only. In Python 3, you can insert an asterisk as so:

def keymap_replace(
 string: str, 
 mappings: dict,
 *,
 lower_keys=False,
 lower_values=False,
 lower_string=False,
) -> str:

and when the function is called, those boolean arguments have to be as keyword arguments, not positional. There can never be ambiguity in how the function is being used.

Dictionaries are unordered

Dictionaries are unordered

There’s a possibility for ambiguity, or for substitutions to be chained. For example:

keymap_replace('Hello world', {
 'J': 'K',
 'H': 'J'
})

Should this call return "Jello world" or "Kello world"?

I think the first is more natural – I don’t expect substitutions to be chained – but when I tried the function, I got the latter. You should try to make this less confusing.

One possibility that springs to mind:

replaced_string = ''.join(mappings.get(char, char) for char in string)

Each character in the original string is subject to at most one substitution.

Mostly this looks good, but I’ll make a few suggestions:

Boolean arguments in your function signature

These control whether certain strings/mappings are lowercased first. I think this behaviour should be removed from keymap_replace(). It's scope creep: it’s gone from being string replacements to more complicated string manipulations. I’d leave that sort of manipulation to the caller.

(And you could continue to add options like this: what about uppercasing the string, or reversing it, or another pattern. Rather than muddling up your function with this, I’d just take it out.)

That makes the arguments simpler, and simplifies the function definition as well.

But if you really want those parameters in there, then I’d suggest making them keyword-only. In Python 3, you can insert an asterisk as so:

def keymap_replace(
 string: str, 
 mappings: dict,
 *,
 lower_keys=False,
 lower_values=False,
 lower_string=False,
) -> str:

and when the function is called, those boolean arguments have to be as keyword arguments, not positional. There can never be ambiguity in how the function is being used.

Dictionaries are unordered

There’s a possibility for ambiguity, or for substitutions to be chained. For example:

keymap_replace('Hello world', {
 'J': 'K',
 'H': 'J'
})

Should this call return "Jello world" or "Kello world"?

I think the first is more natural – I don’t expect substitutions to be chained – but when I tried the function, I got the latter. You should try to make this less confusing.

One possibility that springs to mind:

replaced_string = ''.join(mappings.get(char, char) for char in string)

Each character in the original string is subject to at most one substitution.

Mostly this looks good, but I’ll make a few suggestions:

Boolean arguments in your function signature

These control whether certain strings/mappings are lowercased first. I think this behaviour should be removed from keymap_replace(). It's scope creep: it’s gone from being string replacements to more complicated string manipulations. I’d leave that sort of manipulation to the caller.

(And you could continue to add options like this: what about uppercasing the string, or reversing it, or another pattern. Rather than muddling up your function with this, I’d just take it out.)

That makes the arguments simpler, and simplifies the function definition as well.

But if you really want those parameters in there, then I’d suggest making them keyword-only. In Python 3, you can insert an asterisk as so:

def keymap_replace(
 string: str, 
 mappings: dict,
 *,
 lower_keys=False,
 lower_values=False,
 lower_string=False,
) -> str:

and when the function is called, those boolean arguments have to be as keyword arguments, not positional. There can never be ambiguity in how the function is being used.

Dictionaries are unordered

There’s a possibility for ambiguity, or for substitutions to be chained. For example:

keymap_replace('Hello world', {
 'J': 'K',
 'H': 'J'
})

Should this call return "Jello world" or "Kello world"?

I think the first is more natural – I don’t expect substitutions to be chained – but when I tried the function, I got the latter. You should try to make this less confusing.

One possibility that springs to mind:

replaced_string = ''.join(mappings.get(char, char) for char in string)

Each character in the original string is subject to at most one substitution.

deleted 93 characters in body
Source Link
Ethan Bierlein
  • 15.9k
  • 4
  • 60
  • 146

Mostly this looks good, but I’ll make a few suggestions:

Boolean arguments in your function signature

Boolean arguments in your function signature

These control whether certain strings/mappings are lowercased first. I think this behaviour should be removed from keycap_replace()keymap_replace(). It's scope creep: it’s gone from being string replacements to more complicated string manipulations. I’d leave that sort of manipulation to the caller.

(And you could continue to add options like this: what about uppercasing the string, or reversing it, or another pattern. Rather than muddling up your function with this, I’d just take it out.)

That makes the arguments simpler, and simplifies the function definition as well.

But if you really want those parameters in there, then I’d suggest making them keyword-only. In Python 3, you can insert an asterisk as so:

<!-- language: python -->
 def keymap_replace(
 string: str, 
 mappings: dict,
 *,
 lower_keys=False,
 lower_values=False,
 lower_string=False,
) -> str:

and when the function is called, those boolean arguments have to be as keyword arguments, not positional. There can never be ambiguity in how the function is being used.

Dictionaries are unordered

There’s a possibility for ambiguity, or for substitutions to be chained. For example:

keymap_replace('Hello world', {
 'J': 'K',
 'H': 'J'
})

Should this call return "Jello world" or "Kello world"?

I think the first is more natural – I don’t expect substitutions to be chained – but when I tried the function, I got the latter. You should try to make this less confusing.

One possibility that springs to mind:

replaced_string = ''.join(mappings.get(char, char) for char in string)

Each character in the original string is subject to at most one substitution.

Mostly this looks good, but I’ll make a few suggestions:

Boolean arguments in your function signature

These control whether certain strings/mappings are lowercased first. I think this behaviour should be removed from keycap_replace(). It's scope creep: it’s gone from being string replacements to more complicated string manipulations. I’d leave that sort of manipulation to the caller.

(And you could continue to add options like this: what about uppercasing the string, or reversing it, or another pattern. Rather than muddling up your function with this, I’d just take it out.)

That makes the arguments simpler, and simplifies the function definition as well.

But if you really want those parameters in there, then I’d suggest making them keyword-only. In Python 3, you can insert an asterisk as so:

<!-- language: python -->
 def keymap_replace(
 string: str, 
 mappings: dict,
 *,
 lower_keys=False,
 lower_values=False,
 lower_string=False,
) -> str:

and when the function is called, those boolean arguments have to be as keyword arguments, not positional. There can never be ambiguity in how the function is being used.

Dictionaries are unordered

There’s a possibility for ambiguity, or for substitutions to be chained. For example:

keymap_replace('Hello world', {
 'J': 'K',
 'H': 'J'
})

Should this call return "Jello world" or "Kello world"?

I think the first is more natural – I don’t expect substitutions to be chained – but when I tried the function, I got the latter. You should try to make this less confusing.

One possibility that springs to mind:

replaced_string = ''.join(mappings.get(char, char) for char in string)

Each character in the original string is subject to at most one substitution.

Mostly this looks good, but I’ll make a few suggestions:

Boolean arguments in your function signature

These control whether certain strings/mappings are lowercased first. I think this behaviour should be removed from keymap_replace(). It's scope creep: it’s gone from being string replacements to more complicated string manipulations. I’d leave that sort of manipulation to the caller.

(And you could continue to add options like this: what about uppercasing the string, or reversing it, or another pattern. Rather than muddling up your function with this, I’d just take it out.)

That makes the arguments simpler, and simplifies the function definition as well.

But if you really want those parameters in there, then I’d suggest making them keyword-only. In Python 3, you can insert an asterisk as so:

def keymap_replace(
 string: str, 
 mappings: dict,
 *,
 lower_keys=False,
 lower_values=False,
 lower_string=False,
) -> str:

and when the function is called, those boolean arguments have to be as keyword arguments, not positional. There can never be ambiguity in how the function is being used.

Dictionaries are unordered

There’s a possibility for ambiguity, or for substitutions to be chained. For example:

keymap_replace('Hello world', {
 'J': 'K',
 'H': 'J'
})

Should this call return "Jello world" or "Kello world"?

I think the first is more natural – I don’t expect substitutions to be chained – but when I tried the function, I got the latter. You should try to make this less confusing.

One possibility that springs to mind:

replaced_string = ''.join(mappings.get(char, char) for char in string)

Each character in the original string is subject to at most one substitution.

Source Link
alexwlchan
  • 8.7k
  • 1
  • 23
  • 55

Mostly this looks good, but I’ll make a few suggestions:

Boolean arguments in your function signature

These control whether certain strings/mappings are lowercased first. I think this behaviour should be removed from keycap_replace(). It's scope creep: it’s gone from being string replacements to more complicated string manipulations. I’d leave that sort of manipulation to the caller.

(And you could continue to add options like this: what about uppercasing the string, or reversing it, or another pattern. Rather than muddling up your function with this, I’d just take it out.)

That makes the arguments simpler, and simplifies the function definition as well.

But if you really want those parameters in there, then I’d suggest making them keyword-only. In Python 3, you can insert an asterisk as so:

<!-- language: python -->
 def keymap_replace(
 string: str, 
 mappings: dict,
 *,
 lower_keys=False,
 lower_values=False,
 lower_string=False,
 ) -> str:

and when the function is called, those boolean arguments have to be as keyword arguments, not positional. There can never be ambiguity in how the function is being used.

Dictionaries are unordered

There’s a possibility for ambiguity, or for substitutions to be chained. For example:

keymap_replace('Hello world', {
 'J': 'K',
 'H': 'J'
})

Should this call return "Jello world" or "Kello world"?

I think the first is more natural – I don’t expect substitutions to be chained – but when I tried the function, I got the latter. You should try to make this less confusing.

One possibility that springs to mind:

replaced_string = ''.join(mappings.get(char, char) for char in string)

Each character in the original string is subject to at most one substitution.

lang-py

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