4
\$\begingroup\$

Could you please tell me what you liked or may have disliked about this code? What can be made better?

#!/usr/bin/python3

def change_char_at(ch, s, indx):
    """
    Strings in Python are immutable objects. So, it is not possible to change a
    certain character in a string in place. For example, an attempt to run the
    following code will fail with a TypeError:

        s = "spam"
        s[1] = "z" # TypeError: 'str' object does not support item assignment

    What this function does is it provides you with a nice interface when you
    want to change a character in a string at a certain index position. So, use
    this function instead of doing everything manually which can sometimes be
    tricky. Notice that the character parameter does not necessarily have to be
    one character long. It can be an empty string which means that a character
    at a certain position will effectively be deleted from the string. If it's
    more than one character long, then they all will be inserted into the string
    starting at the specified index position.

    Parameters:
    ch:   character to insert
    s:    string to insert into
    indx: index position where to insert the character
    """
    # Error handling
    if type(ch) is not str:
        raise TypeError("first argument must be a string")
    if type(s) is not str or not len(s) > 0:
        raise TypeError("second argument must be a non-empty string")

    length = len(s) - 1
    if not(indx >= 0 and indx <= length):
        msg  = "string index out of range; "
        msg += "attempt to access index at {0}; ".format(indx)
        msg += "allowable index range 0 to {0}".format(length)
        raise IndexError(msg)

    # Actual function logic
    return s[:indx] + ch + s[indx + 1:]
    """
    Another possible implementation:

        ls       = list(s)
        ls[indx] = ch
        return ''.join(ls)

    This works well too and is equally good, but might be conspired unpythonic
    by some compared to the one right above it.
    """


def main():
    my_string = "spam"
    my_string = change_char_at("z", my_string, 1)
    print(my_string) # prints "szam"


if __name__ == "__main__": main()
\$\endgroup\$
1
  • 2
    \$\begingroup\$ I think it's better to name your parameter index. Doesn't make much sense to shorten word by one character. Also I know several ways of doing this, but yours seems best to me. \$\endgroup\$
    – Alissa
    Feb 18, 2016 at 16:04

4 Answers 4

2
\$\begingroup\$

If this is intended to be a Python utility that can be imported with import, then you don't need the following shebang line:

#!/usr/bin/python3

You typically only use the above when you intend for a Python script to be executed as an executable. As far as I can tell, this script is intended to be a utility.

In addition, if this script is intended to be an importable utility, then the following pieces of code should be placed in a separate tests.py file:

def main():
    my_string = "spam"
    my_string = change_char_at("z", my_string, 1)
    print(my_string) # prints "szam"


if __name__ == "__main__": main()

When you want to perform type checking in Python, it's typically accepted that you don't use the type function. Rather, you should use the isinstance function, like this:

if isinstance(something, sometype):
    # Do something

Unlike simply doing a comparison with the type function, isinstance will return true if the value you're checking inherits from the type you're checking the value against as well as simply returning true if the value and type match. If you don't want the additional behaviour of the isinstance function, then stick with type.

But, type checking really isn't something you should be doing in Python anyways. If the user passes the wrong type, simply let the function throw an error. Don't bother trying to check the type. If you truly want to give your user an indication of what types should be passed, you can use function annotations, like this:

def change_char_at(ch: str, s: str, indx: int):
    pass
\$\endgroup\$
2
  • \$\begingroup\$ Should I now try and make changes to the code as per your instructions? I'm completely new here. \$\endgroup\$
    – misha
    Feb 18, 2016 at 16:55
  • \$\begingroup\$ @user69786 You can update the code stored on your hard drive (obviously), but leave the code in your question as-is so you don't invalidate any answers. \$\endgroup\$ Feb 18, 2016 at 17:00
3
\$\begingroup\$

The fact that your function is named change_char_at and the first parameter is named ch both suggest that ch should be a single character. However, the function could easily be abused to splice in any number of characters at that location, including zero. So, you might as well make it obvious and rename the function to string_splice() or something.

I'm not a fan of this function, though. Basically, it's a long-winded way to do s[:indx] + ch + s[indx + 1:] with some validation.

  • if type(ch) is not str seems pointless. If ch is not a string, it would naturally fail with a TypeError anyway during concatenation.
  • if type(s) is not str might be useful validation, but it also goes against Python's duck-typing philosophy.
  • if … not len(s) > 0 is an unjustified special case. If you're going to allow splicing in an arbitrary number of characters, then why shouldn't one be allowed to add something to an empty string? As a consequence of this failure case, the caller will also need to put in a special case to work around this restriction.
  • if not(indx >= 0 and indx <= length) is weird because length isn't actually the length of the string; it's one less than that. It could be more succinctly written as if not 0 <= indx < len(s). But Python's negative indexing is a feature, not a bug. Why would you want to defeat the convenience?

In summary, if you want to do s[:indx] + ch + s[indx + 1:], then it's probably best to just write that. If you want to write this function anyway, I recommend dropping most, if not all, of the validation.

If you want to do a lot of single-character replacements, then you would probably want to convert the string to a list or bytestring instead.

\$\endgroup\$
0
1
\$\begingroup\$
  1. Don't do if type(ch) is not str:. This check will fail if ch not a string but subclass of string. Use isinstance().

  2. Alternative implementations looks fine and even more Pythonic to me. Not sure about performance though.

  3. change_char_at("z", "", 123) will raise TypeError which does not make sense: types are correct. It should probably be ValueError.

  4. +1 to @Alissa. indx is not a great name.

\$\endgroup\$
2
  • \$\begingroup\$ I'm not quite sure about the first point you made. "True" if type(object()) is str else "False" => "False" / "True" if type("") is str else "False" => "True". I believe, object() is a superclass of str, but as you can see it does not equate to str. \$\endgroup\$
    – misha
    Feb 18, 2016 at 17:20
  • \$\begingroup\$ my bad, I meant subclass, not superclass. \$\endgroup\$
    – rvs
    Feb 18, 2016 at 23:39
0
\$\begingroup\$

I may be incorrect and forgive me if I am being fussy, but it is best if you use a # when commenting so people don't mistake it for code. Otherwise, very nice code, nice job!

\$\endgroup\$
1
  • 1
    \$\begingroup\$ """ """ at the top of the functions, classes, etc in Python is somewhat special. It is called a docstring and it is a standard way of documenting your function/class/etc. \$\endgroup\$
    – rvs
    Feb 18, 2016 at 23:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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