Merge two sorted lists in Python Merge two sorted lists

# Input:    lst1 {List}
# Input:    lst2 {List}
# Output:   {List}
#
# Example: merge([1, 4, 7], [2, 3, 6, 9]) => [1, 2, 3, 4, 6, 7, 9]
#
def merge(items1, items2):
    # merge 2 list
    # divide and conquer
    # combine - step2
    ("\n"
     "    while left_index is not the length of left_array and right_index is not the length of right_array\n"
     "        if left_array[left_index] < right_array[right_index]\n"
     "            add left_array[left_index] to merged array\n"
     "            left_index + 1\n"
     "        else\n"
     "            add right_array[right_index] to merged_array\n"
     "            right_index + 1 ")
    # if we've passed the end of either list, then extend the sorted list
    # with what remains in the other
    left_pointer = 0
    right_pointer = 0
    sorted_list = []

    while len(sorted_list) < len(items1) + len(items2):
        left_item = items1[left_pointer]
        right_item = items2[right_pointer]
        if left_item < right_item:
            sorted_list.append(left_item)
            left_pointer += 1
        else:
            sorted_list.append(right_item)
            right_pointer += 1

        if right_pointer >= len(items2):
            sorted_list.extend(items1[left_pointer:])
            break
        if left_pointer >= len(items1):
            sorted_list.extend(items2[right_pointer:])
            break

    return sorted_list

test cases that i am working on

print('merge tests')
test_count = [0, 0]

def test():
    results = merge([1, 4, 7], [2, 3, 6, 9])
    return (len(results) == 7 and
            results[0] == 1 and
            results[1] == 2 and
            results[2] == 3 and
            results[3] == 4 and
            results[4] == 6 and
            results[5] == 7 and
            results[6] == 9)

expect(test_count, 'should return [1, 2, 3, 4, 6, 7, 9] when merging [1, 4, 7]', test)

def test():
    results = merge([], [2, 3, 6, 9])
    return (len(results) == 4 and
            results[0] == 2 and
            results[1] == 3 and
            results[2] == 6 and
            results[3] == 9)

expect(test_count, 'should handle inputs when left argument is empty array', test)

def test():
    results = merge([1, 4, 7], [])
    return (len(results) == 3 and
            results[0] == 1 and
            results[1] == 4 and
            results[2] == 7)

expect(test_count, 'should handle inputs when right argument is empty array', test)

def test():
    results = merge([], [])
    return len(results) == 0

expect(test_count, 'should handle two empty arrays as inputs', test)

print('PASSED: ' + str(test_count[0]) + ' / ' + str(test_count[1]) + '\n\n')

class TestObject(object):
    def __init__(self, x, y):
        self.x = x
        self.y = y
    def __lt__(self, other):
        return self.x < other.x
    def __str__(self):
        return "LTO (%d, %d)" % (self.x, self.y)

Bug

Your code does not work when either the left or right array is empty.

left_item = items1[left_pointer]
right_item = items2[right_pointer]

When item1 or item2 is an empty array this will break with an IndexError.

if right_pointer >= len(items2):
    sorted_list.extend(items1[left_pointer:])
    break
if left_pointer >= len(items1):
    sorted_list.extend(items2[right_pointer:])
    break

You should move this piece of code up to avoid the issue. Now it will break when either is empty.

Tests

I think you can improve your tests quite some bit using the builtin unittest framework.

import unittest

class TestMergeSort(unittest.TestCase):
    def test_normal_merge(self):
        left = [1, 4, 7]
        right = [2, 3, 6, 9]
        out = [1, 2, 3, 4, 6, 7, 9]
        self.assertEqual(merge(left, right), out)

    def test_right_empty(self):
        left = [1, 4, 7]
        right = []
        out = [1, 4, 7]
        self.assertEqual(merge(left, right), out)

    def test_left_empty(self):
        left = []
        right = [2, 3, 6, 9]
        out = [2, 3, 6, 9]
        self.assertEqual(merge(left, right), out)

if __name__ == "__main__":
    unittest.main()

Merge function

An easier way would be to pop the left sides (index=0) of the list and append to the result until either one is empty, after that you can add the remaining to the result.
Naming: I believe you should rename your item1 and item2 to left and right for clarity.
This will no longer have the bug, that it will raise an IndexError when either of the list's are empty.

def merge_sorted_lists(left, right):
    result = []
    while left and right:
        if left[0] < right[0]:
            result.append(left.pop(0))
        else:
            result.append(right.pop(0))
    return result + left + right


Input-reading library for Node.JS Recently I have been busy with nodejs and I thought to create something like this Java input library, so I started with some rough code like below:

'use strict';
const readline = require('readline');
const fs = require('fs');

class StdIn {
  constructor(opt_name) {
    this.name = opt_name;
    this.rs = this.name ? fs.createReadStream(this.name) : process.stdin
  }

  readInt(callback) {
    this.rs.on('data', (chunk) => {
      let line = chunk.toString();
      let numbers = line.trim().split(/\s/);
      numbers.forEach(num => {
        callback.call(this, num);
      });
    });
    this.rs.on('end', () => {
      process.exit(0);
    });
  }

  readLine(callback) {
    this.rs.on('data', (chunk) => {
      let line = chunk.toString();
      callback.call(this, line);
    });
    this.rs.on('end', () => {
      process.exit(0);
    });
  }
}

const stdIn = new StdIn(process.argv[2]);

//stdIn.readInt(num => {
//  console.log(num);
//});

stdIn.readLine(line => {
  console.log(line);
});

Its working fine till now and buts its still incomplete and the code is too messy, the problem clearly visible here that I am trying to push event oriented code into my object oriented world, I am confused basically for two things:

It is practical to do so?
If yes then what would be the best way?

... the problem clearly visible here that I am trying to push event oriented code into my object oriented world

There is nothing wrong with object oriented and event oriented code being intermingled. It's like saying "toast and jam should not be used in the same breakfast." Not only can you mix the two, depending on your needs you should mix the two. The "problem" I think you're referring to is mixing object oriented with functional programming.

Since this is JavaScript, and it handles both programming paradigms good enough, feel free to mix the two. The only thing to consider is "callback Hell" where you have callbacks nested inside callbacks. For this, the Promise Pattern was created.

On to my favorite subject, naming things.

In the StdIn constructor, the argument is called opt_name, yet everything else appears to be written in camelCase. Pick either snake_case or camelCase and be consistent. To be consistent with other API's I would recommend camelCase
The opt_name argument could just be called name since there is not ambiguous reference between the argument and the property on the StdIn object
Rename the rs property to something meaningful, like:

readStream
input
inputStream
stream

Otherwise I don't see any glaring issues.


Cook and Felderman analysis to perform data reduction on heated surfaces This is a "pain", but I wonder if I can get any real speedup with the follow Python script - it is taking well over 10 minutes to calculate the way it is written now. (And that's only when feeding around 1-2 promille of the data).

I have an with shape (roughly): [260, 300, 300]

Now I am doing a cougar=feldermann analysis on this:

import numpy as np
import math
#COOK AND FELDERMAN
def CandF(dT, time, rho, c, k):
    sqrt = np.sqrt
    sumdata=np.zeros(len(time))
    for n in range(len(time)):
        for data in range(1,n+1):
            sumdata[n] += dT[data] / (sqrt(time[n]-time[data])+sqrt(time[n]-time[data-1]))

    qs = (2*sqrt(rho*c*k))/(sqrt(math.pi)) * sumdata
    return qs

def data_reduction_constant_time(dT, timestep, rho, cp, k):
    #print (len(dT), timestep)
    time = np.linspace(0, len(dT)*timestep, int(len(dT)))
    return data_reduction(dT, time, rho, cp, k )

def data_reduction(dT, time, rho, cp, k):
    return CandF(dT, time, rho, cp, k)

class data:
    def __init__(self):
        #normally lots of file reading

        self.integration = 400
        self.cols = 320
        self.rows = 260
        self.depth = 300
        self.readData()

    def readData(self):
        self.ml_delta_temp = 3 * np.random.random_sample((self.cols, self.rows, self.depth))

    def calcQML(self):
        print("--- Calculating q ---")
        print (self.cols, self.rows, self.depth)
        v = np.zeros([self.cols,self.rows,self.depth])
        T = 590
        P = 28 * 100000
        R = 287
        c = 1005
        k = 1.4
        q = 20
        rho = P/(R*T)
        print(v)

        print("-------------")
        for x in range(self.cols ):
            print ("calculating column: ", x)
            for y in range(self.rows ):
                v_tmp = data_reduction_constant_time(self.ml_delta_temp[x,y,:],
                                                                self.integration,
                                                                rho, c, k)
                v[x,y,:] = v_tmp
        print("--- q done ---")
        print(v)

if (__name__ == "__main__"):
    dat = data()
    dat.calcQML()

self.ml_delta_temp is a (260, 300, 300) shaped numpy array. cols, and rows are the size of the array, and integration is the step size (how much time is between each 3rd axis slice).

Now the problem is: on my computer it takes almost a minute to calculate a single row.

How can I speed it up, or as the same question: in what part is this function losing the most time?

I can show the mathematical formula for the Cook and Felderman equation:

$$qCF = \frac{2\sqrt{\rho ck}}{\sqrt{\pi}}\sum_{i=1}^n \frac{T_i-T_{i-1}}{\sqrt{t_n-t_i}+\sqrt{t_n-t_{i+1}}}$$

Berry, S.A. and Horvath, T.J. Discrete
Roughness Transition for Hypersonic Flight Vehicles. Journal of Spacecraft and Rockets, 2:216{
227, March-April 2008. DOI 10.2514/1.30970.

Now this is to get the total heat at time "t_n", I am however interested in the heat flux profile, how it changes over time. So I need to calculate this formula for each n. Also I do know not T, but only delta-T (but as one can see that is only beneficial).

I suggest the following replacement for function CandF():

def CandF3(dT, time, fac):
sqrt = np.sqrt
sumdata = np.zeros(len(time))
for n in range(len(time)):
    tn = time[n]
    # precompute sqrt(tn-time[0])
    last_sq = sqrt(tn - time[0])
    sum_n = 0
    for j in range(1, n + 1):
        sq_j = sqrt(tn - time[j])
        sum_n += dT[j] / (sq_j + last_sq)
        last_sq = sq_j
    sumdata[n] = sum_n
return sumdata * fac

The most rewarding idea here was that from the 2 square roots per innermost loop one is the second is computed repeatedly and can be saved for the next loop (here: last_sq). Halving the number of np.sqrt() calls nearly halves the runtime for large numbers of time steps (len(time)).
More optimizations are the use of a local variable for a loop-invariant (tn), use of a scalar when indexed variable is not indexed incrementally (sum_n), and precomputing the final constant sqrt(rho*c*k*2.0) in the caller. This seems natural as the contributing constants are all defined there.


String compression algorithm using functional JavaScript I've written an algo to compress a string

eg. aabcbbba to a2bcb3a

I'm pretty new to functional programming, I feel it's easier to debug and learn from functional code if you're good at it already, but total opposite if you're not.

I'm wondering how I can make this code cleaner and more functional.

I feel like there's gotta be a better way to do compressCharacters without the need of an array or a result variable (perhaps substituting forEach with something else) as well as reducing the lines of code in groupCharacters

const signature = `aabcbbba`;

const compressString = signature => {
  return compressCharacters(groupCharacters(signature));
}

const groupCharacters = signature => {
  let newSignature = "", arr = [];

  // convert signature to an array
  let result = [...signature].reduce((accumulator, element, index) => {

    // check if last letter in accumulator matches the current element
    if (accumulator[accumulator.length -1] !== element) {
      // add the accumulator string into an array
      arr.push(accumulator);
      // clear the newSignature string and set it to current element
      newSignature = element;
    } else {
      // if it doesn't match, add it to the accumulator
      newSignature = accumulator += element;
    }

    // check if it's the last item - add to array
    if (index === signature.length - 1) arr.push(element);

    // return newSignature to accumulator
    return newSignature;
  })

  return arr;
}

const compressCharacters = arr => {
  let newArray = [];

  let result = arr.forEach(e => e.length > 1 ? newArray.push(`${e[0]}${e.length}`) : newArray.push(e))

  return newArray.join("");
}

compressString(signature);

Style notes

Not putting {, } around single line statement blocks is a bad habit. Always delimit statement code blocks.
In compressCharacters newArray should be a constant. In groupCharacters arr should be a constant.
Consistent naming is important. Your naming is all over the place. You call a string, characters (in compressCharacters), string (in compressString), signature, and e in the forEach. A character you call element. You abbreviate an array to arr in one function and call it newArray in another. Most of the names are describing the type and not the abstracted data that they hold.
Don't add useless or redundant code. In compressCharacters you create a variable result that you do nothing with. Not to mention that forEach does not have a return defined. Also result in groupCharacters is never used.
Don't declare variables outside the scope that they are to be used. newSignature is only used inside reduce but declared outside the callback.

Functional programing means that functions should not have side effect (change the state of anything outside the function's scope.) The reduce callback uses the array arr which breaks the no side effects rule. And the forEach pushes to arr which is also outside the forEach callbacks scope (use map or reduce in that case).
Applying the above you would get something like the following code.
const groupRuns = str => [...str].reduce((groups, char) => {
        const last = groups.length - 1;
        if (last < 0 || groups[last][0] !== char) { groups.push(char) }
        else { groups[last] += char }
        return groups;
    }, []);

const concatGroups = groups => groups.reduce((str, g) =>
        str + (g.length > 2 ? g[0] + g.length : g)
    , "");

const compressString = str => concatGroups(groupRuns(str));

const signature = `aabcbbbaaaaaabcccccdddddddbbba`;
compressString(signature);

Note that rather than add numbers to groups of size 2 the group must be 3 or larger.


C++17 is_integral trait implementation This is a C++17 is_integral trait implementation.

Most implementations exhaust all integral types:

bool
char, char16_t, char32_t, wchar_t
[unsigned] short
[unsigned] [long [long]] int

However, it is possible that the implementation defines some extended signed integer types and extended unsigned integer types. One workaround is to exhaust all the implementations using #ifdefs, but this workaround is not theoretically (in the language lawyer's point of view) portable. For example, if Mickey Mouse just rolls out a brand-new standard-conforming extended integer type, such workarounds fail.

I theorized that for any type T, is_integral<T>::value is true if and only if it is convertible to std::intmax_t or std::uintmax_t via a non-narrowing standard conversion sequence. I tried to deploy this fact, enforcing the two restrictions respectively, by:

using the so-called "uniform initialization" syntax (formally list-initialization) to prevent narrowing conversions; and
using a dummy class (which I call a generator class) with a conversion operator to prevent user-defined conversion sequences.

I tested with the cases of standard integral types (expecting true,) standard floating point types (expecting false,) user-defined types convertible to some standard integral type (expecting false.) All tests passed.

Here is my code, within 40 lines.

// C++17 is_integral implementation
// which handles extended integer types
// in a portable way
// (without resorting to #ifdef chains)

#include <cinttypes> // for std::intmax_t, std::uintmax_t
#include <type_traits> // for std::is_same, std::bool_constant
                       // can be trivially defined manually, just lazy

namespace my_std {
    namespace detail {
        // Generator type to bypass user-defined conversions
        template <typename T>
        struct Generator {
            operator T();
        };
        // helper type to designate substitution failure
        struct Failed {};
        // check functions (exploit SFINAE)
        // uses brace syntax to prevent narrowing conversions
        template <typename T>
        auto check_signed(int) -> decltype(std::intmax_t{Generator<T>{}});
        template <typename T>
        auto check_signed(...) -> Failed;
        template <typename T>
        auto check_unsigned(int) -> decltype(std::uintmax_t{Generator<T>{}});
        template <typename T>
        auto check_unsigned(...) -> Failed;
        // function invoker trait
        template <typename T>
        struct Int_trait {
            static constexpr bool is_signed_int = !std::is_same_v<
                Failed, decltype(check_signed<T>(0))
            >;
            static constexpr bool is_unsigned_int = !std::is_same_v<
                Failed, decltype(check_unsigned<T>(0))
            >;
            static constexpr bool is_int = is_signed_int || is_unsigned_int;
        };
    }
    // "the" trait
    template <typename T>
    struct is_integral
        :std::bool_constant<detail::Int_trait<T>::is_int>
        { };
}

Any criticism or suggestion is highly appreciated!

<joke>The absolute best implementation with maximum portability and simplicity and clarity that is guaranteed to work on all machines is:

#include <type_traits>
namespace my_std {
    template <typename T>
    struct is_integral
        :std::is_integral<T>
        { };
}

</joke>

I tested with the cases of standard integral types (expecting true,) standard floating point types (expecting false,) user-defined types convertible to some standard integral type (expecting false.) All tests passed.

You should have tested with all the other kinds of types in C++. Reference types, pointer types, enum types, nullptr_t, void, pointer-to-member types, etc. etc.

Your is_integral incorrectly reports that int& is an integral type. (Godbolt.)

It's not a hard fix, of course; but the code-review feedback is "Write more tests."


Car Pooling algorithm time complexity I created a solution for the car pooling algorithm.
Problem:
You are driving a vehicle that has capacity empty seats initially available for passengers.  The vehicle only drives east (ie. it cannot turn around and drive west.)
Given a list of trips, trip[i] = [num_passengers, start_location, end_location] contains information about the i-th trip: the number of passengers that must be picked up, and the locations to pick them up and drop them off.  The locations are given as the number of kilometers due east from your vehicle's initial location.
Return true if and only if it is possible to pick up and drop off all passengers for all the given trips.
Example 1:
Input: trips = [[2,1,5],[3,3,7]], capacity = 4
Output: false

Example 2:
Input: trips = [[3,2,7],[3,7,9],[8,3,9]], capacity = 11
Output: true

My solution:
var carPooling = function(trips, capacity) {
    if(!trips || trips.length === 0)    return true;

    trips.sort((a,b) => a[1]-b[1]);
    let dropOff = new Map();
    let seated = 0;
    while(trips.length > 0) {
        const trip = trips.shift();
        const pickup = trip[1];
        for(const [key, value] of dropOff) {
            if(key <= pickup) {
                seated -= value;
                dropOff.delete(key);
            }
        }
        if(seated + trip[0] > capacity)
            return false;
        seated += trip[0];
        if(!dropOff.has(trip[2]))
            dropOff.set(trip[2], trip[0]);
        else
            dropOff.set(trip[2], dropOff.get(trip[2]) + trip[0]);
    }

    return true;
};

Is the time complexity O(N^2logN)? I sort the input array which gives me O(NlogN). Then, I have a nested loop N^2? I'm not sure about this nested loop because it is a loop inside my hash table.
By the way, do you guys have any idea about how to get a better performance?

The sort is not in the loop, so your algorithm is "just" O(n^2) because the loop dominates, the O(n * log(n)) of the sort is neglegable (for big n).
You can solve the problem in O(n * log(n)) and without sorting or destroying the input.

Walk through the trips and build a Map location => {liftUps, dropOffs} (O(n) in time, O(n) in memory)

Iterate the map and build an array of {location, liftUps, dropOffs} (O(n) in time, O(n) in memory)

sort the array by location (this is now the slowest part of the algorithm: O(n * log(n)) in time, O(1) in memory)

iterate the array adding liftUps and subtracting dropOffs for each location to an aggregate integer that starts at zero. (O(n) in time, O(1) in memory)

whenever you surpass capacity return false; return true if the loop finishes

const carPooling = (trips, capacity) => {
    let i = 0;
  const map = new Map();
  for (i = 0; i < trips.length; ++i) {
    const passangers = trips[i][0];
    const start = trips[i][1];
    const end = trips[i][2];

    if (map.has(start)) {
        map.set(start, map.get(start) + passangers);
    } else {
        map.set(start, passangers);
    }

    if (map.has(end)) {
        map.set(end, map.get(end) - passangers);
    } else {
        map.set(end, -passangers);
    }
  }

  const list = new Array(map.size);
  i = 0;
  for (const [location, exchange] of map) {
    list[i++] = {location: location, exchange: exchange}
  }

  list.sort((a, b) => a.location - b.location);

  let occupied = 0;
  for (i = 0; i < list.length; ++i) {
    occupied += list[i].exchange;
    if (occupied > capacity) {
        return false;
    }
  }

  return true;
}

console.log(carPooling([[2,1,5],[3,3,7]], 4));
console.log(carPooling([[3,2,7],[3,7,9],[8,3,9]], 11));


Loop each folder and subfolder to get values Any tips or improvement to speed my code? I'm looping each file in the specify folder and subfolders. I will open each file and get the values I need and copy it to my activesheet. It is taking quit some time, any tips to help?

FolderName = InputBox("Enter path", "Get File")

Dim fso, oFolder, oSubfolder, oFile, queue As Collection

Set fso = CreateObject("Scripting.FileSystemObject")
Set queue = New Collection
queue.Add fso.GetFolder(FolderName)

Do While queue.Count > 0
    Set oFolder = queue(1)
    queue.Remove 1 'dequeue
    For Each oSubfolder In oFolder.SubFolders
        If UCase(oSubfolder.Name) <> "DO NOT USE" Then
            queue.Add oSubfolder 'enqueue
        Else
        End If
    Next oSubfolder
    Dim lastUsedRow As Long
    lastUsedRow = ActiveSheet.Range("A" & ActiveSheet.Rows.Count).End(xlUp).Row + 1
    For Each oFile In oFolder.Files
        'Process each file but exclude files such as "~xxxxxx" or thumbs.db or xxxx.tmp files
        If oFile.Type = "Microsoft Excel Worksheet" Then
            If Not oFile.Name Like "*~*" Then
                Dim app As New Excel.Application
                Dim Filename As String
                Filename = oFile.Path
                app.Visible = False 'Visible is False by default, so this isn't necessary
                Dim book As Excel.Workbook
                Set book = app.Workbooks.Add(Filename)
                ActiveSheet.Range("A" & lastUsedRow) = oFile.Name
                ActiveSheet.Range("B" & lastUsedRow) = oFile.DateCreated
                ActiveSheet.Range("E" & lastUsedRow) = book.Sheets("mySheet").Range("D3").Value
                ActiveSheet.Range("F" & lastUsedRow) = book.Sheets("mySheet").Range("G12").Value
                ActiveSheet.Range("G" & lastUsedRow) = book.Sheets("mySheet").Range("C9").Value
                ActiveSheet.Range("H" & lastUsedRow) = book.Sheets("mySheet").Range("C13").Value
                book.Close SaveChanges:=False
                app.Quit
                Set app = Nothing
                lastUsedRow = ActiveSheet.Range("A" & ActiveSheet.Rows.Count).End(xlUp).Row + 1
            End If
        End If
    Next oFile
Loop

As Mathieu Guindon mentioned in the comments, the main slow down is that you are creating a new Excel.Application for each file.   I would personally just use the current Application and turn off Application.ScreenUpdating.

Using an InputBox to prompt an user to enter a folder path is very prone to error. You should test if the folder exists.

FolderName = InputBox("Enter path", "Get File")
Do While Len(Dir(FolderName, vbDirectory)) = 0
    If Len(Dir(FolderName, vbDirectory)) = 0 Then
        If MsgBox("Do you wish to continue?", vbYesNo, "Invalid Folder") <> vbYes Then
            Exit Sub
        Else
            FolderName = InputBox("Enter path", "Get File")
        End If
    End If
Loop

Or better yet just use Application.FileDialog(msoFileDialogFolderPicker) to pick the folder.  It's the right tool for the job.

Using an Array to collect the data and writing it to a range of cells in one operation is much faster the writing each piece of data to an individual cell.

Refactored Code

Here is how I would write the code.  Notice that I created a subroutine to collect the files and another to get the Folder Path.  This allows me to debug each part of the code separately.

Sub LoopFoldersAndXLFiles()
    Dim t As Double: t = Timer
    Const SheetName As Variant = 1
    Dim FileList As Collection
    addExcelFileList FileList
    If FileList.Count = 0 Then Exit Sub

    Dim oFile As Object, xlApp As New Excel.Application
    Dim r As Long
    Dim results() As Variant
    ReDim results(1 To FileList.Count, 1 To 8)
    For r = 1 To FileList.Count
        Set oFile = FileList(r)
        With xlApp.Workbooks.Add(oFile.Path)
            results(r, 1) = oFile.Name
            results(r, 2) = oFile.DateCreated
            results(r, 5) = .Sheets(SheetName).Range("D3").Value
            results(r, 6) = .Sheets(SheetName).Range("G12").Value
            results(r, 7) = .Sheets(SheetName).Range("C9").Value
            results(r, 8) = .Sheets(SheetName).Range("C13").Value
            .Close SaveChanges:=False
        End With
    Next

    xlApp.Quit

    With ActiveSheet
        With .Range("A" & .Rows.Count).End(xlUp).Offset(1)
            .Resize(UBound(results), UBound(results, 2)).Value = results
        End With
    End With
    Debug.Print Round(Timer - t, 2)
End Sub

Sub addExcelFileList(ByRef FileList As Collection, Optional FolderName As String, Optional fso As Object)
    If Len(FolderName) = 0 Then
        FolderName = getFolderPath
        If Len(FolderName) = 0 Then Exit Sub
    End If
    If FileList Is Nothing Then Set FileList = New Collection
    If fso Is Nothing Then Set fso = CreateObject("Scripting.FileSystemObject")

    Dim oFolder As Object, oSubfolder As Object

    Set oFolder = fso.GetFolder(FolderName)

    For Each oSubfolder In oFolder.SubFolders
        If UCase(oSubfolder.Name) <> "DO NOT USE" Then addExcelFileList FileList, oSubfolder.Path, fso
    Next

    Dim oFile As Object
    For Each oFile In oFolder.Files
        If oFile.Type = "Microsoft Excel Worksheet" And Not oFile.Name Like "*~*" Then FileList.Add oFile
    Next
End Sub

Function getFolderPath() As String
    With Application.FileDialog(msoFileDialogFolderPicker)
        .Title = "Select a Folder"
        .AllowMultiSelect = False
        .InitialFileName = Application.DefaultFilePath
        If .Show <> -1 Then Exit Function
        getFolderPath = .SelectedItems(1)
    End With
End Function


Selection sort algorithm with increasing/decreasing sort options I am running through some traditional algorithms in Go, and I was hoping to get feedback on efficiency and optimization.

Below is a basic selection sort algorithm that takes a parameter to dictate increasing, or decreasing order.

// SelectionSort
// in: A = {31, 41, 59, 26 , 41, 58}
//
// Increasing order
// out: A = {26, 31, 41, 41, 58, 59}

// Illustration:
// 31 | 41 | 59 | 26 | 41 | 58
// 31 | 41 | 59 | 26 | 41 | 58
// 31 | 41 | 59 | 26 | 41 | 58
// 26 | 31 | 41 | 59 | 41 | 58
// 26 | 31 | 41 | 41 | 59 | 58
//
// Decreasing order
// out: A = {59, 58, 41, 41, 31, 26}
//
// Illustration:
// 31 | 41 | 59 | 26 | 41 | 58
// 41 | 31 | 59 | 26 | 41 | 58
// 59 | 41 | 31 | 26 | 41 | 58
// 59 | 41 | 31 | 26 | 41 | 58
// 59 | 41 | 41 | 31 | 26 | 58
// 59 | 58 | 41 | 41 | 31 | 26
package main

import (
    "fmt"
)

// Function that performs a selection sort.
// Second parameter specifies increasing or decreasing order.
func SelectionSort(A []int, S string) []int {
    for i := range A {
        for j := i + 1; j < len(A); j++ {
            switch {
            case S == "increasing":
                if A[i] > A[j] {
                    A[i], A[j] = A[j], A[i]
                }
            case S == "decreasing":
                if A[i] < A[j] {
                    A[i], A[j] = A[j], A[i]
                }
            }
        }
    }
    return A
}

func main() {
    A := []int{31, 41, 59, 26, 41, 58}
    fmt.Println("Unsorted array: ", A)
    fmt.Println("Increasing sort array: ", SelectionSort(A, "increasing"))
    fmt.Println("Decreasing sort array: ", SelectionSort(A, "decreasing"))
}

There are a few things I would point out as being poor go style. The two different nesting loops is where I would start:

for i := range A {
    for j := i + 1; j < len(A); j++ {

The outer loop does a index-only range on the A slice, which in itself is not a problem, but logically it is for i := 0; i < len(A); i++ {. Again, this is not really a problem, other than the fact that you are looping the indexes of the slice. The problem is that in your inner loop, you make that index-loop obvious with the full-syntax loop.

I would be tempted to use a full loop for the outer loop so that the index  functions on i and j are obvious.

The other thing I would suggest is that you supply a function for the S parameter, instead of a string.

While mentioning parameters, it is not common practice to use upper-case parameter names. These parameters are never exported, so should be lower-case initial, and mixedCase for multi-word names.

If you supply a function for the sort order, then your sort function is improved as well. Consider:

func SelectionSort(data []int, ordered func(int, int) bool) []int {
    for i := 0; i < len(data); i++ {
        for j := i + 1; j < len(data); j++ {
            if !ordered(data[i], data[j]) {
                data[i], data[j] = data[j], data[i]
            }
        }
    }
    return data
}

Now, you can call that code with something like:

increasing := func(a, b int) bool {
    return a <= b
}

decreasing := func(a, b int) bool {
    return a > b
}

data := []int{31, 41, 59, 26, 41, 58}
fmt.Println("Increasing sort array: ", SelectionSort(data, increasing))
....


Killing and reviving TV Characters This program displays a list of TV characters. You can kill them off by random or bring them back by name. I have used the random.choice function. I've defined a couple of functions so that the while loop looks a little more clean. In two of the functions, bring_back and check_decision, we assign True to invalid_input if user input is invalid, like a flag.
This program is for learning purposes.
from random import choice

tv_characters = [
    'john', 'henry', 'maria', 'jason',
    'max', 'lara', 'gilbert', 'paris'
    ]

killed_tv_characters = []
valid_decision_input = ['kill off', 'bring back']

start_prompt = "Would you like to start (yes/no)?"
decision_prompt = "\nWould you like to bring back a character you killed?"
decision_prompt += "\nOr would you like to kill off another character?"
decision_prompt += "\nEnter 'kill off' or 'bring back' ->"
character_from_dead_prompt = "\nPlease enter the name of character you would like to bring back: "
confirm_kill_prompt = "\nEnter 'kill off' to kill your first character: "

def kill_tv_characters():
    """Kills a random tv character and displays the name on the screen."""
    if tv_characters:
        killed_tv_character = choice(tv_characters)
        tv_characters.remove(killed_tv_character)
        print(f"You have killed off {killed_tv_character.title()}")
        killed_tv_characters.append(killed_tv_character)
    else:
        print("You have killed off all the characters!")

def bring_back(name):
    """Brings back a dead character."""
    name = name.lower().strip()
    if name in killed_tv_characters:
        killed_tv_characters.remove(name)
        tv_characters.append(name)
        print(f"\nYou have brought {name.title()} back to life!")
    elif name in tv_characters:
        print(f"{name} isn't dead!")
        invalid_input = True
    else:
        print(f"{name} isn't a character!")
        invalid_input = True

def check_decision(user_input):
    """Checks user input for decision."""
    if user_input in valid_decision_input and user_input == 'kill off':
        return user_input
    elif user_input in valid_decision_input and user_input == 'bring back':
        return user_input
    else:
        invalid_input == True
        print("Please enter a valid input!")
print("\nYou can kill off characters randomly and add them back my name!")

start = input(start_prompt)
if start == 'yes':
    start = True
else:
    start = False

while tv_characters and start:
    invalid_input = None

    print("\nThese are the living TV characters:")
    for tv_character in tv_characters:
        print(tv_character.title())
    if killed_tv_characters:
        user_decision = input(decision_prompt)
        decision = check_decision(user_decision)
        if invalid_input:
            continue
        if decision == 'kill off':
            kill_tv_characters()
        if decision == 'bring back':
            character_from_dead = input(character_from_dead_prompt)
            bring_back(character_from_dead)
            if invalid_input:
                continue

    else:
        confirm_kill = input(confirm_kill_prompt)
        if confirm_kill != 'kill off':
            print("You have entered an invalid input.")
            continue
        kill_tv_characters()

if tv_characters == False:
    print("You have killed off all the characters.")
    print("Thanks for playing!")

else:
    print("Goodbye")

User interface decisions
Having a start option is an odd choice. If the user started the program itself, is it likely that they don't want to start the rest of its logic?
Successive concatenation
This:
decision_prompt = "\nWould you like to bring back a character you killed?"
decision_prompt += "\nOr would you like to kill off another character?"
decision_prompt += "\nEnter 'kill off' or 'bring back' ->"

is better off as
DECISION_PROMPT = '''
Would you like to bring back a character you killed?
Or would you like to kill off another character?
Enter 'kill off' or 'bring back' ->'''

Data structures
It seems that the order of killed_tv_characters does not matter, so it should be a set, not a list.
Input validation
This:
"""Checks user input for decision."""
if user_input in valid_decision_input and user_input == 'kill off':
    return user_input
elif user_input in valid_decision_input and user_input == 'bring back':
    return user_input
else:
    invalid_input == True
    print("Please enter a valid input!")

has a few problems:

invalid_input == True doesn't do what you think it does; it's a no-effect statement
This doesn't really benefit from the combination of valid_decision_input (which should be a set) and checking for specific strings
Early-return means that you don't need to use else

Here is an alternative:
if user_input in valid_decision_input:
    return user_input
invalid_input = True
print("Please enter a valid input!")

Other concerns:

This will set a local invalid_input unless you declare it global at the top
Rather than returning a string, consider returning an Enum to narrowly represent user choice

Booleans
This:
if tv_characters == False:

should be
if not tv_characters:


Extending Abstract Person Validator I have the following code which is responsible for the validation in our app (just an example).

Based on the particular tab where user can add person to the database we need to perform additional validation (more fields in each tab).
Fields that always should be validated are name and surname so we provided an abstract class with subclasses that are specialized for each tab (i.e. Sourcing). Is it somehow possible to simplify this code by using some design patterns or something ?

abstract public class AbstractPersonValidator {

    public boolean isInvalid( Person person ) {
        if ( person.getName() == null || person.getName().length() < 3 )
            return true;

        if ( person.getSurname() == null || person.getSurname().length() < 3 )
            return true;

        return false;
    }
}

Extending:

public class SourcingPersonValidator extends AbstractPersonValidator {
    @Override
    public boolean isInvalid( Person person ) {
        if ( super.isInvalid( person ) )
            return true;

        if ( person.getTechnology() == null )
            return true;

        if ( person.getSource() == null )
            return true;

        return false;
    }
}

There are two suggestions I have for situations like this:

double-negatives are bad
use abstract methods for extensions

Let me explain. A method isInvalid(...) is really is not valid, and that can then be used in an expression !isInvalid(...) which is not is not valid... which is complicated. You should reverse the logic in your code, and call the method isValid(...)

That way, your code keeps a better 'read' to it.

The second issue is about how it is best to manage having some base functionality, and extending it for particular purposes. The way I prefer (and I am not sure if there is a formal 'pattern' for this process), is to have the base class do the actual work, and delegate to the specific class for the more fine-grained validation. Your base class would be (note the final on the isValid method....:

abstract public class AbstractPersonValidator {

    public final boolean isValid( Person person ) {
        if ( person.getName() == null || person.getName().length() < 3 ) {
            return false;
        }

        if ( person.getSurname() == null || person.getSurname().length() < 3 ) {
            return false;
        }

        return detailIsValid(person);
    }

    protected abstract boolean detailIsValid();

}

So, that class declares an abstract method that the child classes will need to implement, and the abstract method will do the child-specific checks:

public class SourcingPersonValidator extends AbstractPersonValidator {
    @Override
    protected boolean detailIsValid( Person person ) {
        if ( person.getTechnology() == null ) {
            return false;
        }

        if ( person.getSource() == null ) {
            return false;
        }

        return true;
    }
}

The advantage of doing things this way is that you don't run the risk of a bug where you forget to call super.

Note also that I added braces to your 1-liners. My experience suggests this makes for more reliable code when your code enters a maintenance cycle.


Evaluation function for minimax/alphabeta algorithm for Tic Tac Toe AI I am trying to develop an optimal evaluation function to use in minimax/alpha-beta algorithm for developing tic-tac-toe AI.

I am counting number of circles/crosses in a row/column/diagonal with empty space behind it (with three-in-a-row, there is no empty space). Based on number of symbols in such line, I multiply the separate scores with \$10^{\text{counter} - 1}\$, which results in \$1,10 \text{ or } 100\$ points. I am sure much can be improved, because optimal solution is rarely found and I am having problems using this function in alphabeta algorithm
My question is - How can this function be improved? Small pieces of code and suggestions appreciated.
My code:

private int h(int[][] field, int depth, int player) //final score of the node
        {
            if (win(field, 1)) //if human won
                return -1000; //very bad for MAX=computer
            if (win(field, 0)) //if computer won
                return 1000;

            int heuristics = individualScore(field, 0) - individualScore(field, 1);
            return heuristics;
        }

private int individualScore(int[][] field, int player)
        {

            int sum = 0;
            int otherPlayer = -1;
            if (player == 0) //if computer is the current player
                otherPlayer = 1; //other player is human
            else
                otherPlayer = 0;//Vice versa
            for (int i = 0; i < 3; i++) // rows
            {
                int counter = 0;
                bool rowAvailable = true;
                for (int l = 0; l < 3; l++)
                {
                    if (field[i][l] == player)
                        counter++;
                    if (field[i][l] == otherPlayer)
                    {
                        rowAvailable = false;
                        break;
                    }
                }
                if (rowAvailable && counter > 0)
                    sum += (int)Math.Pow(10, counter - 1);
            }

            for (int i = 0; i < 3; i++) // columns
            {
                int counter = 0;
                bool columnAvailable = true;
                for (int k = 0; k < 3; k++)
                {
                    if (field[k][i] == player)
                        counter++;
                    if (field[k][i] == otherPlayer)
                    {
                        columnAvailable = false;
                        break;
                    }
                }
                if (columnAvailable && counter > 0)
                    sum += (int)Math.Pow(10, counter - 1);
            }
            int counterD = 0;
            bool diagonalAvailable = true;
            for (int i = 0; i < 3; i++) //diagonals
            {
                if (field[i][i] == player)
                    counterD++;
                if (field[i][i] == otherPlayer)
                {
                    diagonalAvailable = false;
                    break;
                }
            }
            if (diagonalAvailable && counterD > 0)
                sum += (int)Math.Pow(10, counterD - 1);
            counterD = 0;
            diagonalAvailable = true;
            int j = 0;
            for (int i = 2; i >= 0; i--)
            {
                if (field[i][j] == player)
                    counterD++;
                if (field[i][j] == otherPlayer)
                {
                    diagonalAvailable = false;
                    break;
                }
            }
            if (diagonalAvailable && counterD > 0)
                sum += (int)Math.Pow(10, counterD - 1);

            return sum;
        }

I'm going to sidestep the complexities of machine learning itself, it is after all a code review.

Unclear method names and unused parameters

private int h(int[][] field, int depth, int player) //final score of the node

h is a really bad method name. What doest this method do? h isn't particularly explanatory. calculateHeuristics would be considerably better.

I have a lot to say on int[][] field which I will address at a later stage in detail.

Both depth and player are part of the method parameters, but they are never used. You should remove them from the parameter list.

Code readability

if (win(field, 1)) //if human won

if (win(field, 0)) //if computer won

0 and 1 are not particularly readably values for "human" and "computer". Use an enum instead. The good thing is that enums can automatically translate to an int, which means that the odds of encountering breaking changes is minimized.

public enum Player
{
    Computer = 0,
    Human = 1
}

And then your code becomes much easier to read, you wouldn't even need the comments anymore:

if (win(field, Player.Human))

if (win(field, Player.Computer))

Secondly, I would rename field to board. "Field" makes it ambiguous as to whether you're referring to the board (similar to "the football field") or a cell on the board (similar to "a form field").

Thirdly, win is not a good name for a method. I suggest renaming it to IsVictoryFor (or any similarly descriptive name). If you then also create a custom class for your board (instead of using int[][] - I will elaborate on this later in the answer), you can move that method to that custom class. This would give you a massive boost in readability:

if(board.IsVictoryFor(Player.Human))

Encapsulation and SRP

int[][] field

I strongly suggest wrapping your int[][] in a custom class. This allows to you use class methods, which help you categorize your methods.

In your current code, you severely lack any application of Single Responsibility Principle (SRP). Especially in topics such as machine learning, you'll find that the complexity of the code increases exponentially, and you should be prepared for this by properly separating your responsibilities before things get out of hand.

As the adage goes, failure to plan is planning to fail. To that end, I suggest improving your code to make future changes/extensions as seamless as possible.

Don't use ints as placeholder values

You have on several occasions used ints to store your data (players, tokens on board cells). Every time you do that, you inherently require whoever reads your code to be aware of the mapping between the used int value and its particular meaning. This is hugely detracting from your code readability.

As a simple fix, you should use enums.

public enum Player { Computer = 0, Human = 1 }
public enum CellValue { Empty = 0, X = 1, O = 2 }

This dramatically increased the readability of your code, as you no longer will need to use hardcoded magical numbers (0 and 1) and can instead rely on human-readable values (Token.X, Player.Human, ...)

Note: You are cleverly relying on the player int matching with the cell int. I would advise against this. If you equate the two, you're effectively hardcoding which player goes first, and it'll be nigh impossible to reuse your code to have the players switch places (or when you want the compluter to play against itself, which you are inevitably going to have to do if you want to run bulk simulations).
Separating the two does come with the disadvantage that you need to perform some additional checking logic, but the reusability of the code makes it worth it.

Magic numbers

Other than the values I already suggested to encapsulate in enums, you also use other literal values throughout your code:

return -1000;

return 1000;

While you luckily didn't spread this throughout your codebase, the principle still applies; you should use const values.

public readonly const int HEURISTIC_POINTS_VICTORY = 1000;
public readonly const int HEURISTIC_POINTS_LOSS = -1000;

This also makes it a lot easier if you want to tweak the numbers in the future, because you only have to adjust them in one location.

Reusability

Currently, you're using powers of 10 to calculate the score (1,10,100). You have used this calculation in multiple locations:

sum += (int)Math.Pow(10, counter - 1);

sum += (int)Math.Pow(10, counter - 1);

sum += (int)Math.Pow(10, counterD - 1);

sum += (int)Math.Pow(10, counterD - 1);

This is not good. If you decide to change your scoring algorithm tomorrow, you're liable to only update some of the code and potentially forget other instances of the same calculation logic. This is a breeding ground for bugs and unexpected behavior.

You can abstract this to a separate method:

public int CalculateScore(int numberOfTokensInLine)
{
    return Math.Pow(10, numberOfTokensInLine);
}

Should you in the future decide to change your score calculation, you only have to adjust this one method; and then you can be certain that you've changed all of the score calculation logic across the codebase.

Note that you could also convert 10 to a constant, but I don't think it's a requirement here, as there's no reason to assume that you want to reuse this particular value in other locations.
However, if you want to be able to tweak the configuration of your learning algorithm in the future, it may be interesting to abstract this into a const or config parameter, so you have quick access to all values from a single location.

Reusability - Part 2

When you look at the individualScore method body, it should become apparent to you that it is quite repetitive. In many different ways, you are always "checking a line of three cells".

Instead of copy/pasting your approach and slightly tweaking it for every use case, you'd be better off making this reusable. I think it's beneficial for me to explain how/why I do things, so I'll deconstruct (and later reconstruct) the algorithm instead of simply presenting you the end result.

Deconstructing/reconstructing is an invaluable tool for writing clean code.

First, some pseudocode to showcase the general pattern of your logic:

1    FOR a particular line
2        IF I am able to win here (= no opponent token in line)
3            CALCULATE score
4            ADD score to total score
5    RETURN total score

This is the most generic pattern that applies to your logic, without duplicating anything. Now, we simply need to fill in the unknowns.

4 and 5 are not difficult, you've already done these.

3 is effectively the CalculateScore method I already mentioned in the earlier chapter.

2 isn't all that hard. Simply check if the particular line contains the opponent's token in any of the fields.

The big issue is in 1, how do we iteratively check all lines?

Step one - Convert the board to a single array

I'm going to assume you already applies my earlier tips, especially implementing the Board class. This enables you to change your underlying data type. To minimize the breaking changes, I'm going to add a conversion from your old data type:

public class Board
{
    //Your old data type, but now it's using the enum
    public Token[][] Fields;

    //Converted to a single (9 cell) array
    public Token[] FieldArray => Fields.SelectMany(row => row);
}

FieldArray is going to be an array with 9 cells, with indexes ranging from 0 to 8.

Step two - Hardcode all index combinations that define a line.

I know that good code often avoids hardcoding, but I'm willing to make an exception here. There are eight possible lines on a tic tac toe board, and that's a mathematical inevitability.
It it possible to calculate these lines at runtime, and e.g. apply the same logic if you suddently decide to play on a board of variable size (4x4, 5x5, ...) but that's beyond the current scope.

public List<List<int>> AllPossibleLines = new List<List<int>>()
{
    { 0, 1, 2 }, //Top row                    |   |
    { 3, 4, 5 }, //Middle row               0 | 1 | 2
    { 6, 7, 8 }, //Bottom row              ___|___|___
                                              |   |
    { 0, 3, 6 }, //Left column              3 | 4 | 5
    { 1, 4, 7 }, //Middle column           ___|___|___
    { 2, 5, 8 }, //Right column               |   |
                                            6 | 7 | 8
    { 0, 4, 8 }, //Diagonal \                 |   |
    { 2, 4, 6 }, //Diagonal /
};

Every "possible line" is a list of three indexes, which refer to the fields of the board. Therefore, if we iterate over AllPossibleLines, we are iterating over all possible lines on the board (all horizontals + all verticals + all diagonals).

Converting the pseudocode to actual code, with what we now know:

private int individualScore(Board currentBoard, Token currentPlayerToken)
{
    int totalScore = 0;

    //Iterate over all possible victory lines
    foreach(var line in AllPossibleLines)
    {
        //Convert the indexes to actual field values
        List<Token> tokens = line.Select(index => currentBoard.FieldArray[index]).ToList();

        //If the line contains an opponent's token, stop processing this line
        if(tokens.Any(token => token != currentPlayerToken && token != Token.Empty))
            continue;

        //Count how many player tokens are on the line
        int playerTokensOnLine = tokens.Count(token => token == currentPlayerToken);

        //Calculate the score and add it to the total
        totalScore += CalculateScore(playerTokensOnLine);
    }

   return totalScore;
}

And this has dramatically simplified your code and enhanced the readability.

Note that I did use LINQ here instead of simple for/foreach loops, specifically because I find that LINQ enhances code readability.
If you're not experienced with LINQ, I highly suggest you look up a tutorial on the basics as LINQ dramatically helps with code readability and not being bogged down by having to manually iterate over collections.

Footnote Remember your win method? You didn't post the content of that method, but you can also rework this using the AllPossibleLines approach:

public bool win(Board board, Token currentPlayerToken)
{
    return AllPossibleLines
               .Any(line =>
                         line.All(index =>
                              board.FieldArray[index] == currentPlayerToken
                         )
                      );
}

The method effectively checks "is there any line in which all of its tokens equal the current player token?".


Producing and comparing maximum sub array sum algorithms This is an exercise in implementing the maximum sub-array sum problem.

It asks for several solutions of complexity:

\$O(n)\$
\$O(n \log n)\$
\$O(n^2)\$
\$O(n^3)\$

For an additional challenge I completed it using Python.

import random
import timeit

#random array generator
def generate_array(item_count, lower_bound, upper_bound):
    number_list = []
    for x in range(1, item_count):
        number_list.append(random.randint(lower_bound, upper_bound))
    return number_list

#cubic brute force O(n^3)
def max_subarray_cubic(array):
    maximum = float('-inf')
    for i in range(1, len(array)):
        for j in range(i, len(array)):
            current_sum = 0
            for k in range(i, j):
                current_sum += array[k]
            maximum = max(maximum, current_sum)
    return maximum

#quadratic brute force O(n^2)
def max_subarray_quadratic(array):
  maximum = float('-inf')
  for i in range(0, len(array)):
    current = 0
    for j in range(i, len(array)):
       current += array[j]
       maximum = max(current, maximum)
  return maximum

#divide and conquer O(n*lg(n))
def max_cross_sum(array, low, mid, high):
  left_sum = float('-inf')
  sum_ = 0
  for i in range(mid, low, -1):
    sum_ += array[i]
    left_sum = max(left_sum, sum_)

  right_sum = float('-inf')
  sum_ = 0
  for i in range(mid + 1, high):
    sum_ += array[i]
    right_sum = max(right_sum, sum_)
  return left_sum + right_sum

def max_subarray_div_conquer(array, low, high):
  if low == high:
    return array[low]
  else:
    mid = (low + high) / 2
    return max(max_subarray_div_conquer(array, low, mid),
               max_subarray_div_conquer(array, mid + 1, high),
               max_cross_sum(array, low, mid, high))

#Kadane's algorithm O(n)
def max_subarray_kadane(array):
    current = maximum = array[0]
    for x in array[1:]:
        current = max(x, current + x)
        maximum = max(maximum, current)
    return maximum

#to facilitate timing each algorithm
def function_timer(function, array, policy):
    start_time = timeit.default_timer()
    if policy == "divide and conquer":
        print("Maximum sub sum for the %s algorithm: %s"
            % (policy, function(array, 0, len(array) - 1)))
    else:
        print("Maximum sub sum for the %s algorithm: %s" % (policy, function(array)))
    print("Running Time: %s seconds.\n" % (timeit.default_timer() - start_time))

magnitude = input('enter vector size: ')
number_list = generate_array(magnitude, -10, 10)

function_timer(max_subarray_cubic, number_list, "cubic")
function_timer(max_subarray_quadratic, number_list, "quadratic")
function_timer(max_subarray_div_conquer, number_list, "divide and conquer")
function_timer(max_subarray_kadane, number_list, "kadane")

I would like:

To know if this is Pythonic? Am I accurately following conventions?
A general review on the accuracy of the algorithms. To the best of my knowledge I completed the requirements, but just in case. Focusing on the divide and conquer algorithm as it heavily differs and the cubic solution, it's surprisingly difficult to think about how to solve something poorly.
Feedback on the way I measure running time. I gave myself a means to run tests but how effective is it? How reliable are the results?

tl;dr: This is pretty good! A few bugs and weird edge cases, and you can be more Pythonic – no code is perfect – but I like what you’ve done.

Is this Pythonic?

There are several things you could do to make this more Pythonic:

Rather than describing functions using a comment above the definition, use a docstring – that’s the preferred way to document functions in Python.
Rather than iterating over the indexes of an array, it’s better to iterate over the elements. It’s the difference between (bad):

for index in range(len(myarray)):
    element = myarray[index]
    do_stuff_with_element()

and (good):

for element in myarray:
    do_stuff_with_element()

If you need both the index and the element, then you should use enumerate().
Indentation should be four spaces, not two.
Look at list comprehensions; they’re very useful. For example, they can reduce your generate_array() function to one line:

def generate_array(count, lower_bound, upper_bound):
    """
    Generates a random array of integers.
    """
    return [random.randint(lower_bound, upper_bound) for _ in range(count)]

Are these accurate implementations of the algorithms?

max_subarray_cubic()

Because all your ranges start at 1, but Python indexes lists at 0, this function doesn't seem to include the first element in the array.  For example:

>>> max_subarray_cubic([1, 2, 3, -100])
5

but taking the first three elements gives a larger sum of 6.  Adjusting the ranges to range(len(array)) seems to fix the problem.

max_subarray_quadratic()

I haven't spotted any bugs in it; this seems to work correctly.
As I explained above, you could make the inner loop more Pythonic by iterating over the elements, like so:

for elem in array[i:]:
    current += elem
    maximum = max(current, maximum)

max_cross_sum()

There should be something in the docstring to explain how to use the low/mid/high arguments; I had to guess and I think I got it wrong. [I didn't realise this was a helper function for max_subarray_div_conquer when I first read it.]
It's quite unusual to see a variable name with a trailing underscore in Python programs. Good job for not overriding the builtin function (a common mistake among new programmers), but you should try to pick a better variable name rather than just appending an underscore.
When the last argument of range() is -1, I prefer to use reversed(), because I think it's easier to read. Combining with my advice above, your first loop becomes:

for elem in reversed(array[low:mid]):
    sum += elem
    # do stuff with elem

max_subarray_div_conquer()

When this function is first called, I have to pass in the length of the array as arguments, which could cause a problem if I pass in incorrect values. I'm repeating information provided by the first argument.

It would be better to check if the user passes in any values, and if not, compute them based on the length of the array:

def max_subarray_div_conquer(array, low=None, high=None):
    if low is None:
        low = 0
    if high is None:
        high = len(array)

I'm using a default argument of None to check whether the user passed anything in; if they didn't, I'll work it out myself.

max_subarray_kadane()

If I pass this function an empty list, it hits an exception in the first line:

Traceback (most recent call last):
  File "tmp/subarrays.py", line 89, in <module>
    function_timer(max_subarray_kadane, [], "kadane")
  File "tmp/subarrays.py", line 76, in function_timer
    print("Maximum sub sum for the %s algorithm: %s" % (policy, function(array)))
  File "tmp/subarrays.py", line 63, in max_subarray_kadane
    current = maximum = array[0]
IndexError: list index out of range

You should check that I've passed in more than just an empty list at the start of this function. You might still want to raise an exception – a ValueError seems appropriate here – but it should provide a more informative message about why something has gone wrong.

You might want to look at doing the same for the rest of your functions: although they don't throw an exception, they tell me that the maximum subarray sum of an empty list is -inf. Does that seem sensible? Is there a meaningful largest subarray sum of an empty list?

How's the running time function?

The function itself is fine. The fact that you have to special case the divide and conquer algorithm is a little annoying, but that can be addressed by removing the need for additional arguments – see above.
One weakness with the mechanism is that you only check the results once, and for a single array. It’s possible that you could get an array which plays to one algorithm's strengths, or a call is unlucky and takes unusually long time.

You'd get more accurate results if (a) you tested with a variety of arrays, and (b) you repeated and averaged the results.
Your test code is in the top-level of the file, which means it will be run whenever this file is executed – whether directly as a script, or imported as a module. A better construction is to put this code inside if __name__ == '__main__', which means it will only be called if the script is run directly.

That allows you to import functions from this file without any of this timing code being called.


D-flop with enable and clear, translated from VHDL to Haskell I am in the process of translating an implementation of dflop_en_clr from VHDL to Haskell:

if (reset = '1') then
     out_1 <= '0';
elsif rising_edge(clk) then
   if (clear_n = '0') then
      out_1 <= '0';
   elsif (enable = '1') then
      out_1 <= in_1;
   end if;
end if;

Types to know:

data PIn = PIn { _in_1    :: Bit
               , _clk     :: Bit
               , _reset   :: Bool
               , _enable  :: Bool
               , _clear_n :: Bool
               } deriving (Eq)

data St = St { _out_1 :: Bit
             } deriving (Eq)

First attempt

 onTrue :: St -> PIn -> Bool -> St
 onTrue st PIn{..} edgeDetect = shouldReset
   where
     shouldReset = if _reset then St 0 else risingEdge
     risingEdge  = if edgeDetect then shouldClear else st
     shouldClear = if _clear_n then St 0 else enabled
     enabled     = if _enable then st{ _out_1 = _in_1 }  else st

Another:

onTrue'' :: St -> PIn -> Bool -> St
onTrue'' st PIn{..} risingEdge = if _reset then St 0
                                 else if risingEdge  $
                                       if not _clear_n then St 0
                                       else if _enable then st{ _out_1 = _in_1 }
                                            else st
                                     else st

If I were to covert a more complicated nest logic then there would possible be a lot of extra "else"

Current:

ifTrueApplyFun :: Bool -> a -> (a -> a) -> a
ifTrueApplyFun cond value fun = if cond then fun value
                                else value
ifTrueReturnFun :: Bool -> (a -> a) -> (a -> a)
ifTrueReturnFun cond fun = if cond then fun
                           else id
onTrue' :: St -> PIn -> Bool -> St
onTrue' st PIn{..} risingEdge = if _reset then St 0
                                else ifTrueApplyFun risingEdge st $
                                       if not _clear_n then (\_ -> St 0)
                                       else ifTrueReturnFun _enable (\newSt -> newSt{ _out_1 = _in_1 })

This is code that needs to be readable and testable.

I actually think your first attempt is the most readable. The advantage over the VDHL implementation is that you're annotating each case with a function name. This makes it more self-documenting.

I would just multi-line your if-statements, like this:

onTrue :: St -> PIn -> Bool -> St
onTrue st PIn{..} edgeDetect = shouldReset
  where
    nullState = St 0
    shouldReset =
      if _reset
        then nullState
        else risingEdge
    risingEdge =
      if edgeDetect
        then shouldClear
        else st
    shouldClear =
      if _clear_n
        then nullState
        else enabled
    enabled =
      if _enable
        then st{ _out_1 = _in_1 }
        else st


Suspiciously redundant Excel macro code I need a little help in finding a better solution. In the screen shot below any change to the forecast line I need to find the value in column B. The screen shot is only a sample in my production sheet the row count is currently 4200.

           Column A  Col B   Col C   Col D  Col E
Row 5              January
Row 6      Prg Desc   SD      FG      GH      DF
Row 7      Forecast    1       2       3       4
Row 8      Actual
Row 9      Compare

My current code does what I need it to, however, I'm sure there is a better method of accomplish my goal. In my production sheet the value in B is in row 5.

CurMonth = target.Offset(5 - target.Row).Value
        If CurMonth = "" Then
                CurMonth = target.Offset((5 - target.Row), -1).Value
            If CurMonth = "" Then
                CurMonth = target.Offset((5 - target.Row), -2).Value
            If CurMonth = "" Then
                CurMonth = target.Offset((5 - target.Row), -3).Value
            End If
            End If

        End If
    Debug.Print CurMonth

Any suggestions would be very appreciated.

Not relevant to your algo, but you shouldn't be using "" like this; use the vbNullString language constant instead, which consumes 0 memory (as opposed to "" which has the overhead of a string).

As for suggestions, in order to find a better way to do something, it's important to first understand what's going on. And proper indentation greatly helps for that:

CurMonth = target.Offset(5 - target.Row).Value
If CurMonth = vbNullString Then
    CurMonth = target.Offset((5 - target.Row), -1).Value
    If CurMonth = vbNullString Then
        CurMonth = target.Offset((5 - target.Row), -2).Value
        If CurMonth = vbNullString Then
            CurMonth = target.Offset((5 - target.Row), -3).Value
        End If
    End If
End If
Debug.Print CurMonth

Something sticks out right away - every new nested level seems to increment the offset, so what you really have is this:

CurMonth = target.Offset((5 - target.Row), n).Value
If CurMonth = vbNullString Then
    n = n - 1
    [and do that again while n > -3]

This looks like a recursive logic - something like this:

Private Function GetCurMonth(ByVal n As Long) As String
    Dim result As String

    result = target.Offset((5 - target.Row), n).Value
    If result = vbNullString And n > -3 Then
        n = n - 1
        result = GetCurMonth(n) 'recursive call
    End If

    GetCurMonth = result
End Sub

Also CurMonth has no reason not to be called CurrentMonth I guess. As Comintern mentioned, it would be easier to give a better review with more [code] context.


Caesar Cipher encryptor using ASCII I have built a Caesar Cipher in Ruby that utilizes ASCII for the encryption. I am not sure whether or not this was the best way to go about the task but this way made the most sense to me. Please give me any feedback that comes to mind.

puts 'Please enter your string to be coded'
    string = gets.chomp.split("")

puts 'Please enter your number key'
    key = gets.chomp.to_i

    if (key > 26)
        key = key % 26
    end

    string.map! do |i|
        i.ord
    end

    string.map! {|i| i = i + key}.map! {|i|
        if (i > 122)
            then i = i - 26
        elsif (90 < i && i < 97)
            then i = i - 26
        elsif (i > 96 && (i - key) < 91 && (i - key) > 64)
            then i = i - 26
        elsif (i < 65 )
            then i = 32
        else
            i
        end
    }

    string.map! do |i|
        i.chr
    end

    puts "Your coded word is #{string.join("")}"

puts 'Return back to original string? (Yes or No)'
    response = gets.chomp.downcase

    if response == 'yes'
        string.to_s.split("")
        string.map! do |i|
            i.ord
        end

        string.map! {|i| i - key}.map! {|i|
            if (90 < i && i < 97)
                then i = i + 26
            elsif ((i+key) > 96 && i < 91 && i > 64)
                then i = i + 26
            elsif (i < 65 && (i+key) < 91 && (i + key) > 64)
                then i = i + 26
            elsif (i < 65)
                then i = 32
            else
                i
            end
        }

        string.map! do |i|
            i.chr
        end

        puts string.join("")

    end

Your code is one block, one piece of code that goes from start to finish, mixing logic and user-interface. While it works (Great!) this is sub-optimal for readibility and maintanability.

I will now show you what I think is a more readable approach. (The concept is the same, but this code will behave a bit differently than yours. Adapting it to your needs is left as an exercise for the reader).

Without farther ado, let's start:

 require 'arrow_test'

This code imports an automated testing framework I wrote, inatalling it is as easy as gem install arrow_test in the command line. Automated tests run in milliseconds to verify the correctness of your code.

ALPHABET = ("a".."z").to_a

The alphabet. Note that UPPERCASE means that it cannot be changed. It is a constant.

# Moves a letter forward in the alphabet by
# the given key wrapping around.
#

A short summary of what the function does, method level comments are encouraged over line level comments.

#
# shift('a', 2) #=> 'c'
#
#
#
# shift('z', 2) #=> 'b'
#

Example usage that doubles as tests, you may change them to see what happens.

 def shift(letter, key)
     ALPHABET[ (ALPHABET.index(letter) + key) % ALPHABET.length]
 end

This is a function, takes an input and has an output, It applies the Caesar cipher logic to a single character. The modulo operator % makes it behave like the alphabet is a circle. You may see the example with z.

 # Encrypts a text using by shifting all its letters by
 # the given key. All characters that are not lower and alphabetic
 # are deleted.
 #
 #
 # monoalphabetic_cipher('abcd', 2) #=> 'cdef'
 #
 #
 #
 # monoalphabetic_cipher('cdef', 2, decode=true) #=> 'abcd'
 #

A short phrase of description and a pair of tests like the other function.

 def monoalphabetic_cipher(text, key, decode=false)
   text
    .chars
    .select { |char| ALPHABET.include?(char) }
    .map {|char| shift(char, (decode ? 26 - key : key) )}
    .join
 end

Here I discard the non-lowercase chars, (select line), and apply the shift to each char (map line). chars and join are just to go from string to list and viceversa, not important. Seeing a 'flowchart' of the code may help understanding.

And this is all, try it with:

puts 'Enter shift and then text'
shift = gets.chomp.to_i
text = gets.chomp
puts monoalphabetic_cipher(text, shift)

Now user interface and logic are separated and the logic is tested. Feel free to ask any question


Return first duplicate This is a problem from Codesignal.
The guaranteed constraints are 1 <= a.length <= 10^5, and 1 <= a[i] <= a.length.
You have to return the first duplicate encoutered in a, or -1 if no duplicate is found.
def firstDuplicate(a):

    b = [0] * 100001
    for num in a:
        if b[num] == 1:
            return num
        else:
            b[num] = 1
    return -1

Is there a better way to do this? Perhaps faster or using less memory? The unused 0 index in the b array also doesn't feel very clean.

I'll propose an alternate implementation, because dictionaries are good at storing key-value pairs and you don't care about the value:
def first_duplicate(given_list):
    seen = set()
    for value in given_list:
        if value in seen:
            return value
        seen.add(value)
    return -1

A set will buy you basically the same behaviour as the "keys of a dictionary" for these purposes.


Object factory from interface types I have a library that defines a bunch of ISprite interfaces, like IPieceSprite and ITileSprite. Then, it defines a global ISpriteFactory that should be supplied to the library upon initialization. ISpriteFactory should create concrete objects based on the interfaces mentioned.

I made this rough implementation, but it makes my skin crawl. What would be a better approach to this problem?

public class SpriteFactoryMock: ISpriteFactory {

    public SpriteType Create<SpriteType>() where SpriteType: ISprite {

        var type = typeof(SpriteType);
        if (type == typeof(ITileSprite))    return (SpriteType)(Object)(new TileSprite());
        if (type == typeof(IPieceSprite))   return (SpriteType)(Object)(new PieceSprite());

        return default(SpriteType);
    }
}

Considering other alternatives, I guess I could create a method for every ISprite interface, and that would work a bit better. I might do that if all hope for the previous approach is lost.

There is a very simple way to handle this but that is all dependant on the fact you have only one implementation of each ISprite impelementation per interface type. If thats the case (and they all have parameterless constructors) you can simply use reflection to get your type and then construct it.

class SpriteFactory : ISpriteFactory
{
    public T Create<T>() where T : ISprite
    {
        var type = typeof(T);
        var spriteType = AppDomain.CurrentDomain.GetAssemblies()
                                                .SelectMany(s => s.GetTypes())
                                                .Where(p => type.IsAssignableFrom(p) && p.IsClass)
                                                .FirstOrDefault();

        if (spriteType == null)
            return default(T);

        return (T)Activator.CreateInstance(spriteType);
    }
}

Now there is alot wrong with this implementation as it assumes alot.

That there is only one implenetation of the ISprite type passed in.
It does an assembly scan everytime you need to create a new instance of this object.
Requires all implementations to have a default parameterless constructor on the implementation.

We could approach this by only scanning for the object once. But again we are stuck with the issues in point 1 and 3. This implementation is even uglier and (not tested) doesn't appear to be too thread safe.

class SpriteFactory : ISpriteFactory
{
    static object _locker = new object();

    Dictionary<Type, Type> registrations = new Dictionary<Type, Type>();

    public T Create<T>() where T : ISprite
    {
        var type = typeof(T);
        if (!registrations.ContainsKey(type))
        {
            lock (_locker)
            {
                var spriteType = AppDomain.CurrentDomain.GetAssemblies()
                                                        .SelectMany(s => s.GetTypes())
                                                        .Where(p => type.IsAssignableFrom(p) && p.IsClass)
                                                        .FirstOrDefault();
                registrations[type] = spriteType;
            }
        }
        var regType = registrations[type];
        return regType != null ? (T)Activator.CreateInstance(regType) : default(T);
    }
}

What you are really after is some sort of dependency injection and do away with the SpriteFactory all together. Dependency injection was made for this type of work and although the initial code is usually a bit more its typically more elegant. Especially when it comes to managing constructor injection, object disposal etc. The real power comes in when you need to start extending your application and adding more types.

Now without depenency injection you could define your registrations some where else. Where although it is similar to your first approach does abstract the implementation. Something like.

interface ISpriteFactory
{
    T Create<T>() where T : ISprite;
}

class SpriteFactory : ISpriteFactory
{
    static Dictionary<Type, Func<object>> registrations = new Dictionary<Type, Func<object>>();

    public static void AddRegistration<T>(Func<T> aquire)
    {
        registrations[typeof(T)] = aquire as Func<object>;
    }

    public T Create<T>() where T : ISprite
    {
        if (registrations.ContainsKey(typeof(T)))
            return (T)registrations[typeof(T)]();
        return default(T);
    }
}

Now what I have done is basically created static method to create registrations of your ISprite implementations.  This can be used as such.

class Program
{
    static void Main(string[] args)
    {
        SpriteFactory.AddRegistration<IPieceSprite>(() => new PieceSprite());
        SpriteFactory.AddRegistration<ITileSprite>(() => new TileSprite());

        var factory = new SpriteFactory();

        var piece = factory.Create<IPieceSprite>();
    }
}

All in all without a proper framework in place to take care of your dependency resolution and life-time scopes you are stuck with either having our initial 3 problems or writing out your object creation methods manually.


Check the equality for each subset of a string S against the target T and return a count I wrote the following code to solve this leetcode problem:

var numDistinct = function(s, t) {
    if (!s.length) return +(!t.length);
    let res = 0;
    for (let seq of subseq(s)) res += +(t===seq);
    return res;
};
var subseq = function(a) {
    if (a.length==1) return [a[0], ''];
    let subsets = subseq(a.substr(0, a.length-1));
    let n = subsets.length;
    for (let i=0; i < n; i++) {
       subsets.push(subsets[i]+a[a.length-1]);
    }
    return subsets;
};

Obviously, this code is not efficient, as it generates each of \$2^n\$ subsets of the string s and checks each one for equality against t.

I would much appreciate it if someone could explain how to improve this solution using memoization. I am aware that there is a dynamic programming solution. I am more interested in learning how to extend this easy-to-understand solution into something more efficient by using a memo table. Any and all help would be graciously appreciated.

I'd just like to point out, that the code IMHO has far too many "length optimizations". Just take the first line:

if (!s.length) return +(!t.length);

I've been a JS developer for over 20 years and I couldn't say off the top of my head what + does to a boolean. It would be much more readable as:

if (s.length === 0) {
  return (t.length === 0) ? 1 : 0;
}

Also a comment why these are the correct return values would be very helpful.


Cipher decryption and encryption I'm brand new to C# and I've started to make a cipher decryption program. The code works and runs fine, apart from the odd exception. I'm just looking for ways to improve my code and make more efficient.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.IO;

namespace Application
{
class Caesar
{
    static void Main(string[] args)
    {

       try{

        Console.WriteLine("Enter Key:");
        int k = Convert.ToInt32(Console.ReadLine());
        Console.WriteLine("Press E for Encrypting and D for Decrypting:");
        string choice =Convert.ToString(Console.ReadLine()).ToUpper();

        switch (choice)
        {
            case "E": Console.Write("Enter Plain Text:");
                      string pt = Console.ReadLine();
                      caesar_cipher(k, pt);
                       break;

            case "D": Console.Write("Enter Cipher :");
                       string ct = Console.ReadLine();
                      caesar_decipher(k, ct);
                      break;

            default: Console.WriteLine("You've entered an incorrect   option!");
                       break;
        }
       }
       catch(Exception)
       {
           Console.WriteLine ("The value you entered is incorrect");
           Console.WriteLine ("Press any key to try again");

       }

    }
    static void caesar_cipher(int key, string pt)
    {
        int size = pt.Length;
        char[] value = new char[size];
        char[] cipher = new char[size];
        for (int r = 0; r < size; r++)
        {
            value[r] = Convert.ToChar(pt.Substring(r, 1));
        }

        for (int re = 0; re < size; re++)
        {
            int count = 0;
            int a = Convert.ToInt32(value[re]);
            for (int y = 1; y <= key; y++)
            {
                if (count == 0)
                {
                    if (a == 90)
                    { a = 64; }
                    else if (a == 122)
                    { a = 96; }
                    cipher[re] = Convert.ToChar(a + y);
                    count++;
                }
                else
                {
                    int b = Convert.ToInt32(cipher[re]);
                    if (b == 90)
                    { b = 64; }
                    else if (b == 122)
                    { b = 96; }
                    cipher[re] = Convert.ToChar(b + 1);

                }
            }
        }
        string ciphertext = "";

        for (int p = 0; p < size; p++)
        {
            ciphertext = ciphertext + cipher[p].ToString();
        }
        Console.WriteLine("Cipher Text=");
        Console.WriteLine(ciphertext.ToUpper());
    }

    static void caesar_decipher(int key, string ct)
    {
        int size = ct.Length;
        char[] value = new char[size];
        char[] cipher = new char[size];
        for (int r = 0; r < size; r++)
        {
            cipher[r] = Convert.ToChar(ct.Substring(r, 1));
        }

        for (int re = 0; re < size; re++)
        {
            int count = 0;
            int a = Convert.ToInt32(cipher[re]);
            for (int y = 1; y <= key; y++)
            {
                if (count == 0)
                {
                    if (a == 65)
                    { a = 91; }
                    else if (a == 97)
                    { a = 123; }
                    value[re] = Convert.ToChar(a - y);
                    count++;
                }
                else
                {
                    int b = Convert.ToInt32(value[re]);
                    if (b == 65)
                    { b = 91; }
                    else if (b == 97)
                    { b = 123; }
                    value[re] = Convert.ToChar(b - 1);

                }
            }
        }
        string plaintext = "";

        for (int p = 0; p < size; p++)
        {
            plaintext = plaintext + value[p].ToString();
        }
        Console.WriteLine("Plain Text=");
        Console.WriteLine(plaintext.ToLower());
    }
}
}

Code bloat

A simple task is simple. Almost 150 lines of code are not justified for such an easy task. You imported Linq so you arguably know about it, actually using it may do very good to your code.

A general, pseudo-code view of the caesarCipher function may be:

public static string caesarEncrypt(string text, int key) {
    return text.Map( x -> alphabet.include?(x) ? shiftChar(x, key) : x );
}

shiftChar may be written as:

return alphabet [ alphabet.index(char) % alphabet.length ]

And you are done.

And caesarDecrypt is just caesarEncrypt with key = 26 - key, there is no need to write the whole thing again.


Preventing duplicate numbers from being entered I recently wrote a program involving a MySQL database using C#. The program is simple: it allows users to enter numbers and ensures that no same number is entered twice. Now I know that it is not efficient and I need help in order to improve the code.

    using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using MySql.Data.MySqlClient;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace Final
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }
        int i=0;
        int k = 0;
        int count = 0;
        int handler = 0;
        int loop = 0;
        private bool read_data(MySqlConnection def)
        {
            bool yes = false;
            int k = 0;
            int loop = 0;
          while(true)
          {
              string query = "SELECT CNIC FROM `voters`.`voters` WHERE ID=" + k.ToString() + ";";
              MySqlCommand cmd = new MySqlCommand(query, def);
              cmd.ExecuteNonQuery();
              MySqlDataReader Reader = cmd.ExecuteReader();
              if (Reader.Read())
              {
                  loop++;
                  k++;
                  if (textBox1.Text.Equals(Reader.GetString(0)))
                  {
                      yes = true;
                  }
                  Reader.Close();
              }
              else
              {
                  Reader.Close();
                  break;
              }
          }
          return yes;
        }
        private void button1_Click(object sender, EventArgs e)
        {
            bool check;
            string query;
            string CNIC = textBox1.Text;
            string connectionString = "server=localhost;user=root;database=voters;port=3306;password=admin;";
            MySqlConnection conn = new MySqlConnection(connectionString);
            conn.Open();
            if(k==0)
            {
               query = "DELETE FROM `voters`.`voters`;" + "INSERT INTO `voters`.`voters`(`ID`,`Name`,`CNIC`) VALUES(" + i.ToString() + ",'Random'," + CNIC + ");";
             MySqlCommand comd = new MySqlCommand("DELETE FROM `voters`.`voters`;", conn);
                comd.ExecuteNonQuery();
                i++;
            }else{
              query = "INSERT INTO `voters`.`voters`(`ID`,`Name`,`CNIC`) VALUES(" + i.ToString() + ",'Random'," + CNIC + ");";
              i++;
            }
            k=1;
            check = read_data(conn);
            if(!check)
            {
                MySqlCommand cmd = new MySqlCommand(query, conn);
                cmd.ExecuteNonQuery();
            }
            else
            {
                MessageBox.Show("ERROR!.CNIC Already VOTED");
            }
            conn.Close();
        }
    }

}

Use prepared statement

You should never ever formulate an SQL string from variables.
You should use parameterized prepared statements,
and let a library fill in the parameters using the right native types.

Instead of this:

  string query = "SELECT CNIC FROM `voters`.`voters` WHERE ID=" + k.ToString() + ";";
  MySqlCommand cmd = new MySqlCommand(query, def);
  cmd.ExecuteNonQuery();

You should do like this:

var cmd = new MySqlCommand("SELECT CNIC FROM `voters`.`voters` WHERE ID=@id");
cmd.Parameters.AddWithValue("@id", k);
cmd.Prepare();
MySqlDataReader Reader = cmd.ExecuteReader();

This technique effectively prevents the most malicious SQL injection attacks.

Apply this to all your queries.

Extremely confusing code

What is the purpose of setting query when k == 0 in this code?

if(k==0)
{
   query = "DELETE FROM `voters`.`voters`;" + "INSERT INTO `voters`.`voters`(`ID`,`Name`,`CNIC`) VALUES(" + i.ToString() + ",'Random'," + CNIC + ");";
   MySqlCommand comd = new MySqlCommand("DELETE FROM `voters`.`voters`;", conn);
    comd.ExecuteNonQuery();
    i++;
}else{
  query = "INSERT INTO `voters`.`voters`(`ID`,`Name`,`CNIC`) VALUES(" + i.ToString() + ",'Random'," + CNIC + ");";
  i++;
}

=> Nothing. That query is not used, instead, DELETE FROM voters.voters is executed, wiping out the table. Extremely confusing.

read_data

You want to make your program more efficient,
and as you commented to @Hosch,
you want to know the logic is correct.
But the problem is,
without well-named functions and variables,
it's really hard to figure out what your intended logic is.
A function called as vague as "read data",
returning a variable named "yes",
is really hard to review.
We literally have to decipher your intentions,
as it was an encrypted message.

Let's first go over everything that's wrong with this method:

Variables

Not declared in the smallest necessary scope. This is a problem, as readers have to read the entire function to understand all the impact of a variable
Poor names, as mentioned, because it's impossible to guess the intention behind a variable named "yes", or "loop", or "k"
Unused variables, that have no purpose at all, like "loop"

Flow control: what is going on in that while loop? Count up k from 0, select CNIC where ID=k. Exit the loop when there is no match with ID=k. For each row selected one by one, check if CNIC is equal to the value in textBox1, and set yes = true.

This is far more convoluted than it needs to be
After you set yes = true, it's pointless to continue looping, since at that point you already have the final return value for the function
Having Reader.Close() twice is a huge warning sign, you can probably refactor this to close only in one place
For all operations that open resources, you should wrap in a using block

Mixture of SQL and conditions in code: It's inefficient to do a SELECT, and then do additional filtering using conditionals in code. It's best to do all filtering in the WHERE clause of the SELECT, so that the data you get from the query is ready to use, and you also don't waste bandwidth in the communication with the database.

This is untested, but I think should at least point you in the right direction:

private bool HasAlreadyVoted(MySqlConnection conn, string cnic)
{
    string query = "SELECT 1 FROM voters.voters WHERE CNIC = @cnic LIMIT 1";
    using (MySqlCommand cmd = new MySqlCommand(query, conn))
    {
        cmd.Parameters.AddWithValue("@cnic", cnic);
        cmd.Prepare();
        using (MySqlDataReader Reader = cmd.ExecuteReader())
        {
            return Reader.Read();
        }
    }
}

Preventing duplicate votes

First of all, you should use unique constraints on the database table itself. Then when you try to insert a duplicate vote, the query will throw a unique constraint violation exception, which you can catch and respond to.

Another thing, when you do multiple operations in the database that must be all executed without interruption, or not execute at all, such as a delete followed by an insert, then you should wrap these operations in a transaction.

Cleaning up the rest

Apply the same logic to button1_Click as I showed above for read_data.
Some tips:

Start by renaming everything. Go over every single element in your code, the class name, method names, variable names, and ask yourself:

What is the purpose of this class/method/variable?
Does the name reflect that purpose? Would another person understand the purpose by looking at the name?
Is this the right place to declare this variable? Can it be moved to a smaller scope, to avoid exposing it to places where it's not used at all?

Check that all resources are correctly closed. Almost everything that needs to be closed can be put in a using block, which is better, and it also saves you from explicitly calling Close()
All parameterized queries should use prepared statements, and never be constructed as a simple string.


Simple UDP server/client written in Python to report back IP address I'd like to get some feedback/criticism on my attempt to write a simple client/server application that reports back the (client's) global IP address.

It would be nice if you could comment on the UDP socket setup, general security or any other drawbacks you were able to spot. Since this is my first attempt in writing Python network code (or Python in general other than tinkering) please be harsh!

#! /usr/bin/env python

# grip 0.0.1, external IP address lookup utility.
# usage: grip hostname [port]
#        grip -l [hostname] [port]
#        grip -h

from __future__ import print_function

import sys
from socket import *

MODE_SERVER = 0
MODE_CLIENT = 1

READ_BUFFER = 15
NULL_BUFFER = 1

DEFAULT_HOST = ''
DEFAULT_PORT = 20815

EXIT_SUCCESS = 0
EXIT_FAILURE = 1
EXIT_SYNTAX  = 2

USAGE = '''\
grip 0.0.1, external IP address lookup utility.
usage: grip hostname [port]
       grip -l [hostname] [port]
       grip -h

Options:
  -h        display this help message and exit
  -l        run in listen mode, to report back remote hosts' IP addresses

Request mode:
  The hostname may be provided in either IP address or DNS format.
  The port number - if unspecified - defaults to 20815.

Listen mode:
  In listen mode the program will try to serve requests on all
  available interfaces unless explicitly told not to.\
'''

def num(n):
  try:
    return int(n)
  except ValueError:
    return None

class Server:
  """Reports back the remote host's IP address"""
  def __init__(self, socket):
    self.socket = socket

  def listen(self, host, port):
    self.socket.bind((host, port))

    try:
      while 1:
        data, address = self.socket.recvfrom(NULL_BUFFER)
        self.socket.sendto(address[0], address)
    except KeyboardInterrupt:
      pass

    return EXIT_SUCCESS

class Client:
  """xyz"""
  def __init__(self, socket):
    self.socket = socket

  def request(self, host, port):
    code = EXIT_SUCCESS

    self.socket.sendto('', (host,port))
    self.socket.settimeout(2)

    try:
      data, saddr = self.socket.recvfrom(READ_BUFFER)
      print(data)
    except:
      code = EXIT_FAILURE;
      print('Error: Request timed out', file=sys.stderr)

    self.socket.close()
    return code

def main():
  args   = list(sys.argv[1:])
  argsno = len(args)

  mode = MODE_CLIENT

  hostname = DEFAULT_HOST
  port = DEFAULT_PORT

  # Mere invocation is NOT supported
  if argsno == 0:
    print(USAGE, file=sys.stderr)
    sys.exit(EXIT_SYNTAX)

  # Display a helpfull message when asked
  if '-h' in args:
    print(USAGE)
    sys.exit(EXIT_SUCCESS)

  if '-l' in args:
    mode = MODE_SERVER

    args.remove('-l')
    argsno -= 1

  if mode == MODE_SERVER:
    if argsno > 1:
      hostname = args[0]
      port = num(args[1])

    if argsno == 1:
      if num(args[0]) is not None:
        port = num(args[0])
      else:
        hostname = args[0]

    try:
      serv = Server(socket(AF_INET, SOCK_DGRAM))
      retval = serv.listen(hostname, port)
    except:
      sys.exit(EXIT_FAILURE)

  if mode == MODE_CLIENT:
    hostname = args[0]

    if argsno > 1:
      port = num(args[1])

    try:
      cl = Client(socket(AF_INET, SOCK_DGRAM))
      retval = cl.request(hostname, port)
    except:
      sys.exit(EXIT_FAILURE)

  sys.exit(retval)

if __name__ == "__main__":
  main()

sys.argv is already a list. I don't see a purpose of list(sys.argv).
Creating sockets in the mainline looks strange. It is more natural to create them in corresponding __init__ calls. Same applies to serv.listen().
from socket import * is really bad practice, especially when socket is used as a parameter:

self.socket = socket

makes me wonder whether the parameter is an instance or a callable.
recvfrom may raise a socket.error exception. Some of them are recoverable, so you may want to catch then as well.