current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite
1

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()
share|improve this question
2  
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. – Alissa Feb 18 at 16:04
up vote 2 down vote accepted

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
share|improve this answer
    
Should I now try and make changes to the code as per your instructions? I'm completely new here. – user69786 Feb 18 at 16:55
    
@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. – Ethan Bierlein Feb 18 at 17:00
    
Gotcha. Thank you. – user69786 Feb 18 at 17:15
up vote 3 down vote

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.

share|improve this answer
    
All are good points. Thank you. – user69786 Feb 18 at 18:31
up vote 1 down vote

  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.

share|improve this answer
    
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. – user69786 Feb 18 at 17:20
    
my bad, I meant subclass, not superclass. – rvs Feb 18 at 23:39
up vote -1 down vote

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!

share|improve this answer
1  
""" """ 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. – rvs Feb 18 at 23:40

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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