Fixed size 2d array of objects My Objective is to create a fixed column array object of size 4 columns so that I can form a 2d array in the following format which I can use to print to a pdf later. The solution is working fine.
But need help in figuring out if there is a better way to implement this.

let input = [{
  id:123,
  quantity: 4,
  value: "xxxx"
},
{
  id:234,
  quantity: 11,
  value: "xxxx"
},
{
  id:345,
  quantity: 1,
  value: "xxxx"
}]

output = [
    [ objHeader, objValue, objValue, objValue ],
    [ objValue, null, null, null ],
    [ objHeader1, objValue1, objValue1, objValue1 ],
    [ objValue1, objValue1, objValue1, objValue1 ],
    [ objValue1, objValue1, objValue1, objValue1 ],
    [ objHeader2, objValue2, null, null ]
]

where objHeader = {
    id:123,
    header:true,
    quantity: 4,
    value: xxxx
}
objValue = {
    id:123,
    header:false,
    quantity: 4,
    value: xxxx
}

objValue is basically input[0].id repeated by the factor present in quantity

objValue1 is input[1].id repeated by the factor present in quantity and so on

Code that is working now

let input = [
  {
    id: 123,
    quantity: 4,
    value: "x1xxx"
  },
  {
    id: 234,
    quantity: 11,
    value: "x2xxx"
  },
  {
    id: 345,
    quantity: 1,
    value: "x3xxx"
  }
];

class RowRecord {
  constructor(rowsize = 4) {
    this.items = Array(rowsize).fill(null);
    this.rowSize = rowsize;
  }

  push(item) {
    const currentSize = this.size();
    // console.log(currentSize);
    if (currentSize < this.rowSize) {
      // replace 1 items with null based on index
      this.items.splice(currentSize, 1, item);
    }
  }

  get() {
    return this.items;
  }

  size() {
    return this.items.filter(item => item !== null).length;
  }
}

function getRecords() {
  const records = [];
  let rows = new RowRecord(); // fill default
  // create records
  for (let i = 0; i < input.length; i++) {
    // check for pending rows
    if (i !== 0 && rows.size() > 0) {
      records.push(rows);
    }
    // initiate new row
    rows = new RowRecord();
    const item = input[i];
    const quantity = parseInt(item.quantity, 10);
    const value = item.value;
    // +1 for printing the label for each product
    for (let j = 0; j <= quantity; j++) {
      if (j === 0) {
        // title label
        rows.push({
          id: item.id,
          header: true,
          quantity,
          value
        });
      } else {
        if (rows.size() === 4) {
          records.push(rows);
          rows = new RowRecord();
        }
        rows.push({
          id: item.id,
          header: false,
          quantity,
          value
        });
      }
    }
  }
  // push any pending rows
  records.push(rows);
  return records;
}

console.log(getRecords());

You can also use Array.prototype.reduce to generate the fixed size 2d array.

const input = [{
  id: 123, quantity: 4, value: "x1xxx"
}, {
  id: 234, quantity: 11, value: "x2xxx"
}, {
  id: 345, quantity: 1, value: "x3xxx"
}];

function getNewRecords(rowSize = 4) {
  return input.reduce((records, current) => {
    let items = [];
    Array.from({
      length: current.quantity + 1 // +1 for header column
    }).forEach((_, index) => {
      const record = {
        header: index === 0,
        id: current.id,
        quantity: current.quantity,
        value: current.value
      }

      if (items.length === rowSize) {
        records.push({ items, rowSize });

        items = [];
      }

      items.push(record);
    });

    if (items.length < rowSize) {
      items.push(...Array(rowSize - items.length).fill(null));
    }

    records.push({ items, rowSize });

    return records;

  }, []);
}

console.log(getNewRecords());


Filtering input characters for a numeric text field using logical operators I did my code for "Bronze Challenge: Disallow Alphabetic characters" exercise in "iOS programming" book by Big Nerd Ranch (p.85). When I did the exercise I also changed code of func textField which was given in the book.

The exercise was to disallow alphabetic characters when user either use Bluetooth keyboard or by passing copied text from clipboard. Also the func does not allow to enter several decimal separators:

func textField(_ textField: UITextField,
               shouldChangeCharactersIn range: NSRange,
               replacementString string: String) -> Bool {

    let existingTextHasDecimalSeparator = textField.text?.range(of: ".")
    let replacementTextHasDecimalSeparator = string.range(of: ".")

    //check letters in replacement text:
    let letters = NSCharacterSet.letters
    let isRangeInReplacementString = string.rangeOfCharacter(from: letters)

    //check if there are double decimal separators (operator AND)
    //OR if there is a letter
    //then returnValueBool will be TRUE
    let returnValueBool = ((existingTextHasDecimalSeparator != nil)
        && (replacementTextHasDecimalSeparator != nil))
        || (isRangeInReplacementString != nil)

    //So returnValueBool is true if there is double digital separator or there is a letter. Function returns False in this case
     return !returnValueBool

    }

My code works well in simulator but I would like somebody experienced in Swift to have a look at my code and to comment on using logical operators.

Let's start with the assignment

let existingTextHasDecimalSeparator = textField.text?.range(of: ".")

The variable name suggests a boolean condition, but the value
is an optional range. It would be clearer to make it a boolean value:

let existingTextHasDecimalSeparator = textField.text?.range(of: ".") != nil

and similarly

let replacementTextHasDecimalSeparator = string.range(of: ".") != nil

The variable name isRangeInReplacementString is meaningless, this would
become

let existingTextHasLetters = string.rangeOfCharacter(from: .letters) != nil

and here .letters is inferred as CharacterSet.letters, so we don't
need an extra variable for that.

The variable name returnValueBool is also meaningless, that might
become

let isInvalidChange = (existingTextHasDecimalSeparator && replacementTextHasDecimalSeparator)
    || existingTextHasLetters
return !isInvalidChange

Now some simplifications: if the existing text has no decimal separator
then there is no need to check the replacement text for a separator.
And if you already detected a duplicate separator then you don't have
to check for invalid characters (letters). In other words: short-circuit:

func textField(_ textField: UITextField,
               shouldChangeCharactersIn range: NSRange,
               replacementString string: String) -> Bool {

    if textField.text?.range(of: ".") != nil && string.range(of: ".") != nil {
        return false
    }
    if string.rangeOfCharacter(from: .letters) != nil {
        return false
    }
    return true
}

But there are still two problems:

It is not possible to replace text containing the decimal separator
with new text also containing the decimal separator with copy/paste,
e.g. replace 1.2 by 3.4.
The decimal separator is locale dependent, it can be a period,
a comma (e.g. in Germany) and perhaps also other characters.

The first problem can be solved by computing the new string after
the replacement first, and then check if that is valid (only digits
and at most one decimal separator).

The second problem can be solved by using a NumberFormatter to check
if the text is a valid decimal number.

That would lead to the following implementation (which is essentially
a translation of the code in Big Nerd Ranch Bronze Challenge: Disallow Alphabetic Characters
to the current Swift 4):

func textField(_ textField: UITextField,
               shouldChangeCharactersIn range: NSRange,
               replacementString string: String) -> Bool {

    guard let text = textField.text else { return false }
    // Compute new text after replacement:
    let newText = text.replacingCharacters(in: Range(range, in: text)!, with: string)
    // Check for empty string:
    if newText.isEmpty || newText == "-" {
        return true
    }
    // Check for valid decimal number:
    let formatter = NumberFormatter()
    formatter.numberStyle = .decimal
    return formatter.number(from: newText) != nil
}


How can I optimize this for speed? (C++) Here is my method:

// can change
DWORD step[2] = { 0xAC, 0x723A };
packageType = 1;

DWORD computeLevel(DWORD blockNum)
{
    DWORD num = (blockNum / 0x70E4) * step[1];
    if (blockNum < 0x70E4)
        return num + step[0];
    return (1 << packageType) + num;
}

I am trying to optimize this for speed, as it gets called about 300,000 times and one little operation could add alot of time on. If I change the blockStep to just constant values, this makes it much, much faster. The problem is, is that they can change so I cannot make it const. Also, if I just have it return a straight up 0, no comparisons, it saves my code around 1 minute, 30 seconds, so this function is really slowing me down.

Also, it would be nice if this function could also be enhanced. I already tried by best, but it would be great if it can get better.

DWORD computeLevel0(DWORD blockNum)
{
    WORD quotient1 = (blockNum / 0xAA);
    BYTE quotient2 = (blockNum / 0x70E4);
    DWORD num = quotient1 * 0xAC;

    // optimizations (0 comparison)

    WORD mult1 = quotient1;
    mult1 |= mult1 >> 8;
    mult1 |= mult1 >> 4;
    mult1 |= mult1 >> 2;
    mult1 |= mult1 >> 1;
    mult1 &= 1;

    BYTE mult2 = quotient2;
    mult2 |= mult2 >> 4;
    mult2 |= mult2 >> 2;
    mult2 |= mult2 >> 1;
    mult2 &= 1;

    num += ((quotient2 + 1) << packageType) * mult1;
    num += (((1 << packageType)) * mult2) * mult1;

    return num;
}

The best optimization you can do is write code that works.

It makes it infinitely faster:

BYTE step[2] = { 0xAC, 0x723A };

    DWORD num = (blockNum / 0x70E4) * step[2];
                                //    ^^^^^^^   Broken. Undefined behavior.
                                //              The rest of the code is invalid.

Second I don't believe your numbers:

  1 minute, 30 seconds for 300,000 calls.

That's an awfully slow machine. 90 seconds for 300,000 calls or 3,333 calls a second. Even unoptimized this code runs (300,000) in less then 1/100 of a second on my machine when fully optimized it is closer to 1/1000 of a second.

So it is not this code that is causing you to slow down. But the result of this code may be affecting the call path inside your application and causing other more costly code to be executed.

Since this code takes 1/1000 of a second of the 90 second total. It is contributing to 0.0001% of the execution time. This is so insignificant that you should not even be trying to optimize this part of the code.

My test code:

#include <iostream>
#include <time.h>
#include <vector>
#include <algorithm>

int step[2] = { 0xAC, 0x723A };
int packageType = 1;

int computeLevel(int blockNum)
{
    int num = (blockNum / 0x70E4) * step[1]; // changed.
    if (blockNum < 0x70E4)
        return num + step[0];                // changed
    return (1 << packageType) + num;
}

// Add this to pre-vent the code being optimized to zero.
int result = 0;
int check  = 0;
int doComputeLevel(int blockNum)
{
    result += computeLevel(blockNum);
    ++check;
}

int main()
{
    // Set up random test data
    std::vector<int>    data;
    data.reserve(300010);
    for(int loop = 0;loop < 300000;++loop)
    {
        data.push_back(rand());
    }

    // Run the test
    clock_t t = clock();
    std::for_each(data.begin(), data.end(), doComputeLevel);
    clock_t e = clock();

    // print the results;
    std::cout << (e-t) << std::endl;
    std::cout << (e-t)*1.0/CLOCKS_PER_SEC << std::endl;
    std::cout << data.size() << " : " << check << " : " << result << std::endl;
}

> g++ -O3 t.cpp
> ./a.out
1812                           TICKS
0.001812                       TIME in seconds
300000 : 300000 : 62755196     iterations : check of iterations : result


SPOJ problem - The last digit of a number to a power (follow up) As a follow up to my post here. Please refer to the problem statement in that post.

I have edited this code to make it time efficient, but doing so the size of the source file has crossed the limit of 700 bytes. How do I reduce the file size?

#include <stdio.h>

int main()
{
    int t;
    scanf("%d",&t);
    while(t--)
    {
        long long int base,exponent;
        scanf("%lld%lld",&base,&exponent);
        if(exponent==0)
        {
            printf("1\n");
        }
        else
        {
            int i;
            int dig[5];
            dig[0]=1;
            for(i=1;i<=4;i++)
            {
                dig[i]=(base*dig[i-1])%10;
            }
            if(exponent%4==0)
                printf("%d\n",dig[4]);
            else if(exponent%4==1)
                printf("%d\n",dig[1]);
            else if(exponent%4==2)
                printf("%d\n",dig[2]);
            else if(exponent%4==3)
                printf("%d\n",dig[3]);
        }
    }
    return 0;
}

It really isn't that hard to bring your code under 700 bytes.  Just replacing base and exponent with a and b, respectively, would do the trick.  Normally, such renaming would hurt readability.  But here, you are just matching the terminology in the question, which arguably improves intelligibility.

You can also trim obvious fat, such as

eliminating the implicit return 0
long instead of long long int
switching to a more efficient brace style

The cases at the end can all be collapsed using a bit of modulo arithmetic.

There is plenty of room within 700 bytes for writing nice code.  You can expand dig into digit.  You can add customary spaces around operators for readability.  You can even write a long explanatory comment.

#include <stdio.h>

/*
The last digit of pow(a, b) cycles with b modulo 4.

0, 0, 0, 0, 0, ...
1, 1, 1, 1, 1, ...
2, 4, 8, 6, 2, ...
3, 9, 7, 1, 3, ...
4, 6, 4, 6, 4, ...
5, 5, 5, 5, 5, ...
6, 6, 6, 6, 6, ...
7, 9, 3, 1, 7, ...
8, 4, 2, 6, 8, ...
9, 1, 9, 1, 9, ...
*/
int main() {
    int t;
    scanf("%d", &t);
    while (t--) {
        long a, b;
        scanf("%ld%ld", &a, &b);
        if (b == 0) {
            printf("1\n");
        } else {
            int i;
            int digit[4];
            digit[0] = 1;
            for (i = 1; i <= 4; i++) {
                digit[i % 4] = (a * digit[i - 1]) % 10;
            }
            printf("%d\n", digit[b % 4]);
        }
    }
}

And we haven't even touched truly nefarious tricks like replacing spaces with tabs, which would save another hundred bytes!


Inky pinky ponky game / Josephus problem I have written below code for the famous Josephus problem, or inky pinky ponky game, or many other names. Does this looks good or is there any loophole if anyone can highlight. I shall be looking forward to optimize it now as much as possible.

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

typedef struct ll_core
{
    int c_ll;
    struct ll_core * next;
} node_d;

node_d * create_c_list(int ,int);
void printList(node_d *);
node_d * elimination(node_d *, int);

int main(void)
{
    node_d *c_list;
    int n_kids,num;

    printf("How many kids? ");
    scanf("%d",&n_kids);
    printf("How many no. of words? ");
    scanf("%d",&num);

    node_d *first_kid=create_c_list(n_kids,num);    // create the circular list and get the pointer to first node/tail.
    printList(first_kid);

    node_d *victor=elimination(first_kid,num);      //returns pointer to the winner/last kid left
    printList(victor);
}

node_d * create_c_list(int n_k,int n_w)
{
    node_d *first=malloc(sizeof(node_d));
    int dat=1;
    first=malloc(sizeof(node_d));
    first->c_ll=dat;
    first->next=first;

    int iter=0;
    while(iter< n_k-1)
    {
        node_d * new_p=malloc(sizeof(node_d));
        new_p->next=first->next;
        first->next=new_p;                   /*We are not changing the node pointed to by tail pointer anywhere*/
        new_p->c_ll=++dat;
        iter++;
    }

    return first;
}

node_d * elimination(node_d * fix, int cntr)
{
    node_d *it=fix, *prev=fix;
    int iter;

    while(1)
    {
         for(iter=1;iter<cntr;iter++)
    it=it->next;                            /*This pointer is always on the node to be deleted. Once node is deleted, it points to the node from where
                                            from where counting is to start next.*/

    while(prev->next != it)
    prev=prev->next;                        // This always points to the node just before the one pointed to by pointer 'it'.

    //if(prev->next == it)
    prev->next=it->next;
    printf("removed : %d\n",it->c_ll);
    node_d *temp=it;
    free(temp);
    it=prev->next;

    if(it==it->next)
    break;
    }

    return it;
}

void printList(node_d *list_tail)
{
    node_d *p = list_tail->next;

    if(p->next==p)
    {
        printf("Only one element at %p: Value : %d  next : %p\n",p,p->c_ll,p->next);
     return;
    }

    while(p != list_tail)
    {
        printf("Value : %d   next : %p\n",p->c_ll,p->next);
        p=p->next;
    }

    if(p == list_tail)
    {
        printf("tail.. value : %d next : %p\n",p->c_ll,p->next);
    }
}

Sorry, but I don't think this code is very good.

In create_c_list, the first node is allocated twice, leaking memory, and there
are no checks for allocation failure.  Parameters n_k and n_w are badly
named and the latter is not used.  The loop should be a for not while.
new_p could just as well be n, for 'node'.  Also, it is not obvious that
the 'c' in the name means 'circular' and you should add spaces around
operators ('=' etc).

In elimination, the function name should be a verb.  The outer while(1) loop is often preferred as for (;;) , although that is a matter of personal preference. Indefinite loops are usually best avoided anyway.
Inside this loop is a for loop, which should define iter in the loop and call it i.  Parameter cntr means nothing - it could be
'counter' of 'center'.  Give it a better name.  It derives from a value
entered by the user called num which gives away no secrets, and the prompt
was "How many no. of words?" - so num and cntr are the number of 'words',
but the list created by create_c_list makes no mention of 'words'.  It is as
if you don't want the reader to understand what it does.  The for loop locates the node according to the cntr, which takes a long time if cntr is zero, as the loops starts from 1.  Then you do another
loop, this time a while in which you find the list entry before the one
(called it) that you located in the for loop.  You could have combined the two loops.  You then delete it using an unnecessary temporary
variable.
And so the outer loop continues until there is only one entry in the list - that condition could have been the condition for the outer while loop.

In printList (which uses camelCase while create_c_list doesn't)  you don't
modify the list, so it should be passed const.  Parameter list_tail is
inaccurate - a circular list has no tail.  Call it just list.

Also, putting main last avoids the need for prototypes, all functions except
main should be static and the names of struct ll_core and c_ll would
be better as struct node and value.


Markov chains to generate text This is my Python 3 code to generate text using a Markov chain.
The chain first randomly selects a word from a text file. Out of all the occurrences of that word in the text file, the program finds the most populer next word for the first randomly selected word. It continues the process to form a very understandable text.

The best thing about this code is that it copies the style of writing in the text file. At the first trial of the code, I put 3 of the most famous Shakespeare's plays, the Macbeth, Julius Caesar and The Comedy of Errors. And when I generated text from it the outcome was very much like a Shakespeare poem.

My knowledge in Python coding is between intermediate and expert. Please review my code and make changes as you like. I want suggestions from both experts and beginners.

# Markov Chain Poetry

import random
import sys

poems = open("text.txt", "r").read()
poems = ''.join([i for i in poems if not i.isdigit()]).replace("\n\n", "
").split(' ')
# This process the list of poems. Double line breaks separate poems, so they are removed.
# Splitting along spaces creates a list of all words.

index = 1
chain = {}
count = 1000 # Desired word count of output

# This loop creates a dicitonary called "chain". Each key is a word, and the value of each key
# is an array of the words that immediately followed it.
for word in poems[index:]:
    key = poems[index - 1]
    if key in chain:
        chain[key].append(word)
    else:
        chain[key] = [word]
    index += 1

word1 = random.choice(list(chain.keys())) #random first word
message = word1.capitalize()

# Picks the next word over and over until word count achieved
while len(message.split(' ')) < count:
    word2 = random.choice(chain[word1])
    word1 = word2
    message += ' ' + word2

# creates new file with output and prints it to the terminal
with open("output.txt", "w") as file:
    file.write(message)
output = open("output.txt","r")
print(output.read())

Thanks!!!

Functions

Split the code into functions, also split the generation and the presentation. Your algorithm has some clear distinct tasks, so split along these lines:

read input
assemble chain
construct new poem
output

This way, you can reuse parts of the code, save intermediary results and test the parts individually.

generators

instead of keeping all the intermediary lists in memory, generators can be a lot more memory efficient. I try to use them as much as possible. substantiating them to a list or dict when needed is easy.

read the input

There is no need to assemble the intermediary list in ''.join([i for i in poems if not i.isdigit()]). join is perfectly capable of handling any iterable, so also a generator expression.

use the with statement to open files:

def read_input(filename):
    """reads `file`, yields the consecutive words"""
    with open(filename, 'r') as file:
        for line in file:
            for word in line.split(''):
                if word and not word.isdigit():
                    yield word

with regular expressions, and by hoisting the IO, you can ease this method even more:

def read_input_re(file):
    pattern = re.compile("[a-zA-Z][a-zA-Z']+")
    for line in file:
        for word in pattern.finditer(line):
            yield word.group()

which then can be called with a file:

def read_file(filename):
    with open(filename, 'r') as file:
        return read_input_re(file)

or with any iterable that yields strings as argument. For example if poem holds a multi-line string with a poem:words = read_input_re(poem.split('\n'))

This refactoring also makes loading the different poems from different textfiles almost trivial:

filenames = ['file1.txt', 'file2.txt', ...]
parsed_files = (read_file(filename) for filename in filenames)
words = itertools.chain.from_iterable(parsed_files)

If you want all the words in the chain lowercase, so FROM and from are marked as the same word, just add

words = map(str.lower, words)

assemble the chain

Here a collections.defaultdict(list) is the natural datastructure to for the chain.

Instead of using hard indexing to get the subsequent words, which is impossible to do with a generator, you can do it like this:

def assemble_chain(words):
    chain = defaultdict(list)
    try:
        word, following = next(words), next(words)
        while True:
            chain[word].append(following)
            word, following = following, next(words)
    except StopIteration:
        return chain

or using some of itertools' useful functions:

from itertools import tee, islice

def assemble_chain_itertools(words):
    chain = defaultdict(list)
    words, followings = tee(words, 2)
    for word, following in zip(words, islice(followings, 1, None)):
        chain[word].append(following)
    return chain

Or even using a deque:

from collections import deque
def assemble_chain_deque(words):
    chain = defaultdict(list)
    queue = deque(islice(words, 1), maxlen=2)
    for new_word in words:
        queue.append(new_word)
        word, following = queue
        chain[word].append(following)
    return chain

whichever is more clear is a matter of habit and experience, If performance is important, you will need to time them.

create the poem

Since you will be asking for a new word a lot, it can pay to extract it to its own function:

def get_random_word(choices):
    return random.choice(list(choices))

Then you can make an endless generator yielding subsequent words:

def generate_words(chain):
    word = get_random_word(chain)
    while True:
        yield word
        if word in chain:
            word = get_random_word(chain[word])
        else:
            word = get_random_word(chain)

We then us islice to gather the number of words we need, which then can be pasted together with ' '.join()

length = 10
poem = islice(generate_words(chain), length)
poem = ' '.join(poem)

"be tatter'd we desire famine where all eating ask'd where"

Once you have that, making a poem of a number of lines with set length is also easy:

def construct_poem(chain, lines, line_length):
    for _ in range(lines):
        yield ' '.join(islice(generate_words(chain), line_length))

lines = construct_poem(chain, 4, 10)
lines = map(str.capitalize, lines)
print('\n'.join(lines))

Be tatter'd we desire famine where all eating ask'd where
Deep trenches that thereby the riper substantial fuel shall beseige
Treasure of small pity the riper eyes were to the
Foe to the riper by time spring within and make

I think it makes sense to do the capitalization after the line has been assembled. Yet another separation of generation and presentation:

def construct_poem2(chain, line_lengths):
    for line_length in line_lengths:
        yield ' '.join(islice(generate_words(chain), line_length))

line_lengths = [10, 8, 8, 10]
lines = construct_poem2(chain, line_lengths)
lines = map(str.capitalize, lines)
print('\n'.join(lines))

Be tatter'd we desire famine where all eating ask'd where
Deep trenches that thereby the riper substantial fuel
Shall beseige treasure of small pity the riper
Eyes were to the riper memory but eyes were to


Highlight Two Closest Points Java Swing Question from the book:
(Geometry: closest pair of points) Write a program that lets the user click on
the panel to dynamically create points. Initially, the panel is empty. When a
panel has two or more points, highlight the pair of closest points. Whenever a
new point is created, a new pair of closest points is highlighted. Display the
points using small circles and highlight the points using filled circles, as
shown in Figure 16.26a–c. (Hint: store the points in an ArrayList.)

Solution attempt:

import javax.swing.*;
import java.awt.*;
import java.awt.event.*;
import java.util.*;

public class ClosestPairOfPoints extends JFrame {
    private PointsPanel pointsPanel = new PointsPanel();

    public ClosestPairOfPoints() {
        add(pointsPanel);
    }
    //Main method
    public static void main(String[] args) {
        JFrame frame = new ClosestPairOfPoints();
        frame.setTitle("Exercise16_22");
        frame.setSize(300, 300);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setLocationRelativeTo(null);
        frame.setVisible(true);
    }

    private class PointsPanel extends JPanel {
        private ArrayList<Point> points = new ArrayList<Point>();
        private int[] closestPairIndices = {2, 2};

        public PointsPanel() {
            addMouseListener(new MouseAdapter() {
                @Override
                public void mousePressed(MouseEvent e) {
                    addPoint();
                    repaint();
                }
            });
        }

        /** Add new random point **/
        public void addPoint() {
            int randomX = (int)(Math.random() * getWidth());
            int randomY = (int)(Math.random() * getHeight());

            points.add(new Point(randomX, randomY));
            if (points.size() > 1) {
                updateClosestPair();
            }
        }

        /** Alternate method **/
        private int minDistance = Integer.MAX_VALUE;
        private void updateClosestPair() {
            if (minDistance == Integer.MAX_VALUE) {     // Obtain first actual minDistance
                minDistance = getDistance(points.get(0), points.get(1));
                closestPairIndices[0] = 0;
                closestPairIndices[1] = 1;
            }
            else {      // Compare previous points with latest point
                for (int i = 0; i < points.size() - 1; i++) {
                    int testDistance = getDistance(points.get(i), points.get(points.size() - 1));
                    if (testDistance < minDistance) {
                        minDistance = testDistance;
                        closestPairIndices[0] = i;
                        closestPairIndices[1] = points.size() - 1;
                    }
                }
            }
        }

        private int getDistance(Point p1, Point p2) {
            double a = (double)Math.abs(p1.getX() - p2.getX());
            double b = (double)Math.abs(p1.getY() - p2.getY());

            return (int)Math.sqrt(a * a + b * b);
        }

        /** Paint Points from ArrayList and highlight the Points at closestPairIndices **/
        @Override
        protected void paintComponent(Graphics g) {
            super.paintComponent(g);

            // Draw every point
            for (int i = 0; i < points.size(); i++) {
                if (i == closestPairIndices[0] || i == closestPairIndices[1]) {
                    g.fillOval((int)points.get(i).getX(), (int)points.get(i).getY(), 5, 5);
                }
                else {
                    g.drawOval((int)points.get(i).getX(), (int)points.get(i).getY(), 5, 5);
                }
            }
        }
    }
}

I think the logic for getting the minimal distance between the points would work even without the explicit check against MAX_INT:

private void updateClosestPair() {
    for (int i = 0; i < points.size() - 1; i++) {
        int testDistance = getDistance(points.get(i), points.get(points.size() - 1));
        if (testDistance < minDistance) {
            minDistance = testDistance;
            closestPairIndices[0] = i;
            closestPairIndices[1] = points.size() - 1;
        }
    }
}

Although I did not run the above code, I am convinced that it would just work also for the first pair (or it could be fixed with minimal changes), since it would just check the second point against the first one, and set their distance as the minimum.
Going one step further, you also don't need to check that there are at least two points, when calling updateClosestPair. In this case, the iteration would never start at all.


Cleaning up date strings in Python Working from a CSV of about 14.000 lines, I need to match dates in the CSV against some API. I'm reading the CSV using Pandas (for various other reasons). Most often, the date is just a year, i.e. an integer, which Pandas converts to a float. Other date formats also occur (see main in the code below). 12-10-1887 is the 12th of October, not the 10th of December.

The API also returns date strings, often also incomplete, for example when only year but not month and/or day of birth are known. Single-digit days and months do get a leading zero, and format is as close to ISO-date as possible, e.g. March 1675 will be "1675-03".

I think my best bet is not to convert to type datetime, but to try to match strings.

EDIT: because this way I can let the API do more work.

http://baseURL/name:Gupta,Anil&date:1956

Without the date, I get all Anil Gupta's. With it, I get all Anil Gupta's with either born year (most likely), death year, of year of activity 1956. All in all, this is MUCH less work than writing dateconverters for fields that are empty in 90% of the cases. In general, name + exact date of birth is a pretty unique identifier.

So I wrote this dateclean.py (Python 3.7):

import re
from datetime import datetime as dt

def cleanup(date):
    patterns = [# 0) 1-12-1963
                r'(\d{1,2})-(\d{1,2})-(\d{4})$',
                # 1) 1789-7-14
                r'(\d{4})-(\d{1,2})-(\d{1,2})$',
                # 2) '1945-2'
                r'(\d{4})-(\d{1,2})$',
                # 3) 2-1883
                r'(\d{1,2})-(\d{4})$'
                ]

    try:
        return str(int(date))
    except ValueError:
        pass

    for pat in patterns:
        q = re.match(pat, date)
        if q:
            if pat == patterns[0]:
                year = re.sub(patterns[0], r'\3', date)
                month = re.sub(patterns[0], r'\2', date)
                day = re.sub(patterns[0],  r'\1', date)
                return '{0}-{1:0>2}-{2:0>2}'.format(year, month, day)
            if pat == patterns[1]:
                year = re.sub(patterns[1], r'\1', date)
                month = re.sub(patterns[1], r'\2', date)
                day = re.sub(patterns[1],  r'\3', date)
                return '{0}-{1:0>2}-{2:0>2}'.format(year, month, day)
            if pat == patterns[2]:
                year = re.sub(patterns[2], r'\1', date)
                month = re.sub(patterns[2], r'\2', date)
                return '{0}-{1:0>2}'.format(year, month)
            if pat == patterns[3]:
                year = re.sub(patterns[3], r'\2', date)
                month = re.sub(patterns[3], r'\1', date)
                return '{0}-{1:0>2}'.format(year, month)
            else:
                return date

def main():
    dates = 1858.0, '1-12-1963', '1945-2', '7-2018', '1789-7-14',
    for date in dates:
        print('in: {}  out: {}'.format(date, cleanup(date)))

if __name__ == "__main__":
    main()

The list dates in main() contains all formats known until now. On the plus side, by using this method, adding new formats is easy.

There are a lot of patterns[x], which is not at all DRY, yet I don't see how I can avoid that: I have to split each input string into year, month and day, if any (using r'\1' etc.)

Also, I know flat is better than nested, but I don't see how I can improve on that. I do try to return as early as possible.

Readability is more important to me than performance, because the API is a bottleneck anyway. Then again, serious slowing-downers should be avoided.

For starter, congratulations, the code is clean and uses rather good constructs. I just have two nitpicks about the layout:

I prefer to indent lists before the first row:

patterns = [
        # 0) 1-12-1963
        r'(\d{1,2})-(\d{1,2})-(\d{4})$',
        # 1) 1789-7-14
        r'(\d{4})-(\d{1,2})-(\d{1,2})$',
        # 2) '1945-2'
        r'(\d{4})-(\d{1,2})$',
        # 3) 2-1883
        r'(\d{1,2})-(\d{4})$',
]

PEP8 recommends two blank lines before top-level function definition.

You can also simplify the boilerplate of a try: … except Something: pass by using the contextlib.suppress context manager instead:

with suppress(ValueError):
    return str(int(date))

Lastly your pattern manipulation:

a match object can return all groups at once using its groups method, no need to special parse them;
you can unpack the groups tuple as arguments to format directly;
you can compile all your regexp beforehand instead of letting the regexp engine do it on-the-fly;
you should associate the output format string to each pattern to remove the need to search back which pattern matched and how it should be processed.

Proposed improvements:

import re
from contextlib import suppress

PATTERNS = [
        # 0) 1-12-1963 => 1963-12-01
        (re.compile(r'(\d{1,2})-(\d{1,2})-(\d{4})$'), '{2}-{1:0>2}-{0:0>2}'),
        # 1) 1789-7-14 => 1789-07-14
        (re.compile(r'(\d{4})-(\d{1,2})-(\d{1,2})$'), '{0}-{1:0>2}-{2:0>2}'),
        # 2) 1945-2 => 1945-02
        (re.compile(r'(\d{4})-(\d{1,2})$'), '{0}-{1:0>2}'),
        # 3) 2-1883 => 1883-02
        (re.compile(r'(\d{1,2})-(\d{4})$'), '{1}-{0:0>2}'),
]

def cleanup(date):
    with suppress(ValueError):
        return str(int(date))

    for pattern, formater in PATTERNS:
        match = pattern.match(date)
        if match is not None:
            return formater.format(*match.groups())
    return date

def main():
    dates = 1858.0, '1-12-1963', '1945-2', '7-2018', '1789-7-14',
    for date in dates:
        print(f'in: {date}, out: {cleanup(date)}')

if __name__ == '__main__':
    main()


Immutable recursive binary search of a generic array I am new to F#. Is the following code, which represents about three hours of work, a canonical way to achieve binary search? If not, how can it be improved for readability and maintainability?

let binarySearch (array: 'a[]) (target: 'a) =
    let rec search min max =
        match (min, max) with
        | (min, max) when max < min -> -1
        | _ ->
            let guess = ((min + max) / 2);
            match array.[guess] with
            | x when x = target -> guess
            | x when x < target -> search (guess + 1) (max)
            | _ -> search (min) (guess - 1)
    search (0) (array.Length - 1)

Aspects that I already like:

hiding the recursive function within the non-recursive function.
using pattern matching instead of if-else-then, based on suggestions here,
accepting generic types with `a.

I would strive for symmetry:

        match array.[guess] with
        | x when x > target -> search (min) (guess - 1)
        | x when x < target -> search (guess + 1) (max)
        | _ -> guess


Visitor pattern in C++ I have just implemented a quick test of the visitor design pattern in C++ and have a few questions regarding the code.

main.cpp

#include <iostream>

#include "Base.h"
#include "Derived.h"
#include "Visitor.h"

int main() {
    Base base;
    std::cout << "hello\n";
    Base * derived = new Derived();
    Visitor v;
    base.acceptvisitor(v);
    derived->acceptvisitor(v);
}

IVisitable.h

#ifndef IVISITABLE_H
#define IVISITABLE_H

class Visitor;

class IVisitable {
    virtual void acceptvisitor(Visitor v) = 0;
};

#endif

Base.h

#ifndef BASE_H
#define BASE_H

#include <string>

#include "IVisitable.h"

class Base : public IVisitable {
public:
    void acceptvisitor(Visitor v);
};

#endif

Base.cpp

#include "Base.h"

#include "Visitor.h"

void Base::acceptvisitor(Visitor v) {
    v.visit(*this);
}

Derived.h

#include "Base.h"

class Derived : public Base {
public:
    void acceptvisitor(Visitor v);
};

Derived.cpp

#include "Derived.h"

#include "Visitor.h"

void Derived::acceptvisitor(Visitor v) {
    v.visit(*this);
}

Visitor.h

#ifndef VISITOR_H
#define VISITOR_H

class Base;
class Derived;

class Visitor {
public:
    void visit(Base b);
    void visit(Derived d);
};

#endif

Visitor.cpp

#include "Visitor.h"

#include <iostream>

#include "Base.h"
#include "Derived.h"

void Visitor::visit(Base b) {
    std::cout << "Visiting Base\n";
}

void Visitor::visit(Derived d) {
    std::cout << "Visiting Derived\n";
}

And here are some questions, of course I am interested in all remarks you may have not just answers to these.

How do you keep track of includes? Even in this short example I now have includes that are not needed anymore.
Is this #ifndef treatment of header files still normal? I picked it up somewhere but have never actually seen anybody else do it.
Is there any way to treat an object as a superclass without a pointer? I.e. can I do something like this: Base d = Derived();
When can I use a forward declaration like class Visitor; and when do I need to include the actual header file? Or when should I?
Should I make implementations of abstract functions virtual? It seems not to matter.

Well your code is all broken.
But we will get to that as we answer your questions:
Answers to Questions

How do you keep track of includes? Even in this short example I now have includes that are not needed anymore.

Only include a header file if you need the class definition.
Forward declare everything else

Using this rule it is easy to only include files you need. In your header files only include other header files if you need them. You only need them if the object you are defining has a member of that type or is derived from that type (or takes a parameter to a method by value).
In all other situations forward declare.
This reduces the header count considerably.
In the source file you include all the header files of object you use to implement your code (that your header files have not included).
#ifndef BASE_H
#define BASE_H

#include <string>  // There is no need for this.
                   // There are no members that use string
                   // You are not deriving from string (who would)
                   // You are not passing a string as a parameter
                   // Remove this.

#include "IVisitable.h"

class Base : public IVisitable {
public:
    void acceptvisitor(Visitor v);  // By the way this is probably wrong.
                                    // You are passing by value and thus will
                                    // Make a copy.
                                    // Here you (if you really
                                    // wanted to pass by value) you should have
                                    // include "Visitor.h".
                                    //
                                    // But you don't want to pass by value you want
                                    // to pass by reference. So you just need to
                                    // forward declare.
};

#endif

Is this #ifndef treatment of header files still normal? I picked it up somewhere but have never actually seen anybody else do it.

Yes you should always place include guards around header files to protect from multiple inclusion.

Is there any way to treat an object as a superclass without a pointer? I.e. can I do something like this: Base d = Derived();

Well actually what you have written will compile:
Unfortunately it does not do what you expect. Here you have object slicing. The Base part of the object you created is sliced out and copied into d. What you need is a pointer (or a reference).
It looks like you are used to languages like Java. Where all objects are dynamically allocated. C++ has a much superior mechanism that allows us to accurately control the lifetime of the object. The disadvantage is that it adds complexity to the language.
What is called a pointer in C/C++ (to distinguish it from C++ references) would in most other languages be called a reference. C++ has both local objects (automatic storage duration object) and dynamically allocated objects (dynamic storage duration objects) thus we need a convention that allows access to both types of object, hence we use * to refer to objects that are dynamically allocated (to distinguish them from local objects).
BUT it is unusual to use pointers directly in C++ (unless you are implementing some real low level stuff). Most of the time when you dynamically allocate objects you will use a smart pointer that defines the lifespan of the object (much like other languages with garbage collection (but better)). std::shared_ptr<T> would be the equivalent of T in Java.
std::shared_ptr<Base>   d = new Derived();  // dynamically allocated object
                                            // That will be correctly destroyed
                                            // when there are no more references.

When can I use a forward declaration like "class Visitor;" and when do I need to include the actual header file. Or when should I?

As described above.
In the header file (were there is only declaration) only include another header if the header file defines a type that is a member or is used as a parent or used to pass a parameter by value (parameters are infrequent as they are normally passed by reference). In all other cases in a header file you should use forward declaration. In the source file include the header files that define types that you use (ie call methods on).
Note: You only need a forward declaration for objects that are pointers or references.

Should I make implementations of abstract functions virtual? It seems not to matter.

Only virtual methods can be abstract.
If you forget to define a method the compiler will not complain (as you may define it in another compilation unit). The linker will only complain if somebody tries to call the method and can't find a definition. So if you don't call it then there will be no error (but if you don't call it then it does not matter).
When you implement a virtual function in a derived class it is probably best to mark it as virtual to show a subsequent maintainer that it is virtual function (but it is not required by the language).
You should also note that C++11 introduces they keyword override. Which is an indicator that this method overrides a virtual method in a base class. If there is not such method in the base class it is a compilation error.
Other Notes
Your code passes all parameters by value. This is probably not what you want (as a copy will be made). If it is a derived type the base parameter type will be sliced out and passed to the method. So pass by reference.
class IVisitable {
    virtual void acceptvisitor(Visitor& v) = 0;
};                          //       ^^^   pass by reference

Also note that you can have more than one class defined in a file. Thus personally I would have put the IVisitable and Visitor patterns in the same file (they are tightly coupled anyway).


A redundant data service I've got a service that loads data from a databasr and it performs almost the same operations in all methods.
   private async Task GetProvenances()
    {
        try
        {
            var result = await commonRepository.GetProvenances();
            if (result != null)
            {
                provenanceContainerService.Provenances = result;
                LogTo.Information(InitCacheMessages.STR_GET_PROVENANCES, result.Count);
            }
            else
            {
                LogTo.Warning(InitCacheMessages.STR_GET_PROVENANCES_FAILED);
            }
        }
        catch (Exception ex)
        {
            exceptionService.HandleException(ex);
        }
    }

    private async Task GetPkInstrumentMarkets()
    {
        try
        {
            var result = await commonRepository.GetPkInstrumentMarkets();
            if (result != null)
            {
                pkInstrumentMarketContainerService.PkInstrumentMarkets = result;
                LogTo.Information(InitCacheMessages.STR_GET_PK_INSTRUMENT_MARKETS, result.Count);
            }
            else
            {
                LogTo.Warning(InitCacheMessages.STR_GET_PK_INSTRUMENT_MARKETS_FAILED);
            }
        }
        catch (Exception ex)
        {
            exceptionService.HandleException(ex);
        }
    }

    private async Task GetPkMotivations()
    {
        try
        {
            var result = await commonRepository.GetPkMotivations();
            if (result != null)
            {
                pkMotivationsContainerService.PkMotivations = result;
                LogTo.Information(InitCacheMessages.STR_GET_PK_MOTIVATIONS, result.Count);
            }
            else
            {
                LogTo.Warning(InitCacheMessages.STR_GET_PK_MOTIVATIONS_FAILED);
            }
        }
        catch (Exception ex)
        {
            exceptionService.HandleException(ex);
        }
    }

How can I refactor this so I don't have to repeat the same code for about 30 methods?
UPDATE 1
Hello,
I've done this way this night
  protected async Task PerformInitInternal(string okMessage, string koMessage, Func<Task<IList<T>>> function)
    {
        try
        {
            var result = await function();
            if (result != null)
            {
                Items = result;
                LogTo.Information(okMessage, result.Count);
            }
            else
            {
                LogTo.Warning(koMessage);
            }

            isInitialized = true;

        }
        catch (Exception ex)
        {
            ExceptionService.HandleException(ex);
        }
    }

And I call it this way
  public override Task Init()
    {
        return PerformInitInternal(InitCacheMessages.STR_GET_COUNTERPARTS,
            InitCacheMessages.STR_GET_COUNTERPARTS_FAILED, () => CommonRepository.GetCrossSplitAsync());
    }

Can this be ok?

It's hard to review just a small part of your code like this (for example, the way you're using Items and isInitialized looks like code smell to me, but it's hard to tell like this), so I'll focus on what I can actually see.

protected async Task PerformInitInternal(string okMessage, string koMessage, Func<Task<IList<T>>> function)

It's an asynchronous method, so its name should end with Async. Also, PerformInit is pretty vague, maybe a more concrete name could be better, for example, LoadData?
While the symmetry between OK and KO is fun, I don't think koMessage is a good name. successMessage and errorMessage would be better.
Speaking of which, do you actually need both messages? Or any messages at all? I would consider a completely generic message, assuming the rest of the message can be inferred from other parts of the log, like the type name.
function is too vague.


Find all sub matrices of type N*M in it which are palindromic Today I had an interview, where I was asked to solve this question.

A \$N*M\$ matrix is called a palindromic matrix, if for all of its rows and columns, elements read from left to right in a row are same as elements read from right to left, and similarly, elements read from top to bottom in a column are same as elements read from bottom to top in the column.

Example: below matrices are palindromic

3*5 Matrix

1 2 2 2 1
2 1 1 1 2
1 2 2 2 1

  Given a matrix, find all submatrices of type \$N*M\$ in it which are palindromic. If the original matrix is a palindrome, print it as well.
  Constraints: \$2 <= N, 2 <= M\$

  I wrote this code, How do I improve its complexity?

def check_palindrome(mat):
    if mat:
        m = len(mat)
        n = len(mat[0])
        for i in xrange(m/2 + 1):
            for j in xrange(n/2 + 1):
                if mat[i][j] != mat[m-1-i][j] or mat[i][j] != mat[i][n-1-j] or mat[i][j] != mat[m-1-i][n-1-j]:
                    return False

    return True

def check_submatrix_palindome(mat):
    m = len(mat)
    n = len(mat[0])

    for i in xrange(m-1):
        for j in xrange(n-1):
            for k in xrange(i+2,m):
                for l in xrange(j+2,n):
                    sub_mat = [mat[x][j:l+1] for x in xrange(i,k+1)]
                    if check_palindrome(sub_mat):
                        print "palindrome =", sub_mat

    return

m = int(raw_input())
mat = []
for i in xrange(m):
    row = []
    row = map(int, raw_input().strip().split(' '))
    mat.append(row)

check_submatrix_palindome(mat)

Checking if a matrix is a palindrom can be simplified using the fact that you store your matrices as a list of rows.

If the matrix is a palindrom, then:

each line is a palindrom by itself;
each line is equal to its symetic (first with last, second with the one before last, etc).

Which can be written like:

def check_palindrome(mat):
    for i, row in enumerate(mat, 1):
        if row != row[::-1]:
            # The current row is not a palindrome
            return False
        if row != mat[-i]:
            # One of the columns is not a palindrome
            return False
    return True

Here I use the second argument of enumerate to get the correct index when accessing the matrix backwards.

This writing can be simplified using the all builtin:

def check_palindrome(mat):
    return all(
            row == mat[-i] and row == row[::-1]
            for i, row in enumerate(mat, 1)
    )

You can also shave half the tests by using enumerate(mat[:(len(mat)+1)//2], 1) but it won't change the overall complexity.


JavaScript: Find the number in an array which is closest to an given number It's actually an exercise from an Assembly-course I'm doing:

  Find closest number.

Add the following into the data section:

nums  dd  23h,75h,111h,0abch,443h,1000h,5h,2213h,433a34h,0deadbeafh

This is an array of numbers.

Write a program that receives a number x as input, and finds the dword
inside the array nums, which is the closest to x. (We define the distance
between two numbers to be the absolute value of the difference: |a-b|).

Example:

For the input of 100h, the result will be 111h, because 111h is closer to
100h than any other number in the nums array. (|100h - 111h| = 11h).

Full exercise-sheet on GitHub.

Because it's easier for me to develop the algorithm in more familiar JavaScript and because it's a nice exercise I have done it in JavaScript first. Now the only thing left to do is to find an Assembly-specific way.

Anyway: Here's the JavaScript-code I've written to solve the task.

let nums = [ 0x23, 0x75, 0x111,
             0xabc, 0x443, 0x1000,
             0x5, 0x2213, 0x433a34,
             0xdeadbeaf
           ];
let given = 0x100; // { 256 }10

// --- The Relevant Part --------------------------------------
function findIndexClosestNumber(arrayToSearch, compareNumber) {
  'use strict';
  let closest = compareNumber;
  let indexClosest = 0;

  arrayToSearch.forEach((currentNumber, index) => {
    let currentDistance =
      Math.abs(compareNumber - currentNumber);

    if (currentDistance < closest) {
      indexClosest = index;
      closest = currentDistance;
    }
  });

  return indexClosest;
}
// -------------------------------------------------------------

// ------ Testing & Demo ---------------------------------------
console.log(findIndexClosestNumber(nums, given));

nums.length = 0;

for (let i = 0; i <= 90; i += 10) {
  nums.push(i);
}

console.log(nums);

for (let i = 0; i < 10; i++) {
  var randomNumber = Math.floor((Math.random() * 100));

  console.log('%s gives index %s', randomNumber,
    findIndexClosestNumber(
      nums, randomNumber));
}

Can I expect my implementation to solve the task in a correct way?
Was it a good idea to initialize the closest-variable with the value of the compareNumber-parameter?
Was it a good idea to check for currentDistance < closest? Or should I have used currentDistance <= closest?

Currently the first occurrence-index is returned in case the number is contained multiple times. Using <= it would return the last occurrence-index.

I'm not sure what's the better practice.

Any other comments concerning structure, naming, used methods, algorithm appreciated too.

And if someone knows a cool JavaScript specific way to solve the task: That would interest me too. ;)

Your main questions

  Can I expect my implementation to solve the task in a correct way?

You should know that the answer is "yes" before posting a question on Code Review.

  Was it a good idea to initialize the 'closest'-variable with the value of the 'compareNumber'-parameter?

No, it's a mistake. The function incorrectly returns 0 for input [10, 9], 3.
When looking for the minimum distance,
you must choose as the default the largest possible number.
For example if you target ES6, then Number.MAX_SAFE_INTEGER.

  Was it a good idea to check for 'currentDistance < closest'? Or should I have used 'currentDistance <= closest'?

That depends entirely on the specification.
If you're looking for the first closest index, then use <.
If the last, then use >.

If the specification was not clear,
then ask to clarify.

Naming

The names are overly verbose.
When short names are sufficient, they are better than long names.

For example,
in a function to find the closest index to a number,
the array parameter is obviously for searching,
so instead of arrayToSearch, it could be simply nums,
and compareNumber could be target.

I would have written like this:

function indexOfClosest(nums, target) {
  let closest = Number.MAX_SAFE_INTEGER;
  let index = 0;

  nums.forEach((num, i) => {
    let dist = Math.abs(target - num);

    if (dist < closest) {
      index = i;
      closest = dist;
    }
  });

  return index;
}


General practitioner collection The following code is used to find the usual_gp(General Practitioner) from gpCollection variable of type TreeMap<Long(Date), SummableMap<String(GPId), Integer(GPCount)>> and store result (usual GP) along with date on usualGPCollection Map.

Does this code follow common best practices? Logic implemented on below code is

        Long startDate = gpCollection.firstKey();
        Long endDate = gpCollection.lastKey();

        TreeMap<Long, String> usualGPCollection = new TreeMap<Long, String>();

        for (Long i = startDate; i <= new DateTime(endDate).plusYears(1).getMillis(); i = i + new DateTime(i).plusMonths(1).getMillis()) {

            Long beforeOneYearDate = new DateTime(i).minusMonths(12).getMillis();
            Long beforeSixMonthDate = new DateTime(i).minusMonths(6).getMillis();
            Long beforeThreeMonthDate = new DateTime(i).minusMonths(3).getMillis();

            List<String> returnGPCollection = customSubMap(gpCollection, beforeOneYearDate, i);

            if(returnGPCollection.size() == 1) {

                usualGPCollection.put(i, returnGPCollection.get(0));

            } else if(returnGPCollection.size()>1){

                returnGPCollection = customSubMap(gpCollection, beforeSixMonthDate, i);

                if(returnGPCollection.size() == 1) {

                    usualGPCollection.put(i, returnGPCollection.get(0));

                }else  if(returnGPCollection.size()>1){

                    returnGPCollection = customSubMap(gpCollection, beforeThreeMonthDate, i);

                    if(returnGPCollection.size() == 1) {

                        usualGPCollection.put(i, returnGPCollection.get(0));

                    } else if(returnGPCollection.size()>1){

//                        returnGPCollection = customSubMap(gpCollection, i);//todo

                    }
                }

                returnGPCollection = customSubMap(gpCollection, beforeOneYearDate, i);
            }
    }

// customSubMap() Function is used to find max count GP on given date range.

private List<String> customSubMap(TreeMap<Long, SummableMap<String, Integer>>  gpCollectionMap, Long fromDate, Long toDate) {
        List<String> returnMap = null;

        SortedMap<Long, SummableMap<String, Integer>> temp = gpCollectionMap.subMap(fromDate, toDate);
        Collection values = temp.values();
        Iterator<SummableMap<String, Integer>> test = values.iterator();

        SummableMap<String, Integer> resultMap = new SummableMap<String, Integer>();
        TreeMap<Integer, List<String>> reverseTree = new TreeMap<Integer, List<String>>();

        while (test.hasNext()) {
            resultMap.putAll(test.next());
//            String  key = test.next().clone();
        }

        List mapValues = new ArrayList(resultMap.values());
        Collections.sort(mapValues);

        NavigableMap<String, Integer> resultMapUpdated = sortHashMapByValuesD(resultMap);

        String firstKey = resultMapUpdated.firstKey();
        String secondKey = resultMapUpdated.lowerKey(firstKey);

        Integer firstValue = resultMapUpdated.get(firstKey);
        Integer secondValue = resultMapUpdated.get(secondKey);

        if(firstValue == secondValue){
            returnMap.add(firstKey);
            returnMap.add(secondKey);
        }else {
            returnMap.add(firstKey);
        }

        return returnMap;
    }

    private List<String> customSubMap(TreeMap<Long, SummableMap<String, Integer>>  gpCollectionMap, Long toDate) {

        List<String> returnMap = null;
        Long beforeThreeMonthDate = new DateTime(toDate).minusMonths(3).getMillis();

        SortedMap<Long, SummableMap<String, Integer>> temp = gpCollectionMap.subMap(beforeThreeMonthDate, toDate);
        String latestGP = temp.get(temp.lastKey()).keySet().iterator().next();

        returnMap.add(latestGP);

        return returnMap;
    }

When you have cascading conditions like you have, it can become 'messy'. At some point the design-pattern 'Chain of responsibility' becomes useful....

Consider an interface:

public interface GPSelector {
    String selectGP(TreeMap<Long, SummableMap<String, Integer>> gpCollection, Long calcdate);
}

Then, consider an array of concrete implementations of that interface:

private static final GPSelector[] GPRULES = new GPSelector[] {

    // 1-year rule
    new GPSelector() {
        public String selectGP(TreeMap<Long, SummableMap<String, Integer>> gpCollection, Long calcdate) {
            List<String> lastyear = customSubMap(gpCollection, new DateTime(i).minusMonths(12).getMillis(), calcdate);
            if (lastyear.size() == 1) [
                return lastyear.get(0);
            }
            return null;
        }
    },

    // 6-month rule
    new GPSelector() {
        public String selectGP(TreeMap<Long, SummableMap<String, Integer>> gpCollection, Long calcdate) {
            // return not-null String if there is a successful 6-month candidate....
        }
    },

    .......

};

OK, so now you have an ordered array of rules that progressively select the right UsualGP candidate.

I your code, you can now simply have the following:

public String selectUsualGP(TreeMap<Long, SummableMap<String, Integer>> gpCollection, Long calcdate) {
    for (GPSelector rule : GPRULES) {
        String gp = rule.selectGP(gpCollection, calcdate);
        if (gp != null) {
            return gp;
        }
    }
    return null; // nothing matched.... :(
}

Adding a new rule is easy, just insert it in to the chain at the appropriate place....

I hope the above is enough to show how the chain-of-responsibility pattern can be applied.


Generation of Groovy source from parent Java class I use Groovy to write business rules into big, long term Java software. Use Groovy in object way, that is not as groovy Script, but every elementary groovy source overrides some Java API class.

i.e. AtLoginScript, PreparePaport etc.

I had an idea to clone in very little scale IDE functionality: generate class from, or Override method.

My idea is much simpler: do automatically, only first-shot, write method stub where this must be done (abstract method), maybe in comment where may be done (non-abstract).

My mini project has first effects, in this sample GroovyGenerator create his-own 'child class'.

My target for next days is to:

implement negative conditions for final or private method,
some decisions with more deep parent class (now only one-level)
types when applicable
maybe implement groovy property from getter/setter pair?
constructors
extend from 0 or 1 class and implement zero to \$n\$ interfaces (What to do with override level?)

I haven't big trouble with coding (one small problem: reflection usually doesn't give parameters name), but with idea.

This tool is made for in-place IT-man, who generate business rules in, for example, Notepad or simple editor non-capable in generation.

What can be done better in this project?

package pl.cogitat.scripting;

import java.lang.reflect.Method;
import java.lang.reflect.Parameter;

public class GroovyGenerator {
    Class analyzedClass;
    private String packg;
    private String name;

    public GroovyGenerator(String pckg, String name, Class analyzedClass) {
        super();
        this.analyzedClass = analyzedClass;
        this.packg = pckg;
        this.name = name;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public String getPackg() {
        return packg;
    }

    public String performGeneration()
    {
        StringBuilder sb = new StringBuilder();
        sb.append("package ").append(packg).append("\n\n /* Code auto generated\n * do edit what You want \n */\n\n");

        String parentName = analyzedClass.getSimpleName();
        sb.append("class ").append(name).append(" extends ").append(parentName).append(" {\n");

        for(Method m : analyzedClass.getDeclaredMethods())
        {
            sb.append("def ").append(m.getName()).append("(");
            int pcnt = 0;
            for(Parameter p:m.getParameters())
            {
                if(pcnt>0)
                    sb.append(", ");
                sb.append(p.getType().getSimpleName()).append(" ").append(p.getName());
                pcnt++;
            }
            sb.append(")\n{\n}\n");
        }

        sb.append("\n}\n");
        return sb.toString();

    }
}

and untouched generated Groovy:

package my.script

 /* Code auto generated
 * do edit what You want
 */

class SelfGenerator extends GroovyGenerator {
def getName()
{
}
def setName(String arg0)
{
}
def performGeneration()
{
}
def getPackg()
{
}

}

You can use Groovy to generate the Groovy source code.

The source code generator

Here's an example source code generator.

import groovy.text.SimpleTemplateEngine

def generate(Class klass, String packageName, String name, String template) {
    def engine = new SimpleTemplateEngine()
    def binding = [
        packageName: packageName,
        name: name,
        superClass: klass.name,
        methods: klass.metaClass.methods
            .findAll { !(it.toString().contains(' final ')) }
            .collect {
                def args = []
                def m = it.toString() =~ /(.*) (.*\.)(.*)(\((.*)\).*)/

                m.matches()
                if(m.group(5).size() > 0) {
                    m.group(5).split(',').eachWithIndex {arg, index ->
                        args << "$arg p$index"
                    }
                }

                return "${m.group(1).replace('native', '')} ${m.group(3)}(${args.join(', ')})"
            }
    ]

    engine.createTemplate(template).make(binding).toString()
}

How it works

It's actually quite simple. The method signatures are gathered from the class's MetaClass. Then some hefty formatting is applied to write the method signatures correctly. Also, final methods are excluded.

Example

def template = '''
package $packageName

/* Code auto generated
 * do edit what You want
 */

class $name extends $superClass {
<%
methods.each {
    println ''
    println "\t$it {"
    println ''
    println "\t}"
} %>
}
'''

generate(Object, 'my.script', 'SelfGenerator', template)

Generated source code

package my.script

/* Code auto generated
 * do edit what You want
 */

class SelfGenerator extends java.lang.Object {

    public boolean equals(java.lang.Object p0) {

    }

    public  int hashCode() {

    }

    public java.lang.String toString() {

    }

}

Templates

Ideally, you'd have two templates: one for the class and one for the methods. That would avoid the clunky JSP-style programming. You can read more about Groovy's template engines here.


Operations on strings in Java I have a program that makes operations on strings and this is the principal function of one of those operations.  This works perfectly, but it is not efficient:

private void printString(ArrayList<String> operations, ArrayList<String> set) {
    int numerOfStrings = 0;
    int numberOfletters = 0;
    String toPrint = operations.get(1);
    outOfLoop: for (int i = 0; i < set.size(); i++) {
        String[] toFind = set.get(i).split(" ");
        for (int k = 0; k < toFind.length; k++) {
        if (toPrint.equals(toFind[k])) {
            String[] splited = set.get(i).split(" ");
            for (int j = 0; j < splited.length; j++) {
            numberOfletters += splited[j].length();
            }
            numerOfStrings = splited.length;
            break outOfLoop;
        }
      }
    }
    System.out.println(numerOfStrings + " " + numberOfletters);
}

Explanation:

This function takes as parameter an arrayList of operations, and an arrayList of set:

For the arrayList of operations, I get always a specific position, so I don't iterate.  It is always \$O(1)\$.
For the arrayList of set, I have to iterate, or rather, as I think of that to proceed:

For example, if have as operation print foo, I have to do these steps:

First of all, I have to find where foo is:

Inside set, I can have this situation:

position 1 : {car tree hotel}
...
position n : {foo lemon coffee}

When I find the string foo, I have to print the number of strings inside that position and the number of letters of each string, so in this case, I will print:

3(number of strings) 14(sum of number of letters)

My program works as well as this function, but it is a nasty and inefficient solution.  How can I improve the efficiency of my program?

Let me first do a review without getting into the logic of the method.
Before I start making changes I've commented some smelly places in your original code:

//Try to use the most generic type in your arguments. You don't really need an ArrayList<>
//a List<> will suffice.
private void printString(ArrayList<String> operations, ArrayList<String> set) {
    int numerOfStrings = 0;
    int numberOfletters = 0;
    //why take a list parameter when you only use one value (the second element?!)
    String toPrint = operations.get(1);
    outOfLoop: for (int i = 0; i < set.size(); i++) {
        String[] toFind = set.get(i).split(" ");
        for (int k = 0; k < toFind.length; k++) {
            if (toPrint.equals(toFind[k])) {
                // duplicated work: toFind == splitted
                String[] splited = set.get(i).split(" ");
                for (int j = 0; j < splited.length; j++) {
                    numberOfletters += splited[j].length();
                }
                numerOfStrings = splited.length;
                //goto is considered evil
                //Besides, goto to label is not neccessary here.
                //A simple `break` would suffice
                break outOfLoop;
            }
        }
    }
    //you should return the results and let the caller of the method
    //decide what to do with them
    System.out.println(numerOfStrings + " " + numberOfletters);
}

So, in the first refactoring iteration I've ended up with:

public static class Result {
    public final int numerOfStrings;
    public final int numberOfletters;

    public Result(int numerOfStrings, int numberOfletters) {
        this.numerOfStrings = numerOfStrings;
        this.numberOfletters = numberOfletters;
    }

    @Override
    public String toString() {
        return numerOfStrings + " " + numberOfletters;
    }
}

private Result process(String word, List<String> set) {
    int numerOfStrings = 0;
    int numberOfletters = 0;

    for (int i = 0; i < set.size(); i++) {
        String[] toFind = set.get(i).split(" ");
        for (int k = 0; k < toFind.length; k++) {
            if (word.equals(toFind[k])) {
                String[] splited = set.get(i).split(" ");
                for (int j = 0; j < splited.length; j++) {
                    numberOfletters += splited[j].length();
                }
                numerOfStrings = splited.length;
                break;
            }
        }
    }
    return new Result(numerOfStrings, numberOfletters);
}

@Test
public void run() throws Exception {
    Result result = process("foo", Arrays.asList("car tree hotel","foo lemon coffe"));
    System.out.println(result);
}

This keeps the method logic intact, but fixes all the issues I mentioned in the comments.

But actually I think the method could be a lot simpler:

private Result process(String word, Iterable<String> set) {
    for (String string : set) {
        if(string.contains(word)) {
            //" +" is a regex patten for "one or more spaces"
            int numberOfWords = string.trim().split(" +").length;
            int numberOfLetters = string.replace(" ", "").length();
            return new Result(numberOfWords,  numberOfLetters );
        }
    }
    return new Result(0, 0);
}

This version is more readable, but could be optimized for performance. As always with performance you should first check if optimizing here would make any impact on the application as a whole. Sacrificing readability might not be worth it.

Optimizations might include:

avoid new string creation using trim()
avoid new string array creation using split()
avoid new string creation using replace()
avoid iterating over string multiple times - count letters and words while looking for word (instead of contains)


Array flatten with recursion I'd been using this form for a while now with the functional methods map, filter, reduce:

// For example...
$result = array_reduce(array_map(function($value) {
    return explode('-', $value);
}, $input), function($carry, $item) {
    return array_merge($carry, $item);
}, array());

The array_reduce() part, where I need to collapse depth in the result. So I was thinking about this, and this is what I came up with. It actually works as a callback to array_reduce() too:

/**
 * Flatten a multi-dimensional array to a single-level array,
 * sorting the entries in order to depth first.
 *
 * @param array $carry Array to initialize, will be the subject if the only argument passed.
 * @param mixed $subject Optional Array or value that has data in potential multi-dimensions.
 *
 * @return array
 */
function array_flatten(?array $carry = [], $subject = null): array
{
    return array_reduce((array) ($subject ?? $carry), function(array $carry, $item) {
        return is_array($item)
            ? array_flatten($carry, $item)
            : array_merge($carry, (array) $item);
    }, $subject !== null ? (array) $carry : []); // Don't merge onto the $subject...
}

https://3v4l.org/J1NeJ

Thing is, the way it works, I think it literally goes one level further than there are levels to traverse. The reason I think this is because the array_merge() is getting passed a "boxed" element. So the way I'm adding to the array is to box/unbox an array wrapper so it can be merged.

That seems like the most amount of extra effort for one little thing; it happens to every non-array it comes across. Questioning whether I should use it.

It can be called like array_reduce():

array_reduce([1,2,[3,[4]],5], 'array_flatten');

Or more likely:

array_flatten([1,2,[3,[4]],5]);

Is this something I should avoid?

PHP already has a native function for this task.

array_walk_recursive() traverses your "leaf nodes" for you in one step:

Code: (Demo)

//$multi=[1,2,[3,[4]],5];
$multi=[array('A',
        array('B',
            array('CC','CCCC','CCCCCC'),
            array('DDD','EFG','HIJ','LMNOP')
        )
    ),
    array('2'),
    'b',
    array(array(),array(),array())];
array_walk_recursive($multi,function($v)use(&$flat){$flat[]=$v;});
var_export($flat);

Output:

array (
  0 => 'A',
  1 => 'B',
  2 => 'CC',
  3 => 'CCCC',
  4 => 'CCCCCC',
  5 => 'DDD',
  6 => 'EFG',
  7 => 'HIJ',
  8 => 'LMNOP',
  9 => '2',
  10 => 'b',
)


Replacing part of a string with filler characters I came up with this function rangedReplace as an exercice to use recursion and try different ways to make it work (I made at least 3 completely different versions). I think this version is the best but I have a feeling I can improve the way the functions/arguments are handled especially in the censor and in the main functions. How can I get these functions to be more idiomatic Haskell-way of doing, ?

-- Replace the letters in the range [from:to[ ("to" not included) with chr
rangedReplace :: Int -> Int -> Char -> String -> String
rangedReplace from to chr str
    | from == to = str
    | otherwise  = (take from' str) ++ rangedReplace' chr n (drop from' str)
    where from' = min from to
          to'   = max from to
          n     = to' - from'

-- Helper function
rangedReplace' :: Char -> Int -> String -> String
rangedReplace' chr n str
    | n == 0    = str
    | str == "" = ""
    | otherwise = chr : (rangedReplace' chr (n-1) (tail str))

Usage example:

censor (from,to) = rangedReplace from to '*'

main = do
    putStrLn $ foldr id "Fork bombs" (map censor [(1,3),(6,8)])

Parameter design

To facilitate currying, you should arrange a function's parameters starting with the one that is the least likely to vary.  In this case, I would consider the fill character the parameter that is most likely to be fixed.

The next parameter, I think, should be the range, and it should be specified as a pair rather than as two separate parameters.  That would make rangedReplace fill (from, to) a filter that takes a string and produces a string.

Using inclusive bounds for from and exclusive bounds for to is the right way to go.

I don't think it's beneficial to automatically swap from and to.  That just encourages your caller to be sloppy.  Rather, I would expect a rangedReplace from 3 to 1 to act as a no-op.

With rangedReplace fill (from, to) string, your main function no longer needs id, map, and the censor helper function.

main = do
  putStrLn $ foldr (rangedReplace '*') "Fork bombs" [(1, 3), (6, 8)]

Implementation

The use of ++ should be considered a yellow flag, as it indicates a full list traversal.  You'll be doing three O(from') operations: take, drop, and ++.

I'd rather go with a recursive solution that does everything in one pass, and is fully lazy.  The implementation is shorter, too.

rangedReplace :: Char -> (Int, Int) -> String -> String
rangedReplace _ _ [] = []
rangedReplace fill (from, to) s@(c:cs)
  | to <= 0   = s
  | from <= 0 = fill:cs'
  | otherwise = c:cs'
  where cs' = rangedReplace fill ((from - 1), (to - 1)) cs


JS cross-fade transition As the question says....Can this be done any better? I could introduce more variables to cut down on function number, but does it make sense? So this is a simple cross-fade between two images :

var slide_number = 1;
var slide_number2 = 2;
var slide1 = new Image()
slide1.src = "img/top_slideshow/slideshow-1.jpg"
var slide2 = new Image()
slide2.src = "img/top_slideshow/slideshow-2.jpg"
var slide3 = new Image()
slide3.src = "img/top_slideshow/slideshow-3.jpg"
var slide4 = new Image()
slide4.src = "img/top_slideshow/slideshow-4.jpg"
var slide5 = new Image()
slide5.src = "img/top_slideshow/slideshow-5.jpg"
var slide6 = new Image()
slide6.src = "img/top_slideshow/slideshow-6.jpg"
var slide7 = new Image()
slide7.src = "img/top_slideshow/slideshow-7.jpg"
var slide8 = new Image()
slide8.src = "img/top_slideshow/slideshow-8.jpg"

function set_first_image() {
    document.images.slide.src = eval("slide"+slide_number+".src");
}
function set_second_image() {
    document.images.slide2.src = eval("slide"+slide_number2+".src");
}
function first_image_fade_in(){
    document.getElementsByClassName('slideshow')[0].setAttribute('style', 'opacity: 0;');
    slide_number += 2;
    if (slide_number >= 9){
        slide_number = 1;
    }
    setTimeout(function() {set_first_image()}, 2000);
}
function first_image_fade_out(){
    document.getElementsByClassName('slideshow')[0].setAttribute('style', 'opacity: 1;');
}
function second_image_fade_in(){
    document.getElementsByClassName('slideshow2')[0].setAttribute('style', 'opacity: 0;');
    slide_number2 += 2;
    if (slide_number2 >= 9){
        slide_number2 = 2;
    }
    setTimeout(function() {set_second_image()}, 2000);
}
function second_image_fade_out(){
    document.getElementsByClassName('slideshow2')[0].setAttribute('style', 'opacity: 1;');
}

function slideshow(){
    setTimeout(function() {first_image_fade_in()}, 20000);
    setTimeout(function() {second_image_fade_out()}, 20000);
    setTimeout(function() {first_image_fade_out()}, 40000);
    setTimeout(function() {second_image_fade_in()}, 40000);
    setTimeout(function() {slideshow()}, 40000);
}

I must say it was interesting running your code with some sample images-

You have so many variables declared especially the slides - You should try using a Create Factory method for your Images. Example from Creating an array of Image Objects

// Image factory
var createImage = function(src, title) {
  var img   = new Image();
  img.src   = src;
  img.alt   = title;
  img.title = title;
  return img;
};

// array of images
var images = [];

// push two images to the array
images.push(createImage("http://www.lostarkgames.com/images/pokemonlogo.jpg", "foo title"));
images.push(createImage("http://4.bp.blogspot.com/-Jv6VFqBbTIM/VNUbs8ZehLI/AAAAAAAATD8/Dwipe_uNxTQ/s1600/Pok%C3%A9mon_Gotta_Catch_'Em_All_1.png", "bar title"));

// output
console.log(images);

When you use fade out, it's best to fade out slowly before making the image transparent e.g 1-> 0.2->0 . It's just for the user experience. the animation lacks consistency based on the timing, some images stayed longer than 20 seconds


Counter object in Python with public and private methods I am trying to implement an open source desktop application with python. I want to provide both a gui and an API. I know there is no private methods in python but i am trying to find the most elegant way to keep some methods private. I do not want my users to see all list of methods while using API. After some research, I come up with the implementation below and wondering if there is any problem with my implementation of this concept and if there is any room for improvement. Please provide me a review and do not hesitate to ask any questions you have. Here is my try:

from pprint import pprint

# CamelCase because it "acts" like a class
def CounterController():
    class CounterControllerPrivate(object):
        def __init__(self):
            self.counter = 0

        def add_one(self):
            self.counter += 1

        def reset(self):
            self.counter = 0

        def get_counter(self):
            return self.counter

    counter_controller = CounterControllerPrivate()

    class CounterControllerPublic(object):
        def add_one_endpoint(self):
            counter_controller.add_one()
            return counter_controller.get_counter()

        def reset_endpoint(self):
            counter_controller.reset()
            return counter_controller.get_counter()

    return CounterControllerPublic()

# counter attribute is not accessible from out here
controller = CounterController()

print(controller.add_one_endpoint())
print(controller.add_one_endpoint())
print(controller.reset_endpoint())

pprint(dir(controller))

The private objects and attributes are easily accessed using inspect.getclosurevars:

>>> c = CounterController()
>>> import inspect
>>> inspect.getclosurevars(c.add_one_endpoint)
ClosureVars(nonlocals={'counter_controller': <__main__.CounterController.<locals>.CounterControllerPrivate object at 0x10a047cf8>}, globals={}, builtins={}, unbound={'add_one'})
>>> _.nonlocals['counter_controller']
<__main__.CounterController.<locals>.CounterControllerPrivate object at 0x10a047cf8>
>>> _.counter
0

So you might consider whether it's worth spending a lot of effort on this approach. Personally I would spend the effort on API documentation instead.


Efficiently finding approximate fraction, with tolerance for fp rounding I needed to convert floating point numbers to int/int fractions. Python technically has this functionality, but has issues with rounding errors:

>>> (0.1+0.2).as_integer_ratio()
(1351079888211149, 4503599627370496)

This is what I wrote and I was wondering whether it could be further improved for performance?

def as_fraction(number: float, accuracy: float) -> (int, int):
    whole, x = divmod(number, 1)
    if not x:
        return int(whole), 1
    n = 1
    while True:
        d = int(n/x)
        if n/d-x < accuracy:
            return int(whole)*d+n, d
        d += 1
        if x-n/d < accuracy:
            return int(whole)*d+n, d
        n += 1

Convert the float to a Fraction:

>>> from fractions import Fraction
>>> Fraction(0.1 + 0.2)
Fraction(1351079888211149, 4503599627370496)

(this gives you the same ratio as float.as_integer_ratio), and then use the limit_denominator method to return the closest fraction with a denominator of limited size:

>>> Fraction(0.1 + 0.2).limit_denominator(1000000)
Fraction(3, 10)

If you're interested in the maths behind the limit_denominator method, see this answer on Stack Overflow.


Swapping of element with its neighboring element (Python) Does this code seems good or it can be improve (without any library)?
Elements swapping of a list with its neighboring element
alist = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13]

lenofalist = len(alist)

index = 0
temp = 0
last_element = 0
if lenofalist % 2 != 0:

    last_element = alist.pop()
    lenofalist -= 1

    while index < lenofalist:
        temp = alist[index+1]
        alist[index+1] = alist[index]
        alist[index] = temp
        index += 2

    alist.append(last_element)

else:
    while index < lenofalist:
        temp = alist[index+1]
        alist[index+1] = alist[index]
        alist[index] = temp
        index += 2

Your code, while it works, has a lot of room for improvement.

Naming conventions

Python has a best practice guide for style and naming conventions called PEP 8. Your variable names generally follow the lines of this guide, but lenofalist should be called len_of_alist.

Duplicate code

Your code has a lot of duplicate code. 5 lines out of 19 are copied and pasted, that's over 25%. It lowers readability and makes any change to your code harder, as it may have to be changed twice.

In your case, the if ... else construct can be avoided altogether by exiting the loop before the last element is reach if the length of the list is odd.

Use for loops

You use while loops. It works, but is not the best fit for your case. You have to initialize an index and increment it manually, which makes the code longer and more complicated than it needs to be. Also, using i instead of index for a loop counter is generally accepted, as it is very common.

for i in range(0, len_of_alist, 2):
    # your code here

is functionally the same as your while loops, while being more concise and readable.

Swapping elements

In Python, you can swap the values of two variables using the following construct:

a, b = b, a

Your way of using a temporary variable is more language agnostic, but the Python way is much cleaner.

Make your code a function

Let's imagine you want to swap neighboring elements of a list as part of a larger project. It would be far more convenient and clear having this code in a function and calling it where you need it instead of inserting your code snippet where needed.

If you are using Python 3.5 or higher, you can use a type hint to make sure your function is indeed working with a list.

Putting it all together

Here is my take on improving your code:

def swap_neighbors(alist: list):
    for i in range(0, len(alist)-1, 2):
        alist[i], alist[i + 1] = alist[i + 1], alist[i]

my_list = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13]
swap_neighbors(my_list)

As a side effect, there are no variables that need to be declared ahead, cleaning up the code even further.


Remove all characters except My code takes a string and replaces all characters which are not:

English letters
Numbers
, / -

I have tested it and it seems to generally work well enough. But it may have some catastrophic bug in it and/or can be simplified.

x <- "dog/John is a cutting-edge pilot^¢„þ"
gsub("[^a-zA-Z0-9,-:space:]+", " ", x, perl = TRUE)
"dog/John is a cutting-edge pilot "

The :space: portion of the regex makes no sense, and probably does not do what you intend.

> x <- "abc:def."
> gsub("[^a-zA-Z0-9,-:space:]", " ", x, perl = TRUE)
[1] "abc:def."

Notice that the colon and period are still present after the substitution.

In fact, inside the character class, ,-: means "all characters with ASCII codes from 44 (the comma) up to 58 (the colon)".

A literal hyphen must be the first or the last character in a character class; otherwise, it is treated as a range (like A-Z).

If you want a character class for whitespace, use "\\s" or [:space:].

So, if you wanted to convert all consecutive strings of junk to a single space, preserving only letters, digits, commas, slashes, hyphens, and whitespace, you could write:

gsub("[^-,/a-zA-Z0-9[:space:]]+", " ", x, perl = TRUE)

or

gsub("[^-,/a-zA-Z0-9\\s]+", " ", x, perl = TRUE)


Checking for memory leaks in data structures I'm looking for a code review for my JavaScript code that checks if there is any memory leaks in data structures. Please include constructive criticism, as well as details on what I can do to make this code more efficient.

// memoryleak.js
// (C) 2014 Kyle Maune
// checks for memory leaks in data structs

MemoryLeak = {

  uniq_id: (new Date()).getTime(),
  checked: 1,
  is_seen: [],

  checkLeaks: function(obj) {
    var self = MemoryLeak

    if(!obj || (typeof obj == 'function') || self.checked > 20000)
      return;

    if ((self._isArray(obj) || self._isObject(obj))) {
      if (self._isArray(obj)) {
        self._logTooBig(obj, obj.length)
        for (var i = 0; i < obj.length; i++) {
          self._checkIfNeeded(obj[i])
        }
      }
      else if (self._isObject(obj)) {
        self._logTooBig(obj, self._keys(obj).length)

        for (var key in obj) {
          self._checkIfNeeded(obj[key])
        }
      }
    }
  };

  _checkIfNeeded: function(obj) {
    if (!obj)
      return;

    var self = MemoryLeak;
    self.checked++

    if ((self._isArray(obj) || self._isObject(obj))) {
      if(obj.__leaks_checked == self.uniq_id)
        return;
      obj.__leaks_checked = self.uniq_id

      setTimeout(self._partial(self.checkLeaks, obj), 5);
    }
  };

  _logTooBig: function(obj, limit) {
    if (limit > 200) {
      console.log('Object too big, memory leak? [size: ' + limit + ']')
      console.log(obj)
      console.log('-------')
    }
  };

  _keys: function(obj) {
    var rval = [], prop
    for (prop in obj)
      rval.push(prop)
    return rval;
  };

  _isArray: function(obj) {
    try {
      return obj instanceof Array
    }
    catch(e) {
      return false;
    }
  };

  _isObject: function(obj) {
    return (typeof obj == 'object')
  };

  _partial: function(fn) {
    var args = Array.prototype.slice.call(arguments)
    args.shift()
    return function() {
      var new_args = Array.prototype.slice.call(arguments)
      args = args.concat(new_args)
      return fn.apply(window, args)
    }
  }
}

This code

if ((self._isArray(obj) || self._isObject(obj))) {
  if (self._isArray(obj)) {
    self._logTooBig(obj, obj.length)
    for (var i = 0; i < obj.length; i++) {
      self._checkIfNeeded(obj[i])
    }
  }
  else if (self._isObject(obj)) {
    self._logTooBig(obj, self._keys(obj).length)

    for (var key in obj) {
      self._checkIfNeeded(obj[key])
    }
  }
}

should be reduced, remove the outside if statement

  if (self._isArray(obj)) {
    self._logTooBig(obj, obj.length);
    for (var i = 0; i < obj.length; i++) {
      self._checkIfNeeded(obj[i]);
    }
  }
  else if (self._isObject(obj)) {
    self._logTooBig(obj, self._keys(obj).length);

    for (var key in obj) {
      self._checkIfNeeded(obj[key]);
    }
  }

There is no need for it.

Also some of your one liner blocks make use of Curly Brackets and some do not,  this is confusing when put together with your 2-space indenting instead of 4-space indenting.  don't worry about space when writing your JavaScript, use all the characters that you need to be able to maintain the code properly, don't Minify until you are completely finished with the code.

Especially when nesting if statements

if ((self._isArray(obj) || self._isObject(obj))) {
  if(obj.__leaks_checked == self.uniq_id)
    return;
  obj.__leaks_checked = self.uniq_id

  setTimeout(self._partial(self.checkLeaks, obj), 5);
}

you should use brackets and proper indenting to make it clear what is going on

if ((self._isArray(obj) || self._isObject(obj))) {
    if(obj.__leaks_checked == self.uniq_id){
        return;
    }
    obj.__leaks_checked = self.uniq_id;

    setTimeout(self._partial(self.checkLeaks, obj), 5);
}

This is also one that almost confused me as well

_keys: function(obj) {
  var rval = [], prop
  for (prop in obj)
    rval.push(prop)
  return rval;
};

I would write it like this (and don't forget the ;)

_keys: function(obj) {
    var rval = [], prop;
    for (prop in obj){
        rval.push(prop);
    }
    return rval;
};

I just realized how many times you left out the semi-colons after a line of code,  this is a very bad habit to fall into.

Always remember your semi-colons!

There are places where I don't think the code will run correctly because you don't have semi-colons to tell the browser that you have ended the command.


Tokenizing string using strtok In this assignment, I'm supposed to split a string using strtok. (The assignment consists of the comment and the function definition; I fill in the function body.)

It works as far as I've tested. But it seems a little ugly to me. Perhaps it's my Java background, but somehow this just doesn't feel right. Specific concerns:

Is this easily readable to a more experienced C programmer?
Can this be made more idiomatic somewhere?
Is it feasible to split this into several functions? Where would you split it?
Any corner cases where it might break? Does it leak memory?

/* Parses a string into tokens (command line parameters) separated by space.
 * Builds a dynamically allocated array of strings. Pointer to the array is
 * stored in variable pointed by argv.
 *
 * Parameters:
 * argv: pointer to the variable that will store the string array
 * input: the string to be parsed (the original string can be modified, if needed)
 *
 * Returns:
 * number of space-separated tokens in the string */
int parse_cmdline(char ***argv, char *input)
{
    char *token;
    int len, num;

    len = 4;
    *argv = malloc(len * sizeof(char *));
    if (*argv == NULL) {
        return 0;
    }
    for (num = 0;; num++, input = NULL) {
        token = strtok(input, " ");
        if (token == NULL) {
            break;
        }
        if (num >= len) {
            char **temp;

            len *= 2;
            temp = realloc(*argv, len * sizeof(char *));
            if (temp == NULL) {
                for (int i = 0; i < num; i++) {
                    free((*argv)[i]);
                }
                free(*argv);
                *argv = NULL;
                return 0;
            }
            *argv = temp;
        }
        (*argv)[num] = strdup(token);
        if ((*argv)[num] == NULL) {
            for (int i = 0; i < num; i++) {
                free((*argv)[i]);
            }
            free(*argv);
            *argv = NULL;
            return 0;
        }
    }
    return num;
}

The key point in tokenizing strings is that

  The number of resulting token would not be known in prior

One good approach is the one you have followed,

Allocate some memory
Use it
Allocate more if needed

I want to answer some of the questions that you have asked.

  Is it feasible to split this into several functions? Where would you
  split it?

As you see, there is the repetition of error handling (freeing up the argv array). Well this can be separated into another function.

  Is this easily readable to a more experienced C programmer?

Not sure of the level of expertise you mean, but I can read it and understand without any problems. I am a 2 year experienced C Professional.

  Any corner cases where it might break? Does it leak memory?

There is no obvious memory leak I could see in the program. But you can use tools like valgrind to detect such leaks.

I am just posting few code, that I would use, if I am given the same program.

int free_argv_q(struct queue *q)
{
    int i;
    int q_len = q_length(q);

    for (i = 0; i < q_len; i++) {
        char *arg = dequeue(q);
        free(arg);
    }

    return 0;
}

int parse_cmdline(char ***argv, char *input)
{
    int i;
    int argc = 0;

    char *arg = NULL;
    char *token = NULL;

    struct queue q;

    token = strtok(input, " ");

    do {
        /* strdup may fail due to low memory */
        arg = strdup(token);

        if (arg == NULL) {
            free_argv_q(&q);
            return 0;
        }

        /* add to queue */
        enqueue(&q, arg);
        token = strtok(NULL, " ");
    } while (token);

    /* length of the queue is the number of splits */
    argc = q_length(&q);

    *argv = malloc(argc * sizeof(char *));

    if (*argv == NULL) {
        free_argv_q(&q);
        return 0;
    }

    /* now we have all memory allocated */
    for (i = 0; i < argc; i++)
        argv[i] = dequeue(&q);

    return argc;
}

The explanation is as follows.

I use a queue to split the tokens and store them. Well this would grow dynamically.
After parsing whole input, the argv array is allocated
argc is the length of the queue
Memory allocation failures are handled though
The cleanup function is separated

Note: The implementation of queue is skipped for simplicity. Hope this program is self explanatory.