Interpret a date from a string of digits I developed a function that, from a given sequence of digits, extracts the date and reformat it.
This is the code:

from datetime import datetime as dt

def format_dates(field):
    n = len(field)
    match = False
    i = 0
    while match is False:
        try:
            # Take the last four digits
            year = int(field[-4 - i:n - i])
        except ValueError:
            return ''

        # Check if this year is between today's year +/- (15, 100)
        if (1919 <= year <= 2019):
            # Check if there are other 4 digits before these 4 ones
            if (len(field[-8 - i:n - i]) == 8):
                try:
                    f_date = dt.strptime(field[-8 - i:n - i],
                                         '%d%m%Y').strftime('%d/%m/%Y')
                    match = True
                    return f_date
                except ValueError:
                    pass
            else:
                return ''
        i += 1

Explanation:
This function:

Takes a sequence of digits as input.
extracts the last four digits from that sequence.
Checks if the extracted four digits are between 2019 and 1919, if not, it breaks.
If yes, it checks if there are more 4 digits before the previously extracted ones, if not it breaks.
If yes, it tries to format the whole 8 digits.
If there is a ValueError exception, it passes (ValueError, means there are 8 digits, the last four of them represent a correct year, but the fist four digits are wrong. So it passes to increment i + 1 to add a the next digits in the front and remove the last digit in the processed sequence).

Example:

input: '1303201946'

Iteration 1:

i = 0, match = False
year = 1946
test 1 (year between 2019 and 1919): passes.
test2 (there are 4 other digits before 1946, which are 0320): passes.
format the whole 8 digits: ValueError exception, so i = i+1 and pass to the next iteration.

Iteration 2:

i = 1, match = False
year = 0194
test 1 (year between 2019 and 1919): fails, so i = i + 1 and pass to the next iteration.

Iteration 3:

i = 2, match = False
year = 2019
test 1: passes
test 2: passes
format the whole 8 digits (13032019): 13/03/2019 (No ValueError exception) passes
match = True, return the formatted date, break from the while loop.

This function works fine, but the way it handles the errors seems ugly. Also I believe it is not optimized (same exceptions are repeated, a lot of returns and the code does not seem elegant).
How to reformat the code and make it more optimized?

Exception

If your algorithm cannot find a date, it is easier to raise an Exception than to return ''. Returning sentinel values instead of exceptions can lead to unexpected behaviour if the user of this function does not test for this sentinel value.

comments

Comments should explain why you did something, not how. # Take the last four digits tells you nothing more than the code itself. I would rather comment at field[-4 - i:n - i] why you did n - i instead of just -i.

nesting

Instead of nesting a number of if-clauses, it can be better to test the negative of the condition, and continue, so the rest of the code is less nested.

match

Don't test condition is True. Just do condition. In Python a lot of values can act as True or False in tests.

Your match is never used anyway; the moment you set it to True, you also return the result, so a while True: would have sufficed here.

field

This is a very unclear variable name. This method excepts a date in string format, so why not call the argument like that?

Return type

Your code does 2 things now. It looks for a date in the string, and converts that date to another format. It would be better to separate those 2 things, and return a datetime.datetime instead, and let the caller of this method worry about formatting that correctly.

while True

You use a while True-loop, with an incrementing counter. A better way to do this would be to either use for i in range(...) or using itertools.count: for i in itertools.count(). In this case you know there will be no more than len(field) - 7 iterations, so you might as well use that.

Revert the algorithm

You explicitly test whether the substring is 8 characters long, and then if it is in the right format. By changing the while True to the for-loop, you know the substring will be 8 characters long. Then it makes sense to first try to convert it to a datetime, and then check whether the year is correct:

def format_dates2(date_string):
    n = len(date_string)
    for i in range(n - 7):
        sub_string = date_string[-(8 + i) : n - i]
        # not just -i because that fails at i==0
        try:
            date = dt.strptime(sub_string, "%d%m%Y")
        except ValueError:
            continue
        if not (1919 <= date.year <= 2019):
            continue
        return date
    raise ValueError("Date not in the correct format")


Python program to transform an Excel document, with some parsing I wrote this to parse through an Excel doc, extract desired columns, parse certain cells and output another formatted Excel doc.

This is the original Excel doc I have condensed most of the columns for space

Here are the the results of the program that I am mostly happy with, it parses the name column and writes a unit identifier(i.e 83) along with a  tower identifier(i.e 5) on a new line along with a completed date if the completed column is TRUE.  I think the only thing I would change would be getting rid of the blank line under the row that has H 7

And here is the code

import xlrd
import xlwt
import re

# open and create workbook
workbookread = xlrd.open_workbook('seedtestexcelbytask.xls')
workbookwrite = xlwt.Workbook('output.xlsx')

# open and create sheets
worksheetread = workbookread.sheet_by_index(0)
worksheetwrite = workbookwrite.add_sheet('Sheet1')

timelist = []
towerlist = []

i = 0
l = 0
t = 0

# iterate through rows in worksheet, get columns completed, completed_at and name.
# add to timelist[] and towerlist[] if completed == true
for rows in range(worksheetread.nrows):
    cell0 = worksheetread.cell(rows, 2).value
    cell1 = worksheetread.cell(rows, 3).value
    cell2 = worksheetread.cell(rows, 15).value

    if cell0 == 1:
        parsedvalue = re.split(', |/', cell2)
        timelist.append(cell1)
        towerlist.append(parsedvalue)

b = 2
g = 1
for rows in range(len(timelist)):
    row = worksheetwrite.row(i)

# get current index from timelist and towerlist and assign to currenttime  and currentvalue

    currenttime = timelist[t]
    currentname = towerlist[t]

# get the length of the current index this will help us decide how to parse
    length = len(towerlist[t])

# if length <= 2 the index is not formatted properly so we only need index 1 which is the cafe and towers together
    if length <= 2:
        row.write(0, currenttime)
        row.write(1, currentname[1])
        i += 1
        l += 1

# if length = 4 index is formatted properly and we just grab the columns we need
    elif length == 4:
        a = 2

        for values in range(0, 2):
            row = worksheetwrite.row(l)
            row.write(0, currenttime)
            row.write(1, currentname[1])
            row.write(2, currentname[a])
            a += 1
            i += 1
            l += 1

# if length > 4 we need to iterate through each index and write them
    else:

        # define the length of the current cell of the excel sheet we have
        lengthd = (len(currentname)+1)/2
        length = int(lengthd)

        for values in range(length):
            # set variables that we will need to hold on to
            currentcafe = currentname[g]
            currenttower = currentname[b]

            # if the value of the current tower is 2,4,5,6,7 we need to write the current cafe and current tower
            # then move on to the next value and set that as the current cafe, we also need to jump to the
            # value after the current cafe and set that as the current tower
            if currenttower == "2" or currenttower == "4" or currenttower == "5" or currenttower == "6" or currenttower == "7":
                row = worksheetwrite.row(l)
                row.write(0, currenttime)
                row.write(1, currentcafe)
                row.write(2, currenttower)

                b += 1
                i += 1
                g = b
                b = g + 1
            # otherwise we can just write the value fo the current cafe and current tower, advance to the next value
            # which is another tower and set current tower to that
            else:
                row = worksheetwrite.row(l)
                row.write(0, currenttime)
                row.write(1, currentcafe)
                row.write(2, currenttower)
                b += 1
                i += 1

            l += 1
        i += 1
    t += 1
workbookwrite.save('output.xls')

This is the first time I have written anything with Python and I am fairly new to programming, any comments would be greatly appreciated.

Using pandas you can do it in much simpler way.
Steps:
1. Load the data in pandas dataframe from your excel sheet - https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.read_excel.html
2. Filter out the columns which you want to keep (you can specify the column names to be loaded while importing in step 1 also)
3. Transform the columns based on your requirement
4. Delete the rows which are null, in your case as it is H7 using pandas function dropna() - https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.dropna.html
5. Export the data to an excel sheet - https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.to_excel.html

For your reference, refer this blog- https://towardsdatascience.com/replacing-excel-with-python-30aa060d35e


PHP mail form is being abused I'm nowhere near a PHP expert, but I have dabbled a bit. I can make things work just fine, but I am terrible at figuring out what is wrong. I was informed by our server admin that we have a mail script that is being abused, and I am unsure of how to remedy the issue.

I am looking for help to see how someone is using this form to send out spam mail. The issue is that we got a bounce-back email sent from an address that doesn't exist to a hotmail address using this form. As far as I know, this form ONLY sends email to the $recipient which is neither this non-existent email or this Hotmail. I am trying to figure out a way to secure this form so that individuals cannot abuse this form and send mail to others through it.

<?php
$name = check_input($_POST['name'],"Enter your name.");
$email = check_input($_POST['email'],"Email address is required.");
$company = check_input($_POST['company']);
$phone = check_input($_POST['phone']);
$message = check_input($_POST['message'],"Please don't send us a blank message! Let us know what you're thinking.");

if (!preg_match("/([\w\-]+\@[\w\-]+.[\w\-]+)/",$email))
{
    show_error("E-mail address is not valid.");
}

$messageBody ="Hello!

Your contact form has been submitted by:

Name: $name
E-mail: $email
Company: $company
Phone: $phone

Message:
$message

End of message
";

$recipient = "someone@example.com"; // removed real email for SE
$subject = "[Contact Form - Site Name]"; // removed site name for SE

$mailheaders = "From: someone@example.com\n"; // removed real email for SE
$mailheaders .= "Reply-To:". $_POST['email']."\n\n";

mail($recipient, $subject, $messageBody, $mailheaders);

header('Location: http://www.example.com/contact/thankyou.html'); //removed real address for SE
exit();

function check_input($data, $problem='')
{
    $data = trim($data);
    $data = stripslashes($data);
    $data = htmlspecialchars($data);
    if ($problem && strlen($data) == 0)
    {
        show_error($problem);
    }
    return $data;
}

function show_error($mailError)
{
?>
// error message in here, removed for SE as unrelated to problem
<?php
exit();
}
?>

This is the message my server admin sent me:

-----Original Message-----
From: MAILER-DAEMON@ss2.site-hosts.com
[mailto:MAILER-DAEMON@ss2.site-hosts.com]
Sent: Monday, August 04, 2014 2:47 PM
To: anonymous@ssasi.site-hosts.com
Subject: failure notice

Hi. This is the qmail-send program at ss2.site-hosts.com.
I'm afraid I wasn't able to deliver your message to the following addresses.
This is a permanent error; I've given up. Sorry it didn't work out.

<wsinwjfjqxv@hotmail.com>:
65.55.37.104 does not like recipient.
Remote host said: 550 Requested action not taken: mailbox unavailable Giving up on 65.55.37.104.

--- Below this line is a copy of the message.

Return-Path: <anonymous@ssasi.site-hosts.com>
Received: (qmail 6348 invoked by uid 2523); 4 Aug 2014 14:47:01 -0500
X-Qmail-Scanner-Diagnostics: from
199.192.201.219.rdns.continuumdatacenters.com by ss2.site-hosts.com (envelope-from <anonymous@ssasi.site-hosts.com>, uid 2020) with qmail-scanner-2.10st
 (clamdscan: 0.98.4/19261. mhr: 1.0. spamassassin: 3.3.2. perlscan:
2.10st.
 Clear:RC:1(199.192.201.219):.
 Processed in 0.024127 secs); 04 Aug 2014 19:47:01 -0000
Received: from 199.192.201.219.rdns.continuumdatacenters.com (HELO
ssasi.site-hosts.com) (199.192.201.219)
  by ss2.site-hosts.com with ESMTPA; 4 Aug 2014 14:47:01 -0500
Received: (qmail 15903 invoked by uid 10024); 4 Aug 2014 14:47:00 -0500
X-Qmail-Scanner-Diagnostics: from  by ssasi.site-hosts.com (envelope-from <anonymous@ssasi.site-hosts.com>, uid 48) with qmail-scanner-2.10st
 (clamdscan: 0.98.4/19261. mhr: 1.0. spamassassin: 3.3.2. perlscan:
2.10st.
 Clear:RC:1(127.0.0.1):.
 Processed in 0.013521 secs); 04 Aug 2014 19:47:00 -0000
Date: 4 Aug 2014 14:47:00 -0500
Message-ID: <20140804194700.15894.qmail@ssasi.site-hosts.com>
From: anonymous@ssasi.site-hosts.com
To: wsinwjfjqxv@hotmail.com
Subject:
X-PHP-Originating-Script: 10021:vnbMail.php

Your script is vulnerable to a header-splitting attack.  Due to the poor design of PHP's mail() function, it is actually quite easy to introduce that kind of security hole.

In summary, if…

any part of the mail headers consists of user-supplied input,
and you didn't make any effort to prohibit newlines or escape that input,

then you will have a program that can send mail to any recipient of the attacker's choice.

Validation problems

As @EmanuelePaolini points out, you do make an attempt to validate $_POST['email']:

$email = check_input($_POST['email'],"Email address is required.");
# …

if (!preg_match("/([\w\-]+\@[\w\-]+.[\w\-]+)/",$email))
{
    show_error("E-mail address is not valid.");
}

However, the regular expression is not anchored with ^ and $ at each end.  Therefore, the code confirms that $_POST['email'] contains an e-mail address, but it fails to enforce that it contains nothing but a single e-mail address.

On the other hand, the regular expression is also too strict: it rejects many valid characters that can legally appear in an e-mail address.  Read RFC 5322 Sec 3.4 for the specification, or this summary of the older RFC 822 standard.


Tic Tac Toe - Verifying a player won I saw this problem on a mock interview I spectated online, and decided to take a stab at it. The problem as stated originally asked to find if a given player had won a standard game of tic-tac-toe. However, the code below goes for a more general case where: the matrix size is variable, and the number of markers in a row to win is also variable.

If \$n\$ = height of the board, the solution below is \$O(k * N^2)\$, where \$k\$ is the number of markers needed in a row to win. The gist of the approach is to loop through every position and check downward or forward if it is the starting point of a solution.

One clear issue here is that the information gained from a failed traversal isn't stored. For example, if on the first row we have xxo, breaking the traversal on the 'o' doesn't prevent us from calling "test_forward" even though it will clearly fail on the next index.

What are potential improvements that could be implemented made here?

tests = {'a': [['x','x','o'],
               ['-','o','x'],
               ['-','-','x']]}

def test_forward(board, i, j, player, num_needed):
    # Check for 3 in a row horizontally (forward)
    for k in range(1, num_needed):
        if board[i][j+k] != player:
            return False

    return True

def test_down(board, i, j, player, num_needed):
    # Check for 3 in a row vertically (downward)
    for k in range(1, num_needed):
        if board[i+k][j] != player:
            return False

    return True

def test_diag_forward(board, i, j, player, num_needed):
    # Check for 3 in a row diagonally down and right
    for k in range(1, num_needed):
        if board[i+k][j+k] != player:
            return False

    return True

def test_diag_backward(board, i, j, player, num_needed):
    # Check for 3 in a row diagonally down and left
    for k in range(1, num_needed):
        if board[i+k][j-k] != player:
            return False

    return True

def check_neighbors(board, dimension, i, j, player, num_needed):
    if board[i][j] != player:
        return False

    # Avoid array out of bounds
    searchable_space = dimension - num_needed

    if j <= searchable_space:
        won_forward = test_forward(board, i, j, player, num_needed)
        if won_forward:
            return True

    if i <= searchable_space:
        won_down = test_down(board, i, j, player, num_needed)
        if won_down:
            return True

    if j <= searchable_space and i <= searchable_space:
        won_diag_forward = test_diag_forward(board, i, j, player, num_needed)
        if won_diag_forward:
            return True

    if j >= num_needed - 1 and i <= searchable_space:
        won_diag_backward = test_diag_backward(board, i, j, player, num_needed)
        if won_diag_backward:
            return True

    return False

# Loop through every square and check if it is the starting point of a solution
def has_winner(board, player, num_needed):
    dimension = len(board[0])
    for i in range(dimension):
        for j in range(dimension):

            result = check_neighbors(board, dimension, i, j, player, num_needed)

            if result == True:
                return result
            else:
                continue

    return result

print has_winner(tests['a'], 'x', 3)

One thing that comes to mind is probability.

In all winning cases of tic-tac-toe, the middle spot of the involved row/column/diagonal is always occupied.  Therefore, I would look to spread my chances and test from the most frequently occupied space(s) in the middle and keep trying to test one adjacent space outward at a time in an evenly spread fashion.

The benefit of this really comes into focus when you think about the fact that the whole check is really done in one pass in a spiral kind of manner.  Having the check in one pass means that as soon as you run into a space that eliminates the possibility of a win being present, you stop checking it.  Therefore, the spiral will go faster and faster until a win is found or until the whole check is completed.  Pretty cool to think about.

I may code this...but for now that's just an idea of one interesting approach you can take :)


Lazy loaded property readability I currently have code that looks like this

private string _stringField;

protected string StringField
{
    get
    {
        if (_stringField == null)
            _stringField = GetStringField();
        return _stringField;
    }
}

ReSharper is suggesting I change the property to:

protected string StringField
{
    get
    {
        return _stringField ?? (_stringField = GetStringField());
    }
}

This isn't an idiom I've seen before and probably would have to think about the first time I saw it; is this something I should be concerned would confuse other people too?

?? is well known operator in C#.  It doesn't confuse but reduce coding.

Both snippets have same meaning.

But if it is .NET 4.0 onwards, i would rather use Lazy<T>.

private Lazy<string> lazyStringField = new Lazy<string>(GetStringField);
protected string StringField
{
    get
    {
        return lazyStringField.Value;
    }
}


Template Binary Search Tree This is a simple binary search tree implementation written with C++'s templates feature I wrote to learn C++. What improvements would make this cleaner and better as far as C++ practices go, and are there any problems with implementing the algorithm?

#include <iostream>
#include <string>

using namespace std;

template <class T, class V>
class BinarySearchTree {

    struct t_node {
        t_node * parent;
        t_node * left;
        t_node * right;

        T data;
        V value;
        // Sorted by Value
    };

    t_node * root;

    t_node* getMin() {
        return getMin(root);
    }
    t_node* getMin(t_node * x) {
        while (x->left !=nullptr) {
            x = x->left;
        }
        return x;
    }

    t_node* getMax() {
        t_node * x = root;
        while (x->right != nullptr) {
            x = x->right;
        }
        return x;
    }

    t_node* sucessor(t_node* x) {
        if (x->right != nullptr) {
            return getMin(x->right);
        }
        t_node * y = x->parent;
        while (y!=nullptr && x==y->right) {
            x = y;
            y = y->parent;
        }
        return y;
    }

public:

    void inOrderTreeWalk() {
        inOrderTreeWalk(root);
    }

    // Needs Stream Overload on template T type to work.
    void inOrderTreeWalk(t_node * x) {
        if (x != nullptr) {
            inOrderTreeWalk(x->left);
            cout << x->data;
            inOrderTreeWalk(x->right);
        }
    }

    T search(V keyword) {
        t_node * x = root;
        while (x!=nullptr && keyword != x->value) {
            if (keyword < x->value) {
                x = x->left;
            }
            else x = x->right;
        }
        return x->data;
    }

    void insert(V keyword, T object) {
        // Create t_node;
        t_node * z = new t_node;
        z->value = keyword;
        z->data = object;

        t_node * y = nullptr;
        t_node * x = root;

        while (x!= nullptr) {
            y = x;
            if (z->value < x->value) {
                x = x->left;
            }
            else x = x->right;
        }
        z->parent = y;
        if (y == nullptr) {
            root = z;
        } else if (z->value < y->value) {
            y->left = z;
        } else y->right = z;
    }
};

// Demo Code
int main(int argc, const char * argv[]) {

    string Charles = "Charles Finny";
    string CharlesD = "Personal Assistant";
    string Bob = "Bob of Yorkshire";
    string BobD = "Unkown";
    string Sam = "Sam Smith";
    string SamD = "Programmer";
    string Mary = "Mary Jane";
    string MaryD = "Manager";

    BinarySearchTree<string, string> EmployeeRecords;
    EmployeeRecords.insert(Charles, CharlesD);
    EmployeeRecords.insert(Bob, BobD);
    EmployeeRecords.insert(Sam, SamD);
    EmployeeRecords.insert(Mary, MaryD);

    string result = EmployeeRecords.search("Sam Smith");
    cout << result << "\n" ;
    EmployeeRecords.inOrderTreeWalk();

    return 0;
}

Memory Handling

I see new but I don't see delete. Your tree leaks memory. It needs to clean up after itself in a destructor. And once you add that, you get into the Rule of Three: you need to add the copy constructor and the copy assignment operator, otherwise you'd still leak when you copy your tree.

Usage

Your code looks like an implementation of std::map, and your usage suggests the same, so it'd be better to change your template arguments from T and V (which mean nothing) to Key and Value. Using value as the key is confusing - you should name it key. Although conventionally nodes for maps in the C++ standard library use std::pair<const Key, Value>s.

You should make it clear that Key needs to be both LessThanComparable and EqualityComparable, or provide ways of passing comparators in as template arguments.

If I try to search() for something not in the map, x will eventually become nullptr and then you return x->data. Oops!

You can insert elements with duplicate values. Is that intentional?

inOrderTreeWalk should be spelled std::ostream& operator<<(std::ostream&, const BinarySearchTree &).

Other

Avoid using namespace std;, especially in headers.
Take your arguments by reference to const to avoid having to deal with copies.
You should support erasing elements.
You should look into various balancing algorithms for you tree so that if I insert 1 then 2 then 3 then 4 then 5, I don't end up with linear-time search.
return 0; from main() is implied, you don't need to explicitly provide it.


Reformatting a date from YYYY-MM-DD to MM/DD/YYYY In this small bit of JavaScript, a date is passed in as YYYY-MM-DD. It's formatted like this to be passed to the backend system. On the front end, we need to display it as MM/DD/YYYY.

I split the string, and put the each array index in the correct position. However, I feel like there could be a a better way to do this.

This is the code I have at the moment.

static formatCourseDate(date: string): string {
    const _date = date.split('-');
    const dateObj = {month: _date[1], day: _date[2], year: _date[0]};

    return dateObj.month + '/' + dateObj.day + '/' + dateObj.year;
}

Simpler approach

Another option is to construct a Date object (this can be achieved by appending a time at midnight to the date) and pass that to Intl.DateTimeFormat.prototype.format(). Then there is no need to parse it and reconstruct it yourself.

function formatCourseDate(date) {
  const dateObj = new Date(date + 'T00:00:00');
  return new Intl.DateTimeFormat('en-US').format(dateObj);
}
console.log(formatCourseDate('1995-12-17'));

Reviewing your current code

Because this is Code Review and we don't just give alternate approaches, I wanted to also give feedback on the current code.

Given your current approach of splitting the parts of the date:

const _date = date.split('-');
const dateObj = {month: _date[1], day: _date[2], year: _date[0]};

because ecmascript-6 features like const are used, object destructuring and the shorthand property definition notation could also be used to simplify this code:

const date = '2017-11-05';
const [year, month, day] = date.split('-');
const dateObj = {month, day, year};
console.log('dateObj:', dateObj);


Unique key having length of 8 I have to generate the unique key having a length of 8, which should be comprised of some set of characters:

{B,C,D,F,G,H,J,K,L,M,N,P,Q,R,S,T,V,W,X,Y,Z,2,3,4,5,6,7,8,9}

Now I have written a program that will generate it, but I am not sure it will generate the unique key.  Could you please review it?.

static string[]  unicode = {"66","67","68","70","71","72","74","75","76","77","78","80","81","82","83","84","86","87","88","89","90","50","51","52","53","54","55","56","57"};

These are the ASCII representation of above character:

long k=5050505050505050; // this initially starts from 5050505050505050

List<string> lst=new List<string>();
void Main(){
    while(lst.Count()<1000000 && k!=9090909090909090)
    {

        var NoSet=Split(k.ToString(),2);

        if(NoSet.Count()==8){

            var no=string.Join("",NoSet.Select(x=>(char)(Convert.ToInt32(x))));
            lst.Add(no);

        }

        k++;
    }
}

Finally, store the last value of K variable to generate the next 1000000 unique key:

static IEnumerable<string> Split(string str, int chunkSize){
return Enumerable.Range(0, str.Length / chunkSize)
    .Select(i => str.Substring(i * chunkSize, chunkSize)).Where(x=>unicode.Contains(x)).Select(x=>x);}

Output from the list:

  22222222

  22222223

  22222224

  22222225

  22222226

  22222227

  22222228

  22222229

  2222222B

  2222222C

  2222222D

It looks from the data as if you do not need the numbers to be random, only unique.
A solution is to take the numbers from 1 to 1,000,000 but output them in base 29 using the allowed characters.

private static readonly char[] Values = {
        'B', 'C', 'D', 'F', 'G', 'H', 'J', 'K', 'L', 'M', 'N', 'P', 'Q', 'R', 'S', 'T', 'V', 'W', 'X', 'Y', 'Z', '2',
        '3', '4', '5', '6', '7', '8', '9'
    };

private static readonly int ValueCount = Values.Length;

private IEnumerable<string> GenerateValues(int count) {
    for (var c = 0; c < count; c++) {
        yield return FormatNumber(c);
    }
}

private static string FormatNumber(int value) {

    var digits = new[] { Values[0], Values[0], Values[0], Values[0], Values[0], Values[0], Values[0], Values[0] };
    var slotCount = digits.Length;

    var current = value;
    var count = 0;
    while (current > 0) {
        int rem;
        current = Math.DivRem(current, ValueCount, out rem);
        digits[slotCount - ++count] = Values[rem];
    }

    return new string(digits);
}

This will generate:

BBBBBBBB  (0)
BBBBBBBC  (1)
BBBBBBBD  (2)
...
BBBCQBC3  (1,000,000)

you can modify it to start at any point and to generate different amount of Ids (up to \$29^8-1\$)


Get contents from file The following PHP code does exactly what I need it to do but, as a complete PHP novice, I am wondering: what can be improved here?

$theurl='./section-1.html';
function getbody1($filename) {
  $file = file_get_contents($filename);
  $bodypattern = ".*";
  $bodyendpattern = ".*";
  $thecontainer = "id=\"container\"";
  $thecleanup = "";
  $noheader = preg_replace("/".$bodypattern."/", "", $file);
  $noheader = preg_replace("~".$bodyendpattern."~", "", $noheader);
  $noheader = preg_replace("|".$thecontainer."|", "", $noheader);
  $noheader = preg_replace("#".$thecleanup."#", "", $noheader);
  return $noheader;
}
echo getbody1("$theurl");

First things first, you are not using the right tool for the job. Regular expressions can't reliably parse markup. Full stop. But more on that later, let's first take a look at your patterns, as this may help you in the future:

$bodypattern = ".*";//for example
//later
preg_replace('/'.$bodypattern.'/',...);

Now, whenever you quote a string into a regex pattern, you have to stop and think: what if the string happens to contain special chars?. I mean: half of the characters can mean a variety of things depending on the context. Thankfully, there's a handy function called preg_quote to help you with that. The safe option here would be:

preg_replace('/'.preg_quote($bodypattern, '/').'/', ...);

Now, you seem to be under the impression that a dot matches anything. And it does, except line-feeds. If I were to apply preg_match('/.+/', $thisAnswer, $matches); to this answer, the pattern would only match the text until a \n is encountered.
You need a PCRE modifier: s:

preg_match('/.+/s', $thisAnswer, $match);

That's more like it. However, this is just as an aside, because what you really should be looking into are ways to actually treat markup as markup: a DOM needs to be parsed, if you want to get the most out of it. Here's my actual (and as-originally-posted) answer:

Ouch, The first thing I noticed is that you're processing markup, and have chosen to do so using regular expressions. That's a bad idea from the off.
It's especially bad considering you have a tool at your disposal that is specifically built to parse markup, the DOMDocument object.

Basically, what you're trying to do can be written as simple as this:

$DOM = new DOMDocument;//create DOM
$DOM->loadHTMLFile($theUrl);//parse HTML
$container = $DOM->getElementById('container');//get the id=container element
if ($container)
{//make sure it was found
    $container->removeAttribute('id');//remove the id attribute
}
$body = $contents = $dom->getElementsByTagName('body')->item(0);//get body
//remove attributes
while ($body->hasAttributes())
{//while tag has attributes
    //remove attribute
    $body->removeAttribute($body->attributes->item(0)->nodeName);
}
//Get the body as string, use saveXML
//as saveHTML adds DOCTYPE, html, head and title tag again
$body = substr($DOM->saveXML($body), 6, -7);

The reason why you need to remove the attributes for the body tag, is because DOMDocument::saveXML will return the markup string with the outer <body> and </body> tags. Their length is a given, but only if the opening <body> tag hasn't got any attributes. Thankfully, we can remove them quite easily, and slice them off with substr.

If you want, you could easily use:

$body = str_replace(
    array('<body>', '</body>'),
    '',
    $DOM->saveXML($body)
);

Perhaps this is a bit easier to understand. Anyway: read the DOMDocument docs, there's a lot you can do there. Just click on the methods I used, and click the objects they return, too, so you know what instances you're working with.

Now wrap all this in a function, and we can add some checks to avoid exceptions or errors from showing up:

function getBody($file, DOMDocument $dom = null)
{//optionally pass existing DOMDocument instance
 //to avoid creating a new one each time we call this function
    if (!file_exists($file))
    {//check if the file exists, if not:
        throw new InvalidArgumentException($file. ' does not exist!');
    }
    $dom = $dom instanceof DOMDocument ? $dom : new DOMDocument;
    $dom->loadHTMLFile($file);
    $container = $dom->getElementById('container');
    if ($container)
        $container->removeAttribute('id');
    $body = $dom->getElementsByTagName('body')->item(0);
    while($body->hasAttributes())
    {
        $body->removeAttribute(
            $body->attributes->item(0)->nodeName
        );
    }
    $body = substr($dom->saveXML($body), 6, -7);
    //optionally remove trailing whitespace:
    return trim($body);
}

Call this function either like this:

$body1 = getBody('path/to/file1.html');

or, since you're going to process more than 1 file, and we can avoid creating a new DOMDocument instance for every function call:

$dom = new DOMDocument;
$files = array('file1.html', 'file2.html');
$bodyStrings = array();
foreach ($files as $file)
    $bodyStrings[] = getBody($file, $dom);

That's it.

As a guide-to-the-DOMDocument-docs:

Every class extends from the DOMNode class, except for the DOMNodeList class, which is a Traversable containing only DOMNode instances, accessible through the item method. Anyway, check the DOMNode methods and properties. Learn that class by heart, as almost every object you work with inherits its properties and methods.
The other class to check is the DOMElement class. Instances of this class represent a node (a tag if you will). It has some methods that I've used here (like removeAttribute).
Finally, everything, of course starts with the DOMDocument class. I had already linked to the docs, but an extra link never hurt anyone.


A simple project of getting Mars Rover images from NASA API and displaying them in JSON format I am learning to program and I have created this simple project in Python where I extract Mars Rover images for a given date from the NASA API:
https://api.nasa.gov/mars-photos/api/v1/rovers/curiosity/photos?earth_date=2021-05-28&camera=NAVCAM&api_key=DEMO_KEY
and display all the images for the last 10 days from the current date in JSON format. The keys are the dates and their values are a list of all the images taken by Mars Rover on that date.
I stored all the images in a file as a cache to avoid exceeding the rate limit to the API (although a local file is not a great choice of caching, but I didn't want to use any libraries for that).
I have got all the output as desired and the project is working fine. However, I am not sure if my design and approach are acceptable, like following Object-Oriented, SOLID, and DRY principles, or if it's really clumsy.
I would like to have someone review my code, and give me honest feedback on my code, whether this is of the standard that interviewers look for.
This is the Github link to the project and if you wish to run the code for yourself, feel free to do so and follow the instructions in the READMe file in the project.
https://github.com/Vivek-Parupudi/nasaApiImages
Thanks in advance!
Sample output (included only a single date for the sake of simplicity):
{
"2021-05-19": [
    "https://mars.nasa.gov/msl-raw-images/proj/msl/redops/ods/surface/sol/03123/opgs/edr/ncam/NLB_674737022EDR_F0880366NCAM00253M_.JPG",
    "https://mars.nasa.gov/msl-raw-images/proj/msl/redops/ods/surface/sol/03123/opgs/edr/ncam/NLB_674736991EDR_F0880366NCAM00253M_.JPG",
    "https://mars.nasa.gov/msl-raw-images/proj/msl/redops/ods/surface/sol/03123/opgs/edr/ncam/NLB_674736907EDR_F0880366NCAM00253M_.JPG" ]

}

Here are a few things I found:
General practises

Using packages is great. But if you are just going to have one class/file in each package, then you probably don't need it. You could just have all your files in the challenge directory.

If you really needed to use packages, storing a data file in the package is not a great idea. Packages are meant for code. So your cache.json would probably be more suitable elsewhere. You could just have the cache file inside the challenge directory, or create a data directory inside challenge to contain it.

Providing a requirements.txt file is great. It is an easy way for someone to download you code and just be able to download its dependencies.

Files should end with an empty line. See this question.

Code style

If your docstrings are just one line, its fine to have have them as such:

def my_function():
  '''Some docs about my_function'''
  ...

However, if your docstring is longer than one line, its better to write it as:
def my_function():
  '''This is a oneliner description about my_function

  This is further information about it

  Foo

  Bar
  '''
  ...

It seems you are using this latter way of writing docs for you classes, but not for you methods. See NasaClient#generate_responde_data. For further information about docstrings, you can refer to PEP-257.
For further information regarding style, in python there is PEP-8, a general guide about it.
pathlib
Since python 3.4, we have pathlib available in the standard library, which makes the handling of paths more pythonic.
For example, you could change:
import os
absolute_path = os.path.dirname(os.path.abspath(__file__))
file_path = absolute_path + '/cache.json'
...
with open(FileCache.file_path, 'r') as file_reader:
...

in FileCache, with:
from pathlib import Path
absolute_path = Path(__file__).resolve().parent
file_path = absolute_path / 'cache.json'
...
with FileCache.file_path.open() as file_reader:
...

The usage of this library also gets rid of the os dependency if you are only using it for os.path
Handling exceptions / errors
In HttpClient, if the responde status code is not OK, you are returning a string containg an exception message.
That is probably not the best of ideas. Being able to tell, dinamycally, whether a returned value is as expected or not by examining the contents of a string is (which you actually don't even do in your code) is expensive and unnecessary.
You could either use None, or throw an actual exception:
...
def get_response(url):
  response = requests.get(url)
  if response.status_code == 200:
    return response
  else:
    return None
  # If you want to get fancy, you could use the ternary operator here:
  # return response if response.status_code == 200 else None
...
#Then when using get_response:
my_response = get_response()
if my_reponse:
  do_something(my_response)
else:
  handle_not_OK_reponse()

using exceptions:
class NotOKResponseException(Exception):
  pass
...
class HttpClient:
  def get_response(url):
    response = requests.get(url)
    if response.status_code == 200:
      return response
    else:
      raise NotOKResponseException()
...
#Then when using get_response:
try:
  my_response = get_response()
  do_something(my_response)
except NotOkResponseException:
  handle_not_OK_reponse()

Code design (Edit)
Regarding the comment:

The gateway method to actually run the code was 'print_response()' which should actually print the response but it also calls the NasaClient to generate the response. Is that a good design?

That is not the best of designs.
After reading what the OutputGenerator#print_response method does, its probably best to name it something like retrieve_printable_data, which indicates its behaviour more accurately.
Also, it does not seem right to me that the NasaClient stores the data in the cache file for the OutputGenerator to read it. There are two reasons I can currently think of:

NasaClient is already responsible for reading and writing the cache using FileCache. Therefore, to isolate this responsibility, just have NasaClient#generate_response_data return the actual data. That way, the cache is transparent to OutputGenerator.

OutputGenerator is just using the FileCache given to it through its constructor. Now, if you wished to change the cache to an in-memory cache, that would make the logic even more complex, as now whoever is constructing the instances not only has to know that OutputGenerator and NasaClient need a FileCache, but also that they need to have the same instance.


Application for allowing a user to "follow" another user I have taken a stab at creating my first jQuery script from scratch. I use this to post an update to a Django application that allows a user to "follow" another user. I am sure that there are better ways to do this and I appreciate any help that anyone can give.

<script type="text/javascript">
jQuery(function() {
    jQuery('.follow').click(function(event) {
        event.preventDefault();
        var url = jQuery(this).attr("href");
        var followtype = jQuery(this).text();
        var user = jQuery(".follow").attr("href").match('/relationships/[a-z]+/([a-z]+)/following/')[1];
        var postdata = {
            'csrfmiddlewaretoken': '{{ csrf_token }}'
        }
        jQuery.post(url, postdata, function(json){
            if(json.result == '1') {
                if(followtype == 'Follow') {
                    jQuery('.follow').text('Unfollow');
                    jQuery('.follow').attr("href", '/relationships/remove/'+user+'/following/');
                } else {
                    jQuery('.follow').text('Follow');
                    jQuery('.follow').attr("href", '/relationships/add/'+user+'/following/');
                }
            }
        })
    });
});
</script>

This is how I'd do it. There may be flaws because you haven't provided your HTML.

I assume that within the click handler, you only want to interact with the current element being clicked. The reason I say that is because you had $('.follow') a number of times in there, which would affect all .follow elements.

         //     v--- makes the shortcut available inside the .ready() callback
jQuery(function($) {
    $('.follow').click(function(event) {
        event.preventDefault();

            // assuming it's a link, no need for jQuery here
        var url = this.href;

            // Looking at the code in your callback, I assume you only have text content
        var followtype = this.innerHTML;

            // I assume you actually want just the current "follow", not all
        var user = url.match('/relationships/[a-z]+/([a-z]+)/following/');

            // Make certain there was a match before accessing the Array.
            // If there was no match, just return the function
        if( user )
            user = user[1];
        else
            return;

        var postdata = {
            'csrfmiddlewaretoken': '{{ csrf_token }}'
        };
            // Inside the $.post callback, are we still only working with the current
            //   "follow" clicked? If so, cache it here, and use it in the callback
        var this_follow = this;

        $.post(url, postdata, function(json){
            if(json.result == '1') {
                if(followtype == 'Follow') {
                     this_follow.innerHTML = 'Unfollow';
                     this_follow.pathname = '/relationships/remove/'+user+'/following/';
                } else {
                    this_follow.innerHTML = 'Follow';
                    this_follow.pathname = '/relationships/add/'+user+'/following/';
                }
            }
        })
    });
});

As you can see, I tend to eliminate jQuery when convenient.

I should note that using .innerHTML in the way I have is cheating a bit. It's an HTML parser really, but it's convenient for changing small chunks of text quickly.

To do it properly, you'd select the child text node of the element, and change its nodeValue.

this_follow.firstChild.nodeValue = 'Unfollow';


Sending a string from anonymous thread to UI with PostMessage I am sending a string from an anonymous thread to the UI with PostMessage in the following code:

unit Unit2;

interface

uses
  Winapi.Windows, Winapi.Messages, System.SysUtils, System.Variants, System.Classes, Vcl.Graphics,
  Vcl.Controls, Vcl.Forms, Vcl.Dialogs, Vcl.StdCtrls;

const
  WM_SETCAPTION = WM_USER;

type
  TForm2 = class(TForm)
    Button1: TButton;
    procedure Button1Click(Sender: TObject);
  private
    { Private declarations }
  public
    { Public declarations }
    procedure WMSetCaption(var msg: TMessage); message WM_SETCAPTION;
  end;

var
  Form2: TForm2;

implementation

{$R *.dfm}

procedure TForm2.WMSetCaption(var msg: TMessage);
begin
  Self.Caption := PChar(msg.LParam);
end;

procedure TForm2.Button1Click(Sender: TObject);
begin
  System.Classes.TThread.CreateAnonymousThread(
    procedure
    begin
      PostMessage(Handle, WM_SETCAPTION, 0, LParam(PChar('My new caption')));
    end).Start;
end;

end.

This works seemingly well, but could this potentially create memory leaks? Or is there a better way to accomplish this?

Create an object containing the string as a property and pass the pointer to the object as Integer LParam. Then, in the message handler, use the string and free the object. In this way, you could not only send strings but also other data as well.

unit SendingStringWithPostMessageUsingObjectMainForm;

interface

uses
  Winapi.Windows, Winapi.Messages, System.SysUtils, System.Variants, System.Classes, Vcl.Graphics,
  Vcl.Controls, Vcl.Forms, Vcl.Dialogs, Vcl.StdCtrls;

const
  WM_STRINGMESSAGE = WM_USER;

type
  TStrMsgObject = class(TObject)
  private
    fMsg: string;
  public
    constructor Create(const aMsg: string);
    property Msg: string read fMsg;
  end;

type
  TForm2 = class(TForm)
    btnSetCaption: TButton;
    btnChangeCaption: TButton;
    procedure btnChangeCaptionClick(Sender: TObject);
    procedure btnSetCaptionClick(Sender: TObject);
  private
    { Private declarations }
    procedure WMStringMessage(var Msg: TMessage); message WM_STRINGMESSAGE;
  public
    { Public declarations }
  end;

var
  Form2: TForm2;

implementation

{$R *.dfm}

const
  // WM_STRINGMESSAGE Actions:
  ACTION_SETCAPTION    = 0;
  ACTION_CHANGECAPTION = 1;

constructor TStrMsgObject.Create(const aMsg: string);
begin
  inherited Create;
  fMsg := aMsg;
end;

procedure TForm2.WMStringMessage(var Msg: TMessage);
// Process WM_STRINGMESSAGE messages
var
  mo: TStrMsgObject;
begin
  mo := TStrMsgObject(Msg.LParam);
  try
    case Msg.WParam of
      ACTION_SETCAPTION:
        begin
          Caption := mo.Msg;
        end;
      ACTION_CHANGECAPTION:
        begin
          Caption := Caption + mo.Msg;
        end;
    end
  finally
    mo.Free;
  end;
end;

procedure TForm2.btnSetCaptionClick(Sender: TObject);
begin
  System.Classes.TThread.CreateAnonymousThread(
    procedure
    begin
      PostMessage(Handle, WM_STRINGMESSAGE, ACTION_SETCAPTION, Integer(TStrMsgObject.Create('My new caption')));
    end).Start;
end;

procedure TForm2.btnChangeCaptionClick(Sender: TObject);
begin
  System.Classes.TThread.CreateAnonymousThread(
    procedure
    begin
      PostMessage(Handle, WM_STRINGMESSAGE, ACTION_CHANGECAPTION, Integer(TStrMsgObject.Create(' changed')));
    end).Start;
end;

end.


Displaying user selection in the browser I have the following script that reads JSONified data from within a DIV, generates HTML with values using JavaScript, then sends them to another DIV.

What I'd like to improve is to remove HTML from JavaScript.  Namely, not have HTML inside the JavaScript.  I am not 100% on how to do this.  I want "minimal work" but "most separation" of HTML and JS.

var selection = eval("(" + document.getElementById("result").innerHTML + ")");

/*
 * Relevant Example only:
 * selection[index].couplings = "[{"price":13.00,"description":"Couplings"}]";
 */

/**
 * Show Couplings
 */
if (selection[index].couplings != null)
{
    for (i = 0; i < selection[index].couplings.length; i++) {
        result += '
';
        result += '' + selection[index].couplings[i].description + '	';
        result += '' + moneyFormat(selection[index].couplings[i].price) + '	';
        result += '
';
    }
}

document.getElementById('couplings').innerHTML = result;

/*****************/
/* Number Format */
/*****************/
function moneyFormat(num) {
    return '$' + parseFloat(num).toFixed(2).replace(/(\d)(?=(\d{3})+\.)/g, "$1,").toString();
}

var selection = eval("(" + document.getElementById("result").innerHTML + ")");

I'm assuming this is the part that reads the JSON? There's JSON.parse for that. Feed it a JSON string and it should give you the equivalent object. No need to use eval.

/*****************/
/* Number Format */
/*****************/
function moneyFormat(num) {
    return '$' + parseFloat(num).toFixed(2).replace(/(\d)(?=(\d{3})+\.)/g, "$1,").toString();
}

I would suggest putting functions and any other inert operations higher up in the code. It's mostly for readability, knowing what exists before trying to use them.

Another is that parseFloat may return NaN. You may want to guard by checking against isNaN before handing it over to other operations.

Lastly, concatenating a string to a number (i.e.: 'foo' + 2) will automatically coerce the number to a string. Therefore you don't need the trailing toString().

if (selection[index].couplings != null)
{
    for (i = 0; i < selection[index].couplings.length; i++) {
        result += '
';
        result += '' + selection[index].couplings[i].description + '	';
        result += '' + moneyFormat(selection[index].couplings[i].price) + '	';
        result += '
';
    }
}

There's 2 ways to avoid writing markup in JS. One would be to construct the elements using document.createElement or by using a template library and construct the markup elsewhere.

A third option, if you have ES6 at your fingertips is to use template strings. This allows you to write multi-line strings in JS. And guess what? We can do something like this:

result += `

    ${selection[index].couplings[i].description}
    ${moneyFormat(selection[index].couplings[i].price)}

`;


My 'almostIncreasingSequence(sequence)' code is too slow for lists that have 10000+ elements. [CodeSignal] I asked this question on Stackoverflow as well, but I think it's best suited here because my code needs optimization instead of error checking (that I previously thought).
I've made changes to my code as well. But the logic is pretty much the same:

My code first checks the length of the provided sequence, if it is 2 or less it automatically returns True.
Next, it creates a newlist with the first element removed and checks if the rest of the list is in ascending order.
If the sequence is not in order, the iteration breaks and a newlist is generated again and this time with the next element removed.
This continues until there are no more elements to remove (i.e. i == len(sequence) - 1), which ultimately returns as False
If in any of the iterations, the list is found to be in ascending order(i.e. in_order remains True), the function returns True.

def almostIncreasingSequence(sequence):
    # return True for lists with 2 or less elements.
    if len(sequence) <= 2:
        return True

    # Outerloop, removes i-th element from sequence at each iteration
    for i in range(len(sequence)):
        newlist = sequence[:i] + sequence[i+1:]

        # Innerloop, checks if the sequence is in ascending order
        j = 0
        in_order = True
        while j < len(newlist) - 1:
            if newlist[j+1] <= newlist[j]:
                in_order = False
                break
            j += 1
        if in_order == True:
            return True
        elif i == len(sequence)-1:
            return False

I received a suggestion that I should only use one loop, but I cannot think of a way to implement that. Nested loops seems necessary because of the following assumptions:

I have to remove every next element from the original sequence. (outer loop)
I need to check if all the elements are in order. (inner loop)

This is a brief on almostIncreasingSequence() my code follows the logic provided in the answer here, it solves almost all of the Tests as well, but it is too slow for larger lists (that are approx 10000+ elements).

Checking if a list is strictly increasing
The inner loop checks if a list (in this case newList) is strictly increasing:
j = 0
in_order = True
while j < len(newlist) - 1:
    if newlist[j+1] <= newlist[j]:
        in_order = False
        break
    j += 1
if in_order == True:
    return True

In general variable names should use underscores, so newlist becomes new_list. PEP 8.
It can be simplified with a for-else:

for j in range(len(new_list) - 1):
    if new_list[j+1] <= new_list[j]:
        break
else:
    return True

Or using all:
if all(new_list[i] > new_list[i - 1] for i in range(1, len(new_list))):
    return True

Optimization
As you said, the solution is too slow for large input due to the inner loop that runs every time, making the overall complexity \$O(n^2)\$.
The current solution follows this approach:

For each element of the input list
Build a new list without such element and check if strictly increasing

Consider simplifying the approach like the following:

Find the first pair that is not strictly increasing
Build a new list without the first element of the pair and check if it is increasing
Build a new list without the second element of the pair and check if it is increasing

For example:
def almostIncreasingSequence(sequence):
    def is_increasing(l):
        return all(l[i] > l[i - 1] for i in range(1, len(l)))

    if is_increasing(sequence):
        return True

    # Find non-increasing pair
    left, right = 0, 0
    for i in range(len(sequence) - 1):
        if sequence[i] >= sequence[i + 1]:
            left, right = i, i + 1
            break

    # Remove left element and check if it is strictly increasing
    if is_increasing(sequence[:left] + sequence[right:]):
        return True

    # Remove right element and check if it is strictly increasing
    if is_increasing(sequence[:right] + sequence[right + 1:]):
        return True

    return False

I believe there should be an approach that doesn't build the two additional lists to reduce the space complexity, but I'll leave that to you.


Polling an API endpoint with timeout using channels I have written a simple API client for my app in Go.
The client is very simple, it wraps http.Client and has some convenience methods.

The one method I am trying to get some feedback on, does endpoint polling with timeout. It blocks, and returns when either a timeout is reached or status returned by API is successful or in error.

My questions are in comments of the function itself.
I have omitted some parts (structs, the "constructor method") that are not relevant to discussion.

caller (in main)

log.Printf("polling status, id: %d", sid)
status, err := api.PollStatus(sid, 1*time.Second, 5*time.Second)
if err != nil {
    log.Fatalf("PollStatus error: %+v", err)
}

log.Print(status)

the method

// PollStatus will keep 'pinging' the status API until timeout is reached or status returned is either sucesfull or in error.
// NOTES:
//   - this function will block the caller
// QUESTIONS:
//  - is the design/functionality of this method idiomatic Go or is there a better way?
//  - do I need to close ticker and timeout channels? If so how?
//  - is it safe to 'defer ticker.Stop()' at the start of goroutine or do I have to call it before each return?
func (client *APIClient) PollStatus(id int, pollInterval time.Duration, pollTimeout time.Duration) (*Status, error) {
    var status *Status
    var err error
    var wg sync.WaitGroup
    ticker := time.NewTicker(pollInterval)
    timeout := time.After(pollTimeout)
    tickerCounter := 0

    // increment the WaitGroup counter for number of goroutines to wait for...
    wg.Add(1)

    go func() {
        defer ticker.Stop()
        // decrement the counter when the goroutine completes.
        defer wg.Done()

        for {
            select {

            case tout := <-timeout:
                log.Printf("polling timeout of '%s' reached at '%s'...", pollTimeout, tout.Format(time.RFC3339))
                //ticker.Stop()
                return

            case tick := <-ticker.C:
                status, err = client.GetStatus(id)
                tickerCounter++
                if err != nil {
                    //ticker.Stop()
                    return
                }
                log.Printf("tick #%d at '%ss', %s", tickerCounter, tick.Format("05"), status)
                if status.IsErrorStatus() {
                    err = errors.New("error while polling for status")
                    //ticker.Stop()
                    return
                }
                if status.IsCompletedStatus() {
                    err = nil
                    //ticker.Stop()
                    return
                }

            }
        }
    }()

    // wait for polling to complete (or time out)
    wg.Wait()

    if err != nil {
        return nil, err
    }
    if status == nil {
        return nil, errors.New("unable to get status")
    }

    return status, nil
}

How about this?
I used context.Context with the deadline which also gives control to the caller of poll if you want to cancel the poll before the deadline through CancelFunc. I do not have a full context of the problem here, but this should help you.

// PollStatus will keep 'pinging' the status API until timeout is reached or status returned is either successful or in error.
func (client *APIClient) PollStatus(ctx context.Context, id int, pollInterval time.Duration) (*Status, error) {
        ticker := time.NewTicker(pollInterval)
        defer ticker.Stop()
        tickerCounter := 0
        for {
            select {
            case <-ctx.Done():
                return nil, ctx.Err()

            case tick := <-ticker.C:
                status, err := client.GetStatus(id)
                tickerCounter++
                if err != nil {
                    return nil, err
                }

                log.Printf("tick #%d at '%ss', %s", tickerCounter, tick.Format("05"), status)
                if status.IsErrorStatus() {
                    return nil, errors.New("error while polling for status")
                }

                if status.IsCompletedStatus() {
                    return status, nil
                }

            }
        }

        return nil, errors.New("unable to get status")
  }

func someFunc() {
    log.Printf("polling status, id: %d", sid)
    ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Second)
    // Having cancelFunc here gives control if you want to cancel the poll before deadline
    // but for this instance, I'm just gonna defer cancel it
    defer cancelFunc()
    status, err := api.PollStatus(ctx, sid, 1*time.Second)
    if err != nil {
        log.Fatalf("PollStatus error: %+v", err)
    }

    log.Print(status)
}


Prompting for grades, with confirmation if a score higher than 100 is entered This is an exercise from my computer programming class. This program reads in student scores from the user (input) and displays the total points earned along with a grade. I am tasked with modifying it to ask the user for confirmation when a score higher than 100 is entered. Plainly, it should not add a score greater than 100 to the accumulator without permission.

Here is the original source:

 //Lab7-2.cpp - displays the total points earned and grade
 //Created/revised by <your name> on <current date>

 #include <iostream>

 using namespace std;

 int main()
 {
 //declare variables
    int score       = 0;
    int totalPoints = 0;  //accumulator
    char grade      = ' ';

    //get first score
    cout << "First score (-1 to stop): ";
    cin >> score;

    while (score != -1)
    {
        //update accumulator, then get another score
        totalPoints += score;
    cout << "Next score (-1 to stop): ";
    cin >> score;
    }   //end while

    //determine grade
    if (totalPoints >= 315)
    grade = 'A';
    else if (totalPoints >= 280)
    grade = 'B';
    else if (totalPoints >= 245)
    grade = 'C';
    else if (totalPoints >= 210)
    grade = 'D';
    else
    grade = 'F';
    //end if

    //display the total points and grade
    cout << "Total points earned: " << totalPoints << endl;
    cout << "Grade: " << grade << endl;

    system("pause");
 return 0;
 }   //end of main function

Here is the modified source:

//Lab7-2.cpp - displays the total points earned and grade
//Created/revised by Willy Njundong on 2/14/2016

#include <iostream>

using namespace std;

int main()
{
//declare variables
int score = 0;
int totalPoints = 0;  //accumulator
char grade = ' ';
char decision = ' ';

//get first score
cout << "First score (-1 to stop): ";
cin >> score;

while (score != -1)
{
    if (score > 100)
    {                                                                //CONFIRM SCORE VALIDITY
        cout << endl << "Is score correct? (Enter y or n): ";        //IF GREATER THAN 100
        cin >> decision; cout << endl;
        if (decision != 'y' && decision != 'Y')                 //ANY CHARACTER ENTERED OTHER THAN 'Y' FOR YES
            score = 0;                                          //RESULTS IN SCORE OVER 100 BEING IGNORED (SET TO ZERO)
        else
        {                                                       //AND THEN THE
            //update accumulator, then get another score        //PROGRAM RESUMES NORMAL EVALUATION
            totalPoints += score;
            cout << "Next score (-1 to stop): ";
            cin >> score;
        }   //end if

    }
    else                                                        //THESE ARE PROCESSED WHEN SCORES
    {                                                           //ENTERED ARE 100 OR LESS
        //update accumulator, then get another score
        totalPoints += score;
        cout << "Next score (-1 to stop): ";
        cin >> score;
    }   //end if

}   //end while

    //determine grade
if (totalPoints >= 315)
    grade = 'A';
else if (totalPoints >= 280)
    grade = 'B';
else if (totalPoints >= 245)
    grade = 'C';
else if (totalPoints >= 210)
    grade = 'D';
else
    grade = 'F';
//end if

//display the total points and grade
cout << "Total points earned: " << totalPoints << endl;
cout << "Grade: " << grade << endl;

system("pause");
return 0;
}   //end of main function

The only option is this one:

// ...
while (score != -1) {
    if (score > 100) {
        cout << endl << "Is the score correct? (Enter 'y' or 'n'): ";
        cin >> decision; cout << endl;
        if (decision != 'y' && decision != 'Y') score = 0;
    }

    totalPoints += score;
    cout << "Next score ('-1' to stop): ";
    cin >> score;
}   //end while
// ...

It can't be more efficient using given circumstances.


Reading code from array efficiently I'm trying to read data from an array. I manage to get the information I want but my code is extremely inefficiently written.

My array looks like this:

var arTab:Array = new Array();
arTab[0] = new Array(2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012);
var promilleTab:Array = new Array();
promilleTab[0] = new Array(9631, 8593, 8363, 8128, 8514, 8534, 8560, 8146, 8241, 8019,         8759);
var forerkortTab:Array = new Array();
forerkortTab[0] = new Array(13481, 12785, 12585, 12492, 13470, 14181, 14622, 14082, 14287, 13640, 14180);
var hastighetTab:Array = new Array();
hastighetTab[0] = new Array(9863, 12217, 14920, 14929, 15425, 18010, 15909, 14197, 13276, 11158, 12264);
var trafikklovenTab:Array = new Array();
trafikklovenTab[0] = new Array(18862, 19196, 20101, 20026, 21381, 21845, 20005, 19446, 20900, 19346, 20265);

I am using combo boxes to pick a year from the "arTab" array and another to pick the felony done.(the 3 other arrays)

What I am doing right now looks something like this:

function skrivUt(Event:MouseEvent)
{
    if(bruddListe == 1)
    {
        if(arListe == 1)
        {
            txtSvar.text = "I " + arTab[0][0] + " ble det anmeldt " + promilleTab[0][0] + " for promillekjøring";
        }
        if(arListe == 2)
        {
            txtSvar.text = "I " + arTab[0][1] + " ble det anmeldt " + promilleTab[0][1] + " for promillekjøring";
        }
        if(arListe == 3)
        {
            txtSvar.text = "I " + arTab[0][2] + " ble det anmeldt " + promilleTab[0][2] + " for promillekjøring";
        }
    {
{

This is just a small example but the list goes on because I have to do 11 results for all the 4 combo choices.

Is there any way I can write this in a shorter and more efficient way?

Sure,

you are basically copy pasting to keep accessing arTab and promilleTab, you might instead just derive the indexes in arTab from bruddListe and arListe, like this:

function skrivUt(Event:MouseEvent)
{
  txtSvar.text = "I " + arTab[bruddListe-1][arListe-1] + " ble det anmeldt " + promilleTab[bruddListe-1][arListe-1] + " for promillekjøring";

etc. etc.

or you could keep things a bit more readable, by splitting it up, I dont know if this is correct ActionScript, but conceptually you should grok this:

function skrivUt(Event:MouseEvent)
{
  var ar:String =  arTab[bruddListe-1][arListe-1];
  var promille:String = promilleTab[bruddListe-1][arListe-1];
  txtSvar.text = "I " + ar + " ble det anmeldt " + promille + " for promillekjøring";

Furthermore, some other observations :

Try to keep your variables and function Names in English, it's the right thing to do
it seems as if bruddListe and arListe are global variables, you should avoid those


Calculate Digital Root Here's my solution for finding the digital root of a number. Just for purposes of completion, a digital root is:

  "If you take the digits of any number and add them together, and then
  add the digits of the resulting number together, and continue doing
  that until you get a single digit, that single digit is the digital
  root of the original number."

What I was hoping for, is if you guys could take a look and provide a cleaner solution. I'm trying to better myself for an upcoming competition.

import java.util.Scanner;
import java.util.StringTokenizer;

public class DigialRoot {

    public static void main(String[] args) {

        Scanner sc = new Scanner(System.in);
        System.out.print("Enter a number: ");
        System.out.println();

        String str = sc.nextLine();
        String[] numbers = str.split("");
        int[] myList = new int[numbers.length];
        for(int i=0; i < numbers.length; i++){
            myList[i] = Integer.parseInt(numbers[i]);
        }
        //add all numbers together
        int sum = 0;
        for(Integer x : myList){
            sum += x;
        }

        //break down and add sum
        String sumStr = Integer.toString(sum);
        String[] nums = sumStr.split("");
        int[] nxtList = new int[nums.length];
        for(int i=0; i < nums.length; i++){
            nxtList[i] = Integer.parseInt(nums[i]);
        }

        //calculating final result of digital root
        int result = 0;
        for(Integer x : nxtList){
            result += x;
        }

        System.out.println("Original number: " + str);
        System.out.println("Digital root: " + result);

    }

}

Since a digital root is obtained by adding all the digits in a number, and while the resulting number is more than 9, repeating the process, an alternative and equally effective approach could simply be dividing by 9. In most cases, the digital root is simply the remainder of this operation.

Here's an example to see this in action. Take any number; for simplicity's sake, I choose a relatively low number: 625.

625 ->
6 + 2 + 5 = 13 ->
1 + 3 = 4

Thus the digital root is 4.

If you divided 625 by 9 you would get 69 with a remainder of 4. In other words, using modulo, and factoring in our edge cases (When n is 0, or 9 divides n with no remainder), we can write a digital root computing method as straight-forward as:

public static int computeDigitalRoot(int n) {
    return n == 0 ? 0 :
           n % 9 == 0 ? 9 : n % 9;
}

If the ternary operator is not to your liking, this is the equivalent using if statements:

public static int computeDigitalRoot(int n) {
    if (n == 0) {
        return 0;
    }
    if (n % 9 == 0) {
        return 9;
    }
    return n % 9;
}


A PHP Controller, which feels to 'array-like', in Laravel This is a controller, which receives the validated input from a http request, then finds the assorted model and for each translation key in the received data, it will add it to the model.

public function update(UpdateMenuItemRequest $request)
{
    $validated = $request->validated();

    $item = MenuItem::findOrFail($validated['id']);

    foreach($validated['translations'] as $validatedTranslation) {
        $item->translations()->updateOrCreate(['locale' => $validatedTranslation['locale']], $validatedTranslation);
    }

    return $item;
}

Is this Object Oriented enough? It doesn't seem like this method is violating SRP in the first place, or could it be improved? Thank you.

You could use route model binding to remove some lines from your code, but that would require changing the route so it passes the MenuItem's id.

public function update(UpdateMenuItemRequest $request)
public function update(MenuItem $item, UpdateMenuItemRequest $request)
{
    $validated = $request->validated();

    $item = MenuItem::findOrFail($validated['id']);

    foreach($validated['translations'] as $validatedTranslation) {
        $item->translations()->updateOrCreate(['locale' => $validatedTranslation['locale']], $validatedTranslation);
    }

    return $item;
}

Also, if you want legibility and go all out with functional programming, you could replace your loop with a collection method.

# Assuming the request object only has a translation array since you don't need the id anymore
public function update(MenuItem $item, UpdateMenuItemRequest $request)
{
    collect($request->validated()['translations'])->each(function ($translation) use ($item) {
        $item->translations()->updateOrCreate($translation);
    });
}

Depending on what kind of relationship we're dealing with there could be other ways as well.


Simple text based game in C After sticking with the same handful of languages for years, I decided to learn C.

I've written a guess the number game, where it generates a random number based on the difficulty level, and then you have 5 attempts of guessing the number.

If you guess incorrect, it will give you a clue by telling you if the answer is higher or lower than your last guess. I find the game to be pretty interactive, and wanted to get a code review to see how I could improve on it.

This is my first ever C  program and I'm just looking to improve.

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

int generate_random_number(int min, int max) {
    srand ( time(NULL) );
    return min + (rand() % (max - min));
}

void play() {

    int difficulty = 1; // 1 = easy, 2 = medium, 3 = hard, 4 = insane

    printf("1 = EASY, 2 = MEDIUM, 3 = HARD, & 4 = INSANE\n");
    printf("What level would you like to play: ");

    scanf("%i", &difficulty);

    int min = 0;
    int max = 0;
    int random = 0;

    if (difficulty == 1) {
        min = 0;
        max = 25;

        printf("You have selected to play easy\n\n");
    }
    else if (difficulty == 2) {
        min = 0;
        max = 50;

        printf("You have selected to play medium\n\n");
    }
    else if (difficulty == 3) {
        min = 0;
        max = 75;

        printf("You have selected to play hard\n\n");
    }
    else if (difficulty == 4) {
        min = 0;
        max = 100;

        printf("You have selected to play insane\n\n");
    }

    random = generate_random_number(min, max);

    int tries = 5;
    int won = 0;

    while (tries > 0)
    {
        int guess = 1000000; // just so it doesn't accidentally equal to random

        printf("Guess a number %i to %i: ", min, max);
        scanf("%i", &guess);

        if (guess == random) {
            won = 1;
            break;
        }
        else {
            if (guess > random) {
                printf("Incorrect guess, the answer is lower than your guess!\n\n");
            }
            else {
                printf("Incorrect guess, the answer is higher than your guess!\n\n");
            }
        }

        tries -= 1;
    }

    if (won) {
        printf("Congratulations, you have won the game!");
    }
    else {
        printf("Sorry, you are out of tries.\n\n");
    }
}

int main() {
    while (1) {
        play();
    }

    return 0;
}

Good:

no warnings with -Wall -Wextra -pedantic with both gcc and clang, no
memory leaks found with valgrind.

Bad:

use prototypes instead of declarations to give compiler a chance
to issue warnings when an incorrect number of parameters is passed or incorrect types are passed:

void play(void)
int main(void)

You don't check if user passes correct acceptable difficulty level:

$ ./main
1 = EASY, 2 = MEDIUM, 3 = HARD, & 4 = INSANE
What level would you like to play: 6
Floating point exception
$ ./main
1 = EASY, 2 = MEDIUM, 3 = HARD, & 4 = INSANE
What level would you like to play: 0
Floating point exception

You don't check if user passes an integer in the first place:

$ ./main
1 = EASY, 2 = MEDIUM, 3 = HARD, & 4 = INSANE
What level would you like to play: a

will make your program go into an endless loop.

Suggested:

convert int difficulty = 1 into an en enum with additional DIFFICULTY_MAX and DIFFICULTY_MIN values, and check if value passed from the user is lower than DIFFICULTY_MAX and larger or equal to DIFFICULTY_MIN.

Use EXIT_SUCCESS to denote success at the end of main():

return EXIT_SUCCESS;

or just omit the return from main() - compiler will automatically return a success value if we run off the end.


Image processing using Python OOP library I am trying to learn OOP by refactoring my existing code (functions only). This example works but I am not sure whether it is pythonic and good practice.

I have a shared library which consist of several file with functions:

color_spaces.py

import cv2

def rgb_to_hsv(image):
    print ('hsv')
    hsv_image = cv2.cvtColor(image, cv2.COLOR_RGB2HSV)
    hue, sat, val = hsv_image[:, :, 0], hsv_image[:, :, 1], hsv_image[:, :, 2]
    return hsv_image, hue, sat, val

def rgb_to_lab(image):
    lab_image = cv2.cvtColor(image, cv2.COLOR_RGB2LAB)
    l_chan, a_chan, b_chan = lab_image[:, :, 0], lab_image[:, :, 1], lab_image[:, :, 2]
    return lab_image, l_chan, a_chan, b_chan

thresholding.py

import cv2

def treshold_otsu(gray_image):
    ret, thresholded_image = cv2.threshold(gray_image, 0, 255, cv2.THRESH_BINARY_INV + cv2.THRESH_OTSU)
    return thresholded_image

def global_threshold(gray_image):
    ret, thresholded_image = cv2.threshold(gray_image, 127, 255, cv2.THRESH_BINARY)
    return thresholded_image

My class which uses functions of the shared library:

pipline01.py

import cv2
from color_spaces import rgb_to_hsv, rgb_to_lab
from thresholding import treshold_otsu, global_threshold

class Segmentation1(object):
    def __init__(self, filename, path, procedure):
        self.filename = filename
        self.path = path
        self.procedure = procedure

    def start_pipline(self):
        # Read image
        image = cv2.imread(self.path + self.filename)

        # Do segmentation procedure depending on procedure
        if self.procedure == 'test01':
            source = rgb_to_hsv(image)[1]
            binary = treshold_otsu(source)
        elif self.procedure == 'test02':
            source = rgb_to_lab(image)[1]
            binary = global_threshold(source)

        # Save image
        cv2.imwrite(self.path + self.filename.split('.')[0] + '_' + self.procedure + '.png', binary)

Then I call the class in a batch mode by looping over a folder with images:

batch_pipline01.py

import os
import argparse
from pipline01 import Segmentation1

parser = argparse.ArgumentParser(description = 'Test.')
parser.add_argument('-d', '--img_dir', required = True,
                    help = 'directory containing images to segment.')
parser.add_argument('-p', '--procedure', required = True,
                    help = 'procedure to run.')

args = parser.parse_args()
print(args)
img_dir = args.img_dir
procedure = args.procedure
images = os.listdir(img_dir)
#images = [img[2:] for img in args.images]

# Start segmentation for each image
for img in images:
    os.chdir(img_dir)
    print('SEGMENTING ' + img)
    segmenter = Segmentation1(img, img_dir, procedure)
    obj = segmenter.start_pipline()
    del obj

Can I improve my code and design?

This scenario is not best case for doing an OOP conversion: In this example you're not getting real assistance from the Segmentation1 class over what you'd get by using static functions.  The code looks like it does what you need done; it's simply not a great application for OOP as written.  At some point you should check out this talk by Python core dev Jack Diederich for some perspective on when OOP helps in Python and when it doesn't. One of the main takeways there is that classes which expose just one method ususally don't need to be classes.

That said:

If you want to use this as learning case, you can refactor your arrangement so that you have two different classes for the two different strategies.  This is overkill in the code as is, but if you anticipated a much wider range of segmentation tasks with different inner workings it might make sense. It would be a good choice if the segmentation operation had multiple steps and needed to maintain state within a single run -- for example, if it had to collect data from multiple data sources and coordinate it differently for LAB colors and HSV colors.

Your existing code really has only two paths, splitting on the procedure.  So that's a good clue that you want the sibling classes to differ primarily according to the differences  -- the rule is to "encapsulate what varies" -- and, conversely, to share the parts that are the same. The file handling and so on are the same.  So first you can isolate the unchanging bits into mini-methods of their own, and then bookend them around the parts that will change:

class SegmentationType(object):
    DISPLAY_NAME = "invalid"

    def __init__(self, filename, path):
        self.filename = filename
        self.path = path
        self.input_data = None
        self.output_data = None

    def read_image(self):
        self.input_data =  cv2.imread(self.path + self.filename)[1]

    def write_image(self):
        cv2.imwrite(self.path + self.filename.split('.')[0] + '_' + self.DISPLAY_NAME + '.png', self.output_data)

    def process(self):
        # override in derived classes to perform an actual segmentation
        pass

    def start_pipeline(self):
        self.read_image()
        self.process()
        self.write_image()

Then you just need to create subclasses for your strategies:

class HSV_Segmenter(SegmentationType):
    DISPLAY_NAME = 'HSV'

    def process(self):
        source = rgb_to_hsv(self.input_data)
        self.output_data = treshold_otsu(source)

class LabSegmenter(SegmentationType):
    DISPLAY_NAME = 'LAB'

    def process(self):
        source = rgb_to_lab(self.input_data)
        self.output_data = global_threshold(source)

This should be equivalent to your earlier code -- it does have one minor advantage, which is that it will fail at compile time if you don't pick the proper subclass where the original would fail at runtime if you accidentally pass a bad value for procedure.  Then your program simply chooses the right class and calls it:

segmenter_class = {
    'hsv': HSV_Segmentation,
    'lab': LAB_Segmenter
}.get(procedure)

if not segmenter_class:
    raise ArgumentError("Invalid segmentation method '{}'".format(procedure))

for img in images:
    os.chdir(img_dir)
    processor =  = segmenter_class(img, img_dir, procedure)
    processor.start_pipeline()

Again, I personally would stick with static functions in this situation.  In Python you could achieve all the good parts of the class arrangement by just having that dictionary lookup return the right static functions instead of classes.  Lots of languages don't have that neat function and so this kind of structure is very common there.


Printing integers up to 1000 and are not multiples of 5 How can I shorten this?

.equ SWI_PrStr, 0x69
.equ SWI_PrInt, 0x6b
.equ StDout, 1
.equ SWI_EXIT, 0x11

.global _start
start:

MOV R5, #1
MOV R6, #1
MOV R7, #1000

LOOPHERE:

CMP R5, R7
BLE LOOPBODY
B LOOPDONE

LOOPBODY:

CMP R6, #5
BLT LOOPIF
MOV R6, #0
B LOOPNEXT

LOOPIF:

MOV R0, #StDout
MOV R1, R5
swi SWI_PrInt

ldr R1, =EOL
swi SWI_PrStr

B LOOPNEXT

LOOPNEXT:

ADD R5, R5, #1
ADD R6, R6, #1
B LOOPHERE

LOOPDONE:
swi SWI_EXIT

.data

EOL: .asciz "\n"
.end

Instead of jumping over a jump:

BLE LOOPBODY
B LOOPDONE
LOOPBODY:

you can just invert the condition and jump directly:

BGT LOOPDONE

However, as the first iteration will always happen, you can just put the check at the end of the loop instead:

LOOPHERE:

... the code in the loop

CMP R5, R7
BLE LOOPHERE

The jump to LOOPNEXT right before the label can be skipped:

B LOOPNEXT

LOOPNEXT:

So:

.equ SWI_PrStr, 0x69
.equ SWI_PrInt, 0x6b
.equ StDout, 1
.equ SWI_EXIT, 0x11

.global _start
start:

MOV R5, #1
MOV R6, #1
MOV R7, #1000

LOOPHERE:

CMP R6, #5
BLT LOOPIF
MOV R6, #0
B LOOPNEXT

LOOPIF:

MOV R0, #StDout
MOV R1, R5
swi SWI_PrInt

ldr R1, =EOL
swi SWI_PrStr

LOOPNEXT:

ADD R5, R5, #1
ADD R6, R6, #1

CMP R5, R7
BLE LOOPHERE

swi SWI_EXIT

.data

EOL: .asciz "\n"
.end


Rate-limited geographic data lookup I'm looking for a code review of this python I wrote - this code reads the zipcode column value from a CSV file and calls an API to retrieve lat, long, state and city info.

It works fine and gives me the correct results, but I'm looking for ways to improve the code/my approach and exception handling. I am pretty sure there are better ways to write this long code.

All comments and suggestions as brutal as may be are mostly appreciated.

import requests
import pandas as pd
import time
from ratelimiter import RateLimiter
API_KEY = "some_key"
zip_code_col_name = "zipcode"
RETURN_FULL_RESULTS = False
excel_data = pd.read_csv("Downloads/test_order.csv", encoding='utf8')
if zip_code_col_name not in excel_data.columns:
    raise ValueError("Missing zipcode column")
# This will put all zipcodes from column to list including duplicates , hence avoiding it.
#zipcodes = excel_data[zip_code_col_name].tolist()
zipcodes =[]
for i in excel_data.zipcode:
    if i not in zipcodes:
        zipcodes.append(i)

def get_geo_info(zipcode, API_KEY, return_full_response=False):

    init_url = "some_url"
    if API_KEY is not None:
        url = init_url+API_KEY+"/info.json/"+format(zipcode)+"/degrees"

    # Ping site for the reuslts:
    r= requests.get(url)
    if r.status_code != 200:
        #this will print error code to but in reuslt set it will be empty
        print("error is" + str(r.status_code))
    data = r.json()
    output = {
            "zipcode" : data.get('zip_code'),
        "lat" : data.get('lat'),
         "lng" : data.get('lng'),
        "city" : data.get('city'),
        "state" : data.get('state')

            }

    return output

def limited(until):
    duration = int(round(until - time.time()))
    print('Rate limited, sleeping for {:d} seconds'.format(duration))

results = []

### Putting max call to 49 for 1 hour, we can paramterized it also
rate_limiter = RateLimiter(max_calls=49, period=3600, callback=limited)

for zipcode in zipcodes:
    with rate_limiter:
        geocode_result = get_geo_info(zipcode, API_KEY, return_full_response=RETURN_FULL_RESULTS)
        results.append(geocode_result)
#print(results)
ddf = pd.DataFrame(results)
### This is to put the geo_info into same file however I would prefer it to write it to antoher file load it int staging table. to keep it for future refrence purposes.
""""df = pd.read_csv("Downloads/test_order.csv")
df['state'] = ddf.state
df['city'] = ddf.city
df['lattitude'] = ddf.lat
df['Longitude'] = ddf.lng
df['api_zip'] = ddf.zipcode
df.to_csv('Downloads/test_order.csv',index = False)"""
### This is to create a new file with the results
ddf.to_csv('Downloads/geo_info.csv',index = False)

Let's start with the zipcode data imported from the CSV file. This should probably be contained in its own method:

def get_zipcodes(zip_code_col_name, file_name="Downloads/test_order.csv"):
    excel_data = pd.read_csv(file_name, encoding='utf8')
    if zip_code_col_name not in excel_data.columns:
        raise ValueError("Missing zipcode column")
    return excel_data[zip_code_col_name].unique().tolist()

Note that pandas.Series objects have a unique method that avoids your expensive loop (the in lookup is \$\mathcal{O}(n)\$ for lists). If that was not the case you could have used set(excel_data[zip_code_col_name].tolist()), instead. The tolist is not strictly needed, since a pandas.Series is also iterable (and that is all you do with it).

Your main code should also be in a function (called main, if you can think of nothing else):

def main():
    # Putting max call to 49 for 1 hour
    get_geo_info_limited = RateLimiter(max_calls=49, period=3600, callback=limited)(get_geo_info)

    results = [get_geo_info_limited(zipcode, API_KEY, RETURN_FULL_RESULTS) for zipcode in zipcodes]
    df = pd.DataFrame(results)
    df.to_csv('Downloads/geo_info.csv', index=False)

Here I used the fact that RateLimiter can also be used as a decorator (which is usually done using the @RateLimiter syntax during function definition, but can also be done manually like here) and a list comprehension instead of a manual for loop.

You can then call this main function under a if __name__ == "__main__": guard to enable importing from this module from another script.


Class for Switch statements This is more to get clarity on an implementation that I did. I am working on a React, Node, and Electron application that essentially has a form that a user inputs values that will update some content files.

The application has to scale for a lot of different scenarios so I found that I kept writing switch statements to incorporate all of these scenarios. So for the sake of time I wrote a class that:

Takes in two arrays, An array of keys and array of functions that correspond to those keys.
The constructor function creates a table for the values
Then there is a method on the class that allows you to look up that value and returns it

here is the code

Array.prototype.createObjectFromKeysandValues = function(array) {
    let table = {}
    for(let pointer=0;pointer<this.length;pointer++){
        let key = this[pointer]
        let value = array[pointer]
        table[key]=value
    }
    return table
}

  module.exports = class KeyFunctionMappingTable {
      constructor(keys, values){
        this.lookUpTable = Object.assign({},
         keys.createObjectFromKeysandValues(values),
         { default: () =>  new Error("Value Not Found")})
         }

     findHandlerFunction(value){
        return this.lookUpTable[value]|| this.lookUpTable.default
     }
   }

So when the code is implemented it looks like this ( this scenario is for React Components) :

const factory = new KeyFunctionMappingTable(['select', 'input'], [
    (options, labelText) => ( <Select options={options} labelText={labelText} />),
    (options, labelText) => ( <Input labelText={labelText} />)])

I am finding that I have about three of these "mappings" throughout the code.

What I want to know is if this seems like a readable implementation and does this make sense to strangers. If you dont think so please let me know your suggestion.

You have just added an unneeded layer of complexity to the problem.

You can just create the factory as an Object

const factory = {
    select(opts, text) { return (<select options={opts} labelText={text} />) },
    input(opts, text) { return (<input labelText={text} />) },
    default() { return new Error("Value Not Found") }
};

Then call the function with

(factory[value] || factory.default)(opts, text);


Replacing every letter in a string with the letter following it in the alphabet I am working through some of the exercises at Coberbyte. The instructions for this exercise are to replace every letter in a string with the letter following it in the alphabet (i.e., c becomes d, z becomes a), then capitalize every vowel in this new string (a, e, i, o, u) and finally return this modified string.

Is there a better way to advance characters alphabetically than converting to ASCII, adding one, and converting back?
Is there a way to do this with one loop?
In general, are there ways to shorten/improve the code?

var txt = prompt("Please enter some text: ");
var newTxt = "";
var newTxt2= "";

for (var i = 0; i<txt.length; i++){
  if (txt.charAt(i).contains(" ")){
    newTxt = newTxt.concat(" ");//checks for spaces
  }else if (txt.charAt(i) === "z"){
    newTxt = newTxt.concat("a");//checks for "z" and sets to "a"
  }else{
    newTxt = newTxt.concat(String.fromCharCode(txt.charAt(i).charCodeAt(0)+1));
  }//converts letter to the next letter in the alphabet
}

// checks for vowels and sets to upper case
for (var i=0; i < newTxt.length; i++){
  switch(newTxt.charAt(i)){
    case "a":
    case "e":
    case "i":
    case "o":
    case "u":
    newTxt2 = newTxt2 + (newTxt.charAt(i).toUpperCase());    break;
    default:
    newTxt2 = newTxt2 + newTxt.charAt(i);
    break;
 }
}

console.log(newTxt2);

First, I'll note that str.charAt(n).charCodeAt(0) is better written as str.charCodeAt(n).

Strings are immutable, so it's better to work on array elements than to build them by character-by-character concatenation.

function transform(text) {
    var s = text.split('');
    for (var i = 0; i < s.length; i++) {
        // Caesar cipher
        switch(s[i]) {
          case ' ':
            break;
          case 'z':
            s[i] = 'a';
            break;
          case 'Z':     // One case you forgot to handle
            s[i] = 'A';
            break;
          default:
            s[i] = String.fromCharCode(1 + s[i].charCodeAt(0));
        }

        // Upper-case vowels
        switch(s[i]) {
          case 'a': case 'e': case 'i': case 'o': case 'u':
            s[i] = s[i].toUpperCase();
        }
    }
    return s.join('');
}

Edit note: My original answer had the for-loop written as for (var i in s) { ... }.  As pointed out by @EliasVanOotegem, that is more suited for iterating over object properties than array elements.  As suggested by @plalx and confirmed with jsperf, changing it to for (var i = 0; i < s.length; i++) { ... } makes it much faster.  I have therefore amended my response, changing for (var i in s) to for (var i = 0; i < s.length; i++).

Note that your solution and the solution above blindly increments every character except spaces.  If you want to increment just letters, then a replace operation would be more appropriate:

function transform(text) {
    var caesar = text.replace(/[a-z]/gi, function(c) {
        switch (c) {
          case 'z': return 'a';
          case 'Z': return 'A';
          default:  return String.fromCharCode(1 + c.charCodeAt(0));
        }
    });
    return caesar.replace(/[aeiou]+/g, function(vowel) {
        return vowel.toUpperCase();
    });
}

You could cheat on the Caesar cipher, mapping both z and Z to an uppercase A, since you know that vowels will need to be uppercased.

Addendum: Follow-Up Q&A

@Nathan asks in the comments…

  1) I keep reading that strings are immutable and I guess this means that in my code, I'm not really concatenating, I'm replacing. I take it this is bad but I'm not sure why. I understand how your array works, just not why it is better.

If a and b are strings, when you do var c = a + b, then it creates a new string for the result (allocates memory for the length of a and b, then copies the contents of a, followed by the contents of b).  If you iteratively do a concatenation for each character in a string, that works out to be O(n^2), where n is the length of the input.

In contrast, operating on an array of characters lets you modify the elements in place.

  2) Both your solutions were functions. Is this so that they can be reused? The code is more self-contained and complete?

Even if you call the code just once, putting it in a function is beneficial, because it…
- lets you give your code a name.  Naming is important because it describes the purpose of the code.  Good naming is a form of self-documentation.  Awkward naming is a sign of muddled thinking.
- compartmentalizes the code into small, understandable chunks.  In JavaScript, variables can be scoped by function; without functions, all variables would be in the global scope, and assigning to a global variable can have side effects anywhere in your code.

Admittedly, transform(text) is not a very descriptive name, the way caesarCipher(text) is.  However, the problem was a bit weird and artificial to begin with.  Anyway, it's worthwhile to make it a function, to separate it from the prompting and outputting code.  (Someday you might choose to use the same text transformation, but taking the text from an HTML <input> field and outputting the result to a <span>.

  3) How does the function get it's argument (c)?

That's how the String.replace() function behaves.  If you pass a function as the second argument to replace(), then for every match, that function will be called with the matched substring as its first parameter.


Looking for a pattern in a list I've created this function that looks for sequences in a list. If [1,2,3] exists, it will print True. I've tested the code and it works fine.

def looking_patt(pattern,cursor,found):
    big_s = []
    for i in range(1,101):
        if i % 10 == 0:
            big_s.extend([1,2,3])
        else:
            big_s.append(i)
    # check control
    print(big_s)

    for i in big_s:
        if i == pattern[cursor]:
            cursor += 1
            if cursor == len(pattern):
                found.append(pattern)
                print(found)
                cursor = 0
        else:
            cursor = 0

    print(len(found) > 0)

Part of the script (the for i in big_s:) has been taken from an answer in this SO question.

How can it be improved?

I guess that the first part of your code is just to initialize the sample data.

Then here are a few things:

Separate initialization and main loop. I know this is just an exercise, but always doing it will help you keep the mindset.
If you only need to count something, just count it. Don't keep track of everything.
If you only need to check the existence of something, use a boolean value and return as soon as you've found it. Adding elements to a list so that at the end you can count them and return a boolean based on the count is definitely not an efficient way to do it.
What you call cursor is actually an integer, not an object, so the name index may be more appropriate
If you want to give the chance to start searching from a specific index, you may also want to have a default value for that parameter (0 looks like a good choice)
You don't need the third parameter, that's what you are going to return

This is my attempt at it:

big_s = []

def init_big_s():
    for i in range(1,101):
        if i % 10 == 0:
            big_s.extend([1,2,3])
        else:
            big_s.append(i)

def looking_patt(pattern, index=0):
    for i in big_s:
        if i == pattern[index]:
            index += 1
            if index == len(pattern):
                return True
        else:
            index = 0
    return False

init_big_s()
print looking_patt([1, 2, 3])