Parsing XML file containing some events I'm a beginner and have written my first Python module that parses a XML file containing some events and prints them out to stdout as JSON.  I would appreciate some feedback about design, code layout, best practices, what can be optimized and so on.

from __future__ import with_statement

import json
import xml.etree.ElementTree as etree

def _sanitize_string(string):
    """docstring for _clean_string"""
    string = string.replace('\n', '').split()
    return ' '.join(string)

class Event(object):
    """docstring for Event"""
    def __init__(self, date, event, organizer, time, venue):
        self.date = date
        self.event = event
        self.organizer = organizer
        self.time = time
        self.venue = venue

    def provide_data(self):
        """docstring for provide_data"""
        data = {}
        data['date'] = self.date
        data['event'] = self.event
        data['organizer'] = self.organizer
        data['time'] = self.time
        data['venue'] = self.venue
        return data

class EventCollection(list):
    """docstring for EventCollection"""
    def __init__(self):
        super(EventCollection, self).__init__([])

    def as_json(self):
        """docstring for as_json"""
        print json.dumps([event.provide_data() for event in self],
                         sort_keys=True, indent=2)

def read_file(event_collection, filename='events.xml'):
    """docstring for read_file"""
    try:
        with open(filename) as f:
            tree = etree.parse(f)
        for row in tree.findall('.//row'):
            # event = [_sanitize_string(node.text) for node in row]
            # if event[0] and event[0][0].isdigit():
            #     print event
            tmpl = [_sanitize_string(node.text) for node in row]
            if tmpl[0] and tmpl[0][0].isdigit():
                event = Event(tmpl.pop(0), tmpl.pop(0), tmpl.pop(0),
                              tmpl.pop(0), tmpl.pop(0))
            event_collection.append(event)
    except IOError as ioerr:
        print "File Error: %s" % str(ioerr)

def main():
    """docstring for main"""
    event_collection = EventCollection()

    read_file(event_collection)

    event_collection.as_json()
    # event_collection.as_csv()

if __name__ == '__main__':
    main()

from __future__ import with_statement

import json
import xml.etree.ElementTree as etree

def _sanitize_string(string):
    """docstring for _clean_string"""

Docstrings should contain a description of the function, not some old name for the function.

    string = string.replace('\n', '').split()

Do you really want "green\ngrass' to come through as 'greengrass'?

   return ' '.join(string)

class Event(object):
    """docstring for Event"""
    def __init__(self, date, event, organizer, time, venue):
        self.date = date
        self.event = event
        self.organizer = organizer
        self.time = time
        self.venue = venue

    def provide_data(self):
        """docstring for provide_data"""
        data = {}
        data['date'] = self.date
        data['event'] = self.event
        data['organizer'] = self.organizer
        data['time'] = self.time
        data['venue'] = self.venue
        return data

The class as it stands doesn't really do anything. If this is all its ever going to do, just use a dictionary instead.

class EventCollection(list):
    """docstring for EventCollection"""

    def __init__(self):
        super(EventCollection, self).__init__([])

Passing the [] is wasteful, because the empty argument constructor will have the same effect without building a temporary list. There is no need to override the constructor here. The default behavior will build the list just fine.

    def as_json(self):
        """docstring for as_json"""
        print json.dumps([event.provide_data() for event in self],
                         sort_keys=True, indent=2)

This would really be better as a seperate function rather then an entire class.  Its misplaced in a list class because its not really dealing with the list per se.

def read_file(event_collection, filename='events.xml'):
    """docstring for read_file"""
    try:
        with open(filename) as f:
            tree = etree.parse(f)
        for row in tree.findall('.//row'):
            # event = [_sanitize_string(node.text) for node in row]
            # if event[0] and event[0][0].isdigit():
            #     print event

Don't keep dead code in comments, delete it.

            tmpl = [_sanitize_string(node.text) for node in row]

I can't guess what tmpl is supposed to mean. This being xml, does these nodes have tags that you should be checking for correctness?

            if tmpl[0] and tmpl[0][0].isdigit():

I have no idea what this test is doing. It should probably be commented to explain the signficance of that first character being a digit.

                event = Event(tmpl.pop(0), tmpl.pop(0), tmpl.pop(0),
                              tmpl.pop(0), tmpl.pop(0))

Use:
                   date, event, time, organizer, venue = tmpl
                   event = Event(date, event, time, organizer, venue)

It makes is easier to see what is going on

            event_collection.append(event)
    except IOError as ioerr:
        print "File Error: %s" % str(ioerr)

If you catch an error, simply printing it out is insufficient. You should really either recover or abort. You could call sys.exit() here to shutdown the program. I'd probably rethrow the exception and catch it in my main() function. If the script is supposed to be quick and dirty, just don't catch the exception and let the default handling show the error when it dies.

def main():
    """docstring for main"""
    event_collection = EventCollection()

    read_file(event_collection)

Avoid functions which modify arguments. Use a return value. Have read_file create and return event_collection.

    event_collection.as_json()
    # event_collection.as_csv()

if __name__ == '__main__':
    main()

** EDIT **

Ways of doing exception handling:

Don't. Just take your try and except out. If something goes wrong, your program will die with a stack trace. Not very professional, but in some cases that's just fine
Use sys.exit()

try:
     do_stuff()
except IOError:
     print "Went wrong"
     sys.exit() # program will exit here
Use your own exception class (my preffered method)

Code:

class UserError(Exception):
     """
     This error is thrown when it the user's fault
     Or at least the user should see the error
     """

# later
try:
     do_stuff()
except IOError as error:
     raise UserError(error)

# later
def main():
    try:
         do_program()
    except UserError as error:
         print error

Its a bit bulky for a simple script like this, but for bigger programs I think its the best solution.

Two other issues:

You should really print >> sys.stderr, "your error message" so that your errors go to standard error and not standard out
You should also end by calling sys.exit(some non-zero number) when an error happens to indicate failure.


Advent of Code 2018 Day 4 - Pythonic Sleepy Guards Introduction

I'm using Advent of Code 2018 to learn Python better, interested in the new type support for Python 3.7, I decided to go with that.

Here is my solution to Advent of Code Day 4, both part 1 and 2. I'm returning the answers for both part 1 and 2 as a tuple.

Problem Description

The problem essentially boils down to: Given a timestamped unsorted list of events of guards beginning their shift, waking up and falling asleep, determine the following:

Part 1: Which guard is asleep the most and on which minute is that guard mostly asleep? Return guard id multiplied by the minute number.

Part 2: Which guard is most frequently asleep on the same minute? Again, return guard id multiplied by the minute number.

For full problem description, please see Advent of Code Day 4

Concerns

I'm a big fan of Java 8 Stream API and C# Linq, I kind of expected Python to be more like that. I'm not sure if the nested function calls like sorted(list(...)) or len(list(...)) are "Pythonic". Likewise, it feels like I should be able to use some reducer-like function calls instead of imperatively looping through stuff to find the most common sleeper of some kind. Or is the way I have written this code the Python way to do it?

Code

from dataclasses import dataclass
from datetime import datetime
from days import read_file
from enum import Enum
from collections import defaultdict, namedtuple
from statistics import mode
import statistics
import operator
import re

class EventType(Enum):
    STARTS_SHIFT = 1
    FALLS_ASLEEP = 2
    WAKES_UP = 3

@dataclass
class Event:
    time: datetime
    guard: int
    event: EventType

@dataclass
class GuardSleep:
    sleep_total: int
    last_sleep: int
    sleeps: list

    def add_sleeps(self, minute):
        for i in range(self.last_sleep, minute):
            self.sleeps.append(i)

def get_guard(line: str):
    if "Guard" in line:
        guard_id = re.search("Guard #(\\d+)", line)
        return int(guard_id.group(1))
    return -1

def event_type(line):
    if "begins shift" in line:
        return EventType.STARTS_SHIFT
    if "falls asleep" in line:
        return EventType.FALLS_ASLEEP
    if "wakes up" in line:
        return EventType.WAKES_UP
    raise Exception("Unknown line: " + line)

def day4() -> (int, int):
    events = sorted(list(Event(datetime.strptime(line[1:17], "%Y-%m-%d %H:%M"), get_guard(line), event_type(line)) for line in read_file(4)), key=operator.attrgetter("time"))

    guard = -1
    guardsleep = defaultdict(lambda: GuardSleep(0, 0, []))
    for event in events:
        if event.guard >= 0:
            guard = event.guard

        if event.event == EventType.FALLS_ASLEEP:
            guardsleep[guard].last_sleep = event.time.minute

        if event.event == EventType.WAKES_UP:
            guardsleep[guard].sleep_total += event.time.minute - guardsleep[guard].last_sleep
            guardsleep[guard].add_sleeps(event.time.minute)

    most_sleepy_guard_number = max(guardsleep, key=(lambda key: guardsleep[key].sleep_total))

    most_sleepy_guard = guardsleep[most_sleepy_guard_number]
    part1_result = most_sleepy_guard_number * mode(sorted(most_sleepy_guard.sleeps))

    # Part 2
    MostSleepy = namedtuple('MostCommon', ['id', 'minute', 'amount'])
    most_sleepy = MostSleepy(0, 0, 0)
    for k in guardsleep:
        current_guard = guardsleep[k]
        try:
            most_common_minute = mode(sorted(current_guard.sleeps))
            amount = len(list((m for m in current_guard.sleeps if m == most_common_minute)))
            if amount > most_sleepy.amount:
                most_sleepy = MostSleepy(k, most_common_minute, amount)
        except statistics.StatisticsError:
            print("No unique most common minute for " + str(k))

    return part1_result, most_sleepy.id * most_sleepy.minute

if __name__ == '__main__':
    print(day4())

Replacing chained if with dictionary lookup

This function:

def event_type(line):
    if "begins shift" in line:
        return EventType.STARTS_SHIFT
    if "falls asleep" in line:
        return EventType.FALLS_ASLEEP
    if "wakes up" in line:
        return EventType.WAKES_UP
    raise Exception("Unknown line: " + line)

isn't bad, but chained if like this smell. It may be better represented as a dictionary, where the key is the string above, the value is the enum value, and you do a simple key lookup based on the last two words of every line. Whereas chained if is worst-case O(n), dictionary lookup is O(1). Then - no ifs needed, and you get the exception for free if key lookup fails.

Use raw strings

re.search("Guard #(\\d+)", line)

should be

re.search(r"Guard #(\d+)", line)

Settle down with the one-liners

This:

events = sorted(list(Event(datetime.strptime(line[1:17], "%Y-%m-%d %H:%M"), get_guard(line), event_type(line)) for line in read_file(4)), key=operator.attrgetter("time"))

is effectively illegible. Break this up into multiple lines - including a temporary variable for the strptime, as well as linebreaks in the list comprehension itself.

Don't use lists if you can use tuples

This:

MostSleepy = namedtuple('MostCommon', ['id', 'minute', 'amount'])

should be

MostSleepy = namedtuple('MostCommon', ('id', 'minute', 'amount'))

for various reasons - tuples are immutable, so use them for immutable data; and under certain narrow contexts (certainly not this one) they're faster.

Use a sum instead of a list constructor

This:

amount = len(list((m for m in current_guard.sleeps if m == most_common_minute)))

should be

amount = sum(1 for m in current_guard.sleeps if m == most_common_minute)

(Also, even if you kept using len, you should use a tuple constructor instead of a list constructor.)

Another footnote - don't put inner parens in expressions like list((...generator...)). Constructors can accept generator expressions directly.


Evaluating text section answer accuracy This code is from a flex app and is the PHP to determine if a user has read a section of text and answered a question correctly.

I write to a table that is similar to a bookmark, and then update the users points. This code works, but since I am making two calls to the database I am worried that it is too "expensive". I tried a few things like using a subquery (maybe I just don't know the syntax). I am using this for now, but I just want to do as well as I can.

public function createReadSectionAndPnts($userId, $item){
  $stmt = mysqli_prepare($this->connection,
"INSERT INTO readSection (
        user_id, chapter_id, section_letter, section_id, sub_section
        )
    VALUES (?, ?, ?, ?, ?)");
$this->throwExceptionOnError();

mysqli_bind_param($stmt, 'iisss', $userId, $item->chapter_id, $item->section_letter,
$item->section_id, $item->sub_section);
$this->throwExceptionOnError();

mysqli_stmt_execute($stmt);
$this->throwExceptionOnError();

$autoid = mysqli_stmt_insert_id($stmt);

mysqli_stmt_free_result($stmt);

$stmt2 = mysqli_prepare($this->connection,
"UPDATE users SET total_points = (5 + total_points)  WHERE id =?");
    $this->throwExceptionOnError();

    mysqli_bind_param($stmt2, 'i', $userId);
    $this->throwExceptionOnError();

    mysqli_stmt_execute($stmt2);
    $this->throwExceptionOnError();

    mysqli_stmt_free_result($stmt2);
    mysqli_close($this->connection);

return $autoid;
}

Sorry. You need to use two statement since you are updating two different tables.


JSX render method in react Following is the piece of code which is working fine, but somehow it looks bit odd to me to have a function inside a render method and using it in both conditions. Let me know how can I improve this react code as showContainer is only adding a wrapper to the function JSX.
Code -
render () {

  const childJsx = () => (<>
Random Child JSX
</>)

  return (
            <>
            { showContainer ?
             <Modal>
                {childJsx()}
             </Modal>
             :

                {childJsx()}

            }
            </>
        )

}

Your functionalmethod is fine, just the place You have defined is a bit anti pattern. (But well, if you really want to use that component in JUST in that scope its still fine.)
But let me try to refactor it in  a react way:
render () {

  return (
            <>
            {
              showContainer
                ? (
                  <Modal>
                    <ChildJsx />
                  </Modal>
                )
                : (

                    <ChildJsx />

                )
            }
            </>
        )
}

function ChildJsx(props){
  return (
    <>

Random Child JSX

    </>
  )
}

React is about reusability. In this case you can use that small component again and again as many as you want.


Build a stamp duty calculator for various types of buyers I had a task where I had to build a stamp duty calculator, which calculates the tax or stamp duty first-time buyers(ftb) returning buyers (rtb), and second-home buyers (shb) had to pay when purchasing the home.
These were the tax bands, although I think some of the percentages may be slightly wrong.
First Time Buyers
0 to £300,000 - 0%
£300,001 to £925,000 - 5%
£925001 to £1,500,000 - 10%
£1,500,000 and above - 12%

Returning Buyers
0 to £125,000 - 0%
£125,001 to £250,000 - 2%
£250,001 to £925,000 - 5%
£925,001 to £1,500,000 - 10%
£1,500,000 and above - 12%

Second Home Buyers
0 to £125,000 - 3%
£125,001 to £250,000 - 5%
£250,001 to £925,000 - 8%
£925,001 to £1,500,000 - 13%
£1,500,000 and above - 15%

Here is my solution, I look forward to the feedback.
const stampDutyCalc = (price) => {
  let stampdutyObject = {
    ftb: 0,
    rtb: 0,
    shb: 0,
  };

  firstTimeBuyerCalc(price, stampdutyObject);
  returningBuyerCalc(price, stampdutyObject);
  secondHomeBuyerCalc(price, stampdutyObject);

  return stampdutyObject;
};

function firstTimeBuyerCalc(price, stampdutyObject) {
  if (price <= 300000) {
    stampdutyObject.ftb += 0;
  } else if (price > 300000 && price <= 925000) {
    stampdutyObject.ftb += price * 0.05;
  } else if (price > 925000 && price <= 1500000) {
    stampdutyObject.ftb += (price - 925000) * 0.1 + (925000 - 300000) * 0.05;
  } else {
    stampdutyObject.ftb +=
      (price - 1500000) * 0.12 +
      (1500000 - 925000) * 0.1 +
      (925000 - 300000) * 0.05;
  }
}

function returningBuyerCalc(price, stampdutyObject) {
  if (price <= 125000) {
    stampdutyObject.rtb += 0;
  } else if (price > 125000 && price <= 250000) {
    stampdutyObject.rtb += price * 0.02;
  } else if (price > 250000 && price <= 925000) {
    stampdutyObject.rtb += (price - 250000) * 0.05 + (250000 - 125000) * 0.02;
  } else if (price > 925000 && price <= 1500000) {
    stampdutyObject.rtb +=
      (price - 925000) * 0.1 +
      (925000 - 250000) * 0.05 +
      (250000 - 125000) * 0.02;
  } else {
    stampdutyObject.rtb +=
      (price - 1500000) * 0.12 +
      (1500000 - 925000) * 0.1 +
      (925000 - 250000) * 0.05 +
      (250000 - 125000) * 0.02;
  }
}

function secondHomeBuyerCalc(price, stampdutyObject) {
  if (price <= 125000) {
    stampdutyObject.shb = price * 0.03;
  } else if (price > 125000 && price <= 250000) {
    stampdutyObject.shb += (price - 125000) * 0.05 + 125000 * 0.03;
  } else if (price > 250000 && price <= 925000) {
    stampdutyObject.shb +=
      (price - 250000) * 0.08 + (250000 - 125000) * 0.05 + 125000 * 0.03;
  } else if (price > 925000 && price <= 1500000) {
    stampdutyObject.shb +=
      (price - 925000) * 0.13 +
      (925000 - 250000) * 0.08 +
      (250000 - 125000) * 0.05 +
      125000 * 0.03;
  } else {
    stampdutyObject.shb +=
      (price - 1500000) * 0.15 +
      (1500000 - 925000) * 0.13 +
      (925000 - 250000) * 0.08 +
      (250000 - 125000) * 0.0 +
      125000 * 0.03;
  }
}

function returningBuyerCalc(price, stampdutyObject) {
  if (price <= 125000) {
    stampdutyObject.rtb += 0;
  } else if (price > 125000 && price <= 250000) {
    stampdutyObject.rtb += price * 0.02;
  } else if (price > 250000 && price <= 925000) {
    stampdutyObject.rtb += (price - 250000) * 0.05 + (250000 - 125000) * 0.02;
  } else if (price > 925000 && price <= 1500000) {
    stampdutyObject.rtb +=
      (price - 925000) * 0.1 +
      (925000 - 250000) * 0.05 +
      (250000 - 125000) * 0.02;
  } else {
    stampdutyObject.rtb +=
      (price - 1500000) * 0.12 +
      (1500000 - 925000) * 0.1 +
      (925000 - 250000) * 0.05 +
      (250000 - 125000) * 0.02;
  }
}

Hard coded
You have created yourself a future problem by hard coding the data in your code.  When dealing with taxes (or any business logic)  there is one certainty, and that is change.
With the data embedded in the code, making even a minor change will take some effort and will be very prone to error.
You need to create a data structure that allows you to make changes quickly and easily. You should only have to change a value once, in one location, not many times in many places.
Defining the data
The data is defined as an object, with only the data needed, if information can be deduced there is no need to include it.
The example has a helper function rate to reduce the overhead of typing in {level:10000, tax: 0.1} over and over
const rate = (level, tax) => ({level, tax});
const brackets = {
    firstTime: [
        rate(300000, 0),
        rate(925000, 0.05),
        rate(1500000, 0.1),
        rate(Infinity, 0.12),
    ],
    returning: [
        rate(125000, 0),
        rate(250000, 0.02),
        rate(925000, 0.05),
        rate(1500000, 0.1),
        rate(Infinity, 0.12),
    ],
    second: [
        rate(125000, 0.03),
        rate(250000, 0.05),
        rate(925000, 0.08),
        rate(1500000, 0.13),
        rate(Infinity, 0.15),
    ],
};

Applying logic
With the data organised, the code can then just deal with the logic of how the duty is calculated:
function calculateDuty(price, bracket) {
    var duty = 0;
    var prevLevel = 0
    for (const rate of bracket) {
        if (rate.level <= price) {
            duty += (rate.level - prevLevel) * rate.tax;
            prevLevel = rate.level;
        } else {
            return duty + (price - prevLevel) * rate.tax;
        }
    }
}

And now you can get the duty for any bracket: calculateDuty(price, brackets.firstTime)
Rewrite
Thus the whole thing becomes:
const rate = (level, tax) => ({level, tax});
const brackets = {
    firstTime: [rate(300000, 0), rate(925000, 0.05), rate(1500000, 0.1), rate(Infinity, 0.12)],
    returning: [rate(125000, 0), rate(250000, 0.02), rate(925000, 0.05), rate(1500000, 0.1), rate(Infinity, 0.12)],
    second: [rate(125000, 0.03), rate(250000, 0.05), rate(925000, 0.08), rate(1500000, 0.13), rate(Infinity, 0.15)],
};
const stampDutyCalc = price => ({
    ftb: calculateDuty(price, brackets.firstTime),
    rtb: calculateDuty(price, brackets.returning),
    shb: calculateDuty(price, brackets.second),
});

function calculateDuty(price, bracket) {
    var duty = 0, prevLevel = 0
    for (const rate of bracket) {
        if (rate.level <= price) {
            duty += (rate.level - prevLevel) * rate.tax;
            prevLevel = rate.level;
        } else {
            return duty + (price - prevLevel) * rate.tax;
        }
    }
}


Shift vowels towards the end of a Single Linked List We have a linklist which is made of alphabets. We have to shift the vowels towards the end of the linklist, without changing the order in which they appear. So,

A->C->E->F->G->O

should become

C->F->G->A->E->O

This is the code I have written

#include<iostream>

using namespace std;

struct node
{
    char ch;
    node *next;
};

void enqueue (node **head, node **tail, char val)
{
    node *newn = (node *)malloc(sizeof(node));
    newn->ch = val;
    newn->next = NULL;

    if (*head == NULL)
    {
        *head = newn;
        *tail = newn;
    }

    (*tail)->next = newn;
    (*tail) = newn;

}

void print (node *head)
{
    while (head!=NULL)
    {
        cout<<head->ch<<" ";
        head = head->next;
    }
    cout<<endl;
}

bool isVowel (char ch)
{
    ch = ch | 32;

    if (ch == 'a' || ch =='e' || ch=='i' || ch=='o' || ch=='u')
        return true;
    return false;

}

node* segregateVowels (node *head, node *tail)
{
    if (head == NULL || head->next == NULL)
        return head;

    node *temp = head;
    node *fin = tail;

    while (temp->next!=fin)
    {
        cout<<temp->ch<<" "<<fin->ch<<endl;
        if (isVowel(temp->next->ch))
        {
            node *shift = temp->next;
            temp->next = temp->next->next;
            tail->next = shift;
            shift->next = NULL;
            tail = shift;
        }
        else
            temp = temp->next;

    }

    if (temp->next!=NULL && isVowel(temp->next->ch))   /* Handling the case where the last element is a vowel   */
    {
        node *toMove = temp->next;
        temp->next = temp->next->next;
        tail->next = toMove;
        toMove->next = NULL;

    }
    else
        temp = temp->next;

    if (isVowel(head->ch))                  /*  Handling the case where the first element is a vowel    */
    {
        node *toMove = head;
        head = head->next;
        toMove->next = temp->next;
        temp->next = toMove;
    }

    return head;

}

int main()
{
    srand(time(NULL));

    node  *head = NULL, *tail = NULL;

    int i = 20;

    while (i>=0)
    {
        enqueue (&head, &tail, rand()%26+65);
        i--;
    }

    print(head);

    head = segregateVowels (head, tail);

    print(head);
}

Although this does seem to give me correct results, but I have to explicitly check for two edge cases. Also I feel there is a better solution for this problem or a better approach which I seem to be missing. The problem can be handled by creating two separate Link Lists too, and then joining them together. But what modifications do you suggest in the above program?

Don't do using namespace std

Here's a SO question which should provide you more information about this.

For loops are here to help you

You could write

int i = 20;
while (i>=0)
{
    enqueue (&head, &tail, rand()%26+65);
    i--;
}

in a much clearer way using a for-loop : for (int i=20; i>=0; i--) or for (int i=0; i<21; i++). I prefer the latter because I tend to count forward and not backward and it makes much clearer the fact that you want 21 iterations.

Avoid boilerplate and useless code

if (ch == 'a' || ch =='e' || ch=='i' || ch=='o' || ch=='u')
    return true;
return false;

could be simply written :

return (ch == 'a' || ch =='e' || ch=='i' || ch=='o' || ch=='u');

Avoid magic numbers

Even if I can guess them, I don't want to learn the ascii code for the different characters.
You could rewrite enqueue (&head, &tail, rand()%26+65); in a more explicit way : enqueue (&head, &tail, rand()%26+'A');

Be consistent with your brackets

Using brackets for single statement is a personal preference but if you do use them for the then-block, then please use them for the else-block as it makes things look "balanced".

Add a bit of documentation
I know it's tedious and not so interesting and I usually try to avoid to ask for it but here, I am getting a but confused by the two functions taking two lists as parameters and trying to understand what is intended for both of them.

This being said, I have to go and I'll try to have a deeper look later on.

In the meantime, here's your code with the comments above taken into account :

#include<iostream>

struct node
{
    char ch;
    node *next;
};

void enqueue (node **head, node **tail, char val)
{
    node *newn = (node *)malloc(sizeof(node));
    newn->ch = val;
    newn->next = NULL;

    if (*head == NULL)
    {
        *head = newn;
        *tail = newn;
    }

    (*tail)->next = newn;
    (*tail) = newn;
}

void print (node *head)
{
    while (head!=NULL)
    {
        std::cout<<head->ch<<" ";
        head = head->next;
    }
    std::cout<<std::endl;
}

bool isVowel (char ch)
{
    ch |= 32; // make it lowercase
    return (ch == 'a' || ch =='e' || ch=='i' || ch=='o' || ch=='u');
}

node* segregateVowels (node *head, node *tail)
{
    if (head == NULL || head->next == NULL)
        return head;

    node *temp = head;
    node *fin = tail;

    while (temp->next!=fin)
    {
        std::cout<<temp->ch<<" "<<fin->ch<<std::endl;
        if (isVowel(temp->next->ch))
        {
            node *shift = temp->next;
            temp->next = temp->next->next;
            tail->next = shift;
            shift->next = NULL;
            tail = shift;
        }
        else
        {
            temp = temp->next;
        }
    }

    if (temp->next!=NULL && isVowel(temp->next->ch))   /* Handling the case where the last element is a vowel   */
    {
        node *toMove = temp->next;
        temp->next = temp->next->next;
        tail->next = toMove;
        toMove->next = NULL;

    }
    else
    {
        temp = temp->next;
    }

    if (isVowel(head->ch))                  /*  Handling the case where the first element is a vowel    */
    {
        node *toMove = head;
        head = head->next;
        toMove->next = temp->next;
        temp->next = toMove;
    }

    return head;
}

int main()
{
    srand(time(NULL));

    node  *head = NULL, *tail = NULL;

    for (int i=0; i<21; i++)
    {
        enqueue (&head, &tail, rand()%26+'A');
    }

    print(head);

    head = segregateVowels (head, tail);

    print(head);
}


Telling a client that there is no result via the DTO pattern This is how I handle this situation right now:

For example, I have service that returns UserDto by user ID: GetUserById.

Service never returns null or throws exceptions.
Service always returns DTO objects derived from the base class (DtoBase).
Each DTO object relates to some Entity type. Each entity has an assigned ID (In my example I used long for IDs, but the Response class can be made generic).

DtoBase class:

[DataContract]
public abstract class DtoBase
    : IDtoResponseEnvelop
{
    [DataMember]
    private readonly Response _responseInstance = new Response();

    //This constructor should be called when there is no result
    protected DtoBase()
    {}

    //Each DTO object relates to some Entity (each entity has as ID).
    //And if there is some result this constructor should be called.
    protected DtoBase(long entityId)
    {
        _responseInstance = new Response(entityId);
    }

    #region IDtoResponseEnvelop Members

    public Response Response
    {
        get { return _responseInstance; }
    }

    #endregion
}

Basically, the Response class aggregates operation response information such as: if there is any value, exception and warnings:

[DataContract]
public class Response
{
    #region Constructors

    public Response():this(0){}

    public Response(long entityId)
    {
        _entityIdInstance = entityId;
    }

    #endregion

    #region Private Serializable Members

    [DataMember]
    private BusinessExceptionDto _businessExceptionInstance;

    [DataMember]
    private readonly IList<BusinessWarning> _businessWarningList = new List<BusinessWarning>();

    [DataMember]
    private readonly long _entityIdInstance;

    #endregion

    #region Public Methods

    public void AddBusinessException(BusinessException exception)
    {
        _businessExceptionInstance = new BusinessExceptionDto(exception.ExceptionType, exception.Message, exception.StackTrace);
    }

    public void AddBusinessWarnings(IEnumerable<BusinessWarning> warnings)
    {
        warnings.ToList().ForEach( w => _businessWarningList.Add(w));
    }

    #endregion

    #region Public Getters

    public bool HasWarning
    {
        get { return _businessWarningList.Count > 0; }
    }

    public IEnumerable<BusinessWarning> BusinessWarnings
    {
        get { return new ReadOnlyCollection<BusinessWarning>(_businessWarningList); }
    }

    public long EntityId
    {
        get { return _entityIdInstance; }
    }

    public bool HasValue
    {
        get { return EntityId != default(long); }
    }

    public bool HasException
    {
        get { return _businessExceptionInstance != null; }
    }

    public BusinessExceptionDto BusinessException
    {
        get { return _businessExceptionInstance; }
    }

    #endregion
}

Now, having this in place, on server side we have:

    private UserDto GetUserByIdCommand(IRepositoryLocator locator, long userId)
    {
        var item = locator.GetById<User>(userId);
        if (item != null)
             return Mapper.Map<User, UserDto>(item); //mapper calls UserDto(item.ID) constructor

        _businessNotifier.AddWarning(BusinessWarningEnum.Operational,
            string.Format("User with Id=\"{0}\" not found", userId));
        return new UserDto(); //no Entity id is provided here, since no value available
    }

And, on client side:

    var userDto = UserService.GetUserById(1);
    if(!userDto.Response.HasValue)
    {
          //No result is available
    }

Is my approach fine or is there a better way?

I only quickly glanced at your code, so this isn't going to be a thorough review, but I think the basic idea is surprising:

Service never returns null or throws exceptions.

I think this approach is error-prone. When I call a method called GetUserById, for an Id that doesn't exist, I expect either an ArgumentOutOfRangeException (or similar), or a null return value - the last thing I expect is a valid object filled up with default values filling up all members.
As @jlnorsworthy mentioned in his excellent comment, your approach is far from instinctive. Instead of:
var userDto = UserService.GetUserById(1);
if(!userDto.Response.HasValue)
{
      //No result is available
}

I would perfer to have:
var userResult = UserService.GetUserById(1);
if(userResult.Result == null)
{
      //No result is available
}

Also HasValue confusing because it is a well-known member of Nullable<T>, so I'd consider changing EntityId to be Nullable<long> or Nullable<Int64>:
public bool HasValue
{
    get { return EntityId.HasValue; }
}

What is IRepositoryLocator? If it is what I think it is, you should read up on Mark Seeman's blog. Shortly put:

Service Locator is a well-known pattern, and since it was described by Martin Fowler, it must be good, right?
No, it's actually an anti-pattern and should be avoided.

Little nitpick, I wouldn't use ForEach here:
public void AddBusinessWarnings(IEnumerable<BusinessWarning> warnings)
{
    warnings.ToList().ForEach( w => _businessWarningList.Add(w));
}

It would be much more readable (and less semantically controversial) to write it like this:
public void AddBusinessWarnings(IEnumerable<BusinessWarning> warnings)
{
    _businessWarningList.AddRange(warnings);
}


Show one sheet for each field in pivot filter field Into my workbook I've the db of inventory, in another sheet I've one pvt and in the filter field I put the filed "Card". Now I want create one sheet for each "Card" usually I use this code for this operation:

Sub cmdg2b2(control As IRibbonControl)  'genera una sheets per ogni card
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
    Sheets("pivot").Select
    ActiveSheet.Unprotect
    ActiveSheet.PivotTables("Pvt_Pck").ShowPages PageField:="Card"
    Sheets("pivot").Protect DrawingObjects:=True, Contents:=True, Scenarios:=True
    Sheets("pivot").EnableSelection = xlNoSelection
Application.ScreenUpdating = True
Application.Calculation = xlCalculationAutomatic
End Sub

This function always runs right, but this year the sheets are much (175) and my code stopped for exceeded memory. I think to hide half off the total sheets, if I hide the sheets manually with pvt filer the operation run in a couple of minutes, if I use this code I need 9 minutes.

Can somebody help me to improve the performance my code?

Function ShowSheets(ByVal conta As String)  'card serves to define whether to show the first half of the sheets or the second
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
Dim tsheet As Integer
Dim tstart As Date, tend As Date, tteempo As Date
tstart = Now()
    ActiveSheet.PivotTables("Pvt_Pck").PivotFields("Card").CurrentPage = "(All)"
    With ActiveSheet.PivotTables("Pvt_Pck").PivotFields("Card")
    tsheet = Int(.PivotItems.Count / 2)
    Select Case conta
    Case "A"
        For i = 1 To tsheet
            .PivotItems(i).Visible = False
        Next i
    Case "B"
        For i = tsheet + 1 To .PivotItems.Count - 1
            .PivotItems(i).Visible = False
        Next i
    End Select
    End With
    Range("A4").Select
Application.ScreenUpdating = True
Application.Calculation = xlCalculationAutomatic
tend = Now()
ttempo = tend - tstart
Debug.Print ttempo
End Function

Sheets("pivot").Select
ActiveSheet.Unprotect
ActiveSheet.PivotTables("Pvt_Pck").ShowPages PageField:="Card"
Sheets("pivot").Protect DrawingObjects:=True, Contents:=True, Scenarios:=True
Sheets("pivot").EnableSelection = xlNoSelection

This is rather inefficient, and harder to follow than it needs to be. There's no need to Select anything and to work off the ActiveSheet.

Every sheet has a code name, an identifier that you can use in code to refer to it. Think of it as a free object instance living in global scope, waiting to be used. Select the worksheet in the VBE's Project Explorer (Ctrl+R if it's not displayed), and look at its properties (F4 if it's not displayed):

Notice there are two Name properties: one (Name) is the name of the tab in Excel; the other ((Name)) is the name of that "free object" I'm talking about.

If Sheets("pivot") is named Sheet1, then you can .Select it like this:

Sheet1.Select

But you don't need to do this.

You're accessing the Sheets collection (implicitly ActiveWorkbook.Sheets); that collection contains all sheets, and in Excel not all Sheet objects are a Worksheet - the Sheets collection also contains Chart sheets!

So instead of Sheets("name"), you should be accessing Worksheets("name") if you expect to be working with a worksheet. But you don't need to do that either.

And you should avoid implicit references to the ActiveWorkbook or the ActiveSheet: a fully-qualified ThisWorkbook.Worksheets("name") call is more reliable than Worksheets("name").

But I digress. I was saying you don't need to do this. Let's say you gave your "pivot" sheet a code name, and that you can access it in code with the  PivotSheet identifier.

You could then do this:

With PivotSheet
    .Unprotect
    .PivotTables("Pvt_Pck").ShowPages PageField:="Card"
    .Protect DrawingObjects:=True, Contents:=True, Scenarios:=True
    .EnableSelection = xlNoSelection
End With

This does exactly the same thing as the snippet I quoted at the top of this post. Except if you look under the hood. Your code really did this:

In the Sheets collection of the ActiveWorkbook, look for the sheet named "pivot", and Select that sheet (so it becomes the ActiveSheet).
Unprotect the active sheet.
In the PivotTables collection of the ActiveSheet, look for the pivot table named "Pvt_Pck", and change the PageField to "Card".
In the Sheets collection of the ActiveWorkbook, look for the sheet named "pivot", and Protect that sheet.
In the Sheets collection of the ActiveWorkbook, look for the sheet named "pivot", and set EnableSelection on that sheet to xlNoSelection.

Notice everytime you access the Sheets collection, you're working much harder than you need to. Compare to mine:

Using the PivotSheet object reference...

Unprotect it.
In the PivotTables collection, look for the pivot table named "Pvt_Pck", and change the PageField to "Card".
Protect it.
Set EnableSelection to xlNoSelection.

Sub cmdg2b2(control As IRibbonControl)

You should always use names that you can pronounce. cmdg2b2 says nothing more than can be inferred from its IRibbonControl parameter, that it's some Ribbon callback procedure. What does it do? Wouldn't this be easier to work with?

Public Sub SetPivotPageField(ByRef control As IRibbonControl)

The procedure now...

has an explicit access modifier (Public)
is now explicit about how it's receiving its parameter (ByRef)
has a name that starts with a verb, conforms to the PascalCase naming convention of everything else in VBA, and gives us a clue about what's happening in its scope, without having to look at the actual code.

In the ShowSheets method, I can't help but notice these repeated lines:

Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual

You had them in your Ribbon callback procedure too:

Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual

Every time you set Application.Calculation to xlCalculationAutomatic, you trigger a recalculation; in a large complicated workbook with lots of lookups and external dependencies, this single instruction alone can take several minutes to complete: don't toggle calculation mode without needing to.

Now, repeating code like this isn't something you want to have all over the place. Make a small procedure for that.

Private Sub ToggleWaitMode(Optional ByVal wait = True, Optional ByVal toggleCalculation = True, Optional ByVal toggleEvents = True)
    With Application
        .StatusBar = IIf(wait, "Please wait...", vbNullString)
        .ScreenUpdating = Not wait
        .EnableEvents = IIf(toggleEvents, Not wait, .EnableEvents)
        .Calculation = IIf(toggleCalcualtion, IIf(wait, xlCalculationManual, xlCalculationAutomatic), .Calculation)
    End With
End Sub

Now instead of copying the same code to toggle these things, you can simply do this before a long-running operation:

ToggleWaitMode

And then this when you're done:

ToggleWaitMode False

Now, seeing "Please wait..." in the status bar for 9 minutes isn't cool at all, and if I'm the user I totally think Excel is frozen. Consider updating progress in the status bar:

Public Sub UpdateStatus(Optional ByVal text As String = vbNullString)
    Dim appScreenUpdating As Boolean
    With Application
        appScreenUpdating = .ScreenUpdating
        If Not appScreenUpdating Then .ScreenUpdating = True
        .StatusBar = text
        .ScreenUpdating = appScreenUpdating
    End With
End Sub

You need this procedure to make sure ScreenUpdating is on before it updates the status bar text, otherwise the user won't see it changing because Excel isn't repainting itself.

These "helper procedures" can now be used to make your code much more focused on what it's supposed to do.

The ShowSheets function isn't returning anything. Function members should have a return value - if you don't need a return value, then make it a Sub (procedure) instead.

ActiveSheet.PivotTables("Pvt_Pck").PivotFields("Card").CurrentPage = "(All)"

You're not activating/selecting any sheet here - the procedure simply assumes that the active sheet has a pivot table named Pvt_Pck... and things could go horribly wrong if that's not the case: with screen updating turned off, and no error handling to turn it back on, things can get messy.

Handle runtime errors!

Public Sub ShowSheets(ByVal conta As String)

    On Error GoTo CleanFail
    ToggleWaitMode

    '...

CleanExit:
    ToggleWaitMode False
    Exit Sub

CleanFail:
    MsgBox Err.Description, vbExclamation
    Resume CleanExit
End Sub

Again, avoid working with the ActiveSheet, be it explicitly or implicitly.

Range("A4").Select

That Range call is an implicit reference to the active sheet - and if that sheet is not a Worksheet object, the Range method will fail, like this:

Not very graceful.

tsheet = Int(.PivotItems.Count / 2)

The Int function may not be working the way you intend it here: it doesn't round anything, it simply cuts the decimal part of the number - so this:

Int(1/2)

Returns 0. It could matter, or not. But it's good to know.

This is annoying:

Select Case conta
Case "A"
    ...
Case "B"
    ...
End Select

You need a comment to explain what the parameter is used for, and then when you call the method you need to know what magic values are being handled. Not cool. Using magic strings or magic numbers is the same thing: you're using magic values.

Make an enum instead:

Public Enum CardDisplayStyle
    ShowTopHalf
    ShowBottomHalf
End Enum

And now instead of passing a magic string "A" or "B", the call site looks like this:

ShowSheets ShowTopHalf

Or:

ShowSheets ShowBottomHalf

And the signature changes to this:

Public Sub ShowSheets(ByVal conta As CardDisplayStyle)

And the Select Case block no longer works off magic strings:

Case ShowTopHalf
    ...
Case ShowBottomHalf
    ...


Find the last element of a list Find the last element of a list from codewars.com

  Example:

last( [1,2,3,4] ) # => 4

last( "xyz" ) # => z

last( 1,2,3,4 ) # => 4

I completed this kata several times tonight.  My current answer is a mashup of several other people's solutions.

I do not understand why I cant change .respond_to?(:index) into .is_a? Array
Should I use ternary operators in Ruby?
Other suggestions?

def last (*arg, arg_last)
  arg_last.respond_to?(:index) ? arg_last[-1] : arg_last
end

Your input might be a String or an Array. Both of these support the :index method, so foo[-1] will give the last character of a string or the last element of an array. But if you test for it being an array, the solution won't work for strings when it otherwise would.
I don't think there's anything wrong with using ternary operators in ruby, as long as all three 'elements' are fairly short. More concise code is easier to read. See the Ruby Style Guide at https://github.com/bbatsov/ruby-style-guide#ternary-operator.


Print the elements of an array into a table I am trying to print the elements of an array into a table, but I'm having trouble formatting the array elements itself.  Perhaps there is a better solution than what I'm currently using?

import java.util.Scanner;
public class RainFall{

  public static void main(String args[]) {
    //create rainfall array
    double[][]rainfall = new double[2][3];
    double sum = 0;
    double average = 0;
    int count = 0;

    Scanner in = new Scanner (System.in);

    System.out.println("Please enter rainfall for Region 1 and Region 2 one after the other");
    //Create For loop to read in data
    for(int i=0;i<rainfall.length;i++){
      for(int j=0;j<rainfall[0].length;j++){
        System.out.println("Enter rainfall:: ");
        rainfall[i][j]=in.nextDouble();
      }
    }
    //create for loop to sum rainfall
    for (int i=0; i < rainfall.length; i++){
      for (int j=0; j < rainfall[0].length; j++){
        sum += rainfall[i][j];
        count++;
      }//end nested for
     }//end outer for

    //calculate average
    average = sum/count;
    //Print table
    System.out.printf( "%5s %10s %15s %20s %25s %n", "Region 1"," Jan", "Feb", "Mar", "Avg Rainfall");
    System.out.printf( "%d", rainfall[0][0] );

    //Print average

    System.out.println("The average rainfall is: " + average);
  }
}

If the array bounds are fixed, then there's no need for the count variable, since its value will always be the same. There's also no need to run a separate loop for the summation. You can do this at the same time as you're gathering the input. And since your output shows that you want to show the averages by region, you'll need to have somewhere to store those values, too.

Also, try using more descriptive prompts for the user so they know how many inputs are expected.

So the first part of your function becomes

//could also define these as constants
int regions = 2;
int months = 3;
int count = regions * months;

double[][]rainfall = new double[regions][months];
double[] avgByRegion = new double[regions];
double sum = 0;
double average = 0;

Scanner in = new Scanner (System.in);

//Create For loop to read in data
for(int i=0; i < regions; i++){
  double sumByRegion = 0;
  for(int j=0; j < months; j++){
    System.out.printf("Enter rainfall for Region %d, Month %d:: ", i+1,j+1);
    rainfall[i][j]=in.nextDouble();
    sumByRegion += rainfall[i][j];
  }
  avgByRegion[i] = sumByRegion/months;
  sum += sumByRegion;
}

//calculate average
average = sum/count;

Then you can format your output with another set of loops:

for(int i=0; i < regions; i++){
  System.out.printf("Region %d\t  Jan\t  Feb\t  Mar\t  Avg Rainfall%n", i+1);
  for(int j=0; j < months; j++){
    System.out.printf("\t%2.1f",rainfall[i][j]);
  }
  System.out.printf("\t%2.1f%n", avgByRegion[i]);
}
System.out.println("The overall average rainfall is: " + average);


Wiki Link Mapper — A webcrawler which crawls links recursively and prints them into a graph I would like some suggestions on how to make it better and also if any features that i should add to this Script. I am using beautifulsoup,requests for scraping and networkx for plotting the graph.

https://github.com/HardikM1999/WikiLinkMapper

import requests
from bs4 import BeautifulSoup
import sys
import networkx as nx
import matplotlib.pyplot as plt

#Taking Arguments
#Starting string
start = sys.argv[1]

#No of repetitions
rep = int(sys.argv[2])
tot_list = []

#Creating graph
G = nx.Graph()
G.add_node(start)

#recur function to scrape links
def recur(url,depth,tot_list,parent,rep):
    if(depth<=rep):
        page = requests.get('https://www.wikipedia.org/wiki/' + url)
        #parsing the page
        soup = BeautifulSoup(page.text,'html.parser')
        cnt = 0
        #list for particular depth
        depth_list = []
        for para_tag in soup.select('p'):
            for anchor_tag in para_tag.select('a'):
                if cnt>rep:
                    break
                #getting the link
                check_string = anchor_tag['href']
                if check_string.startswith('#cite') == False and check_string.startswith('/wiki/Help') == False and check_string.startswith('/wiki///en:') == False and check_string.startswith('/wiki/Wikipedia') == False :
                    cnt = cnt + 1
                    depth_list.append(check_string[6:])
            if cnt>rep:
                break
        for link in depth_list:
            tot_list.append((link,parent))
            recur(link,depth+1,tot_list,link,rep)
    return tot_list

tot_list = recur(start,0,tot_list,start,rep)
node_sizes = []
node_sizes.append(100*len(start))

#Adding tot_list to the graph
for a,b in tot_list:
    G.add_node(a)
    G.add_edge(a,b)
    node_sizes.append(100*len(a))

#Drawing the Graph
nx.draw(G,node_color = 'orange',node_size = node_sizes,with_labels = True)
plt.draw()
plt.show()

Wikipedia contains circular references. Currently you follow them around until your max depth is reached. Instead, keep a set of all sites visited so far.

You also don't need this function to be recursive, you can make it iterative like this:

from collections import deque

def build_link_graph(start, max_pages=10):
    queue = deque([start])
    visited = set()
    i = 0
    while queue and i < max_pages:
        next_url = queue.popleft()
        urls = get_links(next_url)
        i += 1
        # make sure not to visit any page twice
        queue.extend(url for url in urls if url not in visited)
        visited.update(urls)
        yield next_url, urls

The actual parsing can also be simplified using the CSS selector p > a, meaning all a tags which are contained in a p tag. I would also use a requests.Session to keep the connection alive and use the lxml parser, both of which slightly speed up the scraping.

def filter_links(links):
    blacklist = ['#cite', '/wiki/Help', '/wiki///en:', '/wiki/Wikipedia']
    return [link for link in links
            if not any(link.startswith(prefix) for prefix in blacklist)]

def get_links(url):
    page = SESSION.get(url)
    soup = BeautifulSoup(page.text, "lxml")
    links = [a["href"]
             for a in soup.select("p > a")
             if "selflink" not in a.get("class", [])]
    return [BASE_URL + link for link in filter_links(links)]

With these functions, building the graph is rather easy. Note that you might want to choose a directed graph, since links are usually unidirectional. Also, networkx automatically adds missing nodes when adding edges, so no need to do that.

import sys

if __name__ == "__main__":
    BASE_URL = "'https://en.wikipedia.org'"
    if len(sys.argv) == 2:
        start = sys.argv[1]
    else:
        start = BASE_URL + '/wiki/Earthquake_engineering'
    SESSION = requests.Session()
    g = nx.DiGraph()
    for url, urls in build_link_graph(start):
        graph.add_edges_from((url, link) for link in urls)

I added a if __name__ == "__main__": guard to allow importing from this script from another script without running the scraping

Note that if you increase max_pages too much, you will run into a too many requests.response. Currently there is no code to handle that, but you might add some. The easiest way is to just time.sleep some time and try again afterwards. There are also other ways, some of which may not be in compliance with Wikipedia's ToS:

# robots.txt for Wikipedia and friends
# Please note: There are a lot of pages on this site, and there are
# some misbehaved spiders out there that go _way_ too fast. If you're
# irresponsible, your access to the site may be blocked.

There may be other ways to build this graph than scraping. Wikipedia supplies databases to download which should contain everything you need: https://en.wikipedia.org/wiki/Wikipedia:Database_download Note that it is 58 GB when uncompressed, though.


Add two 4-D arrays in Python I have two arrays, each with shape (1, 51, 150, 207)

I need to add them such that:

newarray[0][0][0][0] = array1[0][0][0][0] + array2[0][0][0][0]
newarray[0][0][0][1] = array1[0][0][0][1] + array2[0][0][0][1]

for every element in the array.

Currently I'm creating a new array using nested while loops but this approach is extremely time consuming. Can anyone offer a better solution?

H = 0
NS = 0
EW = 0
height = []
while H < 51:
    northsouth = []
    while NS < 150:
        eastwest = []
        while EW < 207:
            eastwest.append(PH[0][H][NS][EW] + PHB[0][H][NS][EW])
            EW += 1
            print eastwest
        northsouth.append(eastwest)
        NS += 1
        print northsouth
    height.append(northsouth)
    H =+ 1

Firstly, code that looks like:

foo = 0
while foo < bar:
    ...  # foo doesn't change here
    foo += 1

is a bad sign. In Python, this can immediately be simplified, using range, to:

for foo in range(bar):
    ...

Secondly, hard-coding the shape of your "arrays" everywhere (having the literals 51, 150 and 207 in the code itself), referred to as "magic numbers", is poor practice in every programming language. These are the lengths of each sub-list, so should be calculated as needed using len(...). This makes the code more flexible; it can now be easily applied to add arrays of other sizes. it's also slightly awkward to hard-code the depth of the addition (i.e. that it only handles four-dimensional arrays).

Thirdly, however, code like:

for index in range(len(sequence)):
    element = sequence[index]
    ...

is generally better replaced with:

for element in sequence:
    ...

where you need the index too, you can use enumerate, and where you need to iterate over multiple iterables you can use zip; Python has lots of handy functions for dealing with iterables (see also itertools).

Fourthly, when you have a loop appending to a list, it's usually more efficient to replace it with a "list comprehension". For example:

foo = []
for bar in baz:
    foo.append(bar)

can be replaced by:

foo = [bar for bar in baz]

Finally, however, this is totally trivial if you switch to numpy, which is extremely helpful for manipulating arrays:

>>> import numpy as np
>>> arr1 = np.random.randint(1, 10, (1, 51, 150, 207))
>>> arr2 = np.random.randint(1, 10, (1, 51, 150, 207))
>>> arr1 + arr2  # wasn't that easy!
array([[[[11, 13, 14, ...,  9, 14,  8],
         [15,  4, 10, ..., 14, 10, 16],
         [11, 11,  9, ...,  4,  9,  9],
         ...,
         [17, 10, 14, ..., 10, 12, 12],
         [12, 11, 14, ...,  7,  8,  8],
         [ 9, 11,  6, ...,  3, 11,  8]],

        [[10, 15, 16, ..., 11, 16, 10],
         [17, 17,  8, ...,  8,  7,  8],
         [11,  7,  2, ..., 16, 11,  5],
         ...,
         [15, 10, 16, ..., 16, 13,  9],
         [15,  8,  7, ..., 13,  5,  6],
         [ 4,  5, 10, ...,  7, 10, 10]],

        [[11, 10,  4, ..., 10,  7, 11],
         [12,  3, 14, ..., 11, 12, 12],
         [ 6, 11, 16, ..., 14,  9, 12],
         ...,
         [18, 10, 11, ..., 13, 14,  9],
         [11, 11, 10, ...,  9, 13, 12],
         [10, 10, 10, ..., 12, 14,  8]],

        ...,
        [[16, 11, 12, ..., 13, 13,  9],
         [ 7, 15, 10, ...,  9, 11,  5],
         [ 5, 11, 14, ..., 14,  4, 11],
         ...,
         [15, 13,  6, ..., 17,  6, 10],
         [ 9, 12,  7, ...,  7, 17, 11],
         [12,  9, 10, ...,  4,  9,  5]],

        [[15, 12, 10, ...,  5, 12, 15],
         [17, 14, 15, ..., 13, 11,  8],
         [13, 10, 10, ...,  8,  4,  8],
         ...,
         [12,  9, 10, ...,  9,  7, 12],
         [13, 12, 17, ...,  7, 13, 10],
         [13,  6,  8, ..., 15, 13,  7]],

        [[ 9,  8,  8, ...,  6, 10,  2],
         [ 9, 15, 14, ..., 14,  4,  5],
         [ 3, 12, 12, ...,  5, 10, 14],
         ...,
         [ 6, 11, 15, ..., 11,  4, 15],
         [ 4, 12, 14, ..., 12, 11,  9],
         [10,  3,  5, ...,  5,  7, 10]]]])


Usage of lens for concise update of records I'm new to Haskell. I'm developing a game-like simulation. I try to use lenses to update the state of the world.

Current iteration of code works, but looks clumsy. I am trying to improve it, e.g. I would like to:

Get rid of direct enumeration of q1 and so on, with usage of quantityPolymorph and others in them. Problem is that partial application won't help since the function which gets applied is "in the middle" of arguments;
Get rid of incrementQuantity. I'd like something like fmap (+1), but since Quantity is not an instance of Functor, I can't do that. I can't make Quantity an instance because it has kind *. Is there a more concise way, w/o lambda?
Update world with just lenses, if possible. It looks like simple dispatching and I think it might be possible to move away from direct specification of what lens (i.e. quantityPolymorpth) should be called for which argument (f).

Important note: however the updateWorld never emits Nothing at the moment, I want to keep it returning a Maybe because later at some point it will return Nothings as well.

How can I refactor my code? Are there other things amiss with it?

The source follows.

{-# LANGUAGE NamedFieldPuns #-}
{-# LANGUAGE TemplateHaskell #-}

import Control.Lens
import Control.Monad
import System.Random

data Quantity = Quantity Integer deriving (Show)

instance Random Quantity where
    randomR (Quantity lo, Quantity hi) g =
        let rand = randomR (lo, hi) g
            (r, g1) = rand
        in (Quantity r, g1)
    random g =
        let rand = random g
            (r, g1) = rand
        in (Quantity r, g1)

instance Eq Quantity where
    (==) (Quantity q1) (Quantity q2) = q1 == q2

instance Ord Quantity where
    (<=) (Quantity q1) (Quantity q2) = q1 <= q2

instance Num Quantity where
    (+) (Quantity q1) (Quantity q2) = Quantity (q1 + q2)
    (*) (Quantity q1) (Quantity q2) = Quantity (q1 * q2)
    (-) (Quantity q1) (Quantity q2) = Quantity (q1 - q2)
    abs (Quantity q) = Quantity (abs q)
    signum (Quantity q) = Quantity (signum q)
    fromInteger i = Quantity i
-------------------------------

data QuantityHolder = QuantityHolder { _quantityPolymorph  :: Quantity
                                     , _quantityTeamPlayer :: Quantity
                                     , _quantityLoneWolf   :: Quantity
                                     } deriving (Show)

$(makeLenses ''QuantityHolder)
-------------------------------

data Fighter = TeamPlayer
             | LoneWolf
             | Polymorph
    deriving (Show, Eq, Ord)
-------------------------------

data World = World {
    _quantities :: QuantityHolder
} deriving (Show)

$(makeLenses ''World)

incrementQuantity (Quantity q) = Quantity (q + 1)

updateWorld :: Maybe World -> Fighter -> Maybe World
updateWorld (Just w) f =
    case f of
        Polymorph ->
            Just $ q1 w
        TeamPlayer ->
            Just $ q2 w
        LoneWolf ->
            Just $ q3 w
    where q1 = over quantities . over quantityPolymorph $ incrementQuantity
          q2 = over quantities . over quantityTeamPlayer $ incrementQuantity
          q3 = over quantities . over quantityLoneWolf $ incrementQuantity

updateWorld of @Changaco can still be improved. I'll show it gradually so you can see what's going on.

Firstly, we can eliminate duplication in case statement and use lens composition to eliminate second over:

quantity Polymorph = quantityPolymorph
quantity TeamPlayer = quantityTeamPlayer
quantity LoneWolf = quantityLoneWolf

updateWorld :: Fighter -> World -> World
updateWorld fighter w = increment (quantity fighter) w where
        increment field = over (quantities . field) (+1)

Then we can go pointfree. Pointfree is all about readability so don't do it whenever you feel readability is harmed. With coding experience more and more code will become readable for you.

updateWorld = increment . quantity where
    increment field = over (quantities . field) (+1)

Then we replace over with a more specialized version of it from Control.Lens.Setter:

updateWorld = increment . quantity where
    increment field = (quantities . field) +~ 1

Now it seems beneficial to inline increment. I'll do in 2 steps. First, I make the actual parameter of increment explicit:

updateWorld field = increment $ quantity field where
    increment field = (quantities . field) +~ 1

Then I inline:

updateWorld :: Fighter -> World -> World
updateWorld field = (quantities . quantity field) +~ 1

Another way to improve could be to use a Map or an Array for QuantityHolder:

data World = World { _quantities :: M.Map Fighter Quantity }

Below is full source using Map:

{-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE GeneralizedNewtypeDeriving #-}

import Control.Lens
import System.Random
import qualified Data.Map as M
import Data.Maybe

newtype Quantity = Quantity Integer
    deriving (Eq,Num,Ord,Random,Show)

data Fighter = TeamPlayer
             | LoneWolf
             | Polymorph
               deriving (Eq,Ord,Show)

data World = World { _quantities :: M.Map Fighter Quantity }
    deriving (Show)
$(makeLenses ''World)

atFighter i = at i . iso fromJust Just

updateWorld :: Fighter -> World -> World
updateWorld field = (quantities . atFighter field) +~ 1

But it really depends on the usage scenarios you plan for World.

Also note that lens interact with state monad transformer pretty well, so you may consider

updateWorldS :: Fighter -> State World Quantity
updateWorldS field = (quantities . atFighter field) <+= 1

Note that World is not passed in explicitly any more. And the type is more general than that, so you can combine state with IO and other monads should you wish:

updateWorldS :: MonadState World m => Fighter -> m Quantity

If you want to be able to report any extra value from updateWorld you can use the existing state monad:

updateWorldS :: Fighter -> State World Bool
updateWorldS field = (quantities . atFighter field) <+= 1 >> return errorBool where
      errorBool = False -- or True

Or MonadPlus if you don't want to just report failure without extra info:

updateWorldS field = (quantities . atFighter field) <+= 1 >> when errorBool mzero where
      errorBool = False

Or MonadError if you want to supply an error value (of any type):

updateWorldS4 field = (quantities . atFighter field) <+= 1 >> when errorBool (throwError "Game over")


abort() implementation Here you go:

#define abort(msg) (fprintf(stderr, msg) && *((char*)0))

Non-standard interface to standard function

The obvious criticism of that implementation is that it has a different interface from what the C standard requires:

  §7.20.4.1 The abort function

  Synopsis

#include <stdlib.h>
void abort(void);

  Description

  The abort function causes abnormal program termination to occur, unless the signal
  SIGABRT is being caught and the signal handler does not return. Whether open streams
  with unwritten buffered data are flushed, open streams are closed, or temporary files are
  removed is implementation-defined. An implementation-defined form of the status
  unsuccessful termination is returned to the host environment by means of the function
  call raise(SIGABRT).

  Returns

  The abort function does not return to its caller.

Unreliable implementation of 'crash'

There were systems, notoriously the DEC VAX, where accessing the memory at address 0 did not cause problems (until the programs that were written on the VAX were ported to other platforms that did abort).

Dereferencing a null pointer is undefined behaviour - that means anything could happen, including 'no crash'.

Nitpicks in implementation

If, for some reason, fprintf() returns 0, your program will not abort.  For example:

abort("");

does not abort.  It is also dangerous to use the string as the format string; you should use:

#define abort(msg) (fprintf(stderr, "%s\n", msg) && *((char*)0))

It would be better to use a comma operator in place of the &&:

#define abort(msg) (fprintf(stderr, "%s\n", msg), raise(SIGABRT))

Since the standard library could have defined a macro abort(), you should #undef it before defining it yourself.


Given a date and a duration, calculate the future date The question is basically like this:
Given year, month, day and duration, calculate the future date based on given data and duration. The given date guarantees to be after 1900-01-01, and the duration k is between [0, 10000].

For example:
Input:
2016 10 1 100(meaning given 2016-10-1, what is the future date after 100 days)
Output:
2017-01-09

Another example:
Input:
2017 1 1 10
Output:
2017-01-11

I have done my solution, but it is trivial and ugly. The most frustrating part is line 44 - 47, where I have to handle special cases on month and year. I feel like this special and trivial handling is due to my design of calculating month, but I cannot figure out a more universal/clear solution except for current solution.

Love to hear your advice! Thank you!

Code with line indication and better format fyi : https://paste.ubuntu.com/24765263/

#include <iostream>
#include <string>
#include <unordered_map>
using namespace std; // I know global namespace is bad practice. Just to save time.

int getMonthDays(int y, int m){
    unordered_map <int, int >daysMap = {{1,31}, {2,28}, {3,31}, {4,30}, {5,31}, {6,30}, {7,31}, {8,31}, {9,30}, {10,31}, {11,30}, {0,31}};
    if (m == 2){
        if (y % 400 ==0 || (y % 100 != 0&& y % 4 == 0)){
            return 29;
        }
        else{
            return 28;
        }
    }
    else{
        return daysMap[m];
    }
}

int main (){

    int year, month, day, duration;
    cin >> year >> month >> day >> duration;

    int res_year;
    string res_month, res_day;

    int days = getMonthDays(year, month) - day;
    while ( days < duration ){
        month++;
        if(days + getMonthDays(year + month/12, (month - 12 * (month/13))% 12) > duration){
            break;
        }
        else{
            days += getMonthDays(year + month/12, (month - 12 * (month/13))% 12);
        }

    }

    res_year = year + month / 12 ;
    res_month = (month - 12 * (month/13))% 12 >= 10 ? to_string((month - 12 * (month/13))% 12) : "0" + to_string((month - 12 * (month/13))% 12);
    if (res_month == "00"){
        res_month = "12"; // dont have set month "12", so if get month as "00", convert it to "12"
        res_year -= 1; // if the result month is "12", year minus 1 coz Month December is still in this year.
    }

    if (duration - days > 0 ){
        if (duration - days >= 10){
            res_day = to_string(duration - days);
        }
        else{
            res_day = "0" + to_string(duration - days);
        }
    }
    else{
        res_day = to_string(day + duration);
    }
    cout << res_year << "-" << res_month << "-" << res_day;

}

This is quite tricky problem to get right. As Loki said, we need to convert it to seconds after epoch. But doing so is not trivial...
Standard approach:
Well, not really standard, but in my opinion one should use std::chrono in this case. Some stuff is done in a roundabout way, but I don't think it is a huge problem. Howard Hinnant has written a library for it, but in case the license isn't acceptable, I'll reimplement some functionality by hand.
output std::time_point:
The first issue is outputting a std::time_point. The problem with it is that it doesn't have operator<< overload for streams. The only way possible to deal with this is to implement our own:
Necessary links:

std::chrono::system_clock::to_time_t() (system clock is just example).

std::localtime()

std::put_time()

std::mktime()

The only ways to print time are either std::ctime() and std::put_time() (not counting the "implement by hand" approach). So let's follow the thread and find its end and test it:
#include <iostream>
#include <chrono>
#include <iomanip>

template <typename Clock, typename Duration>
std::ostream& operator<<(std::ostream& os, const std::chrono::time_point<Clock, Duration>& timep)
{
    auto converted_timep = Clock::to_time_t(timep);
    os << std::put_time(std::localtime(&converted_timep), "%Y %b %d %H:%M:%S");
    return os;
}

int main()
{
    auto now = std::chrono::system_clock::now();
    std::cout << now;

    return 0;
}

Output: 2017 Jun 04 12:07:48.
The only problem with implementation is that it can't be used from multiple threads, since std::localtime() returns pointer to static variable. The format could be mutable by some function, but I wouldn't care about it at this point.
Since we can manipulate std::chrono::time_point, and std::chrono::duration, let's use everything we have:
convert given date to std::chrono::time_point<system_clock>:
A good news, at last! From std::mktime():

Converts local calendar time to a time since epoch as a time_t object. time->tm_wday and time->tm_yday are ignored. The values in time are permitted to be outside their normal ranges.

Hooray! So we can just fill in the std::tm structure with year, month, and day since January 1 and we are done!
#include <iostream>
#include <chrono>
#include <iomanip>

template <typename Clock, typename Duration>
std::ostream& operator<<(std::ostream& os, const std::chrono::time_point<Clock, Duration>& timep)
{
    auto converted_timep = Clock::to_time_t(timep);
    os << std::put_time(std::localtime(&converted_timep), "%Y %b %d %H:%M:%S");
    return os;
}

auto convert_to_timepoint(int years, int months, int days)
{
    //perform checks, do division, modulus and stuff...
    years -= 1900; //epoch
    std::tm date = {};
    date.tm_year = years;
    date.tm_mon = months;
    date.tm_mday = days;

    return std::chrono::system_clock::from_time_t(std::mktime(&date));
}

int main()
{
    auto now = std::chrono::system_clock::now();
    std::cout << now << '\n';

    std::cout << "calling convert_to_timepoint:\n";
    auto date = convert_to_timepoint(1991, 0, 23);
    std::cout << date;

    return 0;
}

Output:
2017 Jun 04 13:08:49
calling convert_to_timepoint:
1991 Jan 23 00:00:00

Now the easy stuff, add the given number of days:
#include <iostream>
#include <chrono>
#include <iomanip>

template <typename Clock, typename Duration>
std::ostream& operator<<(std::ostream& os, const std::chrono::time_point<Clock, Duration>& timep)
{
    auto converted_timep = Clock::to_time_t(timep);
    os << std::put_time(std::localtime(&converted_timep), "%Y %b %d %H:%M:%S");
    return os;
}

auto convert_to_timepoint(int years, int months, int days)
{
    years -= 1900; //epoch
    std::tm date = {};
    date.tm_year = years;
    date.tm_mon = months;
    date.tm_mday = days;

    return std::chrono::system_clock::from_time_t(std::mktime(&date));
}

template <typename Clock, typename Duration>
auto add_days(const std::chrono::time_point<Clock, Duration>& timepoint, int days_to_add)
{
    constexpr std::time_t seconds_in_day = 60 * 60 * 24;
    //                                     mm   hh   dd

    std::time_t days = seconds_in_day * days_to_add;
    auto date = Clock::to_time_t(timepoint);
    return Clock::from_time_t(date + days);
}

int main()
{
    auto now = std::chrono::system_clock::now();
    std::cout << now << '\n';

    std::cout << "calling convert_to_timepoint with 1991 0 23 (1991 Jan 23):\n";
    auto date = convert_to_timepoint(1991, 0, 23);
    std::cout << date << '\n';

    std::cout << "10 day later (1991 Feb 02):\n" << add_days(date, 10);

    return 0;
}

Output:
2017 Jun 04 13:21:29
calling convert_to_timepoint with 1991 0 23 (1991 Jan 23):
1991 Jan 23 00:00:00
10 day later (1991 Feb 02):
1991 Feb 02 00:00:00

I left the convert_to_timepoint() so that it would be reusable on its own, but if needed it can be fused with add_days().
Some thoughts:

using namespace std; rarely buys anything, but opens a chance for errors.
standard library has a solution most of the time
modularity is important. Breaking functions into logical pieces makes it very easy to quickly prototype, adjust, mix'n'match.

Conclusion:
In this kind of problem, it is very dangerous try to implement by hand, since there are lots of edge cases. As a result, the problem given was rather a test of knowledge of standard library.


Find entry in hash depending on hash's data I wrote this code and I just can't stomach it.
The idea is that there is a schema, and the goal is to find the entry with autoIncrement set to true. If it's not found, the result should be idProperty.
const idProperty = 'id'
const schema = {
  id: { type: 'number' },
  code: { type: 'number', autoIncrement: true },
  email: { type: 'string', trim: 512 }
}

// Find the auto increment field
let autoIncrementSchemaFieldName = null
for (const name in schema) {
  const entry = schema[name]
  if (entry.autoIncrement) {
    autoIncrementSchemaFieldName = name
    break
  }
}
autoIncrementSchemaFieldName = autoIncrementSchemaFieldName || idProperty

console.log('RESULT:', autoIncrementSchemaFieldName)

Is this really the best way to do it?
(To test whether the idProperty fallback works, just rename autoIncrement into autoIncrement2 and run it again)

When you want to find a particular matching value in a data structure, using Array.prototype.find is usually the right way to go. It's complicated by the fact that the condition involves the value of the object, but you want the key of the object to be found, so to iterate over the keys and values at once, use Object.entries. It'll give you an array of [key, value] subarrays, and you can return the first item in the resulting subarray to get the key, if anything is found:

const idProperty = 'id';
const schema = {
  id: { type: 'number' },
  code: { type: 'number', autoIncrement: true },
  email: { type: 'string', trim: 512 }
};

const autoIncrementSchemaFieldName = Object.entries(schema)
  .find(([, value]) => value.autoIncrement)
  ?.[0] // Extract the key from the found entry
  ?? idProperty; // Otherwise, if no entry was found, use idProperty

console.log('RESULT:', autoIncrementSchemaFieldName)

That's what I'd prefer, but there are a few compatibility caveats:

Object.entries was introduced in 2017. It's not new, but it's not old either
I'm using optional chaining and the nullish coalescing operator for conciseness, which is only supported in environments built in 2020 and later

As always, the usual professional solution to compatibility problems is to use Babel to transpile syntax down to ES5 or ES6, and use polyfills to add new built-in objects and methods.
Without ES2020, it'd look like:

const idProperty = 'id';
const schema = {
  id: { type: 'number' },
  code: { type: 'number', autoIncrement: true },
  email: { type: 'string', trim: 512 }
};

const entry = Object.entries(schema)
  .find(([, value]) => value.autoIncrement);
const autoIncrementSchemaFieldName = entry
  ? entry[0]
  : idProperty;

console.log('RESULT:', autoIncrementSchemaFieldName)


Constructing a packet header I am currently working on writing a library for use with LIFX light bulbs over their UDP API. I have written the code for creating the packet headers, but I'm unsure whether it is the most idiomatic approach, as I am new to Clojure. Please tell me what you think about the following code.

(ns lifx-lib.packet.header
  (:require [clj-struct.core :as struct]))

(def protocol-num 1024)
(def header-size 36)

(defn- frame-header
  [size tagged source]
  (struct/pack
    "<2HI"
    size
    (bit-or protocol-num 0x1000 (if tagged 0x3000 0x0000))
    source))

(defn- address-header
  [address resp ack sequence]
  (struct/pack
    "<2Q"
    address
    (bit-or
      (if resp (bit-shift-left 1 48) 0)
      (if ack (bit-shift-left 1 49) 0)
      (bit-shift-left sequence 56))))

(defn- protocol-header
  [type]
  (struct/pack "<Q2H" 0 type 0))

(defn create-header
  [params]
  (let [
      size     (:size params 0)
      tagged   (:tagged params true)
      source   (:source params 0)
      address  (:address params 0)
      resp     (:resp params false)
      ack      (:ack params false)
      type     (:type params 0)
      sequence (:sequence params 0)]
    (into []
      (concat
        (frame-header size tagged source)
        (address-header address resp ack sequence)
        (protocol-header type)))))

Looks pretty good to me.  There are a few shortcuts you could add if you don't mind using the Tupelo Clojure library.  One of them is vals->map
and with-map-vals.

Called with a list of symbols like (vals->map a b c) returns a map like {:a a :b b :c c}:

(let [a 1
      b 2
      c 3]
  (vals->map a b c))  ;=>  {:a 1 :b 2 :c 3} }

Given a map like {:a 1 :b 2 :c 3} (such as generated by (vals->map a b c)), performs safe let destructuring using grab like:

   (let [some-map  {:a 1 :b 2 :c 3} } ]
     (with-map-vals some-map [a b c]
        (+ a b c)))  ;=>  6

with-map-vals is safe for typos since grab will throw if the requested key is not present. See vals->map for simple creation of labeled data maps.

For default values, I think the simplest way is like this:

  (let [defaults {:a 1 :dir :up :name "joe"}
        v1       {:a 2}
        v2       {:a 9 :dir :left :name "sally"}]
    (spyx (into defaults v1))
    (spyx (glue defaults v2))

There is also the pure clojure way:

; Pure clojure way
(let [{:keys [a dir name ] } v1]
  (spyx :pure-1 (vals->map a dir name)) )

; can't use `defaults` here, need map with keyword => symbol
(let [{:keys [a dir name] :or {a 1 dir :up name "joe"}} v1]
  (spyx :pure-2 (vals->map a dir name)))

(let [{:keys [a dir name ] :or {a 1 dir :up name "joe"}} v2]
  (spyx :pure-1 (vals->map a dir name)) )

See also Clojure Destructuring.

Lastly, instead of concat I like to use glue.

; Glue together like collections:
(is (= (glue [ 1 2] '(3 4) [ 5 6] )       [ 1 2 3 4 5 6 ]  ))   ; all sequential (vectors & lists)
(is (= (glue {:a 1} {:b 2} {:c 3} )       {:a 1 :c 3 :b 2} ))   ; all maps
(is (= (glue #{1 2} #{3 4} #{6 5} )      #{ 1 2 6 5 3 4 }  ))   ; all sets
(is (= (glue "I" " like " \a " nap!" )   "I like a nap!"   ))   ; all text (strings & chars)

; If you want to convert to a sorted set or map, just put an empty one first:
(is (= (glue (sorted-map) {:a 1} {:b 2} {:c 3})   {:a 1 :b 2 :c 3} ))
(is (= (glue (sorted-set) #{1 2} #{3 4} #{6 5})  #{ 1 2 3 4 5 6  } ))

Lastly, I wouldn't bother with defn-  It just makes testing harder and provides little value.  If you have non-public functions that you don't want in the documentation (via the lein-codox plugin), you can add metadata to the function like:

(defn ^:no-doc my-helper-fn
  ...)


Attempt at the Most dynamic FizzBuzz possible in Java I was trying to come up with the most dynamic fizzbuzz answer I could.. and well, this is what I came up with:

public static void main(String[] args) {

    ultraFizzBuzz((Integer arg) -> {
        String retVal = "";
        if(arg % 3 == 0) { retVal += "fizz"; }
        if(arg % 5 == 0) { retVal += "buzz"; }
        return retVal;
    });
}

public static void ultraFizzBuzz(modConditions<Integer> tester) {

    for(int i = 1; i < 100; i++) {
        String result = tester.test(i);
        if(result != "")
            System.out.println(tester.test(i));
        else
            System.out.println(i);
    }
}

In here the modConditions is an interface with a method (test) that returns type String.

What I was basically trying to do is create a super dynamic function that lets you add as many fizzes and buzzes and pops as you wanted.. but I think in the process accidentally made it so you end up writing 90% of the code in a lambda statement everytime you want to call it.

Am I overcomplicating things? Would this be considered bad programming practice?

The most important problem I have with your code is, that it totally fails to achieve the goal you set yourself. This is not dynamic at all. Basically, you take the "normal" method body and put it into a lambda, but this lambda now is a complex function, which contains all of the business logic.

Dynamically, you'd need a collection (probably list for being ordered) of lambdas, which each check a single input value and return an appropriate string representation (i.e. "fizz" or "buzz" or "whatever" or the empty string) and a method to apply this complete list on a single input, accumulate the results and perform the final check whether to output that string or the integer itself.

Thus, start out with something like:

Function<Integer, String> fizz = i -> i % 3 == 0 ? "fizz" : "";
Function<Integer, String> buzz = i -> i % 5 == 0 ? "buzz" : "";
Function<Integer, String> barz = i -> i % 7 == 0 ? "barz" : "";
List<Function<Integer, String>> fizzBuzzList = Arrays.asList(fizz, buzz, barz);

And try to come up with code that operates on such a list. Note: Function is the standard function type with an integer as an input and a string as the output. No need to define your own interface.

If you have achieved this, you can easily add another function to the list to check another value, and then you have achived something that might be considered dynamic.


Simple definite integral calculator I made this code to compute definite integrals in Processing. It works by getting the rectangle between the maximum value between the previous function value and the current function value, and then I calculate the area by multiplying the maximum value described before and the step. And finally, the integral is just summing all the areas.

I will do a version based on trapezoids, but that could get really slow because I would need to use the square root function witch is computationally intensive to compute.

Any suggestions are welcome.

float step = 0.0001;
int iterations = 0;

// y = x
float f1(float x) {
  return 2 * x;
}

float integral(int a, int b) {
  float sum = 0.0;
  float prevI = a;

  for (float i = a; i < b; i += step) {
    float h = max(f1(prevI), f1(i));
    sum += step * h;

    prevI = i;
    iterations++;
  }
  return sum;
}

void setup() {
  println(integral(0, 1));
  println(iterations);
}

Using the max of two consecutive values is asymmetric.
(I'd simply use the value sampled, avoids the following issue, too:)
For most every "sample" (i), the code computes the value of f() twice (f(prevI)): keep prevF instead
a double (64 bits in Processing, as in Java) may be a saner type designator than float (32 bits),
especially for "the accumulator" sum.  When the number of summands approaches the number representable by its mantissa, you need a more sophisticated strategy than adding in numbers one by one.
the code multiplies each and every h by step:
don't, instead, multiply sum by step after the loop
(similarly, don't count trips/iterations, compute as (b-a)/step)
consider making step a parameter
(My pet idea would be setting a time limit and adding samples until time is up.
Depending on sampling strategy, keeping track of weights (width of the trapezoids, step) is a challenge.)


Optimizing the Sieve of Eratosthenes for prime generation I've been on a Project Euler spree and I've been solving problems and as you might know, many of them require you to use prime numbers. I've been using the following code to generate and store primes in a vector, but was wondering if there is any non-obvious speed-ups that will make a large difference on the runtime of the generation.

// Generate the primes upto N using the Sieve of Eratosthenes.
template<typename T>
std::vector<T> generatePrimes(T n) {
    std::vector<T> result = std::vector<T>();
    if(n < 2) {
        return result;
    }
    std::vector<int> input(n + 1, 1);
    // Calculates the upper limit of the numbers to check.
    T sqrtN = (T)sqrt(n);
    // Iterate till the square root.
    for(T i = 2; i <= sqrtN; i++) {
        if(!input[i]) {
            continue;
        }
        // Multiples are marked false.
        for(T j = i * i; j <= n; j += i) {
            input[j] = 0;
        }
    }
    // Add to result vector.
    result.push_back(2);
    for(T i = 3; i <= n; i += 2) {
        if(input[i]) {
            result.push_back(i);
        }
    }
    return result;
}

And it is called like so:

vector<unsigned int> primes = generatePrimes<unsigned int>(10'000);

Is there any other way to speed this up? Before I had changed a line where I create the input vector to int from bool and it resulted in a nice 2.3x speed increase. See here for benchmark.

A few things to consider:

Is having the function generic really necessary? Why not just make the data type unsigned long long instead?  This way you can do some bounds checking.

I would suggest using a bitset to hold the raw data would be more efficient than using 32 bits to represent each 1 or 0.

All primes are odd except for 2.  If you add 2 to the result vector, you can start the outer loop at 3 and increment by 2.

Since the outer loop will only be odd numbers the start of the inner loop will also be odd.  Therefore in order to keep hitting the odd number multiples you can increment by i*2.

You can shorten the second loop by adding to the vector every time you find a prime in the outer loop.  This way the second loop can start where the outer loop finishes.

There is also another algorithm you can use that is more highly optimised for making a list of primes, mind you it's also more complicated( sorry I can't remember the name off hand)


Python tool to assemble CodeReview posts from source files I hate having to format questions around here. Adding four spaces to each source file and the correct name at the top of each source file becomes a headache when I have to post longer questions.

This is a simple python question parser that takes in arguments from the console and prints out correct format for a CodeReview question. Any suggestions on how to make this code more readable, concise and pythonic would be appreciated.

Question Parser.py

import os
import sys
import argparse

def format_string(PATH,EXTENSION):
    source_files=[f for f in os.listdir(PATH) if os.path.isfile(os.path.join(PATH,f))]
    source_files=[f for f in source_files if f.endswith(EXTENSION)]

    indentation=4
    formatted_string=""
    for source_file in source_files:
        formatted_string+= ("**%s**\n\n" % source_file)
        with open(os.path.join(PATH,source_file)) as source:
            for line in source.readlines():
                formatted_string+=(" "*indentation+"".join(line))

    return formatted_string

if __name__=="__main__":
    parser=argparse.ArgumentParser(description="Automatic formatter for CodeReview Stackexchange Questions")
    parser.add_argument("-p","--path",help="Path for the source files. Default is the current directory",nargs="?",default=os.getcwd(),const=os.getcwd())
    parser.add_argument("header",help="The file that is added to the top of the question")
    parser.add_argument("-e","--extension",help="Filter by the type of source files that is present in the directory",nargs="+",default="")
    parser.add_argument("-o","--out",help="Output the result to a file. By default outputs to the console.",nargs="?")
    args=parser.parse_args()

    if not os.path.isfile(args.header):
        raise parser.error("Header file doesnot exist.\nPlease specify a correct header file path")

    if os.path.exists(args.path):
        question="".join(open(args.header).readlines())
        question+="\n"+format_string(args.path,tuple(args.extension,))
        if not (args.out==None):
            with open(args.out,"w") as outfile:
                outfile.write(question)
        else:
            print(question)

    else:
        raise parser.error("Path doesnot exist.")

PEP 8

I would review PEP 8. Some of your style choices are not considered best practice by the Python community. Some quick observations:

Constants are given CAPS_WITH_UNDERSCORES. Not function/method parameters. PATH should be path for instance.
There is a space between the binary operators (there are, however, some exceptions: see Other Recommendations). So, for instance: indentation=4 should be indentation = 4.
Instead of if not (args.out==None):, You should do if args.out is not None: (See Programming Recommendations)
I would advise making a main function. Yes if __name__ == '__main__': is a start but I would recommend having a main function (see this answer, in particular the section "An Even Better Way"):

--

def main():
    parser=argparse.ArgumentParser(...)
    ...

if __name__ == '__main__':
    main()


Circular array rotation Java I have a solution to the HackerRank Circular Array Rotation challenge.  It passes 7 test cases out of 15. The rest of them are getting timed out. I think it's because of the huge data set that has been given as the input.

  Input Format

  The first line contains 3 space-separated integers, n (the length of the array), k (the number of right circular rotations), and q (the number of queries).
  The second line contains n space-separated integers a0, a1, a2, …, an-1.
  Each of the q subsequent lines contains a single integer denoting m.  For each of those queries, output am of the rotated array.

  Constraints

  1 ≤ n ≤ 105
  1 ≤ ai ≤ 105
  1 ≤ k ≤ 105
  1 ≤ q ≤ 500

Can you point me out how can I improve this code in order to avoid those time out of test cases?

public class CircularArrayRotation {

    public static int[] circularArray(int[] beforeArray){
            int[] afterArray = new int[beforeArray.length];
            afterArray[0] = beforeArray[beforeArray.length-1];
            System.arraycopy(beforeArray,0,afterArray,1,beforeArray.length-1);
            return afterArray;
    }

    public static void main(String[] args){
        Scanner sc = new Scanner(System.in);

        int n = sc.nextInt();
        int k = sc.nextInt();
        int q = sc.nextInt();
        sc.nextLine();
        int[] source = new int[n];
        String[] elements = sc.nextLine().split(" ");
        for (int i=0;i<elements.length;i++){
            source[i] = Integer.parseInt(elements[i]);
        }
        source = repeatCirculating(source,k);
        int[] ques = new int[q];
        for (int i=0;i<q;i++){
            int position = Integer.parseInt(sc.nextLine().trim());
            ques[i] = position;
        }
        for (int ask:ques) {
            System.out.println(source[ask]);
        }

    }

    public static int[] repeatCirculating(int[] source, int times){
        for (int i =0; i<times; i++){
            source = circularArray(source);
        }
        return  source;
    }
}

The array may be up to 105 elements long.  If you actually perform the rotation, then you will be copying at least 105 elements.  (You actually do ridiculously more work, as @JoeC and @OhadR have both pointed out.)

However, there will be at most 500 queries.  It would be nice if you didn't have to modify 105 entries just to satisfy 500 queries.  You don't actually need to perform the rotation — you only need to pretend to have performed the rotation.

import java.util.Scanner;

public class CircularArrayRotation {
    /**
     * Performs one query.
     *
     * @param a  The original array
     * @param k  The number of right circular rotations
     * @param m  The index of the rotated array to retrieve
     */
    public static <T> T query(T[] a, int k, int m) {
        int n = a.length;
        return a[(((m - k) % n) + n) % n];
    }

    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);

        int n = sc.nextInt(), k = sc.nextInt(), q = sc.nextInt();
        sc.nextLine();                          // End of first line

        String[] a = sc.nextLine().split(" ");  // Second line

        while (q-- > 0) {                       // Perform q queries
            System.out.println(query(a, k, sc.nextInt()));
        }
    }
}

Strictly speaking, you don't even need to parse the ai as integers — you just need to read and regurgitate them.  You also don't need to store all q queries — you can just reply to each one as soon as you read m.


An assignment algorithm in C As a post processing step of a similarity matrix computation that leads to the non-negative matrix mt4_ in gpu, I am performing an assignment step to determine which element in rows/columns is most similar to which element in columns/rows. So this is more like a first-come-first-serve algorithm in which the largest similarity is picked up and then that particular row and column is excluded (by flagging the first element of that row and column --using -1.0f) and the process continues as such until all rows/columns are exhausted. The results are exported to two contiguous sub-arrays of sms one for each row-to-column and column-to-row assignment. Here is the function in which sZ is a const unsigned int equal to 5000 (mt4_, and by extension mt2, are of size sZ x sZ):

void asn(float *mt2, float *mt4_, unsigned int *sms) {

  unsigned int i, j;
  unsigned int a, b;
  register unsigned long k;
  register float mx;
  register unsigned int mm = 0, nn = 0;

  for (j=0; j<2; j++) {
    k = j*sZ;
    cudaMemcpy(mt2, mt4_, mtb, cudaMemcpyDeviceToHost);
    for (i=0; i<sZ; i++) {
      mx = -1.0f;
      for (a=0; a<sZ; a++) {
        if (mt2[a*sZ] != -1.0f)
          for (b=0; b<sZ; b++) {
            if (mt2[a*sZ+b] > mx && mt2[b] != -1.0f) {
              mx = mt2[a*sZ+b];
              mm = a;
              nn = b;
            }
          }
      }
      if (fabsf(mt2[mm*sZ+mm] - mt2[mm*sZ+nn]) <= thd)
        nn = mm;
      sms[k+mm] = nn+1;
      mt2[mm*sZ] = mt2[nn] = -1.0f;
    }
  }
}

Here thd is the const float threshold that introduces a bias towards assigning a column index to its identical row index if their similarity is a within a tolerance thd of the best available score (this is to compensate for numerical errors).

The code runs fine but my problem is that it takes too long (about 100 seconds on my compute node), and this is something I can't afford due to the large number of calls to this function.

I am new to C and it is very well probable that I am missing straightforward performance optimizations, so I would really appreciate if you would comment in case you see a window for improvement.

Background: Blondel et al., A measure of similarity between graph vertices

All of what I'm suggesting here will be done by a good optimizing compiler. Some of what I'm suggesting is computer architecture dependent so you should depend on the compiler rather than doing it if you can. Generate assembly code and look at it to better optimize.

First, register assignment isn't guaranteed, it is only a recommendation to the compiler. You want to move your register variable declarations up. The compiler ignores your register allocations when it runs out of registers.

void asn(float *mt2, float *mt4_, unsigned int *sms) {
  register float mx;
  register unsigned int mm = 0, nn = 0;
  register unsigned int a, b;
  unsigned int i, j;
  unsigned long k;

You want to base your register allocations on what is being used most, since k is only used twice it shouldn't be a register. The variables mm, nn, a and b are used the most (sZ * sZ * sZ) so they should be registers.

On many computers counting down (auto decrement) is faster than counting up because decrement and test for zero is one opcode:

    for (i=sZ; --i; ) {
      mx = -1.0f;
      for (a=sZ; --a; ) {
        if (mt2[a*sZ] != -1.0f)
          for (b=sZ; --b; ) {
            if (mt2[a*sZ+b] > mx && mt2[b] != -1.0f) {
              mx = mt2[a*sZ+b];
              mm = a;
              nn = b;
            }
          }
      }

This does impact readability and maintainability so it should not be used unless the optimizing compiler doesn't do it for you.

Testing for equivalency of floating points is to be discouraged because of rounding errors (floating point errors).

Variable names and constants should be longer and clearer, sZ => MatrixSize.


UTF8 Codepoint decode and length I needed a function that would:

Decode, and return, the first character in an UTF8 encoded strings
Return the length of encoding with the special case that lenght of '\0' must be 0
Perfomance are important

I had no special requirement on what to do with invalid sequences so I opted for the following behaviour:

The first byte of an invalid sequences is considered as a single "character" (e.g. for the sequence "\xFF\x2F" it would return '\xFF' as value and 1 as length).
Overlong encoding are accepted

I wrote the following function:

static uint8_t LEN[] = {1,1,1,1,2,2,3,0};
static uint8_t MSK[] = {0,0,3,4,5,0,0,0};

static int utf8_cp(char *txt, int32_t *ch)
{
  int len = 0;
  int32_t val = 0;
  uint8_t first = (uint8_t)(*txt);

  len = (first > 0) * (1 + ((first & 0xC0) == 0xC0) * LEN[(first >> 3) & 7]);
  val = first & (0xFF >> MSK[len]);

  for (int k=len; k>1; k--) {
    if ((*++txt & 0xC0) != 0x80) {
      val = first;
      len = 1;
      break;
    }
    val = (val << 6) | (*txt & 0x3F);
  }

  *ch = val;
  return len;
}

So that a code like:

char *t; int l; int32_t c;

t = "aàも";
while(1) {
  l = utf8_cp(t, &c);
  printf("'%s' len:%d cp:0x%05x\n", t, l, c);
  if (*t == 0) break;
  t += l;
}

Produces:

'aàも' len:1 cp:0x00061
'àも' len:2 cp:0x000e0
'も' len:3 cp:0x03082
'' len:4 cp:0x2b014
'' len:0 cp:0x00000

To make it faster I thought about unrolling the for loop (but I wonder how much could I gain) and introducing, at the beginning, some if to handle ASCII character (but I fear that branching could be more costly that just making a bunch of operation).

I will appreciate any comment you may have and any suggestion for improvement.

Rather than use narrow types, use fastest ones

// uint8_t first = (uint8_t)(*txt);
unsigned first = (uint8_t)(*txt);
// or
uint_fast8_t first = (uint8_t)(*txt);

Rather than lookup a value to shift, look up the shifted value.

// static uint8_t MSK[] = {0,0,3,4,5,0,0,0};
//  val = first & (0xFF >> MSK[len]);

static const uint8_t FF_MSK[] = {0xFF >>0, 0xFF >>0, 0xFF >>3,
    0xFF >>4, 0xFF >>5, 0xFF >>0, 0xFF >>0, 0xFF >>0};
val = first & FF_MSK[len];

Some modern compilers can make additional optimizations if the pointers are known to not overlap - use restrict and const where applicable.

// int utf8_cp(char *txt, int32_t *ch)
int utf8_cp(const char * restrict txt, int32_t *restrict ch)

Coding the companion function would aid in testing for both functions.

int utf8_cp_encode(int32_t *ch, char *txt);

As code does not detect invalid encoding like surrogates, redundant patterns and values above max_Unicode, I see little value in handling only a subset of invalid sequences.  Either detect them all (maybe in debug mode)  or skip detection.

Suggest doing a 32-byte (or 256-byte) lookup for performance.  Profile to find optimal.

// len = (first > 0) * (1 + ((first & 0xC0) == 0xC0) * LEN[(first >> 3) & 7]);
len = (first > 0) * LEN_32[first >> 3];
// or
len = LEN_256[first];

Could extend the above to do one lookup for both the len and val.