Removing certain items from list I have to co-operate with third party APIs from where I get a list and I have to remove certain elements before passing it toward.
I have a working solution but I am opened for every suggestion to make it better or more reliable (assuming the third party API maybe will change a bit in the future):

public List<Column> remove(List<Column> columns){
    for(Iterator<Column> iterator = columns.listIterator(); iterator.hasNext();     ){
     Column column = iterator.next();
     try{
      switch(column.getValue()){
        case "FIRST":
        case "SECOND":
        case "THIRD":
          iterator.remove();
      }
     }catch(NullPointerException e){}
    }
    return columns;
}

Edit

Yes, it is not my production code. My production code is something like this:

private static final String FIRST = "first to remove";

private static final String SECOND = "second to remove";

private static final String THIRD = "third to remove";

...lots of code here

List<Column> columns = ThirdPartyLibrary.getColumns();
for(Iterator<Column> iterator = columns.listIterator(); iterator.hasNext();     ){
     Column column = iterator.next();
     try{
      switch(column.getValue()){
        case FIRST:
        case SECOND:
        case THIRD:
          iterator.remove();
      }
     }catch(NullPointerException e){}
}
working with columns

I made the try and catch methods for the case if I get a column from the third party which getValue() returns null.

What's the point of catching a NullPointerException? When do you expect it to happen?
I don't really like that this method mutates the list passed in as a parameter - and then also returns it, even though the returned value is actually redundant as the calling code already has a hold of the now modified list. You're kind of sharing the result of the operation in two ways at once, which feels redundant.

And I know it's often done that way, but I think it's bad practice nevertheless, and can lead to confusion.

Personally I'd prefer to be more explicit about what's going on, and either a) make a deep copy of the original list, and return an independent version of it (with entries removed), without affecting the original one - or b) make this method void, like Collections.sort().

See command-query separation principle. Making a method look like a query can lead me to believe it's a pure function, with no side effects, whereas it actually does cause side effects, mutating the input data.
The name is very vague - "remove", remove what?
Is the blacklisted data ("FIRST", "SECOND", "THIRD") likely to ever change? In the spirit of making this code more modular I would consider extracting that list to a separate parameter.

Single responsibility principle indicates that a method should have one clear responsibility, and one could argue that filtering out certain entries and knowing the blacklist are two responsibilities. Your mileage may vary though, depending on the cnotext - which I don't know.
Not a biggie, but whitespace is out of control (iterator.hasNext();     ), catch(NUllPointerException e){}), not to mention it's NullPointer..., not NUllPointer.
Depending on how Column class is implemented, which I can't see - if it's a light-weight object and if its equals() method would compare Column instances by their value (getValue())... then we could leverage Collections.removeAll and save ourselves writing the iterative removal by hand.

Eg. assuming

class Column {
    private final String value;

    public Column(String value) {
        this.value = value;
    }

    public String getValue() {
        return value;
    }

    @Override
    public boolean equals(Object o) {
        // if a Column instance, compare it by value
    }

    // don't forget hashCode
}

we could just do that:

/* could be a Set actually since order doesn't matter,
 * only Set hasn't got such a convenient initializer in standard Java API */
private static final Collection<Column> BLACKLISTED_COLUMNS = Arrays.asList(
    new Column("FIRST"),
    new Column("SECOND"),
    new Column("THIRD"));

be it a constant, or a collection computed in run-time, doesn't really matter. And then it gets as simple as that:

List<Column> columns = // ... getting the data from wherever
columns.removeAll(BLACKLISTED_COLUMNS);

And that's it, job done. As I said, this solution may not be applicable in your case - I have no way of knowing.


Adding all IDs associated with a Fax to one Row I found a custom function called UniqueItems() to use and then developed my own RemoveDups() and FaxesToUse() code:

Function UniqueItems(ArrayIn, Optional Count As Variant) As Variant
'   Accepts an array or range as input
'   If Count = True or is missing, the function returns the number of unique elements
'   If Count = False, the function returns a variant array of unique elements
    Dim Unique() As Variant ' array that holds the unique items
    Dim Element As Variant
    Dim i As Integer
    Dim FoundMatch As Boolean
'   If 2nd argument is missing, assign default value
    If IsMissing(Count) Then Count = True
'   Counter for number of unique elements
    NumUnique = 0
'   Loop thru the input array
    For Each Element In ArrayIn
        FoundMatch = False
'       Has item been added yet?
        For i = 1 To NumUnique
            If Element = Unique(i) Then
                FoundMatch = True
                Exit For '(exit loop)
            End If
        Next i
AddItem:
'       If not in list, add the item to unique list
        If Not FoundMatch And Not IsEmpty(Element) Then
            NumUnique = NumUnique + 1
            ReDim Preserve Unique(NumUnique)
            Unique(NumUnique) = Element
        End If
    Next Element
'   Assign a value to the function
    If Count Then UniqueItems = NumUnique Else UniqueItems = Unique
End Function

Here is the sub you need to find your Unique IDs and copy them over to the sheet "Use Me":

Sub FaxesToUse()
    Dim LastRow As Long, CurRow As Long, UniqueTotal As Long, SubTotal As Long

    UniqueTotal = InputBox("How Many Unique OtherIDs is Max?")
    If Not UniqueTotal > 0 Then
        Exit Sub
    End If

    LastRow = Range("A" & Rows.Count).End(xlUp).Row
    SubTotal = 0
    For CurRow = 2 To LastRow
        SubTotal = UniqueItems(Range("A2:A" & CurRow))
        If SubTotal > UniqueTotal Then
            SubTotal = UniqueItems(Range("A2:A" & CurRow - 1))
            Range("A1:B" & CurRow - 1).Copy
            Sheets("Use Me").Cells.Clear
            Sheets("Use Me").Range("A1").PasteSpecial xlPasteValues
            Sheets("Use Me").Activate
            MsgBox "Use Me Sheet rows contain " & SubTotal & " Unique OtherIDs"
            Exit Sub
        End If
        Cells(CurRow, 1).EntireRow.Interior.Color = RGB(255, 255, 0)
    Next CurRow

End Sub

That will get you a page that looks like this:

Now we just need to remove all the duplicate Faxes using this macro:

Sub RemoveDups()

Dim CurRow As Long, LastRow As Long, LastCol As Long, DestLast As Long, DestRng As Range, ws As Worksheet

Set ws = Sheets("Use Me")
LastRow = ws.Range("A" & Rows.Count).End(xlUp).Row

For CurRow = LastRow To 3 Step -1
     Set DestRng = ws.Range("B2:B" & CurRow - 1).Find(ws.Range("B" & CurRow).Value, LookIn:=xlValues, LookAt:=xlWhole, SearchDirection:=xlNext)
     If DestRng Is Nothing Then
         'Do Nothing
     Else
        DestLast = ws.Cells(DestRng.Row, Columns.Count).End(xlToLeft).Column + 1
        ws.Cells(DestRng.Row, DestLast).Value = ws.Cells(CurRow, 1).Value
        ws.Cells(CurRow, 1).EntireRow.Delete xlShiftUp
     End If
     Next CurRow
ws.Columns("B:B").Cut
ws.Columns("A:A").Insert Shift:=xlToRight
Application.CutCopyMode = False

LastRow = ws.Range("A" & Rows.Count).End(xlUp).Row
LastCol = 0
For CurRow = 2 To LastRow
    If ws.Cells(CurRow, Columns.Count).End(xlToLeft).Column > LastCol Then
        LastCol = ws.Cells(CurRow, Columns.Count).End(xlToLeft).Column
    End If
Next CurRow

MsgBox "Use Me Sheet Rows contain " & UniqueItems(ws.Range(Cells(2, 2), Cells(LastRow, LastCol))) & " Unique OtherIDs"

End Sub

It leaves you with this:

I'm interested in cleaning this up and making it more efficient for larger data sets.

You could just use a dictionary to identify unique values, then use the dictionary to identify the matches and print them to the new sheet. This also eliminates the need for user input of "How many unique items max".
Doing it this way won't require as much coding, or calling any functions. In fact, it does it in one fell swoop, which should speed it up when there's a large amount of data. You could use the dictionary to pull the items into an array and then print the array, but I'm not certain it would speed up significantly more on this solution.
The trick to the dictionary is that it can't have duplicate keys - so it's doing your entire uniqueitems() in a simple loop.
In faxestouse you're copying and pasting - instead just define the worksheets and set the values without any need to use .activate - this is much cleaner.
Sub CombineOtherID()
Dim dict As Object
Set dict = CreateObject("Scripting.Dictionary")

Dim lastrow As Integer
lastrow = Cells(Rows.Count, "A").End(xlUp).Row

Dim strA As String

'Will use each fax number only once as a dictionary key
For i = 2 To lastrow
    strA = Cells(i, 2)
    dict(strA) = 1
Next

Dim countkey As Integer
countkey = 2
Dim countcol As Integer
Dim wsorigin As Worksheet
Set wsorigin = ActiveSheet
Dim wstarget As Worksheet
Set wstarget = Sheets("Use Me")

wstarget.Range("A1") = "Faxes"
wstarget.Range("B1") = "Other IDs"

'Use the keys to populate the target sheet
For Each Key In dict.keys
    wstarget.Cells(countkey, 1) = Key
    countkey = countkey + 1
    countcol = 2
        For j = 2 To lastrow
            If wsorigin.Cells(j, 2) Like Key Then
                wstarget.Cells(countkey - 1, countcol) = wsorigin.Cells(j, 1)
                countcol = countcol + 1
            End If
        Next
Next
End Sub

Otherwise
Now, if I were to go through your code there's a few things I'd note.
Your inputbox isn't handling any errors. I'd make sure it specifies the numeric type -
UniqueTotal = Application.InputBox("How Many Unique OtherIDs is Max?", Type:=1)

I'm a bit fuzzy how UniqueItems returns to subtotal. If it's a single integer, I guess it would work as a pretty inefficient loop, but as a variant - it should fail:
Next you call the UniqueItems() function which can return the number of uniques or an array of uniques. I'm not confident your matching algorithm is the most efficient - instead I'd use the dictionary object. It will give you the array or if you want the count you can use the .count method to get that.
If you return the count, then If SubTotal > UniqueTotal will execute no problem as subtotal is long. If the UniqueItems returns a variant (array), you will get type mismatch when you set SubTotal = UniqueItems - so I'm not sure what your goal here is - you pass an array to a long to check if your function returns a value greater than user input - is this to check to see if the list is already only uniques? There must be a better way to do that.
Then you're going to use .copy and .paste with .activate to fill in your sheet, but again you call UniqueItems as True so you'll get numunique - which doesn't have a type but would work as an integer. However, if you call it with False you'll get an array passed to a long which would return a type mismatch.
As far as the inefficient loop goes -
For CurRow = 2 To LastRow
        SubTotal = UniqueItems(Range("A2:A" & CurRow))
        If SubTotal > UniqueTotal Then
            SubTotal = UniqueItems(Range("A2:A" & CurRow - 1))
            Range("A1:B" & CurRow - 1).Copy
            Sheets("Use Me").Cells.Clear
            Sheets("Use Me").Range("A1").PasteSpecial xlPasteValues
            Sheets("Use Me").Activate
            MsgBox "Use Me Sheet rows contain " & SubTotal & " Unique OtherIDs"
            Exit Sub
        End If
        Cells(CurRow, 1).EntireRow.Interior.Color = RGB(255, 255, 0)
    Next CurRow

You will loop through everything until you count larger than the user input and then copy and paste the range - how does this work? Won't it just return the x number of rows, indiscriminately?  You might have an incomplete data set at this point on your new sheet.
I don't quite understand the RemoveDups() sub. I think it looks for duplicates in column B because you haven't reversed the array and column B is still Fax Numbers. But after that, I'm not sure how you're getting anywhere close to your output.


Create a weather app in Python I've created the following app in Python 3.5. The app uses pyowm and will take a location and return weather information. I've also built a GUI with tkinter for it.

Keen to find out some tips to improve my coding style.

import tkinter as tk
import pyowm
import datetime, time

class WeatherInfo(tk.Tk):

    def __init__(self):
        tk.Tk.__init__(self)
        self.wm_title('Weather')
        self.temp = tk.StringVar(self,value='')
        self.humid = tk.StringVar(self,value='')
        self.status = tk.StringVar(self,value='')
        self.sunrise = tk.StringVar(self,value='')
        self.sunset = tk.StringVar(self,value='')
        self.ld = tk.StringVar(self,value = '')
        self.ln = tk.StringVar(self,value = '')

        self.ask = tk.LabelFrame(self, text ='Location')
        self.ask.pack(fill='both',expand='yes')
        self.kw_label = tk.Label(self.ask, text ='Get weather in:')
        self.kw_label.pack(side = tk.LEFT)
        self.kw = tk.Entry(self.ask)
        self.kw.pack(side = tk.RIGHT)
        self.rb = tk.Button(self, text='Go', command = self.search)
        self.rb.pack()
        self.owm = pyowm.OWM('*CENSORED*')

        self.output = tk.LabelFrame(self, text ='Information')
        self.output.pack(fill='both',expand='yes')
        tk.Label(self.output, textvariable = self.temp).pack()
        tk.Label(self.output, textvariable=self.humid).pack()
        tk.Label(self.output, textvariable=self.status).pack()
        tk.Label(self.output, textvariable=self.sunrise).pack()
        tk.Label(self.output, textvariable=self.sunset).pack()
        tk.Label(self.output, textvariable=self.ld).pack()
        tk.Label(self.output, textvariable=self.ln).pack()
        button = tk.Button(master=self, text='Quit', command=self._quit)
        button.pack(side=tk.BOTTOM)

    def search(self):
        obs = self.owm.weather_at_place(self.kw.get())
        try:
            w = obs.get_weather()
            self.temp.set('Temperature: ' +str(round(w.get_temperature()['temp'] - 273.15,0))+ ' C')
            self.humid.set('Humidity: '+str(w.get_humidity())+ '%')
            self.status.set('Status: ' + w.get_status())
            self.sunrise.set('Sunrise at '+datetime.datetime.fromtimestamp(w.get_sunrise_time()).strftime('%H:%M:%S'))
            self.sunset.set('Sunset at '+datetime.datetime.fromtimestamp(w.get_sunset_time()).strftime('%H:%M:%S'))
            self.ld.set('Day length: '+str(round((w.get_sunset_time() - w.get_sunrise_time()) / 3600,1))+' h')
            self.ln.set('Night length: '+str(round(24 - (w.get_sunset_time() - w.get_sunrise_time()) / 3600,1))+' h')
        except:
            self.temp.set('Pick a city to display weather.')
    def _quit(self):
        self.quit()
        self.destroy()
app = WeatherInfo()
app.mainloop()

Your code has a few different responsibilities, most of which are currently in the search function:

Setting up the GUI
Retrieving the weather report
Parsing the relevant data out of that report
Displaying the result

Each of these deserves their own function. I would also abstract the format itself away into a constant of the class. This allows looping over the different keys to build the objects instead of having to do it manually. I used a collections.OrderedDict to preserve the order of them.

These template strings are:

class WeatherInfo(tk.Tk):
    templates = OrderedDict([
        ('temp', 'Temperature: {temp:.1f} C'),
        ('humid', 'Humidity: {humid}%'),
        ('status', 'Status: {status}'),
        ('sunrise', 'Sunrise at {sunrise:%H:%M:%S}'),
        ('sunset', 'Sunset at {sunset:%H:%M:%S}'),
        ('day_length', 'Day length: {day_length:.1f} h'),
        ('night_length', 'Night length: {night_length:.1f} h')])

Note that you can already define the format for the date in there and also the rounding for floats.

This allows doing this in __init__:

def __init__(self):
    ...
    self.tk_info = {key: tk.StringVar(
        self, value='') for key in WeatherInfo.templates}
    ...
    self.labels = []
    for key in WeatherInfo.templates:
        self.labels.append(
            tk.Label(self.output, textvariable=self.tk_info[key]).pack())
    ...

search should just be concerned with getting the result from the web, catching an error if needed. Here you should notice that you should never have a bare except. It will also catch e.g. the user pressing CTRL-C to abort the program. Always use at least except Exception which will catch almost everything (and not some special exceptions like CTRL-C). Here you can be more specific, because the code will throw a AttributeError if unsuccessful:

def search(self):
    obs = self.owm.weather_at_place(self.kw.get())
    try:
        return json.loads(obs.get_weather().to_JSON())
    except AttributeError:
        self.tk_info['temp'].set('Pick a city to display weather.')

Note that I am using the json format here, just because I think it is more intuitive to work with. But you can also use the get_* functions in parse.

Speaking of which: I would add a parse function that takes the result of obs.get_weather and does all the conversions/calculations. It returns a dictionary with the raw values we want to put in the template:

def parse(self, w):
    parsed_weather = {'temp': round(w['temperature']['temp'] - 273.15, 0),
                      'humid': w['humidity'],
                      'status': w['status'],
                      'sunrise': datetime.fromtimestamp(w['sunrise_time']),
                      'sunset': datetime.fromtimestamp(w['sunset_time']),
                      'day_length': round((w['sunset_time'] - w['sunrise_time']) / 3600, 1),
                      'night_length': round(24 - (w['sunset_time'] - w['sunrise_time']) / 3600, 1)}
    return parsed_weather

I also used from datetime import datetime in the header to get rid of one level of redundant naming.

Then I would define a update function, which updates all the labels to the correct wording. Here it comes in handy that we already have the template strings defined and have a dictionary of the values we want to put in there!

def update(self, report):
    for key, template in WeatherInfo.templates.items():
        self.tk_info[key].set(template.format(**report))

The whole thing comes together in the main function (for lack of a better word):

def main(self):
    report = self.search()
    if report:
        self.update(self.parse(report))

Of course, this main function has to be linked to the Go button instead of search now.

Finally, I would guard the execution of the app with a if __name__ == "__main__" clause to allow importing of your module without executing it.

Final code:

import tkinter as tk
import pyowm
import time
import json
from datetime import datetime
from collections import OrderedDict

class WeatherInfo(tk.Tk):
    templates = OrderedDict([
        ('temp', 'Temperature: {temp:.1f} C'),
        ('humid', 'Humidity: {humid}%'),
        ('status', 'Status: {status}'),
        ('sunrise', 'Sunrise at {sunrise:%H:%M:%S}'),
        ('sunset', 'Sunset at {sunset:%H:%M:%S}'),
        ('day_length', 'Day length: {day_length:.1f} h'),
        ('night_length', 'Night length: {night_length:.1f} h')])

    def __init__(self):
        tk.Tk.__init__(self)
        self.wm_title('Weather')
        self.tk_info = {key: tk.StringVar(
            self, value='') for key in WeatherInfo.templates}

        self.ask = tk.LabelFrame(self, text='Location')
        self.ask.pack(fill='both', expand='yes')
        self.kw_label = tk.Label(self.ask, text='City:')
        self.kw_label.pack(side=tk.LEFT)
        self.kw = tk.Entry(self.ask)
        self.kw.pack(side=tk.LEFT)
        self.rb = tk.Button(self.ask, text='Go', command=self.main)
        self.rb.pack(side=tk.RIGHT)
        self.owm = pyowm.OWM('*CENSORED*')

        self.output = tk.LabelFrame(self, text='Information')
        self.output.pack(fill='both', expand='yes')
        self.labels = []
        for key in WeatherInfo.templates:
            self.labels.append(
                tk.Label(self.output, textvariable=self.tk_info[key]).pack())
        button = tk.Button(master=self, text='Quit', command=self._quit)
        button.pack(side=tk.BOTTOM)

    def search(self):
        obs = self.owm.weather_at_place(self.kw.get())
        try:
            return json.loads(obs.get_weather().to_JSON())
        except AttributeError:
            self.tk_info['temp'].set('Pick a city to display weather.')

    def parse(self, w):
        parsed_weather = {'temp': w['temperature']['temp'] - 273.15,
                          'humid': w['humidity'],
                          'status': w['status'],
                          'sunrise': datetime.fromtimestamp(w['sunrise_time']),
                          'sunset': datetime.fromtimestamp(w['sunset_time']),
                          'day_length': (w['sunset_time'] - w['sunrise_time']) / 3600,
                          'night_length': 24 - (w['sunset_time'] - w['sunrise_time']) / 3600}
        return parsed_weather

    def update(self, report):
        for key, template in WeatherInfo.templates.items():
            self.tk_info[key].set(template.format(**report))

    def main(self):
        report = self.search()
        if report:
            self.update(self.parse(report))

    def _quit(self):
        self.quit()
        self.destroy()

if __name__ == "__main__":
    app = WeatherInfo()
    app.mainloop()


Riemann Sum Class (with variable endpoint behavior) Here is my class for calculating a Riemann Sum.  I did a cursory google search for a built-in function with these specifications but didn't find anything (I'm sure I just missed it or didn't look hard enough, but this is good practice).

Your input is welcome and appreciated.  I'd like to not have to assign a lambda function to a variable but it works for me.  I just worry that someone using this code may get confused.

from numpy import arange

class RiemannSum(object):

    def __init__(self, index, upper, rectangles, func, endpoints='mid'):
        """
        Constructor for base class for Riemann Sums.
        Will be subclassed for left, right, or mid-points for rectangles.

        :param index: Where to begin 'n' value. [n = index]
        :param upper: Upper bound of sum.  Max 'n' value.  [Number above sigma notation]
        :param rectangles: The number of rectangles to be used in the calculation.
        :param func: pass series function here; suggest assigning lambda function to variable and passing here
        :param endpoints: default to 'mid'. Valid values are 'mid', 'left', 'right' as strings
        """
        self.total = 0
        self.index = index
        self.upper = upper
        self.delta = (upper - index) / rectangles
        self.function = func

        if endpoints.lower() == 'left':  # left endpoints
            self.points = arange(self.index, self.upper, self.delta)
        elif endpoints.lower() == 'right':  # right endpoints
            self.points = arange(self.index + self.delta, self.upper + self.delta, self.delta)
        else:  # mid endpoints, usually most accurate
            diff = self.delta / 2
            self.points = [i for i in arange((self.index + diff),
                                         (self.upper + diff), self.delta)]

    def sum(self):
        outputs = [i for i in map(self.function, self.points)]
        self.total = sum(outputs)
        return self.delta * self.total

Here is an example of the code in use:

from math import sqrt
series = sqrt(x) - 2
num = RiemannSum(1,6,5,series,'mid').sum()
print(num)

If something is called "RiemannSum", then it should return a sum. If you want summation to be a separate method, a better term would be "partition"; you could do RiemannPartition.sum(), for instance.

Several other variable names are bit unintuitive to me (e.g. rectangles, index).

You can eliminate several lines by doing:

offset = {'left':0,'right':self.delta,'mid':self.delta/2}[self.endpoints.lower()]
self.points = arange(self.index+offset, self.upper+offset, self.delta)

General tip: if you're doing an if-then with several elif/else clauses, consider whether a dictionary would be simpler.


Initialize union with array? I want to create a datastructure for network communication (TCP),  where I can build up a byte array from small tagged pieces (like in serialized class). And of course I need a reverse behavior of this, I need a pieces from a byte array that is why a I put a constructor to the union.

typedef union message {

    struct {
        int header;
        int payload;
    } pieces;
    int whole[2];

    message (int* arr) {
        for (int i = 0; i < 2; i++) {
            whole[i] = arr[i];
        }
    }
    message ():iVal(){}
} Message ;

main() {
    int a[2] = {10, 2};
    Message msg(a);
}

This snippet is working. I'm just curious is there any other or better solution?

An old trick is to cast a pointer to an array of bytes to an pointer of a struct (POD) with the exact memory layout of the final message(s).

struct Message {
    int header;
    int payload;
};

// parsing bytes -> messages
char buffer[SIZE];
recv(buffer, 0, SIZE); // replace with actual code for receiving
Message *messages = reinterpret_cast<Message *>(buffer);
size_t max_message_count = SIZE * sizeof(char) / sizeof(Message);

// converting messages -> byte array
Message messages[MSG_COUNT];
// initialize messages
char *bytes = reinterpret_cast<char *>(messages);
size_t byte_count = MSG_COUNT * sizeof(Message) / sizeof(char);

However, this requires a strict memory mapping between the message struct and the serialized byte format (so Message in this case has to be a POD struct). Yes, this effectively bypasses the type system, but if used properly its the fastest way of parsing messages (and in case of embedded systems the reuse of the memory is a nice touch).


Does this code avoid SQL injection? I have the following PHP code but I'm unsure, based on the many things I've read, whether or not this is actually safe from an SQL injection attack.

$mysqli = new mysqli("address.address.address", "username", "password", "database");

/* check connection */
if (mysqli_connect_errno()) {
    printf("Connect failed: %s\n", mysqli_connect_error());
    exit();
}

$stmt = $mysqli->prepare('INSERT INTO tablename (value) VALUES (?)');
$stmt->bind_param("s", $val);

$val = $_GET['val'];
$stmt->execute();

$stmt = $mysqli->prepare('SELECT * FROM tablename WHERE value = ?');
$stmt->bind_param('s', $val);

$val = $_GET['val'];

$stmt->execute();

$result = $stmt->get_result();
while ($row = $result->fetch_assoc()) {
    echo $row['id'].'<br />';
}

$stmt->close();

$mysqli->close();

Can anyone offer their opinion on how to improve this, please?

It looks safe from SQL Injection, your binding and preparing is fine.

This is how I would do it, using the PHP code found here in the manual.

In fact, these calls would benefit from being in their own functions; however this would work the same.

<?php
$mysqli = new mysqli("localhost", "my_user", "my_password", "world");

/* check connection */
if (mysqli_connect_errno()) {
    printf("Connect failed: %s\n", mysqli_connect_error());
    exit();
}

$val = $_GET['val'];

/* create a prepared statement */
if ($stmt = $mysqli->prepare("INSERT INTO tablename (value) VALUES (?)")) {

    /* bind parameters for markers */
    $stmt->bind_param("s", $val);

    /* execute query */
    if($stmt->execute()) {
       echo 'INSERTED';
    }
    /* close statement */
    $stmt->close();
}

if ($stmt = $mysqli->prepare("SELECT * FROM tablename WHERE value = ?")) {

    /* bind parameters for markers */
    $stmt->bind_param("s", $val);

    /* execute query */
    if($stmt->execute()) {
        $result = $stmt->get_result();
        while ($row = $result->fetch_assoc()) {
            echo $row['id'].'<br />';
        }
    }

    /* close statement */
    $stmt->close();
}

/* close connection */
$mysqli->close();
?>


Functions for setting cookies and outputting their IDs Below are two functions.

The first is to set a cookie. This function is called on any product page on my website.

function setcookie() {

    $entry_id = $this->EE->TMPL->fetch_param('entry_id');

    if (isset($_COOKIE['recently_viewed'])) {

        $currentSession = unserialize($_COOKIE['recently_viewed']);

        if (!in_array($entry_id, $currentSession)) {

            if (count($currentSession) > 5) {
                unset($currentSession[0]);
            }

            $currentSession[] = $entry_id;

        } else {}

        $currentSession = serialize($currentSession);
        setcookie('recently_viewed', $currentSession, pow(2,31)-1, '/', '');

    } else {

        $recently_viewed[] = $entry_id;
        $currentSession = serialize($recently_viewed);
        setcookie('recently_viewed', $currentSession, pow(2,31)-1, '/', '');

    }

}

The second function retrieves the IDs in the cookie array and outputs them.

function recently_viewed() {

$tagdata = $this->EE->TMPL->tagdata;

if ( array_key_exists('recently_viewed', $_COOKIE) ) {
    $recent = unserialize($_COOKIE['recently_viewed']);
} else {
    $recent = NULL;
}

if ($recent) {
    $entry_ids = implode('|', $recent);
} else {
    $entry_ids = NULL;
}

$data['entry_ids'] = $entry_ids;

$variables = array();
$variables[] = $data;

return $this->EE->TMPL->parse_variables($tagdata, $variables);

}

The code is built for the content management system Expression Engine. If you don't know 'codeigniter' code, just ignore those bits and take a look at the straight PHP if possible.

Can anyone tell me if there is any way to speed the functions up?

The first function, setcookie, is basically checking to see if a cookie exists.  If it does, it checks if the current entry id is in the array already.  If it's not, then it checks if there are more than 5 items in the array.  If there is, it deletes the first one and adds the new entry id. If no cookie exists, it creates a new cookie and adds the entry id.

The second function (recently viewed) is checks the cookie, converts it to a string with pipelines between entry ids, then outputs this string.

First of all, let me tell you that I have zero experience with Codeigniter, but I suppose it is an MVC / OOP based framework, and I have plenty of experience with those. I am not trying to declare myself an expert or anything, just trying to be helpful and constructive. So here we go...

A few general words of advice:
 - try to avoid nesting control structures. Work with early returns in stead. You hardly ever need to nest an if inside an if, or even use an else. If you realy do, you are probably not following the principles of single responsibility
 - don't waste resources by storing stuff in variables and only using them once immediately after you stored them. Use the value you wanted to store directly.
 - Put comments inside your code. A lot of them! It may take a little bit longer now, but it will safe you time when you ever have to debug or alter your code!

With those hints in mind, I would write your first function as follows:

function setcookie() {
    // prepare the entry id
    $entry_id = $this->EE->TMPL->fetch_param('entry_id');

    // if cookie has not been set before
    if (! isset($_COOKIE['recently_viewed'])) {
        // write the current entry id to a cookie
        setcookie('recently_viewed', serialize(array($entry_id)), pow(2,31)-1, '/', '');
        // and we are done (return self for chaining)
        return $this;
    }

    // convert the cookie to an array
    $currentSession = unserialize($_COOKIE['recently_viewed']);

    // if the current entry id is already in the cookie
    if (in_array($entry_id, $currentSession)) {
        // we are done (return self for chaining)
        return $this;
    }

    // if there are already 5 entries
    if (count($currentSession) > 5) {
        // shift of the first entry
        // don't use an index here, cause there is no garantee the first element actualy has index 0
        array_shift($currentSession);
    }

    // push the current id onto the list
    $currentSession[] = $entry_id;
    // write it to a cookie
    setcookie('recently_viewed',serialize($currentSession), pow(2,31)-1, '/', '');
}

And the second function:

function recently_viewed() {
    // prepare the tagdata
    $tagdata = $this->EE->TMPL->tagdata;

    // if no recently_viewed cookie is present
    // (isset is supposed to be a little better in performance then array_key_exists)
    if (! isset($_COOKIE['recently_viewed']) ) {
        // do something and return the result
        return $this->EE->TMPL->parse_variables(
            $tagdata,
            array(
                array('entry_ids' => null)
            )
        );
    }

    // convert the recently viewed ids to a piped string
    // do something with it and return the result
    return $this->EE->TMPL->parse_variables(
        $tagdata,
        array(
            array(
                'entry_ids' => implode('|', unserialize($_COOKIE['recently_viewed']))
            )
        )
    );
}

You could even put that function into a single 'line' of code, which should be a bit more performant, but a lot less readable. And imo you should always take readable code over performant code when the difference is small. Just as an example though:

function recently_viewed() {
    return $this->EE->TMPL->parse_variables(
        $this->EE->TMPL->tagdata,
        array(
            array(
                'entry_ids' =>
                    isset($_COOKIE['recently_viewed'])
                    ? implode('|', unserialize($_COOKIE['recently_viewed']))
                    : null
            )
        )
    );
}


Shorten code that gathers files from directories I have the following method in Ruby that appears to be just one line too long according to Ruby style guides. (Specifically: Rubocop tells me that my method has too many lines.)

Granted, I'm only one over the apparent allowed "limit" but in the interests of seeing if the style guides are realistic, I'm trying to adhere to them. Here is the method:

def spec_context
  spec_repo.map do |path|
    files_to_sort = []

    if File.directory?(path)
      spec_type.each do |type|
        files_to_sort << Dir["#{path}/**/*.#{type}"].sort
      end

      files_to_sort
    else
      []
    end
  end.flatten.uniq
end

The method essentially looks for any paths passed in, checks if there are files in those paths, and, if so, adds those files to an array (files_to_sort). If the path provided does not exist, an empty array is returned.

This is about as good as I can get this method. What I don't see is any way that I can shorten that to the Ruby style guidelines (10 lines) and still keep the contents nicely expressive.

Responding to a comment below, I wasn't clear at all about what spec_repo and spec_type are. Those are methods that get called which currently just return arrays:

def spec_repo
  %w(specs features stories)
end

def spec_type
  %w(spec feature story)
end

(I say "currently" because eventually the logic in those methods will be reading from a configuration setting.)

So the idea is to look over each directory that is listed as a repo and then, for each such directory, loop over each possible file type. (The file extensions being .spec, .feature, or .story.) If any files of the spec_type are found in any of the spec_repo directories, those are stored in the files_to_sort.

Unless I'm misinterpreting your code, you should be able to get by with something like this:

def spec_context
  directories = spec_repo.select { |path| File.directory?(path) }
  files = directories.map do |dir|
    spec_type.map { |type| Dir["#{dir}/**/*.#{type}"].sort }
  end
  files.flatten.uniq
end

That should be well within the line-limit (you could actually write that in one line, but that'd be kinda' annoying to read).

Point is, we filter to get the directories first, rather than in an if...else, and we map rather than push stuff to an array.

By the way, the title of question is a little confusing. Yes, the code "sorts file names", but only within their directory structure. Perhaps you want to put that sort at the very end, after the flatten, when you got all the files in a single array?
Also, as far as I can tell, the call to uniq isn't actually necessary; the files you'll find will either be in different directories or have different types, so there shouldn't be any repeats. But, if the addition of user configuration might cause repeats, I'd suggest handling that filtering elsewhere, so spec_repo and spec_type are sure to always return arrays with unique items only. Again, you should be able to get rid of the call to uniq here. Perhaps something similar could be done to make sure the spec_repo array only contains directories (assuming that's all it's supposed to contain) so we avoid doing that filtering in the method above.

Speaking of those methods, I'd suggest plural names for those two methods, since they return arrays. Similarly, the name spec_context doesn't quite communicate that the method returns an array of file names. It might be OK within the jargon of your code, but taken out of context, I might prefer something like spec_files as it's slightly more descriptive.

You also seem to be prefixing all your methods (the ones shown here, at least) with spec_. You might consider creating another class for this logic, since prefixing method names is indicative of methods that should be separately encapsulated.

Responding to the questions posed in the comments:

re method names: No, a method name doesn't need to follow any conventions or declare at its return type. In fact, if you overdo it, the code just becomes weirdly stilted. E.g. a method that returns a path ought to just be called path, not path_string - it's just redundant. Besides, Ruby doesn't have strict types, so trying to pseudo-declare types is going against the grain.

But hinting at what something is is very useful, and helps self-document the code. For example, I usually use plural for array variables or methods that return array. Also, somewhat particular to Ruby, a method call without arguments looks like any other variable, since we don't need to add () at the end. For instance, a line like directories.map ... looks the same whether directories is a local variable or a method. So if you were to refactor the code above and extract the first line into a separate method, you might as well call that method directories, since, hey, the name's worked well so far and it's pretty descriptive:

def directories # same name, but now a method
  spec_repo.select { |path| File.directory?(path) }
end

def spec_context # besides removing a line, nothing needs to change here. Yay!
  files = directories.map do |dir| # looks just like before
    spec_type.map { |type| Dir["#{dir}/**/*.#{type}"].sort }
  end
  files.flatten.uniq
end

You could go with directory_paths to further indicate that it's paths we're talking about, but... meh, not necessary. The simpler the better in my opinion. That said, it's always a judgment call.

You certainly shouldn't go for something like setup_spec_context; if it's already in a module called Setup, it's totally redundant to repeat that in the individual method names. For instance, the uniq method isn't called array_uniq, because, well, we know it's an array already.

re the word context: From the code and what you're describing, it sounds like a "context" in your case is actually more than just a list of files. It's also the directories to search, and the file types to search for (if not more). So you may want something like a Context class. Then you could have two instances of that class, spec and execution, each containing all the relevant stuff (spec.files, spec.types, spec.directories, etc.). Basic OOP encapsulation; modelling the notion of a "context".

If you find yourself prefixing the names of a bunch of methods, what you actually want is likely a class to contain those methods, so they're properly encapsulated.


Sum of switch-triggered integers in VHDL I've implemented a VHDL program that sums up the positions of switches and shows it on a 7-segment LED, but I've had to use lots of temporaries and a division. Is there a better way?

library ieee;
use ieee.std_logic_1164.all;

entity light is
  port(
        LEDR : out std_logic_vector(0 to 9);
        LEDG : out std_logic_vector(0 to 7);
        SW : in std_logic_vector(0 to 9);
        KEY : in std_logic_vector(0 to 3);
        HEX0 : out std_logic_vector(0 to 6);
        HEX1 : out std_logic_vector(0 to 6);
        HEX2 : out std_logic_vector(0 to 6);
        HEX3 : out std_logic_vector(0 to 6)
  );
end light;

architecture arch of light is

signal sum, sum2 : integer := 0;
signal x1,x2,x3,x4,x5,x6,x7,x8,x9 : integer := 0;

function led_value(x:integer)
    return std_logic_vector is
begin
    case x is
       when 0 => return "0000001";
        when 1 => return "1001111";
        when 2 => return "0010010";
        when 3 => return "0000110";
        when 4 => return "1001100";
        when 5 => return "0100100";
        when 6 => return "0100000";
        when 7 => return "0001111";
        when 8 => return "0000000";
        when 9 => return "0000100";
        when others => return "1111111";
    end case;
end led_value;

begin
    x1 <= 1 when sw(1) = '1' else 0;
    x2 <= 2 when sw(2) = '1' else 0;
    x3 <= 3 when sw(3) = '1' else 0;
    x4 <= 4 when sw(4) = '1' else 0;
    x5 <= 5 when sw(5) = '1' else 0;
    x6 <= 6 when sw(6) = '1' else 0;
    x7 <= 7 when sw(7) = '1' else 0;
    x8 <= 8 when sw(8) = '1' else 0;
    x9 <= 9 when sw(9) = '1' else 0;

    sum <= x1 + x2 + x3 + x4 + x5 + x6 + x7 + x8 + x9;
    sum2 <= (sum - sum mod 10) / 10;

    hex0 <= led_value(sum mod 10);
    hex1 <= led_value(sum2);
    hex2 <= led_value(11);
    hex3 <= led_value(11);
end arch;

This is some old question, but lets answer it.

Synthesis software is not that dumb. It will evaluate the potential outcomes of a function and generate reduced logic from that. Often you have to aid the synthesis software somewhat. You could do that by writing a function which internally resolves the things you want.

N.B. constraining the range of your integers keep the resource usage low. Else everything will be implemented as a 32-bit value

library ieee;
use ieee.std_logic_1164.all;

entity light is
  port(
        SW : in std_logic_vector(0 to 9); -- why 'to' and not 'downto'??
        KEY : in std_logic_vector(0 to 3); -- downto is normally used
        HEX0 : out std_logic_vector(0 to 6);
        HEX1 : out std_logic_vector(0 to 6);
        HEX2 : out std_logic_vector(0 to 6);
        HEX3 : out std_logic_vector(0 to 6);
        LEDR : out std_logic_vector(0 to 9);
        LEDG : out std_logic_vector(0 to 7)
  );
end light;

architecture arch of light is
    function hex_decode(x:integer)
        return std_logic_vector is
    begin
        case x is
            when 0 => return "0000001";
            when 1 => return "1001111";
            when 2 => return "0010010";
            when 3 => return "0000110";
            when 4 => return "1001100";
            when 5 => return "0100100";
            when 6 => return "0100000";
            when 7 => return "0001111";
            when 8 => return "0000000";
            when 9 => return "0000100";
            when others => return "1111111";
        end case;
    end hex_decode;

    function add_input(input : std_logic_vector(0 to 9))
        return natural is
        variable sum : natural range 0 to 45 := 0;
    begin
        for i in 1 to 9 loop
            if input(i)='1' then
                sum := sum + i;
            end if;
        end loop;
        return sum;
    end function add_input;

    function select_decimal(input : natural; sel: natural)
        return natural is
        variable output : natural range 0 to 9;
    begin
        output := (input / (10**sel)) mod 10;
        return output;
    end function select_decimal;

    signal switch_value : natural range 0 to 45;
begin

    switch_value <= add_input(sw);

    hex0 <= hex_decode(select_decimal(switch_value, 0));
    hex1 <= hex_decode(select_decimal(switch_value, 1));
    hex2 <= hex_decode(select_decimal(switch_value, 2));
    hex3 <= hex_decode(select_decimal(switch_value, 3));
end arch;

Result in Vivado:

---------------------------------------------------------------------------------
Start RTL Component Statistics
---------------------------------------------------------------------------------
Detailed RTL Component Info :
+---Adders :
       8 Input      6 Bit       Adders := 1
+---Muxes :
      11 Input      7 Bit        Muxes := 2
       2 Input      6 Bit        Muxes := 1
---------------------------------------------------------------------------------
Finished RTL Component Statistics
---------------------------------------------------------------------------------
Report Cell Usage:
+------+-------+------+
|      |Cell   |Count |
+------+-------+------+
|1     |CARRY4 |     2|
|2     |LUT2   |     2|
|3     |LUT3   |     1|
|4     |LUT4   |     4|
|5     |LUT5   |     7|
|6     |LUT6   |    15|
|7     |IBUF   |     9|
|8     |OBUF   |    28|
|9     |OBUFT  |    18|
+------+-------+------+

Report Instance Areas:
+------+---------+-------+------+
|      |Instance |Module |Cells |
+------+---------+-------+------+
|1     |top      |       |    86|
+------+---------+-------+------+


SQL subquery to determine whether or not a particular row is the first item that matches the subquery Here is my VB.NET and SQL query (which is one of the things I'm particularly interested in having critiqued):

Private Sub BindHistory()

    Dim list = New List(Of CSRate)

    ' I hate the "case when" and subquery here - it seems like there should
    ' be a better way to do this
    Const Query As String =
        "select n.PayRate,
                n.CreatedOn,
                n.CreatedByUserName,
                (case when exists(select top 1 m.BusinessUnitID
                                  from TrackRates m
                                  where m.RateID = n.RateID AND
                                        m.TrackRateID < n.TrackRateID)
                 then 'entered rate of' else 'edited rate to' end) as Verb
        from    Rates, TrackRates n
        where   Rates.RateID = n.RateID AND
                Rates.ClientID = @id"
    Using reader = DBAccess.ExecuteReader(User.ActiveBusinessUnitID, CommandType.Text, Query, New SqlParameter("@id", SessionState.Client.ID))
        If reader.HasRows Then
            While reader.Read()
                Dim rate = New CSRate()
                rate.ActionDate = reader.Item("PayRate")
                rate.Action = $"User {reader.Item("Verb")} {reader.Item("PayRate")}"
                rate.UserID = reader.Item("CreatedByUserName")

                list.Add(rate)
            End While
        End If
    End Using

    CSHistory.DataSource = list
    CSHistory.DataBind()
End Sub

For the record, yes, I do realize that I should probably move the query to a stored procedure rather than having it "directly" in code-behind like this. Also, n and m are probably terrible names that I should refactor.

What I'm trying to accomplish with this SQL query is to determine whether a particular item is the first row that matches the criteria in the where clause (the text I have to display on the front end will differ). For example:

| TrackRateID | Other columns |
| 1           | ...           |
| 2           | ...           |
| 3           | ...           |

In this case, 2 and 3 should have the verb edited rate to (because there's a TrackRateID that's less than their TrackRateID) and 1 should have the verb entered rate of. (There is, of course, no guarantee that the "first" row will actually have an ID of 1).

Here's my ASP.NET front-end code:

<div id="clientHistoryPopup" style="display:none">
        <asp:GridView runat="server" ID="CSHistory" AutoGenerateColumns="false">
            <Columns>
                <asp:BoundField DataField="UserID" HeaderText="User ID" />
                <asp:BoundField DataField="ActionDate" HeaderText="Date" />
                <asp:BoundField DataField="Action" HeaderText="Pay Rate History" />
            </Columns>
        </asp:GridView>
    </div>

The case when and subquery particularly irk me. If I'm reading this properly (and please correct me if I'm wrong because I'm not too confident about this point), this would make the whole thing O(n log n).

Also, it seems like that makes the database logic too tightly "coupled" to my front-end logic, which strikes me as architecturally inappropriate.

Am I correct about the computational complexity? Is there a better way to do this that has a lower computational complexity and is less "coupled" to the front end?

There is no natural order.  With no order by the order of the rows is not guaranteed.

What you are looking for is not totally clear to me.  But I think this is what you are looking for.  Pretty sure window function is what you are looking and it is efficient.

  select tt.AuctionID, tt.StartingPrice
       , tt.rn
       , case when rn = 1 then 'first' else 'not first' end as [verb]
  from ( SELECT [AuctionID]
              , [Price]
              , ROW_NUMBER() over (partition [AuctionID] by order by [Price]) as rn
         FROM [Test].[dbo].[Auction]
       ) tt

select *
     , case when rn = 1 then 'entered rate of' else 'edited rate to' end as [verb]
from  ( select n.PayRate, n.CreatedOn, n.CreatedByUserName
            ,  row_number() over (partiton by RateID order by TrackRateID) as rn
          from Rates, TrackRates n
         where ClientID = @id
      ) tt
 order by RateID, TrackRateID


Prime factorization - C After learning prime factorization today, below is the C code written,

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

#define FIRST_PRIME 2
typedef enum {false, true}bool;

bool isEvenlyDivide(int compositeNumber, int number){
  return (compositeNumber % number)? false:true;
}

int findOtherFactor(int compositeNumber, int number){
  return compositeNumber / number;
}

bool isPrime(int wholeNumber){
  int factorLimit = wholeNumber / 2;
  for(int isItFactor = 2; isItFactor <= factorLimit; isItFactor++){
    if(isEvenlyDivide(wholeNumber, isItFactor)){
      return false;
    }
  }
  return true;
}

bool isComposite(int wholeNumber){
  return !isPrime(wholeNumber);
}

int findNextPrime(int primeNumber){
  int number = primeNumber+1;
  if(isPrime(number)){
    return number;
  }
  return findNextPrime(number);
}

void findPrimeFactors(int compositeNumber, int primeNumber){

  if(isEvenlyDivide(compositeNumber, primeNumber)){

    printf("%d ", primeNumber);
    int otherFactor = findOtherFactor(compositeNumber, primeNumber);
    if(isComposite(otherFactor)){
      findPrimeFactors(otherFactor, FIRST_PRIME);
    }else{
      printf("%d ", otherFactor);
    }
  }else{

    int nextPrime = findNextPrime(primeNumber);
    findPrimeFactors(compositeNumber, nextPrime);
  }

  return;
}

int main(int argc, char*argv[]){

  int compositeNumber = atoi(argv[1]);

  if(argc == 1){
    printf("usage: \n");
    printf("% ./findPrimeFactors <someCompositeNumber>\n");
    exit(1);
  }

  findPrimeFactors(compositeNumber, FIRST_PRIME);
}

for which, below is the compilation and execution(in cygwin),

Personal-PC ~/code_practice/Computing/recursion
$ gcc dummy.c -o findPrimeFactors

Personal-PC ~/code_practice/Computing/recursion
$ ./findPrimeFactors 29
29 1
Personal-PC ~/code_practice/Computing/recursion
$ ./findPrimeFactors 116
2 2 29
Personal-PC ~/code_practice/Computing/recursion
$ ./findPrimeFactors 16
2 2 2 2
Personal-PC ~/code_practice/Computing/recursion
$ ./findPrimeFactors 14
2 7

Do you think the code is complete from conceptual and implementation
aspect?
If yes, Can we further make this code, elegant & high perform?
If yes, Can I make it a stateless functional program using C?

Note: It took 15 minutes for me to write this program

doing cond?false:true is very odd to see and very much prone to mistakes in writing or reading. It's better to use negation in the condition:

bool isEvenlyDivide(int compositeNumber, int number){
  return (compositeNumber % number) == 0;
}

You can reduce the number of checks you do by using the knowledge that you only need to check prime factors up to the square root of wholeNumber, all but one prime is odd:

if(isEvenlyDivide(wholeNumber, 2)){
  return false;
}
for(int isItFactor = 3; isItFactor*isItFactor <= wholeNumber; isItFactor+=2)
    if(isEvenlyDivide(wholeNumber, isItFactor)){
      return false;
    }
}

That can also be used for findNextPrime, speaking of recursion isn't the best way to describe looping until you find something instead prefer an explicit loop:

int findNextPrime(int primeNumber){

    int number = primeNumber+1;
    if(isEvenlyDivide(primeNumber, 2)){
      number ++;
    }
    while(!isPrime(number)){
        number+=2;
    }
    return number;
}

When looping over all primes you can make use of the previous primes you have found to skip the non-primes. This can be done using the for example sieve of Eratosthenes.


Checking literary works for Zipf's Law I wrote a Python script that, given a literary work in text file format, takes it apart, makes a word list, counts how many times each words appears in the literary work, and then checks if it follows Zipf's Law by performing the chi-square test.

For those of you who don't know Zipf's Law, put simply, it is a law that states that in literary works, the frequency of a word is inversely proportional to its rank in the frequency table. So, the second most common word will appear half as much as the most common words, the third most common word will appear a third as often, and so on.

Also, for those who don't know the Chi-Square Test, it is a test that looks for statistical significance.

It commonly takes the form of:

$$x^2 = \sum \frac{(\text{observed} - \text{expected})^2}{\text{expected}}$$

I made this script because I had recently learned to code Python and I wanted some small project to occupy my mind. However, as I am rather new, I figured that my code was not exactly "Pythonic", so I was hoping that some kind souls could help me to tell me what I'm doing wrong and provide other helpful shortcuts.

from string import maketrans
from operator import itemgetter, attrgetter, methodcaller
import math

punct = [".", ",", "!", "?", "<", ">", "\n", "'", '"', "1", "2", "3", "4", "5", "6", "7", "8", "9", "0", "_"]

f_open = open('C:\Python27\Pride and Prejudice.txt', 'r')
lines = f_open.readlines()
f_open.close()
lines = filter(lambda name: name.strip(), lines)

def cleanup(text):
    counter = 0
    new_text = []
    while counter < len(text):
        if text[counter] in punct:
            counter += 1
        elif text[counter] == " " or text[counter] == "-":
            new_text.append(" ")
            counter += 1
        else:
            new_text.insert(counter, text[counter].lower())
            counter += 1
    words = "".join(new_text)
    return words.split()

def make_list(line):
    words = []
    for x in line:
        words = words + cleanup(x)
    return words

def count_words(words):
    word_count = []
    counter = 0
    for x in words:
        for sublist in word_count:
            if x == sublist[0]:
                sublist[1] += 1
        if all(sublist[0] != x for sublist in word_count):
                word_count.append([x, 1, 0, 0])
    return sorted(word_count, key = itemgetter(1), reverse = True)

def get_numbers(lists):
    numbers = []
    for x in lists:
        numbers.append(x[1])
    return numbers

def check_zipf(counted_words):
    counter = float(1)
    standard = counted_words[0][1]
    new_list = counted_words
    for x in counted_words:
        new_list[counted_words.index(x)][2] = float(standard) / counter
        new_list[counted_words.index(x)][3] =((float(new_list[counted_words.index(x)][1]) - float(new_list[counted_words.index(x)][2]))**2) / float(new_list[counted_words.index(x)][2])
        counter += 1
    return new_list

words = make_list(lines)
counted = count_words(words)

In no particular order:

Your code needs more documentation.

There are no comments or docs to help me understand how the code works, or why it was written the way it was. If I don’t know why the code was written this way, I can’t tell whether it’s working correctly – this makes it harder to review, edit and maintain.

There’s Python PEP 257, which describes how you should document a function with docstrings. (PEP means “Python Enhancement Proposal”, like a public design doc for Python.)

As a side note, when I read make_list(), I assumed that it took a single line (one string), which was a little confusing.  It wasn't until I saw it called later on that I realised it took an iterable of strings – a collection of lines.  Beware confusing typos like this.
Prefer comprehensions to loops and lambdas.

Python has a feature called comprehensions, which is a very powerful way for quickly constructing lists, sets and dictionaries. If you’re unfamiliar with the concept, I quite like this introduction.  This is often considered to be one of the highlight features of Python.

This lets you write for loops in a more succinct way, for example in get_numbers():

def get_numbers(lists):
    return [x[1] for x in lists]

If the for loop is simple, these are generally considered more Pythonic.

Lambdas are considered fairly un-Pythonic, especially when comprehensions are available. For example, I'd rewrite line 10 as:

lines = [line.strip() for line in lines if line.strip()

Use with open() rather than open() … close().

The preferred approach for opening files is as follows:

with open(pride_and_prejudice_path, 'r') as infile:
    # do stuff with infile

This construction ensures that the file is always closed correctly – if you make explicit calls to open() and close(), something could go wrong in the middle and you might not close the file correctly.

Note also that you can iterate over the contents of the file directly, rather than calling readlines() first:

with open(pride_and_prejudice_path, 'r') as infile:
    lines = [name.strip() for name in infile if name.strip()]

Note that calling readlines() will store every line in a list, before you can do anything – depending on the size of the file you’re looking at, this can be prohibitively large.  Often better to skip readlines() and iterate over the file directly.
Try to avoid unnecessary imports.

The only import you’re actually using is operator.itemgetter(). It’s good practice to avoid importing things that you don’t use.

In the same vein, don’t create variables you don’t need, e.g. the counter variable in count_words().
Read PEP 8, the Python style guide.

In general, your code is pretty good. A few small things I noticed:

On line 41, you seem to have over-indented by 4 spaces.
On line 42, you shouldn’t have spaces around the = sign in keyword arguments, i.e.,

return sorted(word_count, key=itemgetter(1), reverse=True)

Line 56 should be broken up for readability, probably by assigning new_list[counted_words.index(x)] to a constant. (Regardless of the line length in the standard, it’s just unwieldy.)

Use __future__.division instead of casting to floats.

I can see that you’re casting to float in quite a few places, because Python 2.7 defaults to integer division. Python 3 behaves in a more sensible manner (and I’d recommend using Py3 if you’re starting from scratch). To get this behaviour in the older versions of Python, you can add

from __future__ import division

to the top of your code. This will let you clean up your float() calls.

Note that __future__ imports must precede any other imports.
Use data structures other than lists.

In count_words(), you're using a list of lists to keep track of how many times a word appears in the text.  A better data structure would be a dictionary.

Further, you're constructing multiple intermediate lists to get this structure.  All of those consume memory and take time to construct.  If you move this up to where we iterate over the file, we can skip those lists and go much faster:

word_counter = dict()
with open('prideandprejudice.txt', 'r') as infile:
    for line in infile:
        if not line.strip():
            continue
        for word in cleanup(line):
            if word in word_counter:
                word_counter[word] += 1
            else:
                word_counter[word] = 1
counted_words = sorted(word_counter.items(),
                       key=itemgetter(1),
                       reverse=True)

On my machine, your original code took ~40s to run.  My updated version runs in <1s.  Those intermediate lists had a big performance impact.

Note that you could use collections.defaultdict to tidy that code up even further.
Use enumerate() for a more Pythonic loop.

Your cleanup() function has a classic pattern for people coming to Python from other languages:

i = 0
while i < len(iterable):
    item = iterable[i]
    # do stuff with item
    i += 1

Python has loop constructions that make this much nicer.  In cleanup(), you can use enumerate to iterate over the index and the character together.  This is more explicit, and makes for cleaner code.  Like so:

for idx, char in enumerate(text):
    if char in punct:
        continue
    elif char in (" ", "-"):
        new_text.append(" ")
    else:
        new_text.append(char.lower())

Use collections.namedtuple to clean up the output of check_zipf.

Using namedtuples is a very easy way to improve the readability of code, by assigning named attributes to items.  Reading the output of your check_zipf() function, all I get is lists of numbers.  What do these numbers mean?  Their indices mean nothing to me (especially as you haven't included comments).

Namedtuples, and good attribute/variable names, can make it much easier to see what's going on.  For example:

Word = collections.namedtuple('Word', 'observed expected chi_square')

def check_zipf(counted_words):
    zipfed_words = {}
    baseline_observed = counted_words[0][1]
    for idx, (word, observed) in enumerate(counted_words):
        expected = baseline_observed / (idx + 1)
        chi_square = ((observed - expected) ** 2) / expected
        zipfed_words[word] = Word(observed, expected, chi_square)
    return zipfed_words


Memorise array and print number of occurrences I have written a slow solution to the Memorise Me problem on hackerearth

  The first line of input will contain N, an integer, which is the
  total number of numbers shown to your team.

  The second line of input contains N space separated integers.

  The third line of input contains an integer Q, denoting the total
  number of integers.

  The Next Q lines will contain an integer denoting an integer,
  Bi, for which you have to print the number of occurrences of that number (Bi) in those N numbers on a new line.

  If the number Bi isn’t present then print NOT PRESENT
  on a new line.

  Constraints

  $$1 \le N \le 10^5$$

  $$0 \le B_i \le 1000$$

  $$1 \le Q \le 10^{55}$$

    #include <stdio.h>
    int check(int z,int c[1001][2],int q,int arr[])
    {
        int i =0;
        while(c[i][0]!=-1)
        {
        if (z==c[i][0])
        {
            return c[i][1];
            break;
        }
    else
    {
        c[i][0]=z;
        c[i][1]=count(z,arr,q);
        return c[i][1];
    }
    i++;
    }

    }
     int count(int z,int arr[],int q)
     {
         int occurence=0;
         for(int k=0;k<q;k++)
         {
             if(z==arr[k])
             {
                 occurence++;
             }
         }
     return occurence;

     }
    int main()
    {
        int c[1001][2];

            for(int i=0;i<=1001;i++)
            {
                c[i][0]=-1;
                c[i][1]=0;
            }
            c[0][0]=9999;
        int n;
        scanf("%d",&n);
        int arr[100000];
        int i;
        for(i=0;i<n;i++){
            scanf("%d",&arr[i]);
            }
        int q;
        scanf("%d",&q);
        int j;
        int B[100000];
        int occurence1=0;

        for(j=0;j<q;j++)
        {
            scanf("%d",&B[j]);
            occurence1=check(B[j],c,q,arr);

            if(occurence1==0)
            {
                printf("NOT PRESENT\n");
            }
            else
            {
                printf("%d\n",occurence1);
            }
            }
        }

Code taking 0.9 seconds for a set of input where \$n=10^5\$ else it's taking 0.1 seconds.  Can you suggest a shorter way to reduce time complexity!  And suggest improvements in the code itself.

One of the things that's terrible about these types of competitions is that the problems are very contrived. They're written like a math problem, which makes it tempting to write your solutions the same way. I'd do the following things to improve your code.

Formatting

The formatting of your code is very messy. Maybe it's just due to getting used to entering code in the Stack Exchange text field, but even if the tabs were correct, you're not using a lot of white space between statements. I recommend looking at other people's code to see how they use whitespace and adopting a style that you like. The book Code Complete has some useful rules of thumb to use for how to style C code. It's a good place to start from.

Naming

This is the number one thing that most developers could improve (myself included!). Just because the problem description names things like N, Q, and B[i] doesn't mean your code has to use those names. You should strive to use readable names like numEntries, numQueries, and queries.

Your function names aren't great either. check() doesn't tell me what it's doing. What is it checking? It turns out nothing. It's tabulating the number of times z occurs in arr and putting the result into c. It might be better to name it something like tabulateEntries() or something along those lines. Likewise, count() is counting something, right? What is it counting? The number of occurrences of z. So maybe it should be named countOccurences()?

Be Careful With The Stack

You're allocating everything on the stack. This can get you in trouble with longer programs. For example, your declaration of B is using either 400 or 800KB of stack space. The default stack size in many implementations is 1MB, so this would use almost 80% of the stack just in your main() function. It might be wiser to allocate that space on the heap or better yet, only allocate as much as you actually need (which will be q elements instead of 100,000). Same thing with arr.

Avoid Magic Numbers

You have several magic numbers in your code. These are numbers that are hard-coded to specific values. Someone reading your code would have no idea where they come from. You should use named constants or enumerations where appropriate. For example,

const int kMaxQueries = 1000;
const int kUnsetEntry = -1;
const int kInvalidEntry = 9999;

Use structs Where Appropriate

You're using a 2 dimensional array to represent the values and their counts. You should instead use a struct that holds those 2 values and have a 1-dimensional array of those structs. Something like this:

typedef struct ValueCounts {
    int value;
    int count;
} ValueCounts;

Then you can write self-documenting code like this:

int check(int z, ValueCounts counts, int q, int arr[])
{
    int i = 0;
    while (counts[i].value != kUnsetEntry)
    {
        if (z == counts [ i ].value)
        {
            return counts [ i ].count;
        }
        else
        {
            counts [ i ].value = z;
            counts [ i ].count = count(z, arr, q);
            return counts [ i ].count;
        }
        i++;
    }
}

Errors

Notice that your check() function can return an undefined value. If c[i][0] == -1, then the while loop ends, and the function will return whatever's on the stack, which is likely uninitialized memory. Is this why you made c[0][0] have an initial value of 9999? While that works around the problem, it's a bad way to do it.

Performance

Looking at your code, it seems to be doing things the long way. arr is an unsorted array. Tabulating how many occurrences there are of a single value in arr potentially involves looping over the entirety of c processed so far. This can happen for every value in arr. It would probably be faster to sort arr before counting the occurrences of each value. If it were already sorted, you'd be able to just run through arr in order, and as soon as the value changed, you'd fill in the next value in c with the total number of occurrences of the value before the change. That would leave c in order and would allow you to do a binary search of c for getting the values needed to print out the result. That should be considerably faster than the linear search you're currently doing.

In fact, I went to the site and tried the above suggestions and passed the tests in the time allotted. For the sorting, I used the standard library qsort() function. I wrote my own binary search, and it seemed to do the trick.


Simplified light Serialize/Deserialize to/from JSON for .NET Just a very simplified and stright-forward way of serialize/deserialize to/from JSON using standard .NET 4+ libs. I was trying to avoid "complexity", so get rid of 3rd part libs and heavy-infrastructured standard libs.

    #region JSON API

    public static string ObjectToJson(object o, Type t, Type[] pTypes = null)
    {
        DataContractJsonSerializer serializer = (pTypes != null ? new DataContractJsonSerializer(t, pTypes) : new DataContractJsonSerializer(t));
        MemoryStream mStrm = new MemoryStream();
        serializer.WriteObject(mStrm, o);
        mStrm.Position = 0;
        using (var sr = new StreamReader(mStrm, Encoding.UTF8))
        {
            return sr.ReadToEnd();
        }
    }

    public static object JsonToObject(string json, Type t, Type[] pTypes = null)
    {
        DataContractJsonSerializer serializer = (pTypes != null ? new DataContractJsonSerializer(t, pTypes) : new DataContractJsonSerializer(t));
        using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(json)))
        {
            return serializer.ReadObject(stream);
        }
    }

    #endregion // JSON  API

And an example of its usage:

    public bool ListOfGames(bool pMyGames, bool pGamesITookPartIn)
    {
        this.GamesListInfo = null;
        this.LastErrorInfo = null;
        try
        {
            string formValues =
                "&playerId=" + this.LogonInfo.userId.ToString() +
                "&createdBy=" + pMyGames.ToString() +
                "&tookPartIn=" + pGamesITookPartIn.ToString() +
                "";

            WebRequest request = createRequest("myGamesList", formValues);
            HttpWebResponse response = (HttpWebResponse)request.GetResponse();
            using (StreamReader sr = new StreamReader(response.GetResponseStream()))
            {
                string responseText = sr.ReadToEnd();

                if (responseText.Contains("\"errType\":"))
                    this.LastErrorInfo = (ErrorInfoContract)JsonToObject(responseText, typeof(ErrorInfoContract));
                else
                    this.GamesListInfo = (GamesListContract)JsonToObject(responseText, typeof(GamesListContract), new Type[] { typeof(GameInfoContract) });

                Trace.WriteLineIf(TrcLvl.TraceInfo, TrcLvl.TraceInfo ? string.Format("NewGame.ListOfGames: {0}{1}",
                    (this.LogonInfo != null ? this.LogonInfo.ToString() : ""), (this.LastErrorInfo != null ? this.LastErrorInfo.ToString() : "")) : "");
            }
        }
        catch (Exception exc)
        {
            this.LastErrorInfo = new ErrorInfoContract()
                {
                    errType = exc.GetType().ToString(),
                    message = exc.Message,
                    stackTrace = exc.StackTrace,
                    timestamp = StrUtils.NskTimestampOf(DateTime.Now)
                };
        }
        return (this.LogonInfo != null && this.LogonInfo.userId > 0 && this.LogonInfo.authKey > 0);
    }

    private WebRequest createRequest(string pAction, string pFormValues)
    {
        WebRequest request = WebRequest.Create(this.ServerUrl + "?action=" + pAction);

        request.Method = "POST";
        request.ContentType = "application/x-www-form-urlencoded";
        request.ContentLength = pFormValues.Length;

        byte[] data = Encoding.UTF8.GetBytes(pFormValues);
        using (Stream strm = request.GetRequestStream())
        {
            strm.Write(data, 0, data.Length);
        }
        return request;
    }

Just searched where to publish a code and example (for some purposes).

I do not expect any inputs but if you would like to - welcome, would be appreciated. :)

Frame challenge

  I was trying to avoid "complexity", so get rid of 3rd part libs

Although it's not strictly code review, I would like to start by challenging this goal. There are many many official Microsoft libraries with dependencies on Newtonsoft.Json. (See this answer on our software engineering sister site).

Core code

    public static string ObjectToJson(object o, Type t, Type[] pTypes = null)

This absolutely needs documentation comments explaining what t and pTypes are for. As a maintenance programmer this signature is unhelpful on two counts:

I have no idea why I need to pass types at all. If ObjectToJson needs the type of the object for some reason it can call o.GetType().
The names are not helpful. I'm guessing that t is the type of o, but that's a pure guess. As for pTypes, is that Hungarian notation for "pointer to type"? That has no place in C# code unless you're using unsafe and actual pointers (and even there the name should tell you what the variable is for and not just what its type is).

With respect to point 1, I would suggest that in addition to improving the names and adding documentation you consider two further refactorings: either making t an optional parameter with default null and calling o.GetType() if necessary; or changing the signature to public static string ObjectToJson<T>(T obj, IEnumerable<Type> whateverPTypesShouldBeCalled) so that the compiler can infer the type but the caller can impose a supertype if that's necessary for some bizarre reason. (In case you don't know: you can use typeof(T) with T a type variable to get a Type object).

        DataContractJsonSerializer serializer = (pTypes != null ? new DataContractJsonSerializer(t, pTypes) : new DataContractJsonSerializer(t));

Might it make more sense to use new DataContractJsonSerializer(t, pTypes ?? Enumerable.Empty<Type>())?

        MemoryStream mStrm = new MemoryStream();
        ...
        using (var sr = new StreamReader(mStrm, Encoding.UTF8))

This is inconsistent: . I would favour the approach of always using using with IDisposable, even when you know that it doesn't use any unmanaged resources.

A comment explaining why Encoding.UTF8 is correct would be an improvement.

    public static object JsonToObject(string json, Type t, Type[] pTypes = null)
    {
        DataContractJsonSerializer serializer = (pTypes != null ? new DataContractJsonSerializer(t, pTypes) : new DataContractJsonSerializer(t));
        using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(json)))
        {
            return serializer.ReadObject(stream);
        }
    }

I think all of the comments on ObjectToJson apply equally to JsonToObject with one exception: this time the signature makes a much stronger argument for use of generics. As it stands you probably have to cast the return value almost every time you use this method.

Usage example

This is again marginally off-topic, so I won't do a detailed review of the example usage code, but I think it's important to address a couple of points.

    private WebRequest createRequest(string pAction, string pFormValues)

Every caller of this method is expected to compose pFormValues. That's the antithesis of Don't Repeat Yourself. The method which factors out the commonalities of requests should be the one place which composes the query string, and unlike the example

            string formValues =
                "&playerId=" + this.LogonInfo.userId.ToString() +
                "&createdBy=" + pMyGames.ToString() +
                "&tookPartIn=" + pGamesITookPartIn.ToString() +
                "";

it should take care to escape the values.

            WebRequest request = createRequest("myGamesList", formValues);
            HttpWebResponse response = (HttpWebResponse)request.GetResponse();
            using (StreamReader sr = new StreamReader(response.GetResponseStream()))
            {
                string responseText = sr.ReadToEnd();

                if (responseText.Contains("\"errType\":"))

Again, I would hope that the web service on the other end is consistent enough that handling the response can be done in one place rather than being repeated for every single call. And I would hope that it uses HTTP well enough that you don't have to do heuristic guessing to figure out whether there was a problem. Instead of looking for "errType": it should switch on response.StatusCode.

        WebRequest request = WebRequest.Create(this.ServerUrl + "?action=" + pAction);

        request.Method = "POST";
        request.ContentType = "application/x-www-form-urlencoded";
        request.ContentLength = pFormValues.Length;

I find it bizarre that the form values are split between the URL and the request body.


Loading site configuration from ini file I load in static setting via an ini file. Ini file have the benefits

of NOT being server by most web servers by default.
easier to edit than XML
even simplier than JSON

Setting get loaded in Application.cfc

void function setupApplication() output="false" {
...
application.stAdminSetting  = application.loadini("admin\standard.ini");
...
}

Application.cfc has this as a function...

<cffunction name="loadini" output="false" returnType="struct">
<cfargument name="configfile" required="true" type="string" hint="reads from currentTemplate">

<cfscript>
var stResult = {};

var iniFilePath = "#GetDirectoryFromPath(GetBaseTemplatePath())##arguments.configfile#";
var stSection = getProfileSections(iniFilePath);

for(var section in stSection)   {

    var CurrentSection = evaluate("stSection.#section#");

    var stData = {};

    for(var i = 1; i <= ListLen(CurrentSection); i++)   {
        var key = ListGetAt(CurrentSection, i);

        stData[key] = getProfileString(iniFilePath, section, key);

        }

    setvariable("stResult.#section#", stData);
    }

return stResult;
</cfscript>
</cffunction>

The ini file can have any number of setting. In particular my ini file has:

[ws]
comment=http://xxxxx.com/resource/comment.cfc?wsdl
ior=http://xxxxx.com/resource/ior.cfc?wsdl
node=http://xxxxx.com/resource/node.cfc?wsdl
pref=http://xxxxx.com/resource/pref.cfc?wsdl
traffic=http://xxxxx.com/resource/traffic.cfc?wsdl

What kinds of things can I do to improve this?

What kind of things are you looking to improve? Your solution looks pretty straightforward. The only problem with an ini file is that ColdFusion can't instantly/natively parse it. Whereas XML/JSON can be parsed directly into a Coldfusion Struct.

<cfscript>
public struct function loadini( required string configFile) {
    var stResult = {};
    var iniFilePath = "#GetDirectoryFromPath(GetBaseTemplatePath())##arguments.configfile#";
    var stSection = getProfileSections(iniFilePath);

    // loop over the sections
     for (section in stSection) {
        // loop over the list of variables in the ini file
        for (var i=0;i<=listLen(stSection[section]);i++) {
            // result.section.variable = value
            stResult[section][listGetAt(stSection[section],i)] = GetProfileString(iniFilePath, section, listGetAt(stSection[section],i) ) ;
        }
     }
    return stResult;
}
</cfscript>

<cfdump var="#loadini('test.ini')#">


C function to check the validity of a date in DD.MM.YYYY format I was tasked with writing a function, that checks whether a date is valid in the following format: DD.MM.YYYY. Here's what I wrote:

bool isValidDate(const char* date)
{
    int a, b, c;
    size_t len = strnlen(date, 255);
    for (size_t i = 0; i < len; i++)
    {
        if (!isdigit(date[i]) && date[i] != '.') return false;
    }
    int validConversions = sscanf_s(date, "%2d.%2d.%4d", &a, &b, &c);
    return validConversions == 3 ? true : false;
}

From my testing I can conclude that this function does reasonably well: it doesn't accept anything except dots and digits, it expects to read exactly 3 numbers and it knows how many digits to read for the days and months.

Something it doesn't check however, is whether the days are less than 31 or the months are less than 12. Another thing it doesn't do is that it doesn't check correct looking, but invalid dates such as 30.02.2013 or 31.06.2012.

Solving that problem is trivial, but I don't wanna write 12 ifs for each month to check the dates. I'm starting to think I should just generate a data structure of valid dates and use that to check, i.e. if the passed date is there, then it's valid.

Is that the best way to do it or are there other ways?

Use descriptive variable names

Instead of int a, b, c, give them more descriptive names:

int day, month year;

sscanf() will ignore trailing garbage

The function sscanf() will stop parsing after the last conversion. So the string "1.2.3...." will be cleared by your check for digits and period characters, and then sscanf() will read 3 integers, and returns 3. But obviously, this is an invalid date.

It would be best if you could have sscanf() determine the validity of the whole input string. One way is to use the %n conversion to check how many characters of the string were parsed so far, and then check that this corresponds to the whole string. This is how you can do that:

int day, month, year, chars_parsed;

if (sscanf(date, "%2d.%2d.%4d%n", &day, &month, &year, &chars_parsed) != 3)
    return false;

/* If the whole string is parsed, chars_parsed points to the NUL-byte
   after the last character in the string.
 */
if (date[chars_parsed] != 0)
    return false;

You don't need 12 ifs to check the month

You can just write:

if (month < 1 || month > 12)
    return false;

And similar for days and perhaps even years if you want to limit the allowed range.

Avoid redundant checks

It is likely that your program will normally handle valid date strings. So you want to optimize for this case. In your code, you are checking for the string to be empty at the start, but this is not necessary; if the input string is empty, then sscanf() will not return 3, so it will already correctly return false. Since most input strings will be valid, checking the string length is just a waste of CPU cycles.

Similarly, checking for each character to be a digit or a period is redundant if you just use sscanf() with the %n method to check that there was no trailing garbage after the year.

Better date checking

Just checking whether the month is between 1 and 12 and day between 1 and 31 is not enough. A given month might have less than 31 days. There are also leap years to consider. And if you want to allow dates far in the past, you run into the problem that we have had different calenders. To give an idea of how difficult the problem is, watch: https://www.youtube.com/watch?v=-5wpm-gesOY

One way to validate the date is to use the C library's date and time conversion routines. After scanning the day, month and year, create a struct tm from that, then convert it to seconds since epoch using mktime(). This might still accept invalid dates, but if you convert that back to a struct tm, you can check whether that conversion matched the original input:

int day, month, year,
sscanf(date, "%2d.%2d.%4d", &day, &month, &year);

struct tm input = {
    .tm_mday = day,
    .tm_mon = month - 1,
    .tm_year = year - 1900,
};

time_t t = mktime(&input); /* note, this might modify input */
struct tm *output = localtime(&t); /* prefer localtime_r() on systems that support it */

if (day != output->tm_mday || month != output->tm_mon + 1|| year != output->tm_year + 1900)
    return false;

These routines will probably still not handle dates hundreds of years in the past correctly, but it should suffice for recent dates.


Binary Search, using .remove I'm aware that a function in the standard library exists. Please give me feedback regarding good coding conventions and standards.

fun main(arg: Array<String>) {
    val intList = listOf<Int>(1, 2, 3, 4, 5, 6, 7, 8, 9, 10)
    println("Enter a valid number:")
    val x = readLine()!!.toIntOrNull() ?: run {
        println("You didn't type a valid number.")
        return
    }
    binarySearch(intList, x)
}

fun binarySearch(list: List<Int>, searchItem: Int){
    val mid = list.size / 2
    val isInFirstHalf =
                list
                    .take(mid)
                    .any { it == searchItem }
    do {
        val result =
                if (isInFirstHalf)
                    list.slice(0 until mid)
                else
                    list.slice(mid until list.size)
        println(result.joinToString())
        if (result.size == 1) break
        binarySearch(result, searchItem)
        return
    } while (true)
    println("Binary search found $searchItem.")
}

Instead of parsing an Int with the catch (e: NumberFormatException) { ... }, consider using the String.toIntOrNull() extension:

val x = readLine().toIntOrNull() ?: run {
    println("You didn't type a valid number.")
    return
}

In Kotlin extension functions, reference the members of the receiver type without explicit this: this.size → size unless the name is ambigous.
Instead of (0 until this.size / 2).map { this.elementAt(it) }, you could just use take(size / 2) (but see below why you should not).
The approach you take to check in which half the item can be located and move the binary search window to a half of the list (by copying or removing the first half) changes the run time of the algorithm significantly: while binary search should take \$ O(log n) \$ time to find an element, your implementation additionally performs \$ O(n/2 + n/4 + n/8 + ...) = O(n) \$ operations checking the first half and then copying or removing the items, making the search asymptotically as slow as serial search through the list. Consider checking only the item in the middle of the list (the assumption that the list is sorted allows you to do that) and then using .subList(...) to get a view of a portion of the original list, in \$ O(1) \$ time.


Toggle between two states in react I have two fetch calls which updates the state. And I have two buttons. One makes a fetch call, another calls componentDidMount().

The first fetch call is in componentDidMount().

The second is On "All Time Sort" button click with handleSortRecent().

On "Recent Sort" button click, I want to show the previous informatin fetched in componentDidMount. To achieve this
I am calling the componentDidMount() method itself from handleSortRecent() method.

Is there a better way to do this without using Redux?

const API = "https://fcctop100.herokuapp.com/api/fccusers/top/";

class App extends Component {
  constructor(props) {
    super(props);

    this.state = {
      dataRecent : [],
      dataAlltime : []
    }

    // This binding is necessary to make `this` work in the callback
    this.handleClick = this.handleClick.bind(this);
    this.handleSortRecent = this.handleSortRecent.bind(this);
  }

  componentDidMount() {
    fetch(API+"recent")
      .then( response => response.json() )
      .then( data => this.setState( { dataRecent: data } ) );
  }

  handleClick () {
    fetch(API+"alltime")
    .then(response => response.json())
    .then(data => this.setState({dataRecent:data}))
  }

  handleSortRecent() {
    this.componentDidMount();
  }

  render() {
    const arrRecentList = this.state.dataRecent;

    return (

      <button onClick={this.handleClick}>All Time Sort</button>
      <button onClick={this.handleSortRecent}>Recent Sort</button>
      {
        arrRecentList.map(data =>

            {data.username}
             -
            {data.recent}
             -
            {data.alltime}

        )
      }

    );
  }
}

It's strange that you would call this.componentDidMount. I have not seen this before. Either way, what if you created an indicator in your state that declared what should be shown? For example:

this.state = {
  dataRecent : [],
  dataAlltime : [],
  display: "recent"
}

Then inside of your render function you can choose what gets displayed.

this.state.display === 'recent'
  ? this.state.dataRecent.map...
  : this.state.dataAlltime.map...

I would create two functions that handle getting both sets of data.

getAlltimeData() => {
   fetch(API+"alltime")
    .then(response => response.json())
    .then(data => this.setState({ dataRecent:data }))
}

getRecentData() => {
   fetch(API+"recent")
    .then(response => response.json())
    .then(data => this.setState({ dataRecent:data }))
}

You can then call both of them when the component mounts.

componentDidMount() {
  this.getAlltimeData();
  this.getRecentData();
}

At this point you have the data you need. What you don't have is a click handler to switch between recent and all time.

handleRecentClick() {
  this.setState({ display: 'recent' });
}

handleAlltimeClick() {
  this.setState({ display: 'allTime' });
}

Pass those to the buttons that change state respectively.

 <button onClick={this.handleRecentClick}>All Time Sort</button>
 <button onClick={this.handleAlltimeClick}>Recent Sort</button>

Make sure to bind your functions in the constructor like you did with the others.


Binning whilst avoiding the need for for-loops in Python? I am working on a relatively simple binning program, where I take a 5D array and bin it based on two 3D arrays to create a contour plot. See the sample code below. In actuality, my arrays are of size [27, 150, 20, 144, 288], so running a 4-nested for loop as shown below takes a LONG time.

Is there any way to speed up this loop and ideally avoid the need for all these loops? Apologies in advance if this isn't clear - I am new to all this!

S_mean = np.random.rand(5,10,10,10)
T_mean = np.random.rand(5,10,10,10)
Volume_mean = np.random.rand(2,5,10,10,10)

T_bins = np.linspace(0,1,36)
S_bins = np.linspace(0,1,100)

int_temp = []
int_sal = []

for i in range(5):
    int_temp.append(np.digitize(np.ndarray.flatten(T_mean[i,:,:,:]), T_bins))
    int_sal.append(np.digitize(np.ndarray.flatten(S_mean[i,:,:,:]), S_bins))

volume_sum = np.zeros((2,5,S_bins.size,T_bins.size))

# This is the problem loop

for k in range(2):
    for l in range(5):
        for i in range(T_bins.size):
            for j in range(S_bins.size):
                volume_sum[k,l,j,i]=(np.nansum(np.ndarray.flatten(Volume_mean[k,l,:,:,:])
                                    [np.argwhere(np.logical_and(int_temp[l] == i, int_sal[l] == j))]))

# The output I am trying to get

plt.pcolormesh(T_bins, S_bins, volume_sum[0,0,:,:])
plt.show()

"Relatively simple" is in the eye of the beholder. Some ideas though:

This code

int_temp = []
int_sal = []

for i in range(5):
    int_temp.append(np.digitize(np.ndarray.flatten(T_mean[i,:,:,:]), T_bins))
    int_sal.append(np.digitize(np.ndarray.flatten(S_mean[i,:,:,:]), S_bins))

can be transformed into two list comprehesions

int_temp = np.array([np.digitize(T_mean[i, :, :, :].flatten(), T_bins) for i in range(5)])
int_sal = np.array([np.digitize(S_mean[i, :, :, :].flatten(), S_bins) for i in range(5)])

I also converted the results into numpy arrays, for reasons that I'll refer to later. Also take note that np.ndarray.flatten(T_mean[i,:,:,:]) was rewritten as T_mean[i, :, :, :].flatten() which is the usual way to use np.ndarray.<fun>: call whatever.<fun>() (<fun> is meant to be a placeholder here, not actual code).

The next part of the code that caught my eye was

np.ndarray.flatten(volume_mean[k, l, :, :, :])
    [np.argwhere(np.logical_and(int_temp[l] == i, int_sal[l] == j))])

This line is very hard to read, but can easily be rewritten as:

indices = np.argwhere(np.logical_and(int_temp[l] == i, int_sal[l] == j))
volume_sum[k, l, j, i] = nansum(volume_mean[k, l, :, :, :].flatten()[indices])

which looks muss less frightening. You can even get rid of np.argwhere and use np.logical_and(int_temp[l] == i, int_sal[l] == j) directly as binary mask:

mask = np.logical_and(int_temp[l] == i, int_sal[l] == j)
volume_sum[k, l, j, i] = (
    np.nansum(volume_mean[k, l, :, :, :].flatten()[mask])
)

To see if there is a difference in performance, I put the original code and the refactored version into functions:

def compute_volume_sum(volume_mean, s_mean, t_mean, s_bins, t_bins):
    """Original implementation"""
    int_temp = []
    int_sal = []
    for i in range(5):
        int_temp.append(np.digitize(np.ndarray.flatten(t_mean[i,:,:,:]), t_bins))
        int_sal.append(np.digitize(np.ndarray.flatten(s_mean[i,:,:,:]), s_bins))

    volume_sum = np.zeros((2, 5, s_bins.size, t_bins.size))
    for k in range(2):
        for l in range(5):
            for i in range(t_bins.size):
                for j in range(s_bins.size):
                    volume_sum[k, l, j, i] = (np.nansum(
                        np.ndarray.flatten(volume_mean[k, l, :, :, :])[np.argwhere(
                            np.logical_and(int_temp[l] == i, int_sal[l] == j))]))

    return volume_sum

def compute_volume_sum_ref(volume_mean, s_mean, t_mean, s_bins, t_bins):
    """Refactored implementation"""
    int_temp = np.array([np.digitize(t_mean[i, :, :, :].flatten(), t_bins) for i in range(5)])
    int_sal = np.array([np.digitize(s_mean[i, :, :, :].flatten(), s_bins) for i in range(5)])

    volume_sum = np.zeros((2, 5, s_bins.size, t_bins.size))
    for k in range(2):
        for l in range(5):
            for i in range(t_bins.size):
                for j in range(s_bins.size):
                    mask = np.logical_and(int_temp[l] == i, int_sal[l] == j)
                    volume_sum[k, l, j, i] = (
                        np.nansum(volume_mean[k, l, :, :, :].flatten()[mask])
                    )

    return volume_sum

BTW: You can confirm that both functions return the same value using np.allclose

I then measured how long it takes to run each function ten times. The results were as follows:

original: 16.860085s
refactored: 11.849922s

Not bad, but those loops still hurt.

Enter numba, a just-in-time Python compiler. It works nicely with numpy and can make loops quite a bit faster since they are compiled to native code. I recommend to use a scientific Python distribution like Anaconda if you want to try/use it.

Unfortunately, numba does not support the full feature set of Python (e.g. list comprehensions are not supported), so I had to resort to a little bit of trickery to get it working:

import numba

def compute_volume_sum_nb(volume_mean, s_mean, t_mean, s_bins, t_bins):
    """numba version"""
    int_temp = np.array([np.digitize(t_mean[i, :, :, :].flatten(), t_bins) for i in range(5)])
    int_sal = np.array([np.digitize(s_mean[i, :, :, :].flatten(), s_bins) for i in range(5)])

    return _numba_inner(volume_mean, s_bins, t_bins, int_temp, int_sal)

@numba.njit()
def _numba_inner(volume_mean, s_bins, t_bins, int_temp, int_sal):
    volume_sum = np.zeros((2, 5, s_bins.size, t_bins.size))
    for k in range(2):
        for l in range(5):
            for i in range(t_bins.size):
                for j in range(s_bins.size):
                    mask = np.logical_and(int_temp[l] == i, int_sal[l] == j)
                    volume_sum[k, l, j, i] = (
                        np.nansum(volume_mean[k, l, :, :, :].flatten()[mask])
                    )

    return volume_sum

numba.njit() is a decorator that can be used on functions to mark their function body for jit-compilation. The first time compute_volume_sum_nb is called, numba triggers the compilation process so be sure to exclude the first call from any timings you do.

Let's see how the timings now look like:

original: 16.860085s
refactored: 11.849922s
numba: 0.833529s

That's quite a bit faster, isn't it?

I'm almost sure that some of the loops could be replaced by clever indexing and use of vectorized functions in numpy, but I don't have the time to dive deeper into the problem.

A few non-performance related notes: be sure so have a look at the Style Guide for Python Code (aka PEP 8). The code sometimes already follows those guidelines, but sometimes also doesn't, e.g. when using uppercase letters in variable names.

Maybe also think about how you can modularize your code, e.g by using functions. Those separated parts are also easier to document. You want to write """"documentation""" to save future you some headache, don't you?