Average movie rankings Given a list of tuples of the form, (a, b, c), is there a more direct or optimized for calculating the average of all the c's with PySpark? Below is what I have, but I feel like there is a more direct/optimized approach?

Classic movie recommendation example, where each tuple is (userID, movieID, rating). How do we get the average of all of the ratings in a direct/optimized fashion?

ds_movie = sc.parallelize([(1,1,2.25), (1,2,3.0), (2,1,4.5)])
total = (ds_movie
         .map(lambda (userid, movieid, rating): rating)
         .reduce(lambda x, y: x + y))
num = ds_movie.count()
average = total / num
# in this example, average = 3.25

I would recommend using mean method:

ds_movie.map(lambda (userid, movieid, rating): rating).mean()

It is not only more concise but should have much better numerical properties (it is using a modified version of the online algorithm).

On a side note it is better to avoid tuple parameter unpacking which has been removed in Python 3. You can check PEP-3113 for details. Instead you can use Rating class as follows:

from pyspark.mllib.recommendation import Rating

ratings = ds_movie.map(lambda xs: Rating(*xs))
ratings.map(lambda r: r.rating).mean()

indexing (arguably much uglier than unpacking):

ds_movie.map(lambda r: r[2]).mean()

or standard function instead of lambda expression (kind of verbose for such a simple use case):

def get_rating(rating):
    userid, movieid, rating = rating
    return rating

ds_movie.map(get_rating).mean()


Repeatedly keep removing 2 smallest elements and add its sum back to the list and return newly inserted elements' total I have a list of unsorted numbers. Each time, I need to remove 2 smallest numbers and insert back its sum into the list. Repeatedly do this till the 1 final element is remaining in the list.

Return the total sum of newly inserted elements

My logic was to use heap sort, remove 2 elems, insert elems sum back - repeat.

This program of input of 10 elements has called heapify function 310 times!

Link: http://cpp.sh/8vkjd

How do I optimize this scenario?

Use Binary Search Tree?
Use Heap Sort on LinkedList?
Is this possible with graphs?
Since there are several insertions, I'm trying to only restrict to O(N) or \$O(\log N)\$ or \$O(n \log N)\$ insertion.
Can this be done with 2 lists?
Since the elements will be sorted after the 1st iteration, is there a good insertion algorithm that does faster insertion?

#include <iostream>
#include <sstream>
#include <vector>
int count = 0; // A global variable to keep track of number of time heapify function is called
void heapify(std::vector<int> &arr, int n, int i)
{
   count++;
   int largest = i;
   int l = 2*i + 1;
   int r = 2*i + 2;

   if(l<n && arr[l] >arr[largest])
       largest = l;

   if(r<n && arr[r] > arr[largest])
       largest = r;

   if(largest != i)
   {
       std::swap(arr[i], arr[largest]);
       heapify(arr, n, largest);
   }
}

void heapSort(std::vector<int> &arr, int n)
{
   for(int i=n/2 -1 ; i>=0 ; i--)
   {
       heapify(arr,n,i);
   }

   for(int i=n-1 ; i>=0 ; i--)
   {
       std::swap(arr[0],arr[i]);
       heapify(arr,i,0);
   }

}

void printArray(std::vector<int> &arr, int n)
{
   for(int i=0 ; i<n ; i++)
   {
       std::cout << arr[i] << " ";
   }
   std::cout << std::endl;
}

int main()
{
   std::vector<int> arr{1,2,3,4,5,6,7,8,9,10};
   int n = arr.size();
   printArray(arr,n);
   heapSort(arr,n);

   int ans=0;
   int tempEle=0;
   while(arr.size()>1)
   {
       heapSort(arr,n);
       tempEle = arr[0]+arr[1];
       arr.erase(arr.begin(),arr.begin()+2);
       ans+=tempEle;
       arr.push_back(tempEle);
       printArray(arr,arr.size());
   }
   ans+=arr[0];

   std::cout << "Repeatedly adding all elements, 2 small elements at a time and reducing it to a single number : " << ans << std::endl;

   std::cout << "Heapify was called " << count << " times" << std::endl;
   return 0;
}

And for an input of 5000 numbers, the heapify() func is being called 246 million times.

#include <sstream>

Only include what is necessary.

    for(int i=0 ; i<n ; i++) { ... }

Use range-based for.

    std::cout << ... << std::endl;

Avoid std::endl.

void printArray(std::vector<int> &arr, int n) { ... }
                                       ^^^^^
int main() {
    int n = arr.size();
          ^^^^^^^^^^^^
    printArray(arr,n);
                   ^

You have an implicit conversions from std::vector::size_type to int when you initialize n and pass an int argument to parameter n.

    printArray(arr, n);
    heapSort(arr, n);

std::vector carries its size. You do not have to capture it in a variable and pass it.

    int n = arr.size();
    while (arr.size() > 1) {
        heapSort(arr, n);
        arr.erase(...);
        arr.push_back(...);
    }

With each iteration of the loop, you pop 2 elements and push 1 element reducing the size of arr by 1. n is a local copy of arr.size() and is never updated. This results in out-of-bounds index access in your sort functions after the first iteration of this loop.

As for your issues with excessive calls to heapify(), you are calling heapSort() on every iteration. heapSort() takes a sequence in any order, sorts all of the values into heap order, then sorts all of the values again into sequential sorted order. This is done so you can operate on the two minimum values at the same time. Instead of fully sorting the sequence on every iteration, you can take a partial sort approach. Maintain the sequence in heap order by sifting values around as you mutate the sequence. Finding each minimum values is simply a swap and a sift down. Repairing the heap order on insertion is simply a sift up. Which brings me to know your <algorithm>s. The C++ standard library provides heap operations (is, make, push, pop, sort).

    // Sort into min-heap order
    std::make_heap(arr.begin(), arr.end(), std::greater())

    while (arr.size() > 1) {
        // rotate out the 1st min keeping heap order intact
        std::pop_heap(arr.begin(), arr.end(), std::greater());
        const auto first = arr.back();
        arr.pop_back();

        // rotate out the 2nd min keeping heap order intact
        std::pop_heap(arr.begin(), arr.end(), std::greater());
        const auto second = arr.back();
        arr.pop_back();

        const auto sum = first + second;
        ans += sum;

        // Move the inserted value into its heap order position
        arr.push_back(sum);
        std::push_heap(arr.begin(), arr.end(), std::greater());
    }

std::priority_queue helps clean up some of the duplication.

    std::priority_queue min_heap{std::greater(), std::move(arr)};
    while (min_heap.size() > 1) {
        const auto first = min_heap.top();
        min_heap.pop();
        const auto second = min_heap.top();
        min_heap.pop();

        const auto sum = first + second;
        ans += sum;
        min_heap.push(sum);
    }


Compiling Go with python I've made a small script to compile Go code with Python.

The result is not something I'm happy with, because:

It seems overly complicated
I'm worried the nested if-else with f string are unreadable
Am I using the subprocess module the correct way?

Any review is welcome.

#!/usr/bin/env python3

import argparse
import subprocess
import os

ARCHS = {
    "32": "386",
    "64": "amd64"
}

BUILDTYPES = ["exe", "dll"]

def parse_arguments():
    parser = argparse.ArgumentParser(usage='%(prog)s [options] <buildtype> <architecture>',
                                     description='Go builder',
                                     formatter_class=argparse.RawDescriptionHelpFormatter,
                                     epilog='''
Python program to build go payloads

Examples:
    ./builder exe 32 --stripped
    ./builder dll 64 --upx --stripped
    ./builder dll 32
''')
    parser.add_argument("buildtype", help="The type to build the paylopad with", type=str)
    parser.add_argument("architecture", help="The architecture to build the paylopad with", type=str, default="32")
    parser.add_argument("--stripped", "-s", help="Strip the payload of symbols", action="store_true", default=False)
    parser.add_argument("--upx", "-u", help="Pack the payload with upx", action="store_true", default=False)
    arguments = parser.parse_args()

    if not arguments.buildtype.lower() in BUILDTYPES:
        parser.error(f"{arguments.buildtype} is not a valid buildtype can only be {', '.join(BUILDTYPES)}")

    if not arguments.architecture.lower() in ARCHS:
        parser.error(f"{arguments.architecture} is not a valid architecture can only be {', '.join(ARCHS.keys())}")
    arguments.architecture = ARCHS[arguments.architecture]

    return arguments

def build_go(**kwargs):
    enviroment = {
        **os.environ,
        "GOOS": "windows",
        "GOARCH": f"{kwargs['architecture']}"
    }
    if kwargs['buildtype'].lower() == "dll":
        enviroment["CGO_ENABLED"] = "1"
        enviroment["CC"] = f"{'i686' if kwargs['architecture'] == '386' else 'x86_64'}-w64-mingw32-gcc"

    build_type = f"go build -buildmode=c-shared" if kwargs["buildtype"].lower() == "dll" else "go build -tags exe"
    stripped = "-s -w" if kwargs['stripped'] else ""
    builder = f'''{build_type} -o GOLoader.{kwargs['buildtype']} -ldflags "-H=windowsgui {stripped}"'''
    subprocess.check_output(builder, shell=True, env=enviroment)

    if kwargs["stripped"]:
        stripped = f"strip GOLoader.{kwargs['buildtype']}"
        subprocess.check_output(stripped, shell=True)

    if kwargs["upx"]:
        upx = f"upx -f GOLoader.{kwargs['buildtype']} -9 --ultra-brute -o GOLoader.upx.{kwargs['buildtype']}"
        subprocess.check_output(upx, shell=True)

if __name__ == '__main__':
    args = parse_arguments()
    build_go(**vars(args))

Some minor notes.

I love the ternary operator and f strings, but...

Here are three ways to express the same thing.  There is the original way, a DRY version of the original way, and a more traditional if/else.  Which is easier to read and understand what is going on?

# variant 1
build_type = f"go build -buildmode=c-shared" if kwargs["buildtype"].lower() == "dll" else "go build -tags exe"

# variant 2
build_type = f"go build {'-buildmode=c-shared' if kwargs['buildtype'].lower() == 'dll' else '-tags exe'}"

# variant 3
if kwargs["buildtype"].lower() == "dll":
    build_type = "go build -buildmode=c-shared"
else:
    build_type = "go build -tags exe"

In this case I would argue that the third variant is MUCH easier to understand what the distinction is between the two build types.  EG: What question is being asked (buildtype) and what is the difference in the result based on the answer.

Environment has a "n" after the "o"

This:

enviroment["CGO_ENABLED"] = "1"

should likely be:

environment["CGO_ENABLED"] = "1"

Click is the thing.

Not sure this is immediately relevant as a code review, but...

I much prefer click to argparse.  In this simple case there is not likely a huge advantage for Click, but there is little downside.  And if you get comfortable with click and the size of your project increases, I think Click can be quite advantageous.


Upvote/downvote toggle I'm sure there is a better way to implement this feature. I am making an app that allows a user "upvote" or "downvote" a post. Right now, the code works and disables the upvote button if pressed and vice versa with the downvote button (the idea is that a user can only upvote or downvote once).

post.js

import React, { Component } from 'react';
import { Card, CardText, CardHeader } from 'material-ui/Card';
import FlatButton from 'material-ui/FlatButton';

class Post extends Component {

  constructor(props) {
    super(props);

    this.state = {
      disabledUpvote: false,
      disabledDownvote: false
    }

  }

  handleUpvoteClicked() {
    const downvoteDisabled = this.state.disabledDownvote;

    if (downvoteDisabled == false) {
      this.setState({
        disabledUpvote: true,
        disabledDownvote: false
      });
    } else {
      this.setState({
        disabledUpvote: true,
        disabledDownvote: false
      });
    }
  }

  handleDownvoteClicked() {
    const upvoteDisabled = this.state.disabledUpvote;

    if (upvoteDisabled == false) {
      this.setState({
        disabledDownvote: true,
        disabledUpvote: false
      });
    } else {
      this.setState({
        disabledDownvote: true,
        disabledUpvote: false
      });
    }
  }

  render() {
    const { content, createdAt, votes } = this.props.post;

    return (

        <Card>
          <CardText>

              {moment(createdAt).fromNow()}

            {content}

              <FlatButton
                label="Upvote"
                primary={true}
                disabled={this.state.disabledUpvote}
                onClick={this.handleUpvoteClicked.bind(this)} />
              <FlatButton
                label="Downvote"
                secondary={true}
                disabled={this.state.disabledDownvote}
                onClick={this.handleDownvoteClicked.bind(this)} />

          </CardText>
        </Card>

    );
  }
}

export default Post;

I am specifically looking for help with the handleUpvoteClicked() and handleDownvoteClicked() functions as they both appear to be somewhat redundant.

One thing I noticed is that you're passing this.handleUpvoteClicked.bind(this) as a prop to your FlatButton components. The problem is that the call to Function.prototype.bind creates a new function instance for both click handlers every time your Post renders, meaning your FlatButton components see a different value (function reference) being passed in as a prop, and they'll also re-render. Probably not a big deal for your simple setup, but it's something to keep in mind for later, because it can drastically affect performance for bigger apps.

The solution is to bind your methods in the constructor. That way .bind(this) is only called once during construction (good because that method is a little slow), and your components will see a consistent function reference being passed in as a prop and will know they don't have to re-render.

I also tweaked your conditionals inside the click handlers a bit, so that they only run if the button isn't disabled.

Here's what I came up with:

import React, { Component } from 'react';
import { Card, CardText, CardHeader } from 'material-ui/Card';
import FlatButton from 'material-ui/FlatButton';

class Post extends Component {

  constructor(props) {
    super(props);

    this.state = {
      disabledUpvote: false,
      disabledDownvote: false
    }

    this.handleUpvoteClicked = this.handleUpvoteClicked.bind(this);
    this.handleDownvoteClicked = this.handleDownvoteClicked.bind(this);
  }

  handleUpvoteClicked() {
    if (!this.state.disabledUpvote) {
      this.setState({
        disabledUpvote: true,
        disabledDownvote: false
      });
    }
  }

  handleDownvoteClicked() {
    if (!this.state.disabledDownvote) {
      this.setState({
        disabledUpvote: false,
        disabledDownvote: true
      });
    }
  }

  render() {
    const { content, createdAt, votes } = this.props.post;

    return (

        <Card>
          <CardText>

              {moment(createdAt).fromNow()}

            {content}

              <FlatButton
                label="Upvote"
                primary={true}
                disabled={this.state.disabledUpvote}
                onClick={this.handleUpvoteClicked} />
              <FlatButton
                label="Downvote"
                secondary={true}
                disabled={this.state.disabledDownvote}
                onClick={this.handleDownvoteClicked} />

          </CardText>
        </Card>

    );
  }
}

export default Post;


Iterate two lists and find matches I have 2 lists with varying length. Both lists contain data which has to be matched with entries from the other list. Currently I am iterating both lists with a nested for-loop.

List<MyClass1> list2 = myClass1dao.findUnmatchedEntries();
List<MyClass2> list1 = myClass2dao.findUnmatchedEntries();

for (MyClass2 list1entry : list1) {
    for (MyClass1 list2entry : list2) {
        if (list1entry.getName().equals(list2entry.getName())
            && list1entry.getID().equals(list2entry.getID())) {
            //update entries
            myClass2dao.update(list1entry);
            myClass1dao.update(list2entry);
            break;
        }
    }
}

I know that this takes exponentially longer the more elements each list has.

So my question is, how can this code be improved?

Note: I am on a java 7 code base so answers to java 7 are preferred, but java 8 is okay as well

I would strongly consider adding a Comparable abstract class with two implementations (one containing a MyClass1, and the other a MyClass2). At that point you can use all the normal collection framework methods to process this. Alternatively you could transform both sets into something like HashMap<String,HashMap<String,List<MyClassX>>>, then just lookup by the name and id.

Additionally, if the lists are sorted, we could do this much more efficiently, by getting two iterators, then simply getting the next element of the one that is behind at each stage.


Check for a win on the board This code is part of a tic tac toe game. It uses a 2D array for the board.

The functionality of the following code is to check for a win on the board, in rows, columns and diagonals. How can I improve it?

public bool WhoWon(Cell cell)
{
    for (int i = 0; i < 3; i++)
    {
        for (int j = 0; j < 3; j++)
        {
            if (_board[i, 0] == cell && _board[i, 1] == cell && _board[i, 2] == cell)
            {
                return true;
            }
            else if (_board[0, j] == cell && _board[1, j] == cell && _board[2, j] == cell)
            {
                return true;
            }
        }
    }

    if (_board[0, 0] == cell && _board[1,1] == cell && _board[2,2] == cell)
    {
        return true;
    }
    else if (_board[0, 2] == cell && _board[1, 1] == cell && _board[2, 0] == cell)
    {
        return true;
    }
    return false;
}

What you've done here is listed all possible checks, but you've compiled it into a single method. Ideally, you should split this up into separate tasks, as there are three categories of similar checks:

Horizontal checks
Vertical checks
Diagonal checks

Maybe the current example is just simple enough to not fully warrant it, but especially if you had had many different win conditions (e.g. another rule that states making a 2x2 square also counts as a win), you'd understand why you need to break your validation into smaller (logically coherent) parts.

The method

public bool WhoWon(Cell cell)

I agree with the other answers here. The name of the method does not match its return type. WhoWon should return a player. HasWon should return a boolean.

I'm also not a fan of the "cell" name. Looking at the code, cell seems to contain the player's letter (X or O).  That is not immediately apparent by calling it a cell, a cell would be a box on the board.

E.g. board[1,1] is a cell, as it represents a box on the board. But I would call the value of board[1,1] a token, i.e. the symbol that is contained within (if it's not empty).

You haven't shared the how Cell is defined. This may already be close to what I'd prefer to use. Based on this, let me suggest an improvement:

public enum Token { X, O, Empty }

public Token[,] _board = new Token[3,3];

public bool HasWon(Token token)
{
    // ...
}

Notice that you could also implement WhoWon() based on HasWon():

public Token WhoWon()
{
    if(HasWon(Token.X))
        return Token.X;

    if(HasWon(Token.O))
        return Token.O;

    return Token.Empty;
}

The method body

I suggest splitting this up, as it will dramatically simplify the validation logic and make it easier to parse.

public enum Token { X, O, Empty }
public Token[,] _board = new Token[3,3];

public bool HasWon(Token token)
{
    return
        IsHorizontalVictory(token)
        || IsVerticalVictory(token)
        || IsDiagonalVictory(token);

}

private bool IsHorizontalVictory(Token token)
{
    for(int y = 0; y <= 2; y++)
    {
        if(_board[0,y] == token && _board[1,y] == token && _board[2,y] == token)
            return true;
    }

    //No victory condition hit = no victory.
    return false;
}

private bool IsVerticalVictory(Token token)
{
    for(int x = 0; x <= 2; x++)
    {
        if(_board[x,0] == token && _board[x,1] == token && _board[x,2] == token)
            return true;
    }

    //No victory condition hit = no victory.
    return false;
}

private bool IsDiagonalVictory(Token token)
{
    if(_board[0,0] == token && _board[1,1] == token && _board[2,2] == token)
        return true;

    if(_board[0,2] == token && _board[1,1] == token && _board[2,0] == token)
        return true;

    //No victory condition hit = no victory.
    return false;
}

Comments:

You could further abstract this, e.g. by making the dimensions of the board variable. However, TicTacToe is always played on a 3x3 board, so this seems unnecessary at the moment.
Instead of using a multidimensional array, you could create a full OO representation of the board with its cells. This is going to cost some effort to put together, but it will make board operations (such as checking for victory) nicer to read. Index-based retrieval is nice when simple, but can become horrendously complex to follow if the logic gets to be too big.
Some things could have been rewritten, e.g. the IsDiagonalVictory() method could have merged the two checks (+ the false return value) into one boolean: return (logic_for_diag_1) || (logic_for_diag_2). However, this would've storngly affected the readability of the code. As the performance difference is negligible, I therefore chose the most readable option.

This was a headscratcher

for (int i = 0; i < 3; i++)  //A
{
    for (int j = 0; j < 3; j++)   //B
    {
        //A
        if (_board[i, 0] == cell && _board[i, 1] == cell && _board[i, 2] == cell)
        {
            return true;
        }

        //B
        else if (_board[0, j] == cell && _board[1, j] == cell && _board[2, j] == cell)
        {
            return true;
        }
    }
}

It took me a while to understand what your intention was here. You've combined two completely separate for-loops, and have for some reason decided to intwine them.

I marked them A and B in the code, because these two are completely separate pieces of logic. Splitting them up:

for (int i = 0; i < 3; i++)  //A
{
    if (_board[i, 0] == cell && _board[i, 1] == cell && _board[i, 2] == cell)
    {
        return true;
    }
}

for (int j = 0; j < 3; j++)   //B
{
    if (_board[0, j] == cell && _board[1, j] == cell && _board[2, j] == cell)
    {
        return true;
    }
}

You may be wondering, does it matter?

Yes, it does. First of all, splitting them dramatically enhances the readability of the code.

Secondly, you're going to end up doing less loops.

Nested for-loops are multiplicative. 3 loops for A times 3 loops for B = 9 loops. Furthermore, each of the 9 loops is doing two checks, thus resulting in 18 checks occurring.
Separate for-loops are additive. 3 loops for A plus 3 loops for B = 6 loops. Each loop only does one check, thus resulting in 6 checks occuring.

Your nested loops were performing every unique check three separate times (18 / 6 = 3), which serves no functional purpose.

Let's look at only the vertical check:

if (_board[i, 0] == cell && _board[i, 1] == cell && _board[i, 2] == cell)

This was being executed for every pair of i,j, while you only needed to test for every value of i.

This is a comparison of the pairs (i,j) and what the vertical check actually did:

(0,0)   =>   Checks vertical column 0
(0,1)   =>   Checks vertical column 1
(0,2)   =>   Checks vertical column 2
(1,0)   =>   Checks vertical column 0
(1,1)   =>   Checks vertical column 1
(1,2)   =>   Checks vertical column 2
(2,0)   =>   Checks vertical column 0
(2,1)   =>   Checks vertical column 1
(2,2)   =>   Checks vertical column 2

Notice how it repeats the same thing for every value of j (0,1,2), even though the value of j has no impact on the outcome of the result.

This is a comparison of the values of i and what the vertical check actually did:

(0)   =>   Checks vertical column 0
(1)   =>   Checks vertical column 1
(2)   =>   Checks vertical column 2

By not observing the value of j, we haven't repeated the same check for every distinct value of j.


Combining two arrays using nested foreach loops I have the following arrays:

Array  (db_values)
(
[0] => Array
    (
        [system_name] => object_car_brand
        [value] => Alfa Romeo
        [id] => 136
    )

[1] => Array
    (
        [system_name] => object_car_model
        [value] => Spider
        [id] => 137
    )

)
Array (db_attributes)
(
[0] => Array
    (
        [id] => 105
        [system_name] => object_car_brand
    )

[1] => Array
    (
        [id] => 106
        [system_name] => object_car_model
    )

)

I combine these two using the following code:

       foreach($db_attributes as $db_attribute){
            foreach($db_values as $db_value){
                if($db_value["system_name"] === $db_attribute["system_name"]){
                    $update[$db_attribute["id"]] = $db_value["value"];
                }

            }

        }

I do not think that this is the most resource friendly way of doing it, is there a better way?

I have made the assumption that there is a 1:1 relationships between the $attributes and $values array elements. With that I mean the array key in the $attributes array corresponds with an entry in $values array.

If that is the case it can be reduced to one foreach loop by using the key from the attributes array:

$combined = []; // Make sure the $combined array exists.

foreach($attributes as $key => $attribute) {

    // First check if the array key exists and that the 'system_name' is the same
    if(array_key_exists($key, $values) && $attribute['system_name'] == $values[$key]['system_name']) {

        $combined[$attribute['id']] = $values[$key]['value'];

    }

}

This should produce the following array with the data you have provided:

Array(combined)
(
    [106] => 'Alfa Romeo'
)

If my assumption is incorrect, then just ignore my answer.


Read list of dictionaries with nested dictionaries I have a file as content a list of dictionaries (Around 75000). For instance, this is an example of first line I got when reading the file (value for v):

{
 "id": 1,
 "name": "Explosives",
 "category_id": 1,
 "average_price": 294,
 "is_rare": 0,
 "max_buy_price": 755,
 "max_sell_price": 1774,
 "min_buy_price": 99,
 "min_sell_price": 18,
 "buy_price_lower_average": 176,
 "sell_price_upper_average": 924,
 "is_non_marketable": 0,
 "ed_id": 128049204,
 "category": {
   "id": 1,
   "name": "Chemicals"
 }
}

My actual working code is :

for v in d:
    commodities_reference = []
    for k, g in v.items():
        if isinstance(g, dict):
            dict1 = g
            my_value1 = dict1.get("id")
            my_value2 = dict1.get("name")

    for s, i in v.items():
        if not isinstance(i, dict):
            commodities_reference.append(i)
    commodities_reference.append(my_value1)
    commodities_reference.append(my_value2)

Output wanted = All the values in same list in the same order for doing a SQL INSERT Statement afterwards (Meaning values from the nested dict must be also at the end.)

[1, 'Explosives', 1, 294, 0, 755, 1774, 99, 18, 176, 924, 0, 128049204, 1, 'Chemicals']

From a performance perspective, with SQLITE3/python 3.7, it is a catastrophe. I am looking for some advices in order to make it more efficient. I am thinking about using executemany statement but it seems it takes tuple instead of list.

Currently you iterate over each dictionary twice. You can do it in one pass:

for v in d:
    commodities_reference = []
    for k, g in v.items():
        if isinstance(g, dict):
            commodities_reference.append(g["id"])
            commodities_reference.append(g["name"])
        else:
            commodities_reference.append(g)

Note that this appends the values when it encounters it. This means that in Python < 3.7 (cPython < 3.6) it is not guaranteed that the dictionary is actually the last item to be looked at, since dictionaries were not to guaranteed to be in insertion order.

You could even make this a generator and slightly more general:

def get_values_recursive(x):
    for value in x.values():
        if isinstance(value, dict):
            yield from get_values_recursive(value)
        else:
            yield value

for v in d:
    commodities_reference = list(get_values_recursive(v))
    # do something with it...
    print(commodities_reference)

When using the given example, this is the result:

>>> list(get_values_recursive(v))
[1, 'Explosives', 1, 294, 0, 755, 1774, 99, 18, 176, 924, 0, 128049204, 1, 'Chemicals']

When putting your code into a function, this generator is almost twice as fast with the given v:

In [13]: %timeit op(v)
5.32 µs ± 43.3 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [14]: %timeit list(get_values_recursive(v))
3.64 µs ± 10.8 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

Note that both take on the order of micro seconds, so unless you need to process more than 100000 items per second your bottleneck is probably in those SQL statements and how you execute them.


Stack data structure implementation in Java I recently made a data structure, stack implementation in Java and I would like feedback on my code to improve it (other than commenting/documenting my code).

public class Stack {

    private int topIndex, maxSize;
    private int[] stack;

    public Stack(int size) {
        maxSize = size;
        stack = new int[maxSize];
        topIndex = -1;
    }

    public int maxSize() {
        return maxSize;
    }

    public int size() {
        return topIndex + 1;
    }

    public boolean isEmpty() {
        return topIndex == -1;
    }

    public boolean isFull() {
        return (topIndex + 1) == maxSize;
    }

    public void push(int value) {
        if (!isFull()) {
            topIndex++;
            stack[topIndex] = value;
        }
    }

    public int peek() {
        if (!isEmpty()) {
            return stack[topIndex];

        } else {
            return 0;
        }
    }

    public void pop() {
        if (!isEmpty()) {
            topIndex--;
        }
    }

    public static void main(String[] args) {

    }

}

I'd like to give some thoughts which spring to my mind when I read your given code. Now, you have an implementation specific for primitive ints. A more advanced step would of course be to implement a generic Stack like the one in the JDK, just mentioning it.

First up, I like that you use clear variable names, and that you reuse methods like isEmtpy \ isFull. Kudos for that! As an aside, you do not need the maxSize variable, as it is the same as stack.length.

It is a fixed size stack, so it does mean you have to know beforehand how much elements it would take up, in order to not waste space (or run out of it). Perhaps for a follow up exercise you could try and make it dynamically resizable?

Now, for the push method. I would not let it silently succeed if the stack is already full. Seeing as it is a fixed size stack, I would probably opt to throw an Exception, because to me it looks like a programming error. A possible second idea could be to return a boolean, signaling succes or failure (though I don't think it is the better solution).

Same with peek. It now silently succeeds and returns 0, which is very well a valid value which could reside in the stack (EmptyStackException exists by the way!). The boolean solution would not work here of course. Also, I personally do not think the extra new line is an improvement, on the contrary.

Finally, to me pop means taking off the top element of the stack. So why not return it?


Override equals() and hashCode() using reflection I wrote a utility method to override equals() using reflection. This works fine, but I wonder if this code will pass all the tests.

public static boolean checkObjectsEqualityFromInstance(Object currentObj,Object otherObj) throws Exception{
      if(currentObj==null || otherObj==null){
          return false;
      }
      else if(otherObj.getClass()!=null&&currentObj.getClass()!=null&&!currentObj.getClass().isInstance(otherObj)){
          return false;
      }
      boolean result =true;
        Field[] fields = otherObj.getClass().getDeclaredFields();
        Field[] currentObjFields = currentObj.getClass().getDeclaredFields();

        Map<String, Object> attriButeNameValueMap=null; /*This map to store *property name and its value of any of object in my calse the otherObj */
        try {
            attriButeNameValueMap=new HashMap<>();
            for (Field field : fields) {
                field.setAccessible(true);

                Object value = field.get(otherObj);
                attriButeNameValueMap.put(field.getName(), value);
            }
            for (Field field : currentObjFields) {
                field.setAccessible(true);

                Object value = field.get(currentObj);
                if(attriButeNameValueMap.containsKey(field.getName())){
                    if(value instanceof Boolean){
                result=areEqual((Boolean)value, (Boolean)attriButeNameValueMap.get(field.getName()));
                    }
                    else if(value instanceof Character){
                        result=result&&areEqual((Character)value, (Character)attriButeNameValueMap.get(field.getName()));
                            }
                    else if(value instanceof Long){
                        result=result&&areEqual((Long)value, (Long)attriButeNameValueMap.get(field.getName()));
                    }
                    else if(value instanceof Float){
                        result=result&&areEqual((Float)value, (Float)attriButeNameValueMap.get(field.getName()));
                    }
                    else if(value instanceof Double){
                        result=result&&areEqual((Double)value, (Double)attriButeNameValueMap.get(field.getName()));
                    }
                    else if(value instanceof Object[]){
                        result=result&&  Arrays.equals((Object[])value, (Object[])attriButeNameValueMap.get(field.getName()));
                    }
                    else{
                        result=result&&areEqual(value, attriButeNameValueMap.get(field.getName()));

                    }
                    if(!result){
                        return result;
                    }
                }
                else{
                    return false;
                }
            }

        } catch (IllegalArgumentException | IllegalAccessException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
throw e;
        }
        return result;
    }

The areEqual() methods are overloaded for different types.

It ignores values in superclasses. You might want to check that too.

Anyway, don't reinvent the weel, there is a library for that! I guess EqualsBuilder.reflectionEquals from Apache Commons Lang does exactly what you want.  It also has solutions to corner cases, like

transient fields (since they are likely derived fields, and not part of the value of the Object) in a configurable way
generated fields with $ (for inner classes).

You can also exclude field names from the comparison.

It's open-source, you can check the source for further details.

See also: Effective Java, 2nd edition, Item 47: Know and use the libraries (The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)
The else keyword is unnecessary here:

if(currentObj==null || otherObj==null){
    return false;
}
else if(otherObj.getClass()!=null&&currentObj.getClass()!=null&&!currentObj.getClass().isInstance(otherObj)){
    return false;
}

The following is the same:

if (currentObj == null || otherObj == null){
    return false;
}
if (otherObj.getClass() != null && currentObj.getClass() != null &&
        !currentObj.getClass().isInstance(otherObj)) {
    return false;
}

It's called guard clause. I've also used a little bit more spacing to make it readable (easier separaton of values, comparisons, method calls).
Guard clauses would make this easier to follow too:

  if(value instanceof Boolean){
      result=areEqual((Boolean)value, (Boolean)attriButeNameValueMap.get(field.getName()));
  }
  else if(value instanceof Character){
      result=result&&areEqual((Character)value, (Character)attriButeNameValueMap.get(field.getName()));
  }
  else if(value instanceof Long){
      result=result&&areEqual((Long)value, (Long)attriButeNameValueMap.get(field.getName()));
  }
  ...
  if(!result){
      return result;
  }

I've used a few exaplanatory variable to remove some duplication:

Object value = field.get(currentObj);
String fieldName = field.getName();
if(attriButeNameValueMap.containsKey(fieldName)){
      Object otherFieldValue = attriButeNameValueMap.get(fieldName);
      if (value instanceof Boolean) {
          if (!areEqual((Boolean) value, (Boolean) otherFieldValue)) {
              return false;
          }
      }
      if (value instanceof Character) {
          if (!areEqual((Character) value, (Character) otherFieldValue)) {
              return false;
          }
      }

Note that it makes superfluous the result variable. The last line of the method could be simply the following, since its never modified anymore:

return true;

Formatting is not consistent. Indentation sometimes is two spaces, sometimes four.

  Map<String, Object> attriButeNameValueMap=null;
  /*This map to store *property name and its value of any of object in my calse the otherObj */
  try {
      attriButeNameValueMap=new HashMap<>();

B should be lowercase in the variable name (I guess it's just a typo) and you could declare it inside the try block:

I'd also rename it to otherObjectAttributeValues to make the comment unnecessary:

try {
    Map<String, Object> otherObjectAttributeValues = new HashMap<>();

(Effective Java, Second Edition, Item 45: Minimize the scope of local variables)
Consistent formatting would be great here too:

          } catch (IllegalArgumentException | IllegalAccessException e) {
              // TODO Auto-generated catch block
              e.printStackTrace();
  throw e;
          }

Furthermore, TODO comments does not suggest professional work. Fix that and remove the comment.

I'd throw out the IllegalAccessException or wrap it into a RuntimeException. The IllegalArgumentException already a RuntimeException, so you don't have to put it into the method signature.


Unpacking tuple from Django's get_or_create function Django's get_or_create function returns 2 values: the object and a boolean whether it was freshly created or not. I'm only really interested in the object it returns and this is how I'm doing it.

keywords = [element for (element, _) in [Keyword.objects.get_or_create(keyword=key) for key in keywords]]

But this doesn't look particularly elegant. Is there a better way to do this, possibly avoiding the double loop?

I'd say this is a good example on how to not use DB.

The problem here is that for each keyword you will call a DB instead of perform this as a single call, so if you list of keywords is 1000 items then you will do 1000 calls to DB.

You can avoid this by splitting your call to:

1. Get existing keywords

existing_keywords = Keyword.objects.filter(keyword__in=keywords).values_list('keyword', flat=True)

2. Get list of not existing keywords

keywords = set(keywords)
not_existing_keywords = keywords.difference(existing_keywords)

3. Create objects with no existing keywords

Keyword.objects.bulk_create(Keyword(keyword=key) for key in keyword))

4. Now finally query your data from DB

keywords_qs = Keyword.objects.filter(keyword__in=keywords)

Note: If you are running on django 1.10+ and using Postgres then step 4 is not needed, since bulk_create will populate your keyword objects with PKs, otherwise you should do step 4


Iterating over object properties I'm working with lots of code that looks like this:

-(void)switchToPercent{
    int tmp;

    tmp = (statisticsObject.idag3_orig != 0) ? statisticsObject.idag3_orig : [self compareValue:statisticsObject.idag2 withValue:statisticsObject.idag3];
    statisticsView.idag3.text = [NSString stringWithFormat:@"%i", tmp];

    tmp = (statisticsObject.igar3_orig != 0) ? statisticsObject.igar3_orig : [self compareValue:statisticsObject.igar2 withValue:statisticsObject.igar3];
    statisticsView.igar3.text = [NSString stringWithFormat:@"%i", tmp];

    tmp = (statisticsObject.veckan3_orig != 0) ? statisticsObject.veckan3_orig : [self compareValue:statisticsObject.veckan2 withValue:statisticsObject.veckan3];
    statisticsView.veckan3.text = [NSString stringWithFormat:@"%i", tmp];

    tmp = (statisticsObject.manaden3_orig != 0) ? statisticsObject.manaden3_orig : [self compareValue:statisticsObject.manaden2 withValue:statisticsObject.manaden3];
    statisticsView.manaden3.text = [NSString stringWithFormat:@"%i", tmp];

    tmp = (statisticsObject.kvartalet3_orig != 0) ? statisticsObject.kvartalet3_orig : [self compareValue:statisticsObject.kvartalet2 withValue:statisticsObject.kvartalet3];
    statisticsView.kvartalet3.text = [NSString stringWithFormat:@"%i", tmp];

    tmp = (statisticsObject.aret3_orig != 0) ? statisticsObject.aret3_orig : [self compareValue:statisticsObject.aret2 withValue:statisticsObject.aret3];
    statisticsView.aret3.text = [NSString stringWithFormat:@"%i", tmp];

    tmp = (statisticsObject.r123_orig != 0) ? statisticsObject.r123_orig : [self compareValue:statisticsObject.r122 withValue:statisticsObject.r123];
    statisticsView.r123.text = [NSString stringWithFormat:@"%i", tmp];

    [self formatText:statisticsView.idag3];
    [self formatText:statisticsView.igar3];
    [self formatText:statisticsView.veckan3];
    [self formatText:statisticsView.manaden3];
    [self formatText:statisticsView.kvartalet3];
    [self formatText:statisticsView.aret3];
    [self formatText:statisticsView.r123];
}

I'm trying to remove the repetition as much as I can. Originally, the business logic was repeated once for each of the properties shown, but I've moved that into a method named compareValue:withValue:. I've also introduced the conditional operator to remove larger ifelse blocks. I imagine the next step is to reduce the seven chunks of code into one, and iterate over a list of keywords, but how?

I can't really judge if it's a good idea, but one way could be creating an NSArray (or NSSet) containing NSDictionaries with the keypaths that should be compared, and then enumerating through that. Something like this:

#define keyOriginal = "original"
#define keyComparisonItem1 = "comparisonItem1"
#define keyComparisonItem2 = "comparisonItem2"

    NSMutableArray * evaluations = [NSMutableArray array];

    [evaluations addObject:@{keyOriginal : @"idag3_orig", keyComparisonItem1 : @"idag2", keyComparisonItem2 : @"idag3"}];
    [evaluations addObject:@{keyOriginal : @"igar3_orig", keyComparisonItem1 : @"igar2", keyComparisonItem2 : @"igar3"}];
        // and so on...

    for (NSDictionary * evaluation in evaluations) {

        if ([statisticsObject valueForKey:keyOriginal] != 0) {
            tmp = (int)[statisticsObject valueForKey:keyOriginal];
        } else {

            tmp = [self compareValue:[statisticsObject valueForKey:keyComparisonItem1]
                           withValue:[statisticsObject valueForKey:keyComparisonItem2]];
        }

        [self formatText:[statisticsView valueForKey:keyOriginal]];

    }


Swift 1.2 Singleton Implementation I am pretty new to Swift, as a solo developer I was wondering if somebody could pass comments on the singleton implementation below. The code does work, but being new to Swift and knowing that there are a lot of gotchas, I wanted to get some feedback from someone more experienced.

// Test Singleton Swift 1.2
class FGSingleton {
    static var instance: FGSingleton!
    var gameScore: Int = 0

    // SHARED INSTANCE
    class func sharedInstance() -> FGSingleton {
        self.instance = (self.instance ?? FGSingleton())
        return self.instance
    }

    // METHODS
    init() {
        println(__FUNCTION__)
    }
    func displayGameScore() {
        println("\(__FUNCTION__) \(self.gameScore)")
    }
    func incrementGameScore(scoreInc: Int) {
        self.gameScore += scoreInc
    }
}

Tests:

FGSingleton.sharedInstance().displayGameScore()
FGSingleton.sharedInstance().incrementGameScore(100)
FGSingleton.sharedInstance().incrementGameScore(1)
FGSingleton.sharedInstance().displayGameScore()
FGSingleton.sharedInstance().incrementGameScore(1000)
FGSingleton.sharedInstance().displayGameScore()

Output:

init()
displayGameScore() 0
displayGameScore() 101
displayGameScore() 1101

You could simplify the above significantly:

class FGSingleton {
    static let sharedInstance = FGSingleton()

    var gameScore: Int = 0

    // METHODS
    private init() {
        println(__FUNCTION__)
    }
    func displayGameScore() {
        println("\(__FUNCTION__) \(self.gameScore)")
    }
    func incrementGameScore(scoreInc: Int) {
        self.gameScore += scoreInc
    }
}

And call it like this:

FGSingleton.sharedInstance.displayGameScore()


Sorting a list of names by greatest lexicographical order This is a puzzle I found online that I have been trying to optimize and make the best version. I've tested it and it works great but I'm curious how it can be improved in any way.

Instructions:

  "You forgot to give Professor Boolean's favorite rabbit specimen a
  name? You know how picky the professor is! Only particular names will
  do! Fix this immediately, before you're... eliminated!"

  Luckily, your minion friend has already come up with a list of
  possible names, and we all know that the professor has always had a
  thing for names with lots of letters near the 'tail end' of the
  alphabet, so to speak. You realize that if you assign the value 1 to
  the letter A, 2 to B, and so on up to 26 for Z, and add up the values
  for all of the letters, the names with the highest total values will
  be the professor's favorites. For example, the name Annie has value 1
  + 14 + 14 + 9 + 5 = 43, while the name Earz, though shorter, has value 5 + 1 + 18 + 26 = 50.

  If two names have the same value, Professor Boolean prefers the
  lexicographically larger name. For example, if the names were AL
  (value 13) and CJ (value 13), he prefers CJ.

  Write a function answer(names) which takes a list of names and returns
  the list sorted in descending order of how much the professor likes
  them.

  There will be at least 1 and no more than 1000 names.  Each name will
  consist only of lower case letters. The length of each name will be at
  least 1 and no more than 8.

  Test cases:

  Inputs:

(string list) names = ["annie", "bonnie", "liz"]

  Output:

(string list) ["bonnie", "liz", "annie"]

  Inputs:

(string list) names = ["abcdefg", "vi"]

  Output:

(string list) ["vi", "abcdefg"]

  Your code will run inside a Python 2.7.6 sandbox.

  Standard libraries are supported except for bz2, crypt, fcntl, mmap,
  pwd, pyexpat, select, signal, termios, thread, time, unicodedata,
  zipimport, zlib.

My answer:

def answer(names):
    # Sort names by reverse alphebetical order
    names = sorted(names, reverse=True)
    # Evaluate the value of each name and build a list with name,value pairs
    name_with_values = sorted([((current_name,value(current_name))) for current_name in names],
        key=lambda value: value[1], reverse=True)
    # Return the list only containing the names
    return [name[0] for name in name_with_values]

def value(name):
    # Evaluate and return value of the name
    return sum([ord(letter) - 96 for letter in name])

print answer(names = ["annie", "bonnie", "liz", "bpnnid"])

Feedback I'm interested in:

Is my logic easy to understand?
How can it be styled or named better?
Is it more complex or more computationally expensive than it needs to be?
How is the general readability/clarity?

You are using the sorted function's key parameter in a very odd way. As you have a function that takes a string and returns a value to sort on, you could implement this much more neatly as:

def answer(names):
    return sorted(names, key=value, reverse=True)

See this Python wiki article for more on sorting. In use:

>>> sorted(["annie", "bonnie", "liz"], key=value, reverse=True)
['bonnie', 'liz', 'annie']
>>> sorted(["abcdefg", "vi"], key=value, reverse=True)
['vi', 'abcdefg']

Note that this doesn't meet the requirements for breaking ties, though; as value('al') == value('cj') and sorted is stable, they stay in the input order:

>>> sorted(('cj', 'al'), key=value, reverse=True)
['cj', 'al']  # ok!
>>> sorted(('al', 'cj'), key=value, reverse=True)
['al', 'cj']  # oh...

Your current pre-sort fixes this, and could be incorporated here too:

>>> sorted(sorted(('al', 'cj'), reverse=True), key=value, reverse=True)
['cj', 'al']

To get around this without overcomplicating the code, value can return a tuple (score, string), so the string is used to break ties:

def value(name):
    """Evaluate and return value of the name."""
    return sum([ord(letter) - 96 for letter in name]), name
                                                   # ^ note

Now 'cj' is always preferred:

>>> sorted(('al', 'cj'), key=value, reverse=True)
['cj', 'al']
>>> sorted(('cj', 'al'), key=value, reverse=True)
['cj', 'al']

Beyond that, note that you should use docstrings rather than just comments:

def value(name):
    """Evaluate and return value of the name."""
    return sum([ord(letter) - 96 for letter in name])

Otherwise it looks pretty good: the code is well laid out, and follows the style guide. If anything, I'd say you have too many comments; they shouldn't explain things the reader could learn from the code, but why the code is written that way (if this is obvious, no comment is needed).


Query of attributes of mobile phone models, using several UNIONs to find distinct values I'd like to know if there's any room to improve the following query which is to find distinct values from multiple tables when someone searches for a model name. Two of the tables are storing phones' series attributes and model attributes. The model attr table is there to deal with special cases, like a series generally use black cases, but it also has released a special edition that uses golden cases.

Here's the query that I have come up. Basically I'm using UNION to unpivot table cpu and find distinct values across cpu,model_attr and series_attr Is there any need for improvement?

Here's a fiddle example.

Query:

SELECT attr_name,attr_val
FROM
(SELECT 'CPU_country' attr_name, c.country attr_val
 FROM cpu c INNER JOIN
 series s ON s.cpu_id = c.cpu_id
 INNER JOIN model m ON m.series_id = s.series_id
 WHERE m.model_name RLIKE 'C79'
 UNION

 SELECT 'CPU_hours' attr_name, c.hours attr_val
 FROM cpu c INNER JOIN
 series s ON s.cpu_id = c.cpu_id
 INNER JOIN model m ON m.series_id = s.series_id
 WHERE m.model_name RLIKE 'C79'
UNION

SELECT a.attr_name attr_name,a.attr_value attr_val
FROM model m INNER JOIN
     series s
     on m.series_id = s.series_id LEFT JOIN
     series_attr sa
     ON sa.series_id = s.series_id LEFT JOIN
     attr saa
     on saa.attr_id = sa.attr_id LEFT JOIN
     model_attr ma
     ON ma.model_id = m.model_id LEFT JOIN
     attr a
     ON a.attr_id = ma.attr_id
     WHERE m.model_name RLIKE 'C79' AND a.attr_value IS NOT NULL
UNION

SELECT saa.attr_name attr_name,saa.attr_value attr_val
FROM model m INNER JOIN
     series s
     on m.series_id = s.series_id LEFT JOIN
     series_attr sa
     ON sa.series_id = s.series_id LEFT JOIN
     attr saa
     on saa.attr_id = sa.attr_id LEFT JOIN
     model_attr ma
     ON ma.model_id = m.model_id LEFT JOIN
     attr a
     ON a.attr_id = ma.attr_id
     WHERE m.model_name RLIKE 'C79' AND saa.attr_value IS NOT NULL
 )k
GROUP BY attr_name,attr_val
ORDER BY attr_name

Table Schema (InnoDB):

CREATE TABLE series
    (`series_id` int, `series_name` varchar(20),`cpu_id` INT ,`ram`int)
;

INSERT INTO series
    (`series_id`,`series_name`,`cpu_id`,`ram`)
VALUES
    (1,'Nokia Series',1,512),
    (2,'Sony Series',2,1024)

;

CREATE TABLE model
    (`model_id` int, `model_name` varchar(20),`series_id` int)
;

INSERT INTO model
    (`model_id`,`model_name`,`series_id`)
VALUES
    (1,'A6578',1),
    (2, 'B2345',1),
    (3, 'C7906',2),
    (4, 'D3544',2)
;

CREATE TABLE attr
    (`attr_id` int, `attr_name` varchar(20),`attr_value` varchar(20))
;

INSERT INTO attr
    (`attr_id`,`attr_name`,`attr_value`)
VALUES
    (1, 'material','Gold'),
    (2, 'material','Plastic'),
    (3, 'color','Grey'),
    (4, 'color','Black'),
    (5, 'color','Green'),
    (6, 'color','White')

;

CREATE TABLE series_attr
    (`series_id` int, `attr_id` int )
;
INSERT INTO series_attr
    (`series_id`,`attr_id`)
VALUES
    (1,2),
    (1,5),
    (2,2),
    (2,3)
;

CREATE TABLE model_attr
    (`model_id` int, `attr_id` int)
;
INSERT INTO model_attr
    (`model_id`,`attr_id`)
VALUES
    (2,1),
    (2,4),
    (4,6)
;

CREATE TABLE cpu
    (`cpu_id` int,`cpu_name` varchar(20), `country` varchar(20),`hours`int,`frequency` int)
;
INSERT INTO cpu
    (`cpu_id`,`cpu_name`,`country`,`hours`,`frequency`)
VALUES
    (1,'CPU A','China',40,24000),
    (2,'CPU B','US',80,30000),
    (3,'CPU C','Japan',100,35000)
;

This is a pet peeve of mine, and others have disagreed with me about it, so please take it with a grain of salt. I really just can't stand single letter aliases. I mean, is cpu really so long that you can't just use the table name? Do you really need to shorten it to c?

It's lazy and it's the worst kind of lazy. It's the kind of lazy that creates more work. Now, instead of plainly seeing series_attr.attr_name, I have to keep in my mind that saa means "series attribute attribute", and ma means "just plain model attribute" and a is a "model attribute attribute" and... Do you see where this is going? Imagine you're brand new to this code base. Would you be able to easily map the alias names to useful meanings? No. You wouldn't, but you shouldn't have to because alias names should be meaningful.

That said, I do like that you've consistently formatted your code. I especially like that you put some whitespace around your UNION statements. However, I really prefer to see subqueries indented one level. It just makes it easier to follow the logic.

You've also hardcoded C79 into your query. What if I want to find info on D82? I have to change the query in at least 4 places. You should be using a variable for this. Better yet, wrap the query in a stored procedure and use a parameter.


Sequence state machine I tried to parse command line arguments. The program requires four arguments. I iterate over the arguments. If the argument is an option, I process the option. Otherwise the argument is one of the required arguments. In order to read the required arguments I need some kind of state machine. In the first case the first argument should be read. In the second case the second argument and so on.
I wrote a Proc class with just one method, which returns again a Proc class.
static abstract class Proc {
  abstract Proc exec (String arg);
}

By this I can execute some action and define what should be done next.

save db host and then read name
save db name and then read user
save db user and then read xml file
save xml file and then nothing

But because of all the class overhead it is hard to read.
Proc proc = new Proc () {
    Proc exec (String arg) {
      db_host = arg;
      return new Proc () {
        Proc exec (String arg) {
          db_name = arg;
          return new Proc () {
            Proc exec (String arg) {
              db_user = arg;
              return new Proc () {
                Proc exec (String arg) {
                  xml_file = arg;
                  return null;
                }
              };
            }
          };
        }
      };
    }
  };

Is there a way to simplify the code? I tried Lambdas, but it seems to me that Lambdas can use only final variables, which is a bit useless, when I want to store a value.
Complete example:
public class Import
{
  static String db_host = null;
  static String db_port = "5432";
  static String db_name = null;
  static String db_user = null;
  static String xml_file = null;

  static void usage ()
  {
    System.err.println ("Usage: Import [-p PORT] HOST DATABASE USER FILE");
  }

  static abstract class Proc {
    abstract Proc exec (String arg);
  }

  static void parse_args (String[] args)
  {
    Proc proc = new Proc () {
        Proc exec (String arg) {
          db_host = arg;
          return new Proc () {
            Proc exec (String arg) {
              db_name = arg;
              return new Proc () {
                Proc exec (String arg) {
                  db_user = arg;
                  return new Proc () {
                    Proc exec (String arg) {
                      xml_file = arg;
                      return null;
                    }
                  };
                }
              };
            }
          };
        }
      };

    try {
      for (int i = 0; i < args.length; i++)
        switch (args[i]) {
        case "-p":
          db_port = args[++i];
          break;
        case "-h":
          usage ();
          break;
        default:
          proc = proc.exec (args[i]);
        }
    }
    catch (Exception ex) {
      throw new Error ("Can not parse args!", ex);
    }
  }

  public static void main (String[] args)
  {
    parse_args (args);

    System.err.println ("db_host: " + db_host);
    System.err.println ("db_port: " + db_port);
    System.err.println ("db_name: " + db_name);
    System.err.println ("db_user: " + db_user);
    System.err.println ("xml_file: " + xml_file);
  }
}

First, you should follow the Java naming conventions.

Second, don't shorten names just because you can. Example, what does "Proc" stand for? Procedure? Processor? Even if names become longer, the readability is worth it!

static String db_host = null;

Your members should not be static, and should ideally be qualified with something else than package-private, like protected or private to show the intent clearly.

In order to read the required arguments I need some kind of state machine.

Is that a requirement you've been given or is that an assumption you had? Because that's not true at all. Overall, your system is completely unnecessarily complex without an obvious reason or benefit. Neither does it seem like a state-machine at all, it's just an overly complex chain of function calls.
A state-machine for your command-argument parsing would be something like this:
for (int index = 0; index < args.length; index++) {
    String arg = args[index];

    if (arg.equals("-p") && databaseHost == null) {
        arg = args[++index];

        databasePort = arg;
    } else if (databaseHost == null) {
        databaseHost = arg;
    } else if (databaseUsername == null) {
        databaseUsername = arg;
    } else if (databasePassword == null) {
        databasePassword = arg;
    } else if (inputFile == null) {
        inputFile = arg;
    }
}

And even that is oddly complex.
What you want is the most simplest solution that works, and that would be a hardcoded extracting of arguments:
if (args[0].equals("-h")) {
    printHelp();
    return;
}

int portProvidedOffset = 0;

if (args[0].equals("-p")) {
    databasePort = args[1];

    portProvidedOffset = 2;
}

databaseHost = args[portProvidedOffset + 0];
databaseUsername = args[portProvidedOffset + 1];
databasePassword = args[portProvidedOffset + 2];
inputFile = args[portProvidedOffset + 3];

Which is, of course, not pretty, but the most simplest solution you can get away with. Of course, the length of the array should be checked beforehand, whether it is 4 elements or more. Afterwards you'd check whether all parameters are set, if not, exit with an error.
If you want a more sophisticated solution, you'd need to write yourself a complete argument parser, a not-so-trivial exercise.


Integer Exponentiation Implementation:

This works based on this observation:

$$a^n = a^{n/2} * a^{n/2}$$

BigInteger Pow(BigInteger x,int n)
{
    if(n == 0) return 1;
    else if(n % 2 == 0) {
        var val = Pow(x,n/2);
        return val * val;
    }
    else return x * Pow(x,n-1);
}

How can I improve this? Is there a more efficient algorithm?

Don't use Egyptian style bracing, that is the style more widely accepted by Java, use the more widely accepted C# bracing style.

Your current structure for the if/else statements is not good.  If you have to use braces in the else/elseif statements then you should use bracing everywhere, even on a single operation inside the if statement

like this

BigInteger Pow(BigInteger x,int n)
{
    if(n == 0)
    {
        return 1;
    }
    else if(n % 2 == 0)
    {
        var val = Pow(x,n/2);
        return val * val;
    }
    else
    {
        return x * Pow(x,n-1);
    }
}

Use if statements like this without the else if

BigInteger Pow(BigInteger x,int n)
{
    if(n == 0) return 1;
    if(n % 2 == 0)
    {
        var val = Pow(x,n/2);
        return val * val;
    }
    return x * Pow(x,n-1);
}

You return inside the if statements so there is no need to else anything just if through because if the first one is true then none of the other if statements will be executed.

I needed to change BigInteger to long. BigInteger it still an INT and so is long, for some reason I could not use BigInteger, so in my equations I will use long

Your naming should definitely be better I went with numBase and numPower I don't normally like hungarian style notation like this, but base is kind of reserved and all.

this is what mine looks like

public static long Pow(long numBase, long numPower)
{
    if (numPower == 0)
    {
        return 1;
    }
    if (numPower % 2 == 0)
    {
        var val = Pow(numBase, numPower / 2);
        return val * val;
    }
    return numBase * Pow(numBase, numPower - 1);
}

also I went with a static method inside of a static class so I could just call it willy-nilly whenever I felt like it.  Like this:

Class1.Pow(numBase,numPower);

the only thing with this is that it looks like you are overloading the already created Math.Pow method, you should probably at the least Comment to that effect, and/or create an actual overload for Math.Pow


Split coins into combinations of different denominations I have 3 types of coins: Gold, Silver, Copper.
1 silver = 100 copper.
1 gold = 100 silver.

My input is always in coppers, and I want to be able to make it a bit more readable. So far my code is:

def api_wallet_translate_gold(value):
    """Translate a value into string of money"""
    if value >= 10000:  # Gold
        return ("{0} gold, {1} silver and {2} copper."
                .format(str(value)[:-4], str(value)[-4:-2], str(value)[-2:]))
    elif value >= 100:  # Silver
        return "{0} silver and {1} copper.".format(str(value)[-4:-2], str(value)[-2:])
    else:  # Copper
        return "{0} copper.".format(str(value)[-2:])

It works, but I am wondering how could it be improved. I think there was a way to format it like {xx:2:2} or something but I can't remember how to do it.

Note: We never know how many gold digits we have, it could be 999999 to 1

It may less fragile if you deal with the numbers directly rather than converting to strings. It will also be cleaner code.

You could start with your values in a list sorted highest to lowest. Then in your function you can find the next-largest value and remained with divmod(). After than it's a matter of deciding how you want to format the resulting dict:

coins = [
    ("gold",  100 * 100),
    ("silver", 100),
    ("copper", 1)
]

def translate_coins(value, coins):
    res = {}
    for coin, v in coins:
        res[coin], value = divmod(value, v)
    return res

translate_coins(1013323, coins)

Result:

{'gold': 101, 'silver': 33, 'copper': 23}


Hashing a struct in C I'm trying to learn how chained hashing works, but can't get any further than this.
I think hashing works, but after finishing this little program, someone told me that what I have created so far has nothing to do with hashing.

#define _CRT_SECURE_NO_WARNINGS
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#define size 5
//This is my struct I wanted to add to the hashtable
struct customer{
    char name[20];
    int id;
    int pin;
    double money;
    struct customer *next;
};

//creating the table
struct customer *hash_table[size];
struct customer *next=NULL;
//my hashfunction
int hash_function(int id){
    unsigned int adress_hash;
    adress_hash = 31 * id;
    return adress_hash % 5;
}

//Adding the cust elements to my table
void add_customer(){
    int id_set = rand() % 8999 + 1000;
    struct customer *start;
    struct customer *pointer;
    int adress = hash_function(id_set);

     //first element

    if(hash_table[adress]==NULL)
    {
        hash_table[adress] = malloc(sizeof(struct customer));
        hash_table[adress]->id= id_set;
        hash_table[adress]->pin= rand() % 8999 + 1000;

        hash_table[adress]->money= 0;
        hash_table[adress]->next=NULL;
    } else {
        //Adding new files if the first element of the table is NOT NULL
        pointer=hash_table[adress];
        while(pointer->next != NULL)
        {
            pointer= pointer->next;
        }
        pointer->next = malloc(sizeof(struct customer));
        pointer=pointer->next;
        pointer->id=id_set;
        pointer->pin= rand() % 8999 + 1000;
        pointer->money = 0;

        pointer->next=NULL;
    }
}

void list_customer(int a){
    struct customer *pointer;
    int x;
    for(x=0;x<5;x++){
        pointer = hash_table[x];
        printf("%d. ----------\n", x);
        while(pointer != 0){
            printf("KNR: %d, PIN: %d\n", pointer->id, pointer->pin);
            pointer = pointer->next;
        }
        printf("-------------------------\n");
    }
}

int main(){
    int adr,x;
    srand(time(NULL));
    for(x=0;x<5;x++){
        add_customer();
    }
    list_customer(size);
    getchar();getchar();
    return 0;
}

I see a number of things that may help you improve your code.

Understand the purpose of a hash table

The purpose of a hash table is as an associative array.  In this code, the id element of the customer structure is used as the hashed value and the hash function converts that int value into a number in the smaller range of size.  It's a valid use of a hash function and hash table.  The only thing missing is a means to actually use the hash function to quickly find a customer record.  To do that, you might implement a find_customer function that would search only in the appropriate "bucket" of the hash_table given the id.  With a large number of records \$n\$ where \$n \gg\$ size, this could save look-up time by needing to search only 20% of the records rather than all of them.  With that said, what follows is a review of the code you posted.

Eliminate unused variables

The parameter a in the list_customer() function, adr in main(), and start in add_customer() are all unused variables and should be omitted from the program.

Free allocated memory

Generally speaking, if you allocate memory using malloc or calloc, you should also free that memory to avoid "leaking" memory.

Eliminate "magic numbers"

In a number of cases, the code uses "magic numbers" such as 8999 and 1000 that have no obvious meaning.  These would be better as named constants.  Also, a named constant size is already defined in the code, and should be used within list_customer() instead of hard coding 5.

Use better variable names

The names hash_table and id are good names, but pointer is not a good name because it is too generic.  I'd recommend something like cust_ptr (or Kunde if you prefer German).

Declare variables in the smallest practical scope

By declaring variables in the smalles practical scope, you reduce the chance for name collisions and make it clear to the reader of your code where variables are and are not needed.  With any C compiler conforming to the 1999 specification (which should be all of them at this point!) one could rewrite list_customer like this for example:

void list_customer(){
    for(int x = 0; x < size; x++){
        printf("%d. ----------\n", x);
        for (struct customer *cust_ptr = hash_table[x];
                cust_ptr != NULL; cust_ptr = cust_ptr->next)
        {
            printf("KNR: %d, PIN: %d\n", cust_ptr->id, cust_ptr->pin);
        }
        printf("-------------------------\n");
    }
}

Note also that I have converted your while loop into a for loop which I think clarifies the nature of the code.

Check the return value of malloc

If the system is running out of memory, malloc will return NULL.  Code must check the return value to make sure it is not NULL before dereferencing the variable or the program will crash.

Avoid repeating code

Two places in the code use the construct rand() % 8999 + 1000 which suggests that they should probably be made into a function or macro.

Functions taking no parameter should be declared void

A function declared as void add_customer() in C may actually take any number of parameters which is an old misfeature of the C language.  What you probably want instead is to declare it as taking no parameters which is done like this:

void add_customer(void)

Create subfunctions to keep the code clear and neat

The add_customer() code needs to create a new customer and then put it into the hash_table.  We can delegate the creation to a separate function:

struct customer *new_customer(int id)
{
    struct customer *cust_ptr = malloc(sizeof(struct customer));
    if (cust_ptr != NULL) {
        cust_ptr->name[0] = '\0';
        cust_ptr->id = id;
        cust_ptr->pin = myrand();
        cust_ptr->money = 0;
        cust_ptr->next = NULL;
    }
    return cust_ptr;
}

Note that this code initializes the name member which the original code did not.  Now the add_customer code can be much simpler:

void add_customer(void){
    int id_set = myrand();
    int adress = hash_function(id_set);
    struct customer *cust_ptr = hash_table[adress];
    if (cust_ptr == NULL) {
        hash_table[adress] = new_customer(id_set);
        return;
    }
    while(cust_ptr->next != NULL) {
        cust_ptr = cust_ptr->next;
    }
    cust_ptr->next = new_customer(id_set);
}

And of course the myrand() function:

int myrand(void)
{
    return rand() % 8999 + 1000;
}

Omit return 0 at the end of main

The compiler will automatically generate the equivalent to return 0 for code that successfully gets to the end of main.


Time complexity of anagram solution I have written code to return anagrams from a list of words. I'm a newbie in time complexity analysis so pardon my ignorance if this is a blatantly obvious question. Am I right in thinking this is in O(n log n) time, since it's just performing a quicksort and the rest is done in O(1) time because it's hashmaps? Also any suggestions on improving the code?

    public class anagram {

public static ArrayList<String> anagrams(String[] original) {

    HashMap<String, ArrayList<String>> map = new HashMap();

    for(String str : original) {

        String key = sortChars(str);

        if(!map.containsKey(key)) {
            map.put(key, new ArrayList<String>());
        }

        ArrayList<String> anagramList = map.get(key);
        anagramList.add(str);
    }

    ArrayList<String> result = new ArrayList();

    for(String key : map.keySet()) {

        ArrayList<String> anagramList = map.get(key);

        if(anagramList.size() > 1) {

            result.addAll(anagramList);
        }
    }

    return result;
}

You are close to right on the complexity. You have two dimensions that are independent in your system though. The sort is sorting the letters in a word, and its performance is dependent on the length of the word, not the number of words.

You have essentially an outer loop which is linear \$O(n)\$ and an inner sort which is dependent on m, the number of letters in each word...., and those combine to create an overall complexity of \$O({n} \times {m}\log{m})\$

If you double the average size of each word, the execution time will slightly-more-than-double. If you double the number of words, the same will happen.

Now, how can it be improved?

your class is called anagram ? It should be capitalized to Anagram.
you should use the interface declaration not the implementation, where you can. For example, your HashMap should be HashMap<String, List<String>> instead of HashMap<String, ArrayList<String>> (note, use List instead of ArrayList).
The two lines:

ArrayList<String> anagramList = map.get(key);
anagramList.add(str);

could be simplified to:

map.get(key).add(str);

You iterate through the keys of the HashMap to get the value arrays, but you don't need to. Consider the following (which just iterates the values...):

for (List<String> anagramList : map.values()) {
    if (anagramList.size() > 1) {
        result.addAll(anagramList);
    }
}


Golang channel in C# I have tried to mimic Golang channels in C# and its performance is pretty good compared to golang itself. On my machine, each channel operation of Golang takes ~75 nano-sec and each Chan<T> (in C#) operation takes ~90 nano-sec.

Please let me know if this code can be improved in any way.

class Chan<T>
{
    readonly int size;

    T[] buffer;
    long head = -1;
    long tail = -1;
    long closed = 0;

    public Chan() { this.size = 0; }
    public Chan(int size)
    {
        if (size < 0) throw new ArgumentOutOfRangeException();

        this.size = size;
        this.buffer = new T[this.size];
    }

    object headLock = new object();
    public bool To(T t)
    {
        lock (headLock)
        {
            long localClosed = 0L;

            if (tail - head == buffer.Length) SpinWait.SpinUntil(() => (localClosed = Interlocked.Read(ref closed)) > 0 || tail - head < buffer.Length);
            if (localClosed > 0) return false;

            var newTail = Interlocked.Increment(ref tail);
            buffer[newTail % buffer.Length] = t;

            return true;
        }
    }

    object tailLock = new object();
    public bool From(out T val)
    {
        lock (tailLock)
        {
            long localClosed = 0L;

            if (tail - head == 0) SpinWait.SpinUntil(() => (localClosed = Interlocked.Read(ref closed)) > 0 || tail - head > 0);
            if (localClosed > 0)
            {
                val = default(T);
                return false;
            }

            var newHead = Interlocked.Increment(ref head);
            val = buffer[newHead % buffer.Length];

            return true;
        }
    }

    public void Close()
    {
        Interlocked.Increment(ref closed);
    }
}

Chan sounds like the abbreviated name for Channel and apparently it's a channel. So I'd use Channel or maybe even GoChannel.
The most commonly used naming convention I have seen for private members is to prefix them with an underscore. This way you can see at the first glance whether it's a local variable or a class member. This also means you can get rid of this. most of the time.
I really prefer to spell out the access modifier even if private is default but YMMV.
Method names should describe actions or operations (because they operate on data and sometimes modify the state of the object). To and From are not actions or operations. That being said: The channel seems to have fixed size queue semantics (FIFO) so I'd consider calling the operations Enqueue and Dequeue which would make it immediately clear how the data is being processed. (The semantic of the underlying data structure should not be exposed. Don't know what I was thinking there.) Rather use the Send and Receive semantics from the go definition.
Given the previous point it could be useful to have a Peek method to check what will come next.
_head and _tail are longs and access is not guaranteed to be atomic so you should use Interlocked.Read to obtain them.
Also the implementation is actually broken. Assume two threads A and B, A calls Send() and B calls Receive(), first execution _head == _tail == -1:

A: execute Interlocked.Increment(_tail) (_tail is now 0)
B: _tail - _head > 0 is true (0 - -1 == 1), leaves spinlock
B: execute Interlocked.Increment(_head) (_head is now 0)
B: read _buffer[_head]
A: write _buffer[_tail]
B has read from buffer before element was written.
This problem can be easily reproduced with this test case (almost every iteration results in dupes):

[TestCase]
public void TestSPSC()
{
    int numItems = 10000;
    int numIterations = 100;

    for (int i = 0; i < numIterations; ++i)
    {
        var channel = new Channel<int>(100);
        var writer = Task.Factory.StartNew(() => { foreach (var num in Enumerable.Range(1, numItems)) { channel.Send(num); } channel.Close(); });
        var reader = Task.Factory.StartNew<List<int>>(() => {
            var numbers = new List<int>(numItems);
            for (int idx = 1; idx <= numItems; ++idx)
            {
                int num;
                var res = channel.Receive(out num);
                numbers.Add(num);
            }
            return numbers.OrderBy(x => x).ToList();
        });
        Task.WaitAll(writer, reader);
        var dupes = reader.Result.GroupBy(x => x).Where(g => g.Count() > 1).ToList();
        if (dupes.Count > 0)
        {
            Console.WriteLine("{0}: {1} DUPES!", i, dupes.Count);
        }
    }
}

I changed the implementation to use .NET's BlockingCollection<T> wrapped around a ConcurrentQueue<T>:

public class Channel<T>
{
    private BlockingCollection<T> _buffer;

    public Channel() : this(1) { }
    public Channel(int size)
    {
        _buffer = new BlockingCollection<T>(new ConcurrentQueue<T>(), size);
    }

    public bool Send(T t)
    {
        try
        {
            _buffer.Add(t);
        }
        catch (InvalidOperationException)
        {
            // will be thrown when the collection gets closed
            return false;
        }
        return true;
    }

    public bool Receive(out T val)
    {
        try
        {
            val = _buffer.Take();
        }
        catch (InvalidOperationException)
        {
            // will be thrown when the collection is empty and got closed
            val = default(T);
            return false;
        }
        return true;
    }

    public void Close()
    {
        _buffer.CompleteAdding();
    }

    public IEnumerable<T> Range()
    {
        T val;
        while (Receive(out val))
        {
            yield return val;
        }
    }
}

The code is much easier to read and has probably less bugs than your self implemented one. It's also fast. I can pump 10,000,000 items (I tested with int) through a channel (buffer size 100) with single producer single consumer in 5sec. That's 0.5ns per item.

    [TestCase]
    public void TestSPSC_Performance()
    {
        int numItems = 10000000;
        int numIterations = 10;

        var stopWatch = new Stopwatch();
        stopWatch.Start();
        for (int i = 0; i < numIterations; ++i)
        {
            var channel = new Channel<int>(100);
            var writer = Task.Factory.StartNew(() => { foreach (var num in Enumerable.Range(1, numItems)) { channel.Send(num); } channel.Close(); });
            var reader = Task.Factory.StartNew<List<int>>(() => { var res = new List<int>(numItems); foreach (var num in channel.Range()) { res.Add(num); } return res; });
            Task.WaitAll(writer, reader);
        }
        stopWatch.Stop();

        var elapsedMs = stopWatch.Elapsed.TotalMilliseconds;
        Console.WriteLine("SPSC N = {0}: {1:.00}ms/iteration, {2:.00}ns/item (tx+rx)", numItems,  elapsedMs / numIterations, elapsedMs * 1000.0 / numItems / numIterations);
    }


Statistics mode function I will get straight to the point - I am a freshman at uni and I am currently working on my first bigger personal project - Statula is a terminal tool for data analysis which takes in dataset (as a file) and then spits out bunch of numbers about the numbers. I am quite satisfied with where it is going but I am not gonna lie -  I have some concerns.

Here is the git repo.

I will say it beforehand in case someone goes for a lecture - I am aware that I did not check for returned errors in compute_dataset.
I have just reworked some of them and I am kind of busy this week - I will get onto it as soon as possible. Also, my tests suck - or rather a single one. I have just introduced new structure which basically broke all of my tests. I am also working on that.

So, back to the point.
 My main focus is performance. I am satisfied with the fact that on my rig  (i5-4670k) this program can finish it's job in under 240ms. However, that is not good enough for me. I believe that the main performance hog might be the mode function - and for a good reason.

Here is the code:

int mode(struct dataset *set)
{
    assert(set->number_count>0);
    double max_value=0;
    int max_count=0,i,j,mode_count=1;
    for(i=0;i<set->number_count;i++){
        int count=0;
        for(j=i+1;set->numbers[j]==set->numbers[i]&&j<set->number_count;j++);
        count=j-i;
        if(count>max_count){
            mode_count=1;
            max_count=count;
            max_value=set->numbers[i];
        } else if(count==max_count) {
            mode_count++;
            i=j-1;
        }
    }
    if(mode_count==1){
        *(set->mode)=max_value;
        set->is_mode_present=1;
    } else{
        *(set->mode) = 0;
        set->is_mode_present=0;
    }
    return mode_count==1?1:0;
}

Perhaps there is some room for improvement. For the record, the number set is qsorted before being passed to the function. That's the main premise. Can it get any faster than that?

I also wonder whether the struct itself is a good idea - I guess there is no going back. Do not get me wrong, I have put a lot of thought into this matter and I came to a conclusion that by using struct I can:

Analyse multiple sets by reading multiple datasets - each has it's own struct. Beforehand, all I had was a local variable.
Avoid some unneccessary function calls - for instance, I have needed mean to compute standard deviation. Sure, I could pass it as an argument to a function, but.. where is the fun?

Unfortunately, that means that you cannot just copy-paste the function out of the source code and expect it to work - it relies on dataset structure being present.

So that is more of a general question - do you prefer your code to be correct (in a very loose sense) or portable?

General code suggestions are also welcome!

Well, of course your code is too slow, it's O(n²) but should be O(n):
Unless you already found a run of the same length, you don't skip to the end of the run!

Your inner for-loop only tests whether you are beyond your list after already having read there. That's slightly too late, rearrange that.

A side-question: Why do you insist on the dataset not being empty?

Why do you call your mode max_value? That's strange.

Restrict the scope of your values to the minimum you need: If it's not in scope, there's nothing for you to keep track of, allowing you to concentrate on other issues.

Consider investing in a bit more whitespace, especially around binary operators.

You know that true is 1 and false is 0?

Never use ; as the empty statement: It always looks suspicious. Either put a comment before it /**/; or use a block {} instead.


Two Datasets Same Query I have a form that allows the user to select a server.  This will execute the exact same query just on a different server.

private void QueryButton_Click(object sender, EventArgs e)
{
    switch (this.FindByComboBox.SelectedIndex)
    {
        case 0:
            ByTargetVDN();
            break;
        // ...
    }
}
void ByTargetVDN()
{
    int value = 0;
    if (this.ValueTextBox.Text.Length > 0)
    {
        value = Convert.ToInt32(this.ValueTextBox.Text);
    }
    switch (this.ServerComboBox.SelectedIndex)
    {
        case 0:
            var gr01 = new GR01.TargetVDNDataTable();
            var adapter = new ICMQuery.Models.GR01TableAdapters.TargetVDNTableAdapter();
            if (value == 0)
            {
                adapter.Fill(gr01);
            }
            else
            {
                adapter.FillByPeripheralNumber(gr01, value);
            }
            this.FindScriptsDataGrid.DataSource = gr01;
            break;
        case 1:
            var ds01 = new DS01.TargetVDNDataTable();
            var adapterNew = new ICMQuery.Models.DS01TableAdapters.TargetVDNTableAdapter();
            if (value == 0)
            {
                adapterNew.Fill(ds01);
            }
            else
            {
                adapterNew.FillByPeripheralNumber(ds01, value);
            }
            this.FindScriptsDataGrid.DataSource = ds01;
            break;
    }
}

There has got to be a way I can streamline this.  I was hoping for something along the lines of

switch (this.ServerComboBox.SelectedIndex)
  {
    case 0:
      var ds = new GR01.TargetVDNDataTable();
      var adapter = new ICMQuery.Models.GR01TableAdapters.TargetVDNTableAdapter();
      break;
    case 1:
      ds = new GR01.TargetVDNDataTable();
      adapter = new ICMQuery.Models.GR01TableAdapters.TargetVDNTableAdapter();
      break;
  }
if (value == 0)
  {
    adapter.Fill(ds);
  }
else
  {
    adapter.FillByPeripheralNumber(ds, value);
  }
this.FindScriptsDataGrid.DataSource = ds;

but of course this doesn't work.

if (this.ValueTextBox.Text.Length > 0)
{
    value = Convert.ToInt32(this.ValueTextBox.Text);
}

This will throw a nullpointerexception if Text is null, use TryParse instead

int value;
bool result = Int32.TryParse(ValueTextBox.Text, out value);
if(result){
 // use value
}

I don't recommend you using SelectedIndex, because it breaks your code if the ordering get changed in the future, I would use SelectedItem instead.

Notice that you are repeating code, which is not a brilliant idea.

TheTypeForThis gr01 = null;
TheTypeForThis adapter = null;
switch (this.ServerComboBox.SelectedIndex)
{
    case 0:
        gr01 = new GR01.TargetVDNDataTable();
        adapter = new ICMQuery.Models.GR01TableAdapters.TargetVDNTableAdapter();
        break;
    case 1:
        var ds01 = new DS01.TargetVDNDataTable();
        var adapterNew = new ICMQuery.Models.DS01TableAdapters.TargetVDNTableAdapter();
       break;
}

 if (value == 0)
 {
    adapter.Fill(gr01);
 }
 else
 {
    adapter.FillByPeripheralNumber(gr01, value);
 }
 this.FindScriptsDataGrid.DataSource = gr01;


Creating and finding posts using Parse.com The following code calls Parse. And uses its API to create two functions: post.create and post.find. As the names imply, one is for creating posts and the other for fetching them.

Example:

import Parse from 'parse'

Parse.initialize('APP_ID', 'CLIENT_KEY')

const post = {}

post.create = json => {
  const Post = Parse.Object.extend('Post')
  const post = new Post()
  post.save(json).then(object => {
    console.log('yay! it worked', object)
  })
}

post.find = () => {
  const query = new Parse.Query(Post)
  let arr = []
  query.find({
    success: function (results) {
      results.forEach(function (result) {
        arr.push(result.toJSON())
      })
    },
    error: function (error) {
      alert('Error: ' + error.code + ' ' + error.message)
    }
  })
  return arr
}

export default post

As you can see, const Post = Parse.Object.extend('Post') is being written twice. I could just declare it once at the top of the file, on the other hand, they would be farther from the place they are used.

What's the conventional option here?

This depends on the scope in which you want the variable to have.

Is the variable the same between the two function calls? if so, you would tend to only define it once, as calling it twice may be expensive.

Interestingly however, you've defined an ES6 const which will complain if you redefine the same value (which is why I presume you've named the variable differently).

If you intended the variable to be different between the two calls, you would normally not use const.

Looking at your example, I would define this variable once, and use the const keyword in order to explicitly avoid is being redefined by accident elsewhere.

You also ask about conventions. Looking at the MDN page for const, they advise naming constants with uppercase characters. So I would probably go down that route unless it contravenes your coding style: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const


Are there any bugs or ways to make my divisor code better? After coding my divisor code which I did as a practice for my GCSE stuff which I have to take soon, I have decided to ask the community if there are any bugs or ways I can improve my code.

import time, math
from array import *
def programme():
    num = input("Enter your Number: ")
    try:
        intnum = int(num)
        if(intnum < 0):
            error("POSITIVE NUMBERS ONLY")
        else:
            numbers = array("i",[])
            starttimer = time.time()
            i = 1
            print("\nThe divisors of your number are:"),
            while i <= math.sqrt(intnum):
                if (intnum % i) == 0:
                    numbers.insert(0,i)
                    numbers.insert(0,int(intnum/i))
                    print(i,":", int(intnum/i))
                i += 1
            numbers = sorted(numbers, reverse = True)
            print("The Highest Proper Divisor Is\n--------\n",str(numbers[1]) , "\n--------\nIt took"  ,round(time.time() - starttimer, 2)  ,"seconds to finish finding the divisors." )
    except ValueError:
       error("NOT VALID NUMBER")
    except OverflowError:
       error("NUMBER IS TO LARGE")
    except:
       error("UNKNOWN ERROR")

def error(error):
    print("\n----------------------------------\nERROR - " + error + "\n----------------------------------\n")
running = True
while(running):
    programme()
    print("----------------------------------")
    restart = input("Do You Want Restart?")
    restart = restart.lower()
    if restart in ("yes", "y", "ok", "sure", ""):
        print("Restarting\n----------------------------------")
    else:
        print("closing Down")
        running = False

I don't care how small the improvement is all I want are ways in which I can make it better!

When inputting 262144 I get

Enter your Number: 262144

The divisors of your number are:
1 : 262144
2 : 131072
4 : 65536
8 : 32768
16 : 16384
32 : 8192
64 : 4096
128 : 2048
256 : 1024
512 : 512
The Highest Proper Divisor Is
--------
 131072
--------
It took 0.09 seconds to finish finding the divisors.
----------------------------------
Do You Want Restart?

Here are some thoughts on this code:

Chose descriptive function names

The name programme is not really very descriptive of what the function does.  You might call it something like factor to give the reader of your code a better idea of what it actually does.

Document your code

Python supports the use of multiple different kinds of comments, including ones that can be made into unit tests.  Get in the habit of commenting your code carefully.

Separate I/O from calculation

The main purpose of the program is to factor numbers into divisors which is something that is potentially reusable.  Both to make it more clear as to what the program is doing and to allow for future re-use, I'd suggest extracting the factoring part into a separate function and the have the input from the user be done within the main routine or some other input-only function.

Separate timing from the thing being timed

On my machine, the timer always reports 0.0 seconds, which makes it hard to compare the effects of changes in the code.  For that reason, what's often done is to call the routine to be timed multiple times to get better resolution.  In order to do that, you'd have to remove the timing from the routine you're timing, which is good design practice anyway.  Right now, the print function is part of the code that's being timed.

Think of your user

Right now, the user has to enter "yes" or the equivalent and then enter another number to be factored.  However, the prompt doesn't suggest that "y" is a valid answer.  Adding that to the prompt would help the user understand what the computer is expecting.  Better still, would be t eliminate that question and to simply ask for the next number with a number '0' being the user's way to specify no more numbers.  Of course the prompt should be changed to tell the user of this fact.

Expand the range

Right now, the size of numbers that can be factored is limited to what will fit within an int, but it would be much more interesting to be able to factor really large numbers.

Use divmod() to save some time

Right now, for every successfully discovered divisor, the program performs three division operations:  one for %, and then two more to insert the number into numbers and again to print.  Instead, you could do the division once using the divmod function which gives you both the quotient and remainder at the same time.


Finding previous/next item in an OrderedDict I have an OrderedDict and, given a key in it, need to find the previous and next keys accordingly.  The implementation should wrap (i.e. if the given key is the first in the dictionary, the previous key should be the last one and vice versa).

Here's what I've come up with so far (adapted from production code) which works, but seems a bit... clunky.  Any tips for improvement would be appreciated!

from collections import OrderedDict

# Create a sample
a_dict = OrderedDict()
for n in range(25):
    a_dict.update({n: n + 1})

# The currently-selected key (not a plain int in my actual production code)
selected = 0

the_keys = list(a_dict.keys())
# Only find the prev/next item if we have more than one key
if len(the_keys) <= 1:
    return

current = None
for index, key in enumerate(the_keys):
    if key == selected:
        current = index
        break

# For the majority of cases
prev = current - 1
next = current + 1

# For the edge cases (ends of the list)
if current == 0:
    prev = -1
    next = 1
elif current == len(the_keys) - 1:
    prev = current - 1
    next = 0

One way of simplifying the problem is to provide a thin wrapper around the OrderedDict. Something along the lines of:

class DictCycler:
    def __init__(self, data):
        self.data = data
        ...

    def next(self):
        ...

    def previous(self):
        ...

An other way is to make use of itertools capability. Especially cycle and islice. The idea being that, using cycle, cycling through the elements in the proper order is straightforward, and cycling in reverse can be done by going forward N - 2 times (where N is the length of the dictionary) with the help of islice. This yield:

import itertools

class DictCycler:
    def __init__(self, data):
        self.data = data
        self.iterator = itertools.cycle(data)
        self.jump_to_previous = len(data) - 2

    def next(self):
        return self.data[next(self.iterator)]

    def previous(self):
        prev = itertools.islice(self.iterator, self.jump_to_previous, None)
        return self.data[next(prev)]

You then just have to populate your dictionary and freeze it into this wrapper.

If you need to be able to modify the dictionary contained in data after passing it to this class, you will need to adapt the recipe a bit.