111

I'm an experienced developer, but have not done many code reviews. I'm being asked to review code written in Python but I do not know Python.

Does it make any sense at all to review code in a language I don't know?

Heather Smith
  • 1,001
  • 2
  • 7
  • 4
  • 62
    Tangentially, consider going to CodeReview.SE and glancing at the python tag. Looking only at the question, consider what advice you would give to the code and then look to see if that is represented in the answers. –  Jan 13 '16 at 15:19
  • 10
    Also related: [Should Junior Programmers be involved as code reviewers in the projects of Senior Programmers?](http://programmers.stackexchange.com/q/186761/40980) –  Jan 13 '16 at 15:26
  • I don't think I've ever had a development job where a superior who doesn't know the language has felt they couldn't contribute to a code review. It rather depends on whether you're expected to do it by yourself or with a more [Python] knowledgeable colleague. – Robbie Dee Jan 13 '16 at 15:33
  • 14
    Sounds like an excellent way to learn more about Python. Question anything that looks funny - some of it will be you not understanding the language, and some will be legitimate code review items. – Dan Pichelman Jan 13 '16 at 15:33
  • 2
    It isn't also necessarily *just* about your comprehension - it is also theirs. Ask them to talk thru some of the code. It will help both of you. – Robbie Dee Jan 13 '16 at 15:37
  • 2
    Possible duplicate of [How to review code that you do not understand?](http://programmers.stackexchange.com/questions/33968/how-to-review-code-that-you-do-not-understand) – gnat Jan 13 '16 at 15:39
  • 9
    @RobbieDee Absolutely! Explaining your code to someone is often worthwhile *even if it's just a teddy bear*. – Kilian Foth Jan 13 '16 at 15:41
  • Hypothetical example: Imagine trying to review Javascript code if you didn't know that functions inside functions is the way to do encapsulation, or didn't understand how closures work. I can't give you a Python example because... well, I don't know Python, nor would I attempt to review it. Some code practices are common to all programming languages, but each language (including Python) has its own unique conventions. – Robert Harvey Jan 13 '16 at 15:50
  • @RobertHarvey which gets to the essence of 'what is the goal of the code review *for this person / language / code combination*?' It is perfectly possible that the answer is "massage it with one's eyeballs so that after you fill the goal the manager set of 'learn Python' (you'll get that next week), you will have a passing familiarity with the code." It might also be "we need a scapegoat for not identifying the bugs in the code." Or it may be "we don't 100% trust this contractor - does anything jump out at you?" All valid review possibilities (though run from the second). –  Jan 13 '16 at 16:05
  • 1
    @MichaelT: True. But you're not describing anything that passes for a formal code review in a professional setting, nor should it. – Robert Harvey Jan 13 '16 at 16:15
  • 1
    Just learn Python before review. It is one of the easiest (among "serious") programming language to learn – porton Jan 13 '16 at 16:22
  • 34
    You say you are being asked to do so. The person asking you thinks that you performing this task will add value to your organization. If you want to know what the nature of that value is **ask that person**, not strangers on the internet! We don't know what is going on inside that person's head. Perhaps the code is of such low quality that even novices can find problems. Perhaps the code is of such high quality that you will learn good habits from it. Who can say? Someone thinks it is of value; ask that person what the value is. – Eric Lippert Jan 13 '16 at 16:46
  • 1
    @porton: It's not the language so much as its *idioms* that need to be fully understood first. – Robert Harvey Jan 13 '16 at 18:14
  • Also related: [How can I make being code reviewed by someone who doesn't know the language easier?](http://programmers.stackexchange.com/q/196212/6384) – user Jan 14 '16 at 09:56
  • 2
    @EricLippert The possibility may be that someone has assumed "all code must be code reviewed", and not added that crucial step "by a person capable of providing reasonable feedback", instead replacing it with "by a person who is not currently busy". I can see why asking that question to that person might be an issue. – deworde Jan 15 '16 at 12:37
  • @deworde - if you are indeed being asked to do something because "a warm body needs to do X so we can check a box in our 'quality' process", then it's time to find out and move on... – Floris Jan 16 '16 at 17:44

11 Answers11

120

Any sense? Yes. Even if you know nothing about the semantics of a programming language, you can still read characters and notice inconsistent formatting, missing comments, badly chosen identifiers, obvious duplication etc.

Much sense, or enough sense to repay the cost of your time? I'm not sure. This depends on your position, the importance of code reviews in the workflow of your team, and several other factors that we can't quantify well enough.

Kilian Foth
  • 107,706
  • 45
  • 295
  • 310
  • 51
    Doing some code reviews is a great way to get to know a module, framework or even language, so it might be worth your time anyway (and maybe you're being hinted at it...). I wouldn't sign off on such a review though, not without getting someone who is familiar with both the language and the code to look at it. – Ordous Jan 13 '16 at 15:19
  • 8
    Also if you have good domain knowledge and the code is reasonably well named/commented you can still spot incorrect/misunderstood/missing functionality at a high level (you may not spot mistakes due to syntax issues though) – Dan Jan 13 '16 at 17:04
  • As a lone programmer, I'd say it's better than nothing, but time is a factor. – JeffO Jan 13 '16 at 17:26
  • 4
    As an outsider, you might even be _more_ likely to spot fencepost errors, or errors in applying the correct amount of equal signs, and that kind of things. – KlaymenDK Jan 13 '16 at 20:06
  • 2
    @KlaymenDK Assignments in boolean contexts are (at least typically) syntax errors in Python. Fencepost errors are less likely since well written Python pretty much never has you working directly with array/list indexes. (Even when you do, it's usually with `enumerate`.) I think your comment is a great example of why trying to review a language you're not familiar with should at most be educational for yourself. – jpmc26 Jan 13 '16 at 21:54
  • IMO, code review is more about checking whether the code does what the design spec says that it ought to than any of those (also important) points which you list. – Mawg says reinstate Monica Jan 14 '16 at 10:47
  • 1
    @Mawg - I'd tend to say that's what automated testing is for. Even with expert knowledge of the language, it's difficult to say whether or not code will actually meet the design spec _by just looking at it_ / without executing it and observing the result (unless your design spec is so detailed that it's essentially code in itself). The aspects the answer enumerates cover many (though not all) valid reasons for doing code reviews. A code review does not generally include execution of the code being reviewed. – aroth Jan 14 '16 at 13:19
  • A very valid point (I also champion automated testing), but Ii won't stop me trying to do so ;-) – Mawg says reinstate Monica Jan 14 '16 at 13:33
  • 1
    I would add, that even you don't know the language, you can detect algorithm errors quadratic complexity when it could be linear et al. – bgusach Jan 15 '16 at 08:20
  • If this were 1995 and the language in question were MUMPS, your average competent C programmer of that era would be utterly unable to review the code. Since MUMPS of that era had stupid interpreters, good 1995-MUMPS style used no comments, short identifiers (often 1-2 characters), minimal newlines (newlines are syntactic and carried a performance cost), and considerably less functional abstraction (hence, more duplication) than one would use in C (function calls were quite expensive). This is obviously an edge case, but weird languages that require specific expertise to review do exist. – senshin Jan 17 '16 at 01:27
56

As a regular contributor over at Code Review Stack Exchange, I encounter a lot of questions suffering from Language-agnostic issues, for example:

  • Formatting, indentation
  • Scope
  • Loops
  • Type operations

and the list goes on. However, while I don't need to know the language, I can still review those issues / points.

A few of our top users have top answers in languages they either don't actively use, or don't know. Even two of my top ten are in languages I neither know nor can compile / run on my machine.

I'd even say it'd be the same as reviewing someone's pseudo code. As long as you can observe and comment on things relevant to things you understand, you'll be fine, and it'll be relevant.

Quill
  • 669
  • 6
  • 14
  • 2
    I even [wrote an answer](http://codereview.stackexchange.com/a/91973/34073) to a PHP program. It isn't the greatest answer, but I still maintain that the original loop declaration is ugly. –  Jan 13 '16 at 17:17
  • 4
    I learned C# through a combination of reviewing it, writing it, and having what I wrote reviewed. – RubberDuck Jan 14 '16 at 10:13
  • 5
    If you don't know Python, you are unlikely to appreciate the importance of indentation in the language... – Floris Jan 16 '16 at 17:39
  • 2
    Regarding 2/10 of your top answers being in languages you don't know. Who's to say the voters know anything about them either? – Martin Smith Jan 17 '16 at 13:43
  • While I don't know the languages, I still fact-check my statements, and if I messed up... Trust me, I'd hear about it – Quill Jan 17 '16 at 13:45
44

General advice

Here's the bottom line, in my opinion:

  • If you don't know the language well enough to know the features and common idioms, you are probably not going to contribute much to the review.
  • If you want to learn the language's features and idioms, you could participate in the review. Your focus should be observing the idioms and asking questions about patterns and organization that don't make sense to you. This may help identify problem areas, but only in the sense that you might force the developer to defend what they've done. Ask your questions in a way that leaves room for your gap in knowledge. Note that this may end up being a net cost in the review itself, but that cost is an investment in your knowledge.
  • Your ability to contribute until you have familiarized yourself with the language's features, idioms, and standards will be limited. I would not expect this to change until you have actually written a significant amount of code in the language.

Python specific considerations and examples

For the specific situation of not knowing Python, I would be especially wary of this. Python has a lot of idioms and standard practices that end up making good Python look very different from what you might expect in other languages. (Indeed, I think the things Python emphasizes have made my code look better in other languages, and not the other way around.) Beyond PEP8 has a good example of how you might completely miss the mindset Python encourages.

Let's look at a simple example. Take this code:

f = open('/home/me/something.txt')
try:
    content = f.read()
finally:
    f.close()

See the problem with this code? If you haven't worked with Python, you probably don't. The problem is that there is a much preferred style in Python that does exactly the same thing:

with open('/home/me/something.txt') as f:
    content = f.read()

This is a context manager. Do you know what they're good for? Do you know when it would be appropriate to use one? Do you know when it would be appropriate to create your own? No? Then you're probably not ready to review Python.

Let's look at another example.

def add_fifty(other_list):
   result = list()
   for i in other_list:
       result.append(i + 50)
   return result

x = range(10)
y = add_fifty(x)

See the problem? The problem is this method is completely unnecessary. You should probably just use a comprehension in place, when the operation is this simple:

x = range(10)
y = [i + 50 for i in x]

If you didn't see that, you're not familiar with Python's features and idioms.

jpmc26
  • 5,389
  • 4
  • 25
  • 37
  • 1
    In your last example, if I saw the final code I'd probably have some idea what it might do (without knowing Python) and I might start wondering if I can do something similar in my favourite language, which I never considered before. – gnasher729 Jan 15 '16 at 15:58
  • 1
    @gnasher729 "Indeed, I think the things Python emphasizes have made my code look better in other languages, and not the other way around." =) The only thing I know of in other languages that would be even remotely similar is .NET's LINQ and Java 8's [Stream](https://docs.oracle.com/javase/8/docs/api/java/util/stream/Stream.html)s. Ruby might have something, and I'm *sure* functional languages come close with something. My point with the example is you might not even know to do that if you're not familiar with Python, though, so you wouldn't even know to challenge the "bad" version. – jpmc26 Jan 15 '16 at 17:10
  • @x = 1 .. 10; @y = map { $_+50 } @x; – pipe Jan 15 '16 at 20:00
  • 2
    Code review is more about logic errors than implementing some (to outside programmers) obscure syntax in a particular language. It's about finding logic bugs, like implementing an algorithm completely incorrectly (like using division when it should be addition). So the code won't be precisely up to specs. That's not important as long as the resultant output is correct. At the least, if you're worried about particular optimizations like this, a team code review would be even better. – phyrfox Jan 15 '16 at 22:33
  • 3
    @phyrfox That is simply not true. Logic errors may be found, but code review is also (or primarily) about best practices, security, performance, reliability, readability/maintainability, etc. Before something is reviewed, it really ought to have been tested. StackExchange's [definition of code review](http://codereview.stackexchange.com/help/on-topic) is pretty spot on, imo. In Python, the examples I've mentioned are not "small optimizations." A Python developer who doesn't use these patterns is very inexperienced, behind the times, or incompetent. These are *basic* elements of the language. – jpmc26 Jan 15 '16 at 22:51
22

They may have asked you to review Python code precisely because you don't know Python. There's a management theory that it's useful to have a "fool" on a team. I'm not calling you a bad name :) The idea is that a team may suffer from group think and develop tunnel vision. One way to break out of this is to include someone on the team who the other team members would consider a "fool", that is, someone who doesn't know the subject matter. You'll ask questions to inform yourself, and the questions will come from a point-of-view that the other team members likely never considered.

You don't know Python, so what may seem ordinary to the Python coders may seem strange to you. You might suggest an improvement that the team never considered.

user2023861
  • 770
  • 4
  • 10
  • 1
    This was to be my answer. I'd just add that, on any given day with any given Python routine and any given group of Python gurus...that code is GARBAGE. – dwoz Jan 16 '16 at 04:08
  • And often, the mere act of explaining something to "a fool" will elicit the "Doh!" moment from an "expert" when they suddenly realise that the code isn't doing what it's meant to, or misses some key edge-case. It's also a decent way of starting the spreading of knowledge. – TripeHound Jan 18 '16 at 09:23
21

Code review is not about searching for variables with invalid spelling and wrong formatting. If you use code review to find such things, then stop wasting your time and use a tool.

Code review is about improving design and detecting common mistakes by a novice programmers.

Since I program in C++, and I don't know Python well enough, I wouldn't dare to review Python code. However I could help with a Java code review.

You didn't say in which language you program, but I do not see what you could contribute in a code review, if you do not know the language it is programmed in.

Sign
  • 2,643
  • 19
  • 22
BЈовић
  • 13,981
  • 8
  • 61
  • 81
  • 24
    Really, you have a tool that can distinguish badly named variables from good ones? I'd be reeeeealy interested in seeing one. – Davor Ždralo Jan 13 '16 at 16:24
  • 1
    @DavorŽdralo Off the bat I know Java has Checkstyle. More formal static analysis tools are common for many languages, and having them enforce a coding standard is typically the least of their duties. – Shaz Jan 13 '16 at 16:32
  • @DavorŽdralo I don't. But if the only remark is about naming, then it is a failed code review. Such code review are doing incompetent people, and are just waste of time. – BЈовић Jan 13 '16 at 16:36
  • 9
    I have a feeling there's a bit of difference between the definitions of Code Review here, kinda reminds me of this question on CR Meta: [What IS a Code Review?](http://meta.codereview.stackexchange.com/q/5407/31562). I would not exclude something specific and say "Code Review is NOT about XYZ". It also sounds like you're saying that experienced programmers does not need their code reviewed, something that I very much disagree with. – Simon Forsberg Jan 13 '16 at 18:08
  • @DavorŽdralo at some level it is possible to find *some* badly named variables with a static analysis tool. One example: A one/two char identifier that isn't a loop index (or, arguably, a lambda expression param). You'll never find all badly named things this way, but you can find a subset of them. – RubberDuck Jan 14 '16 at 02:19
  • 3
    @SimonForsberg That is not what I am saying. Even senior programmers can learn something in a good code review. But if the only comments are "you mispelled a variable here"-kind of comments, then they are wasting their time. – BЈовић Jan 14 '16 at 08:31
  • 1
    @BЈовић And if the only comment is of the form "wouldn't variable flugh be better named bloops, as you have typed it as a bloop and it's an array?" ? – Vatine Jan 14 '16 at 12:10
  • 1
    @Vatine Do you think that is the worst things that you can find in a code review? On my previous job, I worked with one great and one mediocre developer. One would put comments of such type (mispeled variable, invalid formating in a line, etc) and the other would sometimes completely rip off my code, but the result would be clearly superb to the original solution. Guess who made which comments, and from whom I learned more. If you do not learn anything new from a code review, then it is a waste of your time. – BЈовић Jan 14 '16 at 12:31
  • 2
    @BЈовић Nope, it's not the worst you can find, but it is pretty close to "the smallest thing found that is still worthwhile". – Vatine Jan 14 '16 at 13:34
10

Code reviews (in addition to actually looking for flaws) are a good introduction from one team member to others for the code being added or changed. If you are an experienced developer, you should be able to read through enough to mostly understand what is going on.

Look at a code review from a team leader's point of view: there is someone there who understands what the application should be doing (business logic), there is someone there who understands the code is doing (implementation logic), and possibly several other people there who need to have an idea of how all of that fits together.

Adam Zuckerman
  • 3,715
  • 1
  • 19
  • 27
7

You should definitely not be the only reviewer, but there are lots of good reasons for you to be one of the reviewers. Not knowing the language is not much of a hindrance for a lot of questions that need answering in a code review. As an example, I'm one of the top 20 answerers in the C# tag on this site, and I've not so much as compiled hello world in C#.

Some expertise you can share without knowing the language:

  • Domain knowledge.
  • General object-oriented design.
  • General programming practices: naming, clarity, and so forth.

It's also a good way to come up to speed on a new product. I just joined a new team, where I know the languages used quite well, but don't know the domain. Participating in code reviews has helped me learn the domain side better, even though I haven't been able to contribute much along those lines yet.

In your case, it will be a good way to learn the idioms of a new language, as you see the comments other reviewers leave. These are the kinds of things that are very difficult to learn any other way, because your interpreter doesn't care if your code is pythonic or not.

Karl Bielefeldt
  • 146,727
  • 38
  • 279
  • 479
5

This could be a win-win situation. I would go as far as to say that you could be an especially valuable reviewer because you are a Python virgin who has not been tainted by the Curse of Knowledge.

Think of it this way: if code is clear enough that even a Python virgin can understand it, then it must be good code. The parts that you have trouble understanding might be candidates for rework or better commenting.

Obviously, it would also be beneficial for you, because you would be picking up a new language as you go. (Hopefully, the code you are given is a good example to learn from.) This arrangement should work particularly well for Python, a language that has a reputation of being "executable pseudocode". If you are an experienced developer, then you shouldn't have much difficulty understanding the gist of a Python program.

The caveat would be that you wouldn't be expected to spot bugs arising from language-specific gotchas. But bug-finding is not the only purpose of code reviews. If nothing else, you would be participating in knowledge transfer simply by being aware of what kind of stuff goes on in your colleague's code.

200_success
  • 1,568
  • 11
  • 20
2

I was once asked to audit a project that was being undertaken by a subcontractor and appeared to have serious performance problems. I fairly quickly established that the critical factor was a single Perl module. I had never come across Perl before and we had no-one in the organisation who knew it, so I set about trying to understand it myself. I never got as far as understanding the detail, but it was very clear that the algorithm it was using was quadratic in data size and this was the cause of all the trouble. So yes, reading code in a language you don't fully understand can definitely be productive. The bonus is that you learn new tricks while you're about it.

Michael Kay
  • 3,360
  • 1
  • 15
  • 13
0

A few observations:

1) If you are an experienced developer, you will pick up Python (or at least as much as you need to know), just by working with it. It will be a case of "learning by doing." It will be hard at first, but will get easier as you pick up the language. Think of this as an opportunity to learn another language (people often learn "foreign" languages through "immersion.")

2) There are a number of valuable people on SE sites that are "non-technical," but are skilled with grammar, communications and logic. Such people bring a "fresh eye" to subjects, and make a number of "no brainer" fixes that others miss, because they are too "tied up" in the material. You are being consulted presumably for your non "technical" (i.e. non Python) skills such as logic and overall programming savvy.

And if you haven't done a lot of code review, almost any code review experience will help you as a developer. This looks like a good match between your skills and needs and the team's.

Tom Au
  • 893
  • 7
  • 17
  • 1
    I question whether reviewing code constitutes "learning by doing" to a meaningful degree. Perhaps my reviewing experience is different from yours, but there's very rarely meaningful or substantial writing of code, and almost never execution. – Esoteric Screen Name Jan 13 '16 at 16:42
  • @EsotericScreenName - I think it depends on how much feed-back you get from someone who wrote or knows the code. It's also more likely to be in a domain area you're working in with another language making it more relavant than the typical code example. Most coding examples just show how to do small snippets of a tack and are rarely shown as part of a large project. – JeffO Jan 13 '16 at 16:51
0

That depends on what the goal of the review is; i.e. what you mean by effective.

You will still likely be able to detect some problems. If you're all they got to review with and they're just hoping that you taking a look over it helps some and possibly catches something, then sure. Many concepts of structure are similar between languages. One especially is being able to review the commenting. It should be commented well enough that a programmer not of that particular language should still be able to get a good feel of what's happening. If not...then you can tell them where their commenting is lacking. If it is that well commented...then you should be able to review quite a bit of their structure just through the annotations of what's going on rather than actually reading the code of what's going on.

But you will not likely detect many other problems. So if they intend for your review to be an exhaustive determination of whether or not this is a well made / workable program they will be disappointed.

Whether or not that outcome is worth the time of you doing it depends largely on the project.

Jimbo Jonny
  • 109
  • 2