8
\$\begingroup\$

A few days ago I finished a coding challenge for a potential job. I was super happy with my code, till I got the response that my code wasn't good enough. :( So, apparently I'm still making mistakes, I've asked for feedback but no response. I really want to know what are my weak points are so I can improve. Can anyone please take a quick look and tell me what could be better?

The description of the challenge:

Write a Python 3 package which generates the most important key-phrase (or key-words) from a document based on a corpus. Attached you will find a zip archive with:

  • one script file (script.txt)
  • 3 transcript files (transcript1...3.txt)

Instructions:

  • Compute the most important key-words (a key-word can be between 1-3 words)
  • Choose the top n words from the previously generated list. Compare these key- words with all the words occurring in all of the transcripts.
  • Generate a score (rank) for these top n words based on analysed transcripts.

Deliverables

  • upload the solution on GitHub
  • write inside of the Readme file instructions on how to get started with the package (installing dependencies, running, testing, etc.)
  • consider reusability when implementing your package. it should be generic enough that given a certain input, it will provide the required output

My whole submission: https://github.com/GMathyssen/NLP-challenge

keywords.py

# -*- coding: utf-8 -*-
__author__ = 'Gert'

import string
import pandas as pd
import nltk
import sys
from nltk.corpus import stopwords
nltk.download('stopwords')

def main():
    # Amount of max words in key-word
    number_grams = 3

    number_top_keywords = 20
    save_file = open(sys.argv[1], 'a')

    # Reading in the minimum data
    script = open(sys.argv[2], "r").read()
    total_trans = open(sys.argv[3], "r").read()
    names_trans = [str(sys.argv[3]) + "\n"]

    # Reading in optional extra transcripts
    for tran in sys.argv[4:]:
        total_trans += open(tran, "r").read()
        names_trans.append(str(tran) + "\n")

    # Processing text from the script and group key-words in script dataframe
    script_data = ngrams_to_strings(get_n_grams(text_process(script), number_grams))
    script_df = group_in_dataframe(script_data, "Main script")

    # Taking the top n words from the script dataframe
    script_df_top = script_df.head(number_top_keywords)

    # Processing text from all the transcripts and group key-words in a dataframe
    total_trans_data = ngrams_to_strings(get_n_grams(text_process(total_trans), number_grams))
    total_trans_df = group_in_dataframe(total_trans_data, "Transcripts")

    # Merge script dataframe and transcripts dataframe into one
    script_trans_df = pd.concat([script_df_top, total_trans_df], axis=1, join="inner")

    # Sort merged dataframe to appearance in transcipts
    script_trans_df = script_trans_df.sort_values("Transcripts", ascending=False)


    string1 = "\nMain script:\n%s" % sys.argv[2]
    string2 = "\nTranscripts:\n"
    string3 = "\nThe top %s key-words in the main script:\n" % number_top_keywords
    string4 = "\nThe top %s key-words in the main script, ranked by appearance in the transcripts:\n" % number_top_keywords

    # Print and write to .txt file
    printlist = [string1, string2] + names_trans + [string3, str(script_df_top), string4, str(script_trans_df)]

    for string in printlist:
        print(string)
        save_file.write(string)


def text_process(text):
    # Check characters to see if they are in punctuation
    no_punc = [char for char in text if char not in string.punctuation]

    # Join the characters again to form the string
    no_punc = ''.join(no_punc)

    # Remove any stopwords
    no_stopw = [word for word in no_punc.split() if word.lower() not in stopwords.words('english')]

    # Stemming the words
    stemmer = nltk.stem.snowball.EnglishStemmer(no_stopw)
    return [stemmer.stem(i) for i in no_stopw]


def get_n_grams(word_list, n):
    ngrams = []
    count = 1
    while count <= n:
        for i in range(len(word_list)-(count-1)):
            ngrams.append(word_list[i:i+count])
        count += 1
    return ngrams


def ngrams_to_strings(ngrams):
    # First doing a sort, so that the grams with an other word order are the same
    ngrams_sorted = ([sorted(i) for i in ngrams])
    return [' '.join(i) for i in ngrams_sorted]


def group_in_dataframe(data, column_name):
    df = pd.DataFrame(data=data, columns=["key-word"])
    df = pd.DataFrame(df.groupby("key-word").size().rename(column_name))
    return df.sort_values(column_name, ascending=False)


if __name__ == "__main__":
main()

test_keywords.py

# -*- coding: utf-8 -*-

import unittest
from keywords import text_process, get_n_grams, ngrams_to_strings



class TestKW(unittest.TestCase):
    def test_text_process(self):
        self.assertEqual(text_process("This is a special test, monkeys like tests!"),
                          ['special', 'test', 'monkey', 'like', 'test'])

    def test_get_n_grams(self):
        self.assertEqual(get_n_grams(['special', 'monkey', 'like'], 2),
                          [['special'], ['monkey'], ['like'], ['special', 'monkey'], ['monkey', 'like']])

    def test_ngrams_to_strings(self):
        self.assertEqual(ngrams_to_strings([["apple"], ["the", "king"]]),
                         ['apple', 'king the'])


if __name__ == '__main__':
unittest.main()
\$\endgroup\$
  • \$\begingroup\$ Is this the complete description of the challenge or your summary afterwards? As written, there seems to be no need to use word stems and no output format is defined... \$\endgroup\$ – Graipher May 31 '17 at 7:20
  • 1
    \$\begingroup\$ @Graipher The join rebuilds the string from characters which can include spaces. \$\endgroup\$ – Janne Karila May 31 '17 at 18:37
  • 1
    \$\begingroup\$ @Graipher I just added the deliverables. Thanks for the info! \$\endgroup\$ – GMath May 31 '17 at 19:22
  • \$\begingroup\$ (I meant to add this before). To be clear, your code looks solid and well organized. Regarding unit testing and OOP: these things can be learned as much as anything else, and your code is in good enough shape that I'm sure you will become excellent at them. If you don't mind me "evaluating" you for this, I would take my general statements before (that there are some things missing that I would expect a senior dev to already know) and add that I do see evidence off a dev who can quickly and easily become a senior engineer. So I wouldn't sweat it too much: we all have more to learn. \$\endgroup\$ – Conor Mancone Jun 11 '17 at 22:06
  • \$\begingroup\$ Also worth adding that different companies are looking for different things, and often a very talented developer can get turned down just because the rest of the competition is also very awesome, or because they are looking for a special something in particular. My current position is as a lead engineer. In my last job I was also a lead engineer. While job hunting I applied for a senior engineer position and got turned down (basically) for not being good enough, and was told that if a mid-level engineer opened up they would let me know. Goes to show: sometimes its just a crap shoot. \$\endgroup\$ – Conor Mancone Jun 11 '17 at 22:11
2
\$\begingroup\$

I didn't comb through your code in detail, partly because I doubt the people who gave you this task did either. I presume it actually does what it is supposed to do, and properly accomplishes the requested task without bugs. In that case, their issue is likely with the overall package structure/design/implementation. In general though, the answer partly depends on the job you were applying to. Expectations for a mid-level engineer position are obviously very different than for a senior-level engineer, and so to some extent the answer depends on what kind of position you were applying to. Some hints in that area would help. Regarding the structure/design/implementation of your code, without getting into the nitty-gritty, I would make just a few comments which could have been part of their issue:

  1. Your test coverage is very low. You run one test each on three functions, leaving more than half of your code untested, and the part of your code that is tested is not tested very thoroughly. Most importantly, those three functions are really the least important part of your code: the part that ties them all together (i.e. your main function) is what is really doing all the work, and it is the part without tests. They specifically ask how to run your code tests, which means they were also evaluating your ability to write coherent unit/integration tests. Most likely they wanted to see a lot more in this area. I consider test writing fluency to be a requirement for a senior level engineer.
  2. I don't think your code meets this requirement: consider reusability when implementing your package. it should be generic enough that given a certain input, it will provide the required output, although it is hard to say because I could be incorrectly guessing at their intent. You allow the user to specify different input variables via the command line, but this is also a python package. A big part of python packages is that they can be imported by other packages/modules and used as needed. The way you have your package structured, it can pretty much only be used from the command line. In my mind making it more generic and resuable means that you can import it from within other python code and use it to do these same computations with little effort. As it stands, your three methods are importable from other methods, but they only provide a small part of the whole system functionality. The code that most needs to be reusable is your main function which is locked down behind the main() function which and not at all reusable because it takes its input from the command line.
  3. Depending on the job this might not be an issue, but your code is pretty standard procedural code. Not that there is anything wrong with procedural code, but a well thought out use of OOP principles would make your code more reusable (i.e. it would help with my point #2 above) and also show an understanding of the concepts that most companies these days work with. Like it or not, OOP is the primary paradigm used by most these days.
  4. Your github repository doesn't seem to have a commit history. I could be misreading github though. If I were evaluating something like this, I would also want to know that the candidate understands the benefits of a VCS enough to use it themselves. As a result, I would definitely check the commit history of anything uploaded to github to verify that they actually used git when developing the code. A lack of a commit history implies you didn't actually use git while building this: you just uploaded it to git when you were done. Granted, this is small enough that that is not a crazy choice, and I wouldn't rule someone out for a lack of a commit history, but I would definitely taken note of it.
  5. Your code has comments but no docblocks. The latter are used when building documentation, so I would definitely put more stock in a candidate who has docblocks in his code.

As I said, I don't know what they saw, but these are the things that come to mind as I look at your code and their requirements (both stated and not).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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