How to deal with someone's ugly code gently?

Question

I've recently been given the task of finishing an incomplete project for my University. It'll count as credit for one Class so I'll shave a month off of my studies!

Here's the thing. The that was left behind is horrible: unorganized and not well thought-out. For example:

Protected Sub btnAceptar_Click(ByVal sender As Object, ByVal e As EventArgs) Handles btnAceptar.Click
        Try
            If accion = "Nuevo" Then 'Para crear uno nuevo
                If txtIdMotivo.Text <> "" And txtDescripcion.Text <> "" Then
                    Dim cmd As New SqlCommand("insert into Motivos values(" & CInt(txtIdMotivo.Text) & ",'" & txtDescripcion.Text & "')", cn)
                    cn.Open()
                    Resp = cmd.ExecuteNonQuery() 'Devuelve 1 si no hubo error al guardar
                    cn.Close()
                Else
                    MsgBox("Hay datos en blanco", MsgBoxStyle.Critical, "Revisar")
                End If
            Else
                If txtIdMotivo.Text <> "" And txtDescripcion.Text <> "" Then
                    Dim cmd As New SqlCommand("update Motivos set descripcion= '" & txtDescripcion.Text & "' where IdMotivo= " & CInt(txtIdMotivo.Text), cn)
                    cn.Open()
                    Resp = cmd.ExecuteNonQuery()  'Devuelve 1 si no hubo error al modificar
                    cn.Close()
                Else
                    MsgBox("Hay datos en blanco", MsgBoxStyle.Critical, "Revisar")
                End If
            End If
        Catch ex As Exception
            MsgBox(ex.Message)
        End Try

        'Verificamos si todo salio ok
        If Resp = 1 Then
            btnAceptar.Enabled = False
            btnCancelar.Enabled = False
            MsgBox("Accion realizada exitosamente", MsgBoxStyle.Information, "Listo")
            Response.Redirect("frmMotivosRecuperacionListado.aspx")
        Else
            MsgBox("Error al intentar realizar la acción", MsgBoxStyle.Exclamation, "Revisar")
            cn.Close()
        End If
    End Sub

Methods returning numbers just for the hell of it, passwords being saved in plaintext, you name it.

I have a meeting with the original author and I don't know how to handle this situation because I'm quite green in this area.

How can I tactfully mention that I want to rewrite the whole thing?

Answer 1

Don't tell him, just do it! No excuses should ever have to made for improving code. Just make doubly sure it's understandable and it works.

Answer 2

1. Write the code from scratch.
2. Verify the code works as intended.
3. Verify the code works at a reasonable speed.
4. Show the new code to the programmer.
5. Show benchmarks to convince him the code is better.

Answer 3

I'd go with one of these options:

(a) Don't mention it at all. Just do it. If the project is your responsibility, then take charge of it and do what you think you need to do.

(b) If keeping the original author involved and happy is necessary, then just say you have trouble understanding it the way that it is, and reorganizing it will help you figure it out. (That is, you are doing it because of your own limitations, not because it is "bad".)

Cleaning up or rewriting bad code is a valuable activity which makes development go faster, but remember that your ultimate goal is to get the project done, so you need to balance your desire to clean things up with your need to implement missing functionality. Ugly code that works is a lot more valuable than code you didn't have time to write.

Answer 4

Tell him straight forward, build up your case, ask whether or not he thought his project through decently. Chances are he realises his code isn't pretty, and will help you explain why he made certain decisions (Which might save you time getting the scope and reasoning)

After that, write it decently, document it, and make sure the case you built "against" the original author wasn't in vain.

Answer 5

Approach it with the attitude that you're trying to understand the code. If you don't understand why he's returning random numbers, there's a good chance that you don't completely understand the meaning of the code. Make suggestions for more important things (e.g. plaintext passwords), not accusations. Overall, just try to be diplomatic and look past the formatting of the code to what it actually does.

Answer 6

Well, proposing a re-write is always sure to perturb someone.

But I think you should resist the urge to simply "throw it away, rewrite it all" just like that.

When you say "I want to rewrite the whole thing" you're really saying "This is so impervious to me, I don't even want to spend the time to grok it."

Naturally you'll offend the original programmer. But his feelings don't matter.

What matters is the person who is supervising/sponsoring the project. What do they want?

I'd suggest the following: spend some time groking and bugfixing the existing code. Through this (painful) process you will learn the intentions behind the project, and pick up the patterns/antipatterns.

Then, after you're sure you've fully understood what they're trying to do, still avoiding the "rewrite" mentality, keep pushing the crappy code base towards whatever goal you have.

Then, when you are SURE you can re-write it all with ALL the existing functionality and then some, propose it.

The points are:

1) Don't propose a rewrite off the bat unless you can do it in a weekend, and present it Monday. If you can't do that, then you really shouldn't be proposing a rewrite.

2) Working with the existing code base, albeit crummy, helps you learn a lot about the intentions of the original project, so it isn't really a waste of time to do so.

Answer 7

Projects, especially university projects, often get thrown together in great haste, with good intentions of later fixing that are often not followed through on. Your colleague is probably glad to not have to deal with his old code any more.

First, do you need to tell him what you think of his code? What will that accomplish? If you need more of his help you may not want to alienate him. So to confront him with the ugliness should have some useful motive, e.g. helping him improve, or helping him help you with the code.

If you do need to deal with the ugliness, triage it: Make a list of the more problematic anomalies that you feel will really get in your way, and ignore the rest. You can confront him with that list if you feel it will do someone some good.

Consider this: If you spend a month cleaning up the code, you will effectively not have gained any time. Will you be graded on the code's appearance? If not, the only benefit from cleaning up would be your own ease of working with it. In any case, resist the temptation to go over it with a fine toothed comb - there comes a point where the cleanup is more effortful than simply working with the less than perfect code.

Answer 8

There's always the "Oops! I accidentally deleted the only copy!" method...

Seriously though, if you want to completely re-do someone's code without offending them, you have to watch how you do it. Definitely don't say "this code is horrible" and suddenly replace the whole module with your re-written version (after all, your re-done version might look just as bad to the original author).

Instead, make small changes over time, and make them for valid reasons that the original author would have a hard time arguing against. If you re-write a section of code, show that your version runs faster or uses less resources and you can pass it off as an optimization. Modifications to code that interacts with the poorly-written module can change the interface, giving you the opportunity to make more changes to this code under the guise of "making it compatible with the changes to the other module". Fixing the problem with cleartext passwords should be easy to do under the mantle of "security improvements". Just make sure that you are making the same sort of changes to other parts of the code and not only to the code of that one author.

Answer 9

Be professional and honest. You mentioned passwords in plaintext; tell him this is a security risk and needs to be changed. You mentioned that (magic?) numbers are randomly returned; if you can salvage the code, tell him you're going to replace them with meaningful constants and add documentation for everyone's benefit. If not, explain why you can't / won't reuse it. As for the other poor code, tell him you'd like to redesign and/or refactor his work to make it easier to extend and work with. He'll understand the importance of refactoring and redesigning code as it evolves, assuming he's a developer. If he doesn't understand the concept of refactoring, then he really should go back to school until he figures it out!

Answer 10

1. Are you writing unit tests? If so, and the code doesn't allow you to do this quickly and easily, you have a reason for "cleaning up" not rewriting.
2. Are the methods long and hard to follow? An easy fix (quick and without much ceremony) is to use Refactor Method (pull bits of a large method out into small helper methods). Now everything is clearer. Cleaning up the more atrocious bits from here is much easier but you may find it unnecessary.
3. Are you just wanting to use design patterns? Don't. Patterns are great, and you should refactor to patterns when you can, but don't do it just to make it clean. Do it b/c it helps you in other ways. (Sometimes this is necessary.)
4. Are you doing it to show how awesome you are? Don't. Just don't. You'll have someone else come along later and show you how awesome you aren't. :)

Answer 11

My priority when write code is maintenance and validation.

This sample code, demonstrates the unwillingness for these two tasks.

The validations redundant are made for each option, so that doubles the test effort.

Do not have any care about the appropriate point to close the connection.

Sends two messages to the user when leaves blank data.

Does Not use parameters for sql execution. Notes that if user includes quotes in the text, it can fail.

If all of the code is like this, do not hesitate.

Answer 12

Well, I'm too late - I agree with the answers above the only thing I can still give you is a great collection of good humor on the subject: just imagine that your code would have start with the following comment:

// no comments for you // it was hard to write // so it should be hard to read

for more: http://stackoverflow.com/questions/184618/what-is-the-best-comment-in-source-code-you-have-ever-encountered

It helped me have some laugh at really dark days with code I really prefer to forget...

Hope it will help you to put a smile on your face and see this as a challenge. Good luck