Optimizing NGRX reducer I am trying to optimize my reducer code, preferably into one-liner with high-order-functions.

Reducer should return new array with names of groups, with new number.

Below are correct inputs and outputs:

Input:        |          Output:
              |
[]            |          ['A1']
['A1']        |          ['A1', 'A2']
['A1', 'A3']  |          ['A1', 'A2', 'A3']
['WORD', 'A2']|          ['WORD', 'A2', 'A1']

Code below is part of switch-case block:

case DrivesPageActions.DrivesPageActionTypes.AddCustomGroup:
  if (state.customGroups.includes('A1') === false) {
    return {
      ...state,
      customGroups: [...state.customGroups, 'A1']
    };
  } else if (state.customGroups.includes('A2') === false) {
    return {
      ...state,
      customGroups: [...state.customGroups, 'A2']
    };
  } else {
    return {
      ...state,
      customGroups: [...state.customGroups, 'A3']
    };
  }

preferably into one-liner with high-order-functions.

Machines execute code the same way, minified or not. But a human doesn't read or understand minified code the same way as verbose JS. Write for a human, don't one-line code.

Now one thing about your code. You do NOT compare booleans. if statements evaluate booleans. If your expression is already a boolean, then just use that boolean. If you need to negate it, use !.

case DrivesPageActions.DrivesPageActionTypes.AddCustomGroup:
  if (!state.customGroups.includes('A1')) {
    return {
      ...state,
      customGroups: [...state.customGroups, 'A1']
    };
  } else if (!state.customGroups.includes('A2')) {
    return {
      ...state,
      customGroups: [...state.customGroups, 'A2']
    };
  } else {
    return {
      ...state,
      customGroups: [...state.customGroups, 'A3']
    };
  }

Looks like your code is looking for A1, A2 and A3 in that order and adding the first one that's not found. You could just create an array of the three, then filter it by the array in the state, removing the ones already present. The first item in the filtered array is the item you want.

const inserts = ['A1', 'A2', 'A3']
const defaultInsert = inserts[inserts.length - 1]

function reducer(state, action){
  switch (action) {
    case 'AddCustomGroup':
      const missing = inserts.filter(v => !state.customGroups.includes(v))
      const insert = missing[0] || defaultInsert
      return { ...state, customGroups: [...state.customGroups, insert] }
    default:
      return state
  }
}

console.log(reducer({ customGroups: [] }, 'AddCustomGroup'))
console.log(reducer({ customGroups: ['A1'] }, 'AddCustomGroup'))
console.log(reducer({ customGroups: ['A1', 'A3'] }, 'AddCustomGroup'))
console.log(reducer({ customGroups: ['WORD', 'A2'] }, 'AddCustomGroup'))


Texas Holdem Poker Game I'm making a Texas Hold'em game for multiple players. So far, I have finished the flow control for betting / calling sequences, player stack / pot updating, and just now - the part responsible for drawing cards and distributing them. It is this last part that I would like reviewed, if possible.

Essentially, I wanted to know what I could do differently / more efficiently, suggested changes, really just anything that could be helpful. Things like checks for proper input from player etc are not yet in place so please ignore them.

import itertools
import random

player_count = int(input("How many players? "))

suits = ['s', 'c', 'd', 'h']
faces = ['A', '2', '3', '4', '5', '6', '7', '8', '9', 'T', 'J', 'Q', 'K']

# Defining deck and drawing cards (5 + 2 per player):

deck = set(itertools.product(faces, suits))
drawn_cards = random.sample(deck,(5 + 2 * player_count))

# Storing a list with player object instances

players = []

class Players:

    def __init__(self, stack):

        self.stack = stack
        self.last_action = "none"  ## last action taken (check, call etc).
        self.street_bets = 0       ## total bets on current street

    def holecards(self):

## pop cards from list and pritifying their display. Cards will later be
## run through a function for checking value of hands:

        card1 = drawn_cards.pop(len(drawn_cards)-1)
        card2 = drawn_cards.pop(len(drawn_cards)-1)
        self.card1 = (card1[0]+"["+card1[1]+"]")
        self.card2 = (card2[0]+"["+card2[1]+"]")
        self.cards = self.card1+ " - " +self.card2
        print (self.cards)

class Table:

    def __init__(self):

        self.total_pot = 0

    def flop(self):

        card1 = drawn_cards.pop(len(drawn_cards)-1)
        card2 = drawn_cards.pop(len(drawn_cards)-1)
        card2 = drawn_cards.pop(len(drawn_cards)-1)
        self.card1 = (card1[0]+"["+card1[1]+"]")
        self.card2 = (card2[0]+"["+card2[1]+"]")
        self.card3 = (card2[0]+"["+card2[1]+"]")
        self.flop = self.card1+ " - " + self.card2 + " - " + self.card3
        print (self.flop)

    def turn(self):

        card = drawn_cards.pop(len(drawn_cards)-1)
        self.card = (card[0]+"["+card[1]+"]")
        self.turn = self.card
        print (self.turn)

    def river(self):

        card = drawn_cards.pop(len(drawn_cards)-1)
        self.card = (card[0]+"["+card[1]+"]")
        self.river = self.card
        print (self.river)

# Add player stacks and distribute holecards:

for i in range(player_count):
    players.append(i)
    players[i] = Players(100)
    players[i].holecards()

# Add dealer to table:

dealer = Table()

dealer.flop()
dealer.turn()
dealer.river()

Reasonable quality code. Here are a few comments:

1) I'd add a method drawNextCard, which encapsulates retrieval of the next card, and makes the code a bit more readable

def draw_next_card(drawn_cards):
    card = drawn_cards.pop(len(drawn_cards)-1)
    return readable_card_name(card)

def readable_card_name(short_card_name):
    return (short_card_name[0]+"["+short_card_name[1]+"]")

Then, for example, your flop() method would look like this:

def flop(self):
    (self.card1, self.card2, self.card3) = (draw_next_card(drawn_cards), draw_next_card(drawn_cards), draw_next_card(drawn_cards))
     self.flop = self.card1+ " - " + self.card2 + " - " + self.card3
     print (self.flop)

More readable, shorter, and on the way we fixed a copy/paste defect where you initialized card2 twice instead of card2 and card3.

2) You convert each drawn card to a readable representation. It's good for your debugging, but not so good for adding future functionality for actually calculating poker hands, and developing strategy. So, while my draw_next_card above returns a readable representation, you're better off creating a class Card, which has an __str__ method that has the same implementation as readable_class_name. The flop method would remain the same, but it will be much easier to extend the code.

3) Note that flop and river are copy/pastes. Both could just call draw_next_card. Also, while the flow and river are true to the game terms, they're not much benefit to software; you'd be better off collapsing flop, turn and river to a table_cards[5] vector. Once you start developing the actual game, it will make things much easier for you.

Hope this helps.


Refactoring large switch statement Lately I have been trying to extract till I drop. Well not necessarily till I drop, but I've been trying to be more strict and look at some metrics of my code.

I have now come along an old class of mine which has a rather large switch case. Metrics say the method lines of code are beyond evil. It goes something like this:

private void startProcessing(Map<MyKeyEnum, String> map) {
    Processor myProcessor = new Processor();
    for (Entry entry : map.entrySet()) {
        switch(entry.getKey()) {
            case KEY1:
                myProcessor.processStuffAboutKey1(entry.getValue());
                break;
            case KEY2:
                myProcessor.processStuffAboutKey2(entry.getValue());
                break;
            case KEY3:
                myProcessor.processStuffAboutKey3(entry.getValue());
                break;
            case KEY4:
                myProcessor.processStuffAboutKey4(entry.getValue());
                break;
            ...
            ...
        }
    }
}

So basically you can gather that it is necessary for me to invoke very different things for each key IF it is in the map. I have therefore already created the Processor class and mind you this is already the absolute shortest and most compact that I was able to come up with. Basically this method does already do only one thing.

But isn't it possible to build this differently so that it is shorter? I currently have 25+ cases to handle

Edit: made clear what Key,Value-Pair is and what they're used for

You can create a KeyProcessor interface and implementation for every key (move the body of the current Processor.processStuffAboutKeyX() methods to these classes):

public interface KeyProcessor {
    void processStuff(String data);
}

Then fill a map with the available implementations:

Map<Key, KeyProcessor> processors = new HashMap<>();
processors.add(key1, new Key1Processor());
...
processors.add(key4, new Key2Processor());

And use that map in the loop:

for (Entry<Key, String> entry: map.entrySet()) {
    Key key = entry.getKey();
    KeyProcessor keyProcessor = processors.get(key);
    if (keyProcessor == null) {
        throw new IllegalStateException("Unknown processor for key " + key);
    }
    final String value = entry.getValue();
    keyProcessor.processStuff(value);
}

With the original 25+ cases the switch-case method is at least 75 lines long (25 case statement, 25 break statement, and at least 1-1 method call line for every case). The separate classes reduces the size and complexity which a developer see at once (+no need to scroll), the code is easier to maintain and test because you can test every case in isolation.

I think it's a lot more easier to have (and handle) a couple of small classes and corresponding *Test classes (with a few test methods for every class) than having a big class (25+ cases) and a lot of tests in one corresponding *Test class or a lot of *Test classes which test separate case branches of the same startProcessing method.

I admit that these arguments might be weak ones but without an actual implementation of the Processor class and the code which creates the input map is hard to say more.

Further readings:

Refactoring: Improving the Design of Existing Code by Martin Fowler: Replacing the Conditional Logic on Price Code with Polymorphism
Replace Conditional with Polymorphism


Re-write custom feature encoding function python I have a question regarding code quality and best practices. My task - to write a feature encoding function that will encode categorical labels, boolean labels as one-hot encoding, timestamps for further usage for ml training.

Input - dictionary of several dataframes, each dataframe consist of several columns of different types.

The function should return a dictionary of correctly encoded dataframes and a dictionary of label encoders for categorical columns.

Here is what I did:

# Encode bool values to one hot encoding, change NaN numerical values to single const value, make timestamp be time columns, add categorical encoding
def df_encoder(data_dict) :
    #encode all NA values of continuos data as a constant
    NA_values = 0.001
    # dictionary to save dictionary of label encodings from LabelEncoder
    labels_codes = dict()

    for names_df in data_dict:
        # list, where to save label encodings from LabelEncoder from one dataframe
        labels_codes[names_df] = list()

        #take iteratively dataframe from the dictionary of dataframes
        df_additional = data_dict[names_df]

        for col in df_additional:

            if is_bool_dtype(df_additional[col]):
                loc_col = df_additional.columns.get_loc(col)
                df_additional_one_hot = pd.get_dummies(df_additional[col], prefix=col, dummy_na=True)
                df_additional = pd.concat([df_additional.iloc[:, :loc_col], df_additional_one_hot, df_additional.iloc[:, loc_col:]], axis=1).drop(col, axis=1)

            elif is_numeric_dtype(df_additional[col]):
                df_additional[col].fillna(NA_values)

            elif is_datetime64_any_dtype(df_additional[col]):
                loc_col = df_additional.columns.get_loc(col)
                date_df = pd.DataFrame()
                date_df[col+'_year'] = df_additional[col].dt.year.fillna(0)
                date_df[col+'_month'] = df_additional[col].dt.month.fillna(0)
                date_df[col+'_day'] = df_additional[col].dt.day.fillna(0)
                date_df[col+'_hour'] = df_additional[col].dt.hour.fillna(25)
                date_df[col+'_minute'] = df_additional[col].dt.minute.fillna(60)
                date_df[col+'_seconds'] = df_additional[col].dt.second.fillna(60)
                df_additional = pd.concat([df_additional.iloc[:, :loc_col], date_df, df_additional.iloc[:, loc_col:]], axis=1).drop(col, axis=1)

            elif is_categorical_dtype(df_additional[col]) and df_additional[col].nunique()== 2:
                loc_col = df_additional.columns.get_loc(col)
                df_additional_two_val_categ = pd.get_dummies(df_additional[col], prefix=col, dummy_na=True)
                df_additional = pd.concat([df_additional.iloc[:, :loc_col], df_additional_two_val_categ, df_additional.iloc[:, loc_col:]], axis=1).drop(col, axis=1)

            elif is_categorical_dtype(df_additional[col]) and df_additional[col].nunique()>2:
                #keep only alphanumeric and space, and ignore non-ASCII
                df_additional[col].replace(regex=True,inplace=True,to_replace=r'[^A-Za-z0-9 ]+',value=r'')
                label_enc = LabelEncoder()
                df_additional[col] = label_enc.fit_transform(df_additional[col].astype(str))
                labels_codes[names_df].append({col: label_enc})

            data_dict[names_df] = df_additional
    return data_dict, labels_codes

The functions work well, but I'm not happy with its quality. I need some useful advice or examples of how to make this function more efficient, and more "best-coding practises" alike. Will appreciate any insights and critique.

Just have the time to take a quick view on your code, but one of the first thing you have to learn to do (before using design pattern and such things), is to refactor your code.

By simply scrolling, we can see df_additional[col] more than ten times. For all the places where you just need to read this value, use a local variable to store it. If you need to write it, in that case leave it.

There is also df_additional = pd.concat([df_additional.iloc[:, :loc_col], df_additional_one_hot, df_additional.iloc[:, loc_col:]], axis=1).drop(col, axis=1) a couple of time, with just a local variable used as a parameter which changes between those calls. Maybe you can also refactor that?

It's also worth noting that if your if (or whatever loop or control statement) begins to be long (dozen of lines), and even more when you have several if like that, think about isolating each block in a separate function. Or at least all the complete if/elif block in a function.


E-commerce product price tracker I building a very simple price tracker web app. I am using MongoDB with pymongo. The user will enter the URL of the product he wishes to track and the desired amount, when the price goes below this amount, he should get an alert. It doesn't matter how many products he is tracking, I will send the alert when price changes in one product. I will sending generalized alert like 'Some of the product/s available at cheaper price' or something like that.

There are three collections in the database, one to store product details, one for user details and one to map products to the users who are tracking it.

A typical document in products collection will be:

{
  _id: ObjectId("53a2bfcfa7603606c2765342"),
  name: "Nexus 7 from Google (7-Inch, 16 GB, Black)",
  url: "http://rads.stackoverflow.com/amzn/click/B00DVFLJDS",
  base_price: 222,
  current_price: 213.96,
  img_url: "http://i.imgur.com/something.jpg",
  history: [
    [
      ISODate("2014-06-19T12:08:13.354Z"),
      293
    ],
    [
      ISODate("2014-06-24T14:31:38.216Z"),
      424
    ],
    [
      ISODate("2014-06-24T14:32:06.992Z"),
      424
    ]
  ]
}

in users, the tracked_products contain the Object Id of the product document and the desired price:

{
  _id: ObjectId("539c4adea760360886d7ef02"),
  email_id: "johnappleseed@apple.com",
  channels: {
    'twitter': 'usertwitterid',
    'ios_push_id': '45462722672762576422'
    'android_push_id': '2572652754762474'
    'chrome_push_id': '456456454646464'
  },
  tracked_products: [
    [
        ObjectId("53a2bfcfa7603606c2765342"),
        200
    ],
    [
        ObjectId("53a2d2ada7603606c2765344"),
        345
    ],
    [
        ObjectId("53a2d294a7603606c2765343"),
        120
    ]
  ]
}

and lastly trackers, here subscribers contain the Object Id of user document who are tracking this product:

{
    _id: ObjectId("77a2bfcfa7603606c2765399"),
    product_id: ObjectId("53a2bfcfa7603606c2765342"),
    subscribers: [
        [
            ObjectId("539c4adea760360886d7ef02"),
            200
        ],
        [
            ObjectId("12a2bfcfa7603606c2765399"),
            234
        ],
        [
            ObjectId("14a2bfcfa7603606c2765399"),
            345
        ],
}

Following is the code, which checks for the current_price and alerts the user if there is change in the price:

subscribers = set()

for product in product.collection:
    # price changed? if yes, then update
    current_price = get_current_price(product['url'])

    if current_price == product['current_price']:
        # no price change. move on
        continue

    product['history'].append((datetime.datetime.utcnow(), current_price))

    # find the subscribers:
    tracker_document = db.trackers.find_one({'product_id': product['_id']})

    for tracker in tracker_document['subscribers']:
        user_id, desired_price = tracker
        if current_price <= desired_price:
            subscribers.add(user_id)

    product.collection.save(product)

if not subscribers:
    # no price changes in any products!
    sys.exit()
email_queue = list()
twitter_queue = list()
ios_push_queue = list()
droid_push_queue = list()
chrome_push_queue = list()

for user_id in subscribers:
    user_document = db.users.find_one({'_id': user_id})
    email_queue.append(user_document['email_id'])
    twitter_queue.append(user_document['channels']['twitter'])
    ios_push_queue.append(user_document['channels']['ios_push_id'])
    droid_push_queue.append(user_document['channels']['droid_push_id'])
    chrome_push_queue.append(user_document['channels']['chrome_push_id'])

alert_users(email_queue, twitter_queue, ios_push_queue, droid_push_queue, chrome_push_queue)

I am looking for any suggestions, code improvements/readability, performance improvement, schema change etc. Anything!

PS: I am aware this would be lot easier with RDBMS and join operations, but I am sticking with MongoDB as of now.

You should create a function that returns all the subscribers for a product:

def get_subscribers(product, current_price):
    tracker_document = db.trackers.find_one({'product_id': product['_id']})

    subscribers = set()
    for user_id, desired_price in tracker_document['subscribers']:
        if current_price <= desired_price:
            subscribers.add(user_id)

    return subscribers

This could even be condensed even more using a list comprehension:

def get_subscribers(product, current_price):
    tracker_document = db.trackers.find_one({'product_id': product['_id']})

    return set([user_id for user_id, desired_price in tracker_document['subscribers']
                if current_price <= desired_price])

This change makes your code look like this:

subscribers = set()
for product in product.collection:
    current_price = get_current_price(product['url'])
    if current_price == product['current_price']:
        continue

    product['history'].append((datetime.datetime.utcnow(), current_price))
    product.collection.save(product)

    subscribers |= get_subscribers(product, current_price)

The next change I would suggest is: instead of using several lists for your queues, wrap them into a single defaultdict. This simplifies this code and would require only minimal changes in your alert_users code.

It may actually be better, depending on your alert_users function, to to alert a single user at a time instead of building queues of the users and calling alert_users with a batch of users.

Here is the batch example:

from collections import defaultdict

queues = defaultdict(list)
for user_id in subscribers:
    user_document = db.users.find_one({'_id': user_id})

    queues['email_id'].append(user_document['email_id'])
    for queue_type in ['twitter', 'ios_push_id', 'droid_push_id', 'chrome_push_id']:
        queues[queue_type].append(user_document['channels'][queue_type])

alert_users(queues)

Here is the user-at-a-time example:

for user_id in subscribers:
    user_document = db.users.find_one({'_id': user_id})

    alert_user(user_document['email_id'], *user_document['channels'])


Assigning zero to random cells in a pandas.DataFrame I am trying to assign 0 to random cells in a one dimensional pandas.DataFrame.
The below code is the best way I could think of doing this, however I believe there may be a neater approach to this problem.

import numpy as np
import pandas as pd
from random import randint

df = pd.DataFrame(np.random.randint(20, size=(10, 1)), columns=list('A'))
col_size = len(df.columns)
row_size = len(df.index)
df[df.columns[randint(0, col_size-1)]][randint(0, row_size-1)] = 0

Could someone please review and suggest if this is a good approach or there are better ways to do this?

Right now, you are only setting a single cell to zero. If this is what you want, then your solution works well. However, if you want to randomly set cells to be 0, then the solutions below might be better.
For an arbitrary dimensional df:

# Create Random Mask
rand_zero_one_mask = np.random.randint(2, size=df.shape)
# Fill df with 0 where mask is 0
df = df.where(rand_zero_one_mask==0, 0)

Note: df.where is not to be confused with np.where


Window timeout alert How can I improve this window timeout alert code? I need to add it to some third-party master page and don't want to add a separate file for it.

It must be JS only and IE-8 supported.

jsFiddle

(function () {
//    timedRefresh(10); //3600000
    delayedAlert();

    var timeoutID;

    function delayedAlert() {
        clearAlert();
      timeoutID = window.setTimeout(slowAlert, 10000);
    }

    function slowAlert() {
      alert("You may have been logged out due to inactivity. Click OK to refresh the page.");
    }

    function clearAlert() {
      window.clearTimeout(timeoutID);
    }
})();

A few points to make about this:

(function () {
//    timedRefresh(10); //3600000
    delayedAlert();

    var timeoutID;

    function delayedAlert() {
        clearAlert();
      timeoutID = window.setTimeout(slowAlert, 10000);
    }

    function slowAlert() {
      alert("You may have been logged out due to inactivity. Click OK to refresh the page.");
    }

    function clearAlert() {
      window.clearTimeout(timeoutID);
    }
})();

slowAlert/delayedAlert can be improved by moving slowAlert into delayedAlert as follows:

function delayedAlert() {
    clearAlert();
    timeoutID = window.setTimeout(function(){
        alert("You may have been logged out due to inactivity. Click OK to refresh the page.");
    }, 10000);
}

I can't see why the clearTimeout call deserves its own function, when it only gets called from delayedAlert; You could just put it in delayedAlert:

function delayedAlert() {
    window.clearTimeout(timeoutID);
    timeoutID = window.setTimeout(function(){
        alert("You may have been logged out due to inactivity. Click OK to refresh the page.");
    }, 10000);
}

You can make delayedAlert an anonymous function, so that it cannot be called other than inside that block by changing function delayedAlert() into var delayedAlert = function()
You call delayedAlert before timeoutID is defined:

delayedAlert();

var timeoutID;

If you're going to use globals (which are recommended against), declare them first, at the top.

(function(){
    var timeoutID;
    // functions and whatnot
    delayedAlert();

All resulting in:

(function(){
    var timeoutID;
    var delayedAlert = function() {
        window.clearTimeout(timeoutID);
        timeoutID = window.setTimeout(function(){
            alert("You may have been logged out due to inactivity. Click OK to refresh the page.");
        }, 10000);
    }
    delayedAlert();
})();


Safe Dispose of Timer I am currently refactoring a larger solution where the compiler gave multiple warnings about disposing the used System.Timers.Timer instances. The timers are running in short intervals so I would have to check before I dispose the timer if the elapsed callback is currently active.

Following the implementation with which I want to replace the System.Timers.Timer instances.

public class DisposableSafeTimer : IDisposable
{
    public event ElapsedEventHandler Elapsed;

    private System.Timers.Timer _timer;
    private readonly object _syncObject = new object();
    private volatile bool _isDisposing = false;

    public double Interval
    {
        get { return _timer.Interval; }
        set { _timer.Interval = value; }
    }

    public DisposableSafeTimer()
    {
        _timer = new System.Timers.Timer();
        _timer.Elapsed += _timer_Elapsed;
    }

    public void ExternalStart()
    {
        _timer.Start();
    }

    public void ExternalStop()
    {
        _timer.Stop();
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        lock (_syncObject)
        {
            if(_isDisposing)
            {
                return;
            }

            if (disposing)
            {
                _isDisposing = disposing;
                _timer.Stop();
                _timer.Elapsed -= _timer_Elapsed;
                _timer.Dispose();
            }
        }
    }

    private void _timer_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
    {
        lock (_syncObject)
        {
            if (_isDisposing)
            {
                return;
            }

            try
            {
                _timer.Stop();
                Elapsed?.Invoke(sender, e);
            }
            finally
            {
                _timer.Start();
            }
        }
    }
}

The Methods ExternalStart() and ExternalStop() are named with the intention that I get compiler errors wherever the Timer.Start() and Timer.Stop() methods are called. The stops and starts from the classes which use my timer implementation shouldn't care about the cyclic starts and stops of the internal timer.

So far I had no problems with my tests. I just want to make sure that I haven't overlooked something. Suggestions for improvements are welcome.

Review

property Interval and methods ExternalStart, ExternalStart should throw ObjectDisposedException if _isDisposing is true
property Interval and methods ExternalStart, ExternalStart should also acquire a lock on _syncObject
when implementing the dispose pattern make sure to include a destructor ~DisposableSafeTimer or seal your class
_isDisposing should be renamed to _disposed
when disposing, you should also clean your event listeners Elapsed = null to avoid a memory leak
do you really want to put Elapsed?.Invoke(sender, e); inside the lock? Think about possible race conditions or other side effects. What if a registered listener calls Dispose in the listener?
check out different suggestions to reset the timer. Yours is fine though.
a tiny enhancement might be to create the lock only when it is null using Interlocked.CompareExchange, instead of immediately creating an instance.


Populate two arrays with two different SQL queries I am using Joomla and connecting to an MSSQL database to store the resulting set(s) in arrays. I am utilizing this syntax, but there must be a more efficient way of coding this.

<?php
    $option = array();
    $option['driver'] = 'mssql';
    $option['host'] = '555.555.55.5';
    $option['user'] = 'username';
    $option['password'] = 'password';
    $option['database'] = 'database';
    $option['prefix'] = '';
    $db = JDatabaseDriver::getInstance($option);
    $sql = $db->getQuery(true);
    $sql = "Select ranchstyle from information";
    $db->setQuery($sql);
    $rows = $db->loadRowList();
    $output = array();
    foreach ($rows as $row) {
        array_push($output, $row);
    }
    $data = json_encode($output[0]);

    $query2 = $db->getQuery(true);
    $query2 = "Select maestro from musicinfo";
    $db->setQuery($query2);
    $rows1 = $db->loadRowList();
    $output1 = array();
    foreach ($rows1 as $r) {
        array_push($output1, $r);
    }

    $data1 = json_encode($output1[0]);
?>

There are two possible answers to this question, a generalized one and a specific one.

To help with such questions in general, there is one programming concept which is often underestimated by PHP users. It is called user-defined functions

In a nutshell, you can write a code once and then use it any number of times.

function getArrayFromSql($db, $sql)
{
    $db->setQuery($sql);
    $rows = $db->loadRowList();
    $output = array();
    foreach ($rows as $row) {
        array_push($output, $row);
    }
    return $data;
}

Here you created a function which you can use any number of times:

$sql = "Select ranchstyle from information";
$output = getArrayFromSql($db, $sql);
$data = json_encode($output[0]);

$sql = "Select maestro from musicinfo";
$output = getArrayFromSql($db, $sql);
$data1 = json_encode($output[0]);

Whereas for the specific answer we need to review your code more closely. It does unnecessary job all the time.

First, you are making a useless call to $db->getQuery(true);, as with the very next step the $sql variable gets overwritten.

Second, you actually have an array already, as $db->loadRowList(); gives you a first class array. But for some reason you are duplicating it into $data. So your code actually should be

$sql = "Select ranchstyle from information";
$db->setQuery($sql);
$rows = $db->loadRowList();
$data = json_encode($rows[0]);

$sql = "Select maestro from musicinfo";
$db->setQuery($sql);
$rows = $db->loadRowList();
$data1 = json_encode($rows[0]);

Third, given you are encoding only the first item of the array, you seems don't need an array at all, so select and fetch one line only.

$sql = "Select ranchstyle from information limit 1";
$db->setQuery($sql);
$row = $db->loadRow();
$data = json_encode($row);

$sql = "Select maestro from musicinfo limit 1";
$db->setQuery($sql);
$row = $db->loadRow();
$data1 = json_encode($row);


File size when parsing XML The code below takes a directory of XML files and parses them into a CSV file.
Right now parsing around 60 XML files is fast and the output is a CSV file that is around 250MB.

That is a really big file and the reason is because columns are being repeated. I'm repeating the columns for the reason that every element should have all the information. In red is one of the cases where the ID Z048 had multiple lines of setdata so that is why the other columns in red had to be repeated.

I'm planning to increase the number of XML files to 5k, meaning that the CSV file will be relatively large.

Asking this question to maybe get any answer if the size of my CSV file can be lowered somehow. Even though I tried to code with the mindset that I want my code to be fast and produce not too big CSV files.

from xml.etree import ElementTree as ET
from collections import defaultdict
import csv
from pathlib import Path

directory = 'path to a folder with xml files'

with open('output.csv', 'w', newline='') as f:
    writer = csv.writer(f)

    headers = ['id', 'service_code', 'rational', 'qualify', 'description_num', 'description_txt', 'set_data_xin', 'set_data_xax', 'set_data_value', 'set_data_x']
    writer.writerow(headers)

    xml_files_list = list(map(str, Path(directory).glob('**/*.xml')))
    print(xml_files_list)
    for xml_file in xml_files_list:
        tree = ET.parse(xml_file)
        root = tree.getroot()

        start_nodes = root.findall('.//START')
        for sn in start_nodes:
            row = defaultdict(str)

            repeated_values = dict()
            for k,v in sn.attrib.items():
                repeated_values[k] = v

            for rn in sn.findall('.//Rational'):
                repeated_values['rational'] = rn.text

            for qu in sn.findall('.//Qualify'):
                repeated_values['qualify'] = qu.text

            for ds in sn.findall('.//Description'):
                repeated_values['description_txt'] = ds.text
                repeated_values['description_num'] = ds.attrib['num']

            for st in sn.findall('.//SetData'):
                for k,v in st.attrib.items():
                    row['set_data_'+ str(k)] = v
                for key in repeated_values.keys():
                    row[key] = repeated_values[key]
                row_data = [row[i] for i in headers]
                writer.writerow(row_data)
                row = defaultdict(str)

At this point your code is starting to become hard to follow. If you add any more complicated logic, you will not be able to easily understand it yourself if you return to it a few weeks later.

I would first of all separate out the parsing of an XML file so that you can change that independently of all the rest. You can make this a generator, so it doesn't need to know what happens with the data it provides (being written to a CSV file, printed to the terminal, shucked into /dev/null/, ...).

Using findall to find one element is not the right approach. Either there is exactly one e.g. './/Rational' element, in which case you can just use find, or there is not and you have to store all of them (or rethink your XML design).

You can use a dictionary comprehension and the relatively new f-strings to easily generate your row data (don't worry about missing keys here, we'll take care of them outside).

def parse_file(xml_file):
    tree = ET.parse(xml_file)
    for node in tree.getroot().findall('.//START'):
        repeated_values = dict(node.attrib)
        repeated_values['rational'] = node.find('.//Rational').text
        repeated_values['qualify'] = node.find('.//Qualify').text
        description = node.find('.//Description')
        repeated_values['description_txt'] = description.text
        repeated_values['description_num'] = description.get('num')

        for data in node.findall('.//SetData'):
            row = {f"set_data_{key}": value for key, value in data.attrib.items())}
            row.update(repeated_values)
            yield row

The csv module has a DictReader and, more importantly, a DictWriter class. These are a bit easier to use since you don't need to enforce the correct order in the parsing. It also has the writerows method (like all other csv writers) that can take an iterable of dictionaries to write. This is slightly faster than writing each row manually, and also more readable.

def join_xml_files(files, output_file, headers):
    with open(output_file, 'w') as f:
        writer = csv.DictWriter(f, headers)
        writer.writeheader()
        for xml_file in files:
            writer.writerows(parse_file(xml_file))

Note that restvals="" is already set by default and replaces missing keys with an empty string, so no need for collections.defaultdict(str) anymore.

You can then put the actual calling code under a if __name__ == "__main__": guard which allows you to import from this script without running it:

if __name__ == "__main__":
    directory = 'path to a folder with xml files'
    xml_files = map(str, Path(directory).glob('**/*.xml'))
    headers = ['id', 'service_code', 'rational', 'qualify', 'description_num',
               'description_txt', 'set_data_xin', 'set_data_xax', 'set_data_value',
               'set_data_x']

    # actually call the function
    join_xml_files(xml_files, "output.csv", headers)

As to reducing the file size: that's not possible without removing information. You could zip up the resulting file (it would still be just as large in memory when unzipping it, though), or you can leave out the redundant information (but then all programs reading this file need to fill in the information when necessary).


Setting receive_payment_on flag for employment applicants I would like to refactor this block, it looks clunky:

# refactor me
receive_payment_on = false
config[:sections].each do |section|
  if section[:applicants]
    section[:applicants][:sections].each do |app_sec|
      if app_sec[:employment] && app_sec[:employment][:receive_payment_on]
        receive_payment_on = true
      end
    end
  end
end

Put your code into a method. So you get rid of the temporary variable, its cleaner and you can leave your method with a return as soon as you find the first true. (A break to leave the loop will have the same effect.)

In addition to that I'm a big friend of guard conditions. Invert if section[:applicants] ... end to if !section[:applicants] next, so you have one nesting level less.

...
config[:sections].each do |section|
   if !section[:applicants] next
   section[:applicants][:sections].each do |app_sec|
       if app_sec[:employment] && app_sec[:employment][:receive_payment_on]
           return true
       end
   end
end
return false


Is there a better way to combine these arrays? Is there a better way to do this kind of array combination with a PHP native function like array_combine?  I'm trying to improve my skills, and at certain points I note that I am using too many nested foreach for almost everything.

Using PHP 5.4

   $array = Array(
      'GroupName' => 'Emilio',
      'Moduleid' => Array(
          0 => 15,
          1 => 12,
          2 => 9,
          3 => 1,
          4 => 11,
          5 => 10
      ),
      'Permission' => Array(
          15 => 'W',
          12 => 'R',
          14 => 'W',
          9 => 'W',
          1 => 'R',
          13 => 'W',
          11 => 'W',
          10 => 'R'
      )
  );

  foreach ($array['Moduleid'] as $value) {
    foreach ($array['Permission'] as $key1 => $value1) {
      if ($value == $key1) {
        $result[$key1] = $value1;
      }
    }
  }
  $newArray = array(
      'GroupName' => $array['GroupName'],
      'ModulePermission' => $result
  );
  unset($array);

  echo '<pre>';
  print_r($newArray);
  echo '</pre>';

Output

Array
(
    [GroupName] => Emilio
    [ModulePermission] => Array
        (
            [15] => W
            [12] => R
            [9] => W
            [1] => R
            [11] => W
            [10] => R
        )

)

Using in_array can be droped one foreach

$result = array();
foreach ($array['Permission'] as $key => $value) {
  if (in_array($key, $array['Moduleid'])) {
     $result[$key] = $value;
  }
}


Modular exponentiation optimisation in Java As part of an assignment in Cryptography I've been asked to write code that involves calculating modular exponentiation. The BigInteger modPow is not allowed in this case, we have to implement it ourselves.

I have the assignment working, but I think my modular exponentiation function is not the greatest in terms of run time. It's based on the right-to-left binary method.
 Here is it :

The algorithm calculates (a^b) mod c.

private static BigInteger modulo(BigInteger a, BigInteger b, BigInteger c) {

    BigInteger x = BigInteger.ONE;
    final BigInteger TWO = new BigInteger("2", 16);

    while(b.compareTo(BigInteger.ZERO) > 0) {

        BigInteger compareVal = b.mod(TWO);
        if(compareVal.compareTo(BigInteger.ONE) ==0) {
            x = (x.multiply(a)).mod(c);
        }

        a = (a.multiply(a)).mod(c);
        b = b.divide(TWO);
    }
    return x.mod(c);

}

However, that solution uses 2 comparisons, 2 multiplications and a division within the loop. Can anyone point me in the direction of something more efficient? I'm not looking for a specific answer here, maybe just some ideas. Googling modular exponentation doesn't yield much that's useful in this case, and when I time this algorithm it's performance is very poor.

Any direction or feedback would be hugely appreciated.

However, that solution uses 2 comparisons, 2 multiplications and a division within the loop.

You are doing 3 mod, they are not free, too (mod creates new internal copies and does a divide).

final BigInteger TWO = new BigInteger("2", 16);

I do not understand, why you parse 2 to basis 16? It does not change the result, but i suggest to use:

final BigInteger TWO = new BigInteger("2");

I assume here:

BigInteger compareVal = b.mod(TWO);
    if(compareVal.compareTo(BigInteger.ONE) ==0) {

You want to check for oddnes. You can to this in constant time if you look at the last bit:

if (b.testBit(0)) // then it is odd

And you can get rid of this TWO

Instead of dividing by 2

b = b.divide(TWO);

You can shift:

b = b.shiftRight(1);

This has a better implementation.

The last mod for the return:

return x.mod(c);

is not needed:

return x;

and after all, you can choose better names and you could end up with:

private static BigInteger fastModPow(BigInteger base, BigInteger exponent, final BigInteger modulo) {
    // plan: exploit the binary representation of the exponent, see for example http://en.wikipedia.org/w/index.php?title=Modular_exponentiation&oldid=517653653#Right-to-left_binary_method
    BigInteger result = BigInteger.ONE;
    while (exponent.compareTo(BigInteger.ZERO) > 0) {
        if (exponent.testBit(0)) // then exponent is odd
            result = (result.multiply(base)).mod(modulo);
        exponent = exponent.shiftRight(1);
        base = (base.multiply(base)).mod(modulo);
    }
    return result.mod(modulo);
}

Small and meaningless test with base and exponent length of 50.000:
old: around 20s
new: around 1s
(Java: couple of ms)

If you want further optimization, you have to choose better algorithms. One little trick could be to look into the Java source code. If you are not allowed to use the Java method, noone forbids you to use the code inside?

(One last hint: If you do some non trivial algorithms, add a comment about it)


Userform that calculates gas mileage I created a project for a programming class that asks you to make a userform in Visual Studio that calculates mileage, given the user inputs of gallons of gas consumed and distance traveled. The code does its job fine, but I can't help but feel that my use of Do...While and Try...Catch statements are less than elegant. If anyone has suggestions or tips or cryptic hints about what would improve my code, I would be grateful!

Private Sub btnMileage_Click(sender As Object, e As EventArgs) Handles btnMileage.Click
    Dim sngGas As Single, sngDistance As Single, sngMileage As Single

    Do While sngGas <= 0 'force user to input positive number for gallons of gas consumed
        Try 'reject all inputs that are not real numbers
            sngGas = CSng(InputBox("Please enter gasoline consumed in gallons:"))
            If sngGas <= 0 Then 'remind user to input number greater than 0
                MsgBox("Please make sure your input is greater than 0.")
            End If
        Catch ex As Exception
            MsgBox("Error: Please enter a valid number.") 'remind user that all inputs that aren't numbers are rejected
        End Try
    Loop

    Do While sngDistance <= 0
        Try 'reject all inputs that are not real numbers
            sngDistance = CSng(InputBox("Please enter distance travelled in miles:")) 'force user to enter positive value for distance
            If sngDistance <= 0 Then
                MsgBox("Please make sure your input is greater than 0.")
            End If
        Catch ex As Exception
            MsgBox("Error: Please enter a valid number.") 'reminder user that all inputs that aren't numbers are rejected
        End Try
    Loop

    sngMileage = Math.Round((sngDistance / sngGas), 1, MidpointRounding.AwayFromZero) 'calculate mileage

    PopulateForm(CStr(sngGas), CStr(sngDistance), CStr(sngMileage)) 'update form with gas, distance & mileage

    MsgBox("All done!")
End Sub

The sub I use to populate the form follows:

Private Sub PopulateForm(Optional ByVal strGas As String = "", Optional ByVal strDistance As String = "", Optional ByVal strMileage As String = "") 'wipes form or populates with new values

    lblGas.Text = "Gas Consumed: " & strGas
    lblDistance.Text = "Distance Travelled: " & strDistance
    lblMileage.Text = "Mileage: " & strMileage

End Sub

You have a form -- by definition, a window in which text boxes and such can appear. Why are you using MsgBox and InputBox to communicate with the user?

If you instead put elements on the form and used those (particularly NumericUpDown), the code part would be so much simpler -- and far more like, well, a Windows app.

Watch this. Create a form, put three elements on it -- two NumericUpDown controls named GasAmountBox and DistanceBox, and one text control (text box or label, your choice really) named MileageOutput. Add other elements (like labels for the controls) to suit your taste.

Then, inside the form class, put something like:

Private Sub InputsChanged(sender As Object, e As EventArgs) _
  Handles GasAmountBox.ValueChanged, DistanceBox.ValueChanged
    If (GasAmountBox.Value = 0) Then
        MileageOutput.Text = "---"
    Else
        Dim mileage = DistanceBox.Value / GasAmountBox.Value
        MileageOutput.Text = mileage.ToString() & " MPG"
    End If
End Sub

Aaaaand done.

That's the entirety of the code you have to write to make the form work.  The controls take care of ensuring that the user inputs a number. You can even set the allowable range of values, so you don't have to care about negative numbers. You can add some code to, say, round the output, if you want.


Add event listener when window loads I've been using this code for quite some time, but am wondering if there's any improvements I can make to it. Basically it's just code that runs when the page loads or if the code is run after page load, it will run right away. Do you see any cross browser compatibility issues or any more efficient ways of writing this? ol() is a predefined function to be run.

(document.readyState=='complete') ?
  ol():(window.addEventListener&&window.addEventListener("load",ol,false)||
        window.attachEvent&&window.attachEvent("onload",ol));

Just for dealing with ready alone I use jQuery -- it does the cross-browser heavy lifting, is well-tested and maintained. Minified, it's only 32K, and included from a CDN, you get fast delivery that may already be cached in your user's browser.

And there is a lot more code than what you have above to deal with all of the browsers being ready.

There's some good coverage in Paul Irish's 10 Things I learned from the jQuery source screen-case .ready coverage starts around 21:20. There's also some links to jQuery sources.


Getting the first selected item from a list of Backbone models Current code (ES6):

getSelectedItem() {
    let result;
    return ((result = this.getItems().map((s) => {
        return Util.isItemSelected(s.toJSON()) ? s.toJSON() : false;
    }).filter(Boolean)) && result.length) ? result[0] : null;
},

s is a Backbone.js Model (hence the need to .toJSON() on it).

Apart from a very generic, "How can I improve this?", I'm specifically looking to reduce the duplication of .toJSON(), the anonymous function I pass into .map() and then also the need to declare result outside of my return statement.

I assume you want to return the first model from an array of models. Since you're using Backbone, you'll be able to use Underscore. So, if first model has length, return it:

    function getItems() {
        var model1 = new Backbone.Model({
            firstName: "Wassily",
            lastName: "Kandinsky"
        });

        var model2 = new Backbone.Model({
            firstName: "Jhon",
            lastName: "Doe"
        });

        var model3 = new Backbone.Model({
            firstName: "Jane",
            lastName: "Doe"
        });
        return [model1, model2, model3]

    }

    function getSelectedItem() {
        var isFilled =  _.map(getItems(), function (s) {
            return s.toJSON();
        });
        return isFilled.length ? isFilled[0] : null;
    }

    console.log(getSelectedItem().firstName);

I do not use Util since I don't know what that is but it shoudn't be neccesary. You can test it here: http://jsfiddle.net/jgauna/Lnmsp6jf/


Creating a form based on flight intentory I have an Angular 5 project with a method named createForm. It will basically create just a form. However, the form created changes depends on the flight inventory code. If flight inventory code id TAG_ON I will create a form omitting messagePrefixSMSValidator and also form control named message to empty.

createForm() {

    this.formGroup = this.fb.group(
      {
        defaultTemplate: [this.defaultInitialValue],
        language: [null, Validators.required],
        message: [ this.messagePrefix ? this.messagePrefix:'', [Validators.required]],
        longUrl: ['']
      },
      {
        validator: [
                    hasUrlTagValidator(TemplatesService.urlTag),
                    messagePrefixSMSValidator(this.messagePrefix? this.messagePrefix: null, 'message')
                  ]
      }
    );

   if(this.flight.inventory.code === FlightInventoryCode.TAG_ON) {

    this.formGroup =  this.fb.group(
      {
        defaultTemplate: [this.defaultInitialValue],
        language: [null, Validators.required],
        message: [ '', [Validators.required]],
        longUrl: ['']
      },
      {
        validator: [
                    hasUrlTagValidator(TemplatesService.urlTag),
                  ]
      }
    );
   }

  }

Is there a way to rewrite my code so that it looks simple?

Suggested changes

Formatting. Typically you want to use TSLint, and possible also Prettier. I've ran your code through Prettier with the standard configuration. (Note that the point is consistency and readability. It doesn't matter if you want to use ' or ", tabs or spaces, etc)
this.messagePrefix ? this.messagePrefix : "" can be simplified as this.messagePrefix || ""
Looking at your code, it looks like we want to assign different values to message and validator depending on the values of this.flight.inventory.code. So lets keep everything else the same rather than having separate code paths. If we were to keep the current structure, then we should at least put the first assignment to this.formGroup in an else block, so we don't assign it twice.


Concurrent activity on ArrayList I have an immutable Point class and a CollectionPoint class which manages instances of my Point class. The CollectionPoint class owns points under its control and must synchronize concurrent activity. I have below what I think is a thread-safe solution but would like another opinion. Is this solution thread-safe?

class Main {
    public static void main(String[] args) {
        int N = 10;

        double x = 0.0, y = 0.0;

        for(int i=0;i<N;i++) {
            x = (Math.random()*N);
            y = (Math.random()*N);
            Point p = new Point(x,y);
            Thread cp = new CollectionPoint(p);
            cp.start();

            try {
                cp.join();
            } catch(InterruptedException e) {}

        }

    }
}

class CollectionPoint extends Thread{
    // Class Needs to be thread safe
    private List<Point> list = new ArrayList<>();
    Point p;

    public CollectionPoint(Point pi) {
        p = pi;
    }

    public void run() {
        synchronized(this) {
            for(int i=0;i<1;i++) {
                add(p);
            }
        }
        display();
    }

    synchronized void add(Point x) {
        list.add(x);
    }

    synchronized boolean search(Point p) {
        for(int i=0;i<list.size();i++) {
            if(list.contains(p)) {
                return true;
            }
        }
        return false;
    }
    synchronized double getAllX(int x) {
        for(int i=0;i<list.size();i++) {
            if(list.contains(p.x() == x)) {
                return p.x();
            }
        }

        return 0.0;
    }

    synchronized void replace(Point p, Point new_p) {
        for(int i=0;i<list.size();i++) {
            if(list.contains(p)) {
                list.set(i, new_p);
            }
        }
    }

    synchronized void display() {
        for(Point x : list) {
            System.out.println("Point: "+x);
        }
    }
}

final class Point {
    // Class Needs to be thread safe
    private final double x,y;

    public Point(double x0, double y0) {
        x = x0;y = y0;
    }

    public double x() {return x;}
    public double y() {return y;}
    public String toString() {return "("+x+","+y+")";}
}

Point

Your Point class is immutable, which makes it thread safe. So, you have that right. The code style is very compact, and not very readable. I would prefer you to have it as:

final class Point {
    // Class Needs to be thread safe
    private final double x, y;

    public Point(double x0, double y0) {
        x = x0;
        y = y0;
    }

    public double x() {
        return x;
    }

    public double y() {
        return y;
    }

    public String toString() {
        return "(" + x + "," + y + ")";
    }
}

Synchronization

OK, I started off writing a bunch of stuff here, but it's all sort of moot.

You do not run any concurrent threads.... and, even if the threads were concurrent, you are not actually running multiple threads against the same instances... you are not testing anything....

For example, you have the synchronized add() method. That's great, but, your test loop is:

public void run() {
    synchronized(this) {
        for(int i=0;i<1;i++) {
            add(p);
        }
    }
    display();
}

Now, since you synchronize outside the add method (and outside the loop), there's no other thread that can call the add anyway.

Still, that's not a problem, because your main method does:

    for(int i=0;i<N;i++) {
        x = (Math.random()*N);
        y = (Math.random()*N);
        Point p = new Point(x,y);
        Thread cp = new CollectionPoint(p);
        cp.start();

        try {
            cp.join();
        } catch(InterruptedException e) {}

    }

So, you create N threads. Each thread gets its own instance of CollectionPoint, so, none of the threads are adding content to the same CollectionPoint, there's no problem there...

but, even that is moot, because you only ever have one thread running at any one time...

You start the thread, and join to it in the same loop, so you only run one test thread, wait for it to complete, then run the next thread, etc.

Bottom line is that your code is not tested in a meaningful way, there is no concurrency on any one instance, and the actual test you do ensures that no other thread can access the class anyway.

So, your basic question is: "Is my solution thread safe?". The answer is "Yes", because you never allow more than one thread to run at any one time.


Proper way of using IXMLDOMDocument I am trying to use IXMLDOMDocument for XML reading/writing. I am not good COM, and I am not aware that I am doing things right or wrong. I am very unsure that there could be some problems with COM initialization and relase. Here is my code below and let me know If there are any possibles bugs / memory leaks here due to COM.

void MyClass::ReadXML(BSTR *pVal)
{
    IXMLDOMDocument * pXMLDoc;
    IXMLDOMNode * pXDN;

    HRESULT hr = CoInitialize(NULL);
    hr = CoCreateInstance(CLSID_DOMDocument, NULL, CLSCTX_INPROC_SERVER,
       IID_IXMLDOMDocument, (void**)&pXMLDoc);

    if (SUCCEEDED(hr))
    {
        IXMLDOMNode* pEntityNode = CDOMHelpers::InsertDOMElement(pDoc, NULL, L"Person", NULL);

        if (SUCCEEDED(hr))
        {
            SomeClassObject->SerializeXML(pXMLDoc, pXDN);
            pXMLDoc->get_xml(pVal);
            pXDN->Release();       // Is this proper way to release COM?
            pXDN = NULL;
            pXMLDoc->Release();
            pXMLDoc = NULL;
        }
    }
}

void SomeOtherClass::SerializeXML(IXMLDOMDocument* pDoc, IXMLDOMNode* pXDN)
{
    CStringW text;
    IXMLDOMNode* pNewNode;

    text.Format(L"%u", Name);
    pNewNode = CDOMHelpers::InsertDOMElement(pDoc, pEntityNode, L"Name", text);

    text.Format(L"%u", Address);
    pNewNode = CDOMHelpers::InsertDOMElement(pDoc, pEntityNode, L"Address", text);
}

MSDN documentation for this is a mess and for the most part, inadequate (it's better than MSDN's C++ ADO documentation though).

If you have access to MSXML, then you can use smart pointer wrappers that allow you to avoid worrying about releasing the objects. These smart pointers come with simplified function calls.

For instance, here is the normal function prototype for a IXMLDOMNodeList:

HRESULT IXMLDOMNodeList::get_length(long *listLength);

Here is the "smart" version of that function for IXMLDOMNodeList object:

long IXMLDOMNodeList::Getlength ();

This allows you skip the error checking (it throws an exception on failure).
You also have access to the source code for these new functions as well. The source code is in an automatically generated .tli file, so you can always know what they do. The .tli file is usually located in C:\Users\*User*\Local\Temp\msxml6.tli. Just looking through this file can help you partially learn how to use IXMLDom.

If you have access to comutil.h, you can use the _bstr_t type instead of BSTR for automatic memory management as well.

Here are a few examples of using the smart pointers. I used standard library containers such as std::string and std::vector, but you could easily change these out with CStrings or other string types.

#include <string>
#include <vector>
#include <tchar.h>
#include <comutil.h>
#import <msxml6.dll>

typedef std::basic_string <TCHAR>       tstring ;

typedef MSXML2::IXMLDOMNodePtr          NodePtr ;
typedef MSXML2::IXMLDOMNodeListPtr      NodeListPtr ;
typedef MSXML2::IXMLDOMDocumentPtr      DocumentPtr ;
typedef MSXML2::IXMLDOMElementPtr       ElementPtr ;
typedef MSXML2::IXMLDOMNamedNodeMapPtr  NamedNodeMapPtr ;

DocumentPtr LoadXMLDocument (const tstring &strFile)
{
    // Call ::CoInitialize or ::CoInitializeEx before this.
    HRESULT hr ;
    DocumentPtr pDoc = NULL ;
    hr = pDoc.CreateInstance (__uuidof (MSXML2::DOMDocument60)) ;
    if (hr != S_OK) {
        // This could happen if ::CoInitialize or ::CoInitializeEx hasn't been called.
        // Throw an exception.
    }

    pDoc->async = VARIANT_FALSE ;
    pDoc->validateOnParse = VARIANT_FALSE ;
    pDoc->resolveExternals = VARIANT_FALSE ;

    VARIANT_BOOL vbOk = pDoc->load (_variant_t (strFile.data ())) ;

    if (vbOk == VARIANT_FALSE) {
        // Throw an exception.
    }

    return pDoc ;
}

tstring GetAttribute (const NamedNodeMapPtr &pAttrs, const tstring &strKey)
{
    tstring str ;

    NodePtr pAttr = pAttrs->getNamedItem (_bstr_t (strKey.data ())) ;
    if (pAttr == NULL) {
        // Throw an exception.
    }

    _variant_t var = pAttr->GetnodeValue () ;
    if (var.vt == VT_BSTR) {
        str = (TCHAR *) (_bstr_t) var ;

    } else {
        // Throw an exception.
    }

    return str ;
}

tstring GetAttribute (const NodePtr &pNode, const tstring &strKey)
{
    NamedNodeMapPtr pAttrs = pNode->Getattributes () ;
    return GetAttribute (pAttrs, strKey) ;
}

tstring GetSubElementText (const ElementPtr &pNode, const tstring &strSubElement)
{
    NodePtr pSub = pNode->selectSingleNode (_bstr_t (strSubElement.data ())) ;
    if (pSub == NULL) {
        // Throw an exception.
    }

    _bstr_t bsRoot = pSub->Gettext () ;
    tstring strText = (TCHAR *) bsRoot ;
    return strText ;
}

void GetAttributes (const NodePtr &pNode, const std::vector <tstring> &vecKeys, std::vector <tstring> &vecValues, const tstring &strDefault)
{
    NamedNodeMapPtr pAttrs = pNode->Getattributes () ;
    for (unsigned n = 0; n < vecKeys.size (); ++n) {
        const tstring &strKey = vecKeys [n] ;
        tstring str = BVLib::GetAttribute (pAttrs, strKey) ;
        vecValues.push_back (str) ;
    }
}


100 doors exercise code using C# Can you please check my code if it is correct for the problem 100 doors?

This is the problem:

  You have 100 doors in a row that are all initially closed. You make 100 passes by the doors. The first time through, you visit every door and toggle the door (if the door is closed, you open it; if it is open, you close it). The second time you only visit every 2nd door (door #2, #4, #6, ...). The third time, every 3rd door (door #3, #6, #9, ...), etc, until you only visit the 100th door.
  Question: What state are the doors in after the last pass? Which are open, which are closed?

static void Main(string[] args)
    {
        bool[] doors = new bool[100];

        bool test = true;
        bool temp1 = false;
        bool temp2 = true;
        bool temp;

        for (int i = 1; i <= 100; i++) {

            if (i == 3) {
                temp = temp1;
                temp1 = temp2;
                temp2 = temp;
            }

            if (test == true) {

                for (int a = 1; a <= 100; a++) {

                    if (a % i == 0) {

                        doors[a - 1] = temp1;
                    }
                    else if(a % i != 0)
                        doors[a - 1] = temp2;
                }
            }
            else if (test == false) {

                for (int c = 0; c < 100; c++) {

                    doors[c] = true;

                }
                test = true;
            }
        }
        for (int d = 0; d < 100; d++) {
            Console.WriteLine(doors[d]);
        }

            Console.ReadKey();
    }
}

I have no idea what this does:

if (i == 3) {
    temp = temp1;
    temp1 = temp2;
    temp2 = temp;
}

Please don't say if (test == true) or if (test == false)

but just if (test) or if (!test)

and you can just say else instead else if (test == false)

But your code in overly complex anyway. How about just this:

    bool[] doors = new bool[101];
    for (int step = 1; step <= 100; ++step) {
        for (int door = step; door <= 100; door += step) {
            doors[door] = !doors[door];
        }
    }

    for (int door = 1; door <= 100; ++door) {
        if (doors[door]) Console.WriteLine(door);
    }

You'll see that the output is all the square numbers: 1, 4, 9, 16, 25, 36, 49, 64, 81 and 100


Get persons full name from their First and Surname My Person class has two properties; name and surname, and a function that returns the first name and the surname together.

Is there anything that can be done better? Anything that is done poorly?

public class Person
{
    private string Firstname { get; set; }
    private string Surname { get; set; }

    public Person(string firstname, string surname)
    {
        this.Firstname = firstname;
        this.Surname = surname;

    }
    public string GetFullName()
    {
        string fullname = String.Format("Firstname is: {0}, Surname is {1}", Firstname, Surname);
        return fullname;
    }
}

Naming

Whenever you have a simple method that starts with Get, it's an indication that should should consider making it a property (in this case a read only one).

Unnecessary variable
The fullname variable is unnecessary, you might was well return simply the result of the String.Format.

Alternate for higher .Net versions

Depending on your .Net version, you might also want to use string interpolation instead.  If you combine these three things then your GetFullName becomes:

public string FullName {
                  get { return $"Firstname is: {Firstname}, Surname is {Surname}"; }
              }

As an aside, usually you would simply expect the combined full name, rather than it being explicitly stated which is which, so you would have:

get { return $"{Firstname} {Surname}"; }

or using the expression bodied auto property suggested by @Risky Martin you would end up with simply this:

public string FullName => $"{Firstname} {Surname}";

Parameter Validation

It may seem trivial for such a small class, but depending on the use case you should consider if you need to add validation to your constructor.  Is it really valid to construct a Person with a null or empty string for example?  As has been said by @Rick Davin, do you really want/need the setter for FirstName and Surname to be public, does it make sense for a Person object to change its name?  If it does make sense, then again, you should consider if rather than using the auto property you should be performing validation in your set methods.


Picking the top 5 results for each individual For loops are the devil. I can't see a way to speed this up with apply or other more speedy functions though...

pred <- read.csv("predictions_from_external_multinomial_model.csv")

pred$id <- test_users$user_id

pred$first   <- "a"
pred$second  <- "b"
pred$third   <- "c"
pred$fourth  <- "d"
pred$fifth   <- "e"

for(i in 1:nrow(pred)){
  pred$first[i]   <- names(pred[which(pred[i,] == max(pred[i,2:13]))])
  pred[i,names(pred[which(pred[i,] == max(pred[i,2:13]))])] <- 0

  pred$second[i]  <- names(pred[which(pred[i,] == max(pred[i,2:13]))])
  pred[i,names(pred[which(pred[i,] == max(pred[i,2:13]))])] <- 0

  pred$third[i]   <- names(pred[which(pred[i,] == max(pred[i,2:13]))])
  pred[i,names(pred[which(pred[i,] == max(pred[i,2:13]))])] <- 0

  pred$fourth[i]   <- names(pred[which(pred[i,] == max(pred[i,2:13]))])
  pred[i,names(pred[which(pred[i,] == max(pred[i,2:13]))])] <- 0

  pred$fifth[i]   <- names(pred[which(pred[i,] == max(pred[i,2:13]))])
  pred[i,names(pred[which(pred[i,] == max(pred[i,2:13]))])] <- 0
}

data_long <- melt(pred, id.vars=("id"), measure.vars = c("first", "second", "third", "fourth", "fifth"), value.name = "country")
data_long <- data_long[order(data_long$id),]
data_long$variable <- NULL

What it's doing is picking the top 5 results for each individual, based on the column value of 12 columns in the original prediction dataset.

In the imported csv file each row is a unique individual and each possible prediction class in in a column indexed from 2 to 13.

Here's an example dataset in case you wish to reproduce it (you can use line numbers or an arbitrary sequence in place of test_users$user_id in the code above):

http://wikisend.com/download/535006/2023bd19b880.csv

This is a solution using data.table and without loops.

require(data.table)

pred <- fread("/home/djhurio/Downloads/2023bd19b880.csv")

pred[, id := 1:.N]

# Melt to long format
data_long <- melt(pred, id.vars = "id", measure.vars = names(pred)[2:13],
                  variable.name = "country")

# Order data by id (A) and value (D)
setorderv(data_long, c("id", "value"), order = c(1, -1))

# Numerate records for each id
data_long[, i := 1:.N, by = id]

# Select top 5
data_long <- data_long[i < 6, .(id, country)]

data_long


Converting Integer to dynamically allocated Char array, digit by digit using pointers Problem: Write a function to convert Integer to String, using only
  pointers, without array indexes, string must be dynamically
  allocated.

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

// move each character in array one place to the right
// we need to make place for new character on the left
void moveArrayElementsRight(char *ptr, int len) {
    for (int j = len; j > 1; j--) {
        *(ptr + j - 1) = *(ptr + j - 2);
    }
}

void intToStr(int myNumber, char* myString){
    int i = 1;    //track size of allocated memory
    bool isMinus = false;

    if (myNumber < 0) {

        myNumber *= -1;    //without this (myNumber % 10) + '0' wont work
        isMinus = true;
    }

    if (myNumber == 0){    //special case for 0

        myString = (char*)realloc(myString, ((i + 1) * sizeof(char)));
        *myString = '0';
        *(myString + 1) = '\0';
    }

    while (myNumber != 0) {

        myString = (char*)realloc(myString, ((i + 1) * sizeof(char)));  //increse memory =+1 for next digit
        i++;
        moveArrayElementsRight(myString, i);
        *myString = (myNumber % 10) + '0'; //put digit to array
        myNumber /= 10;
    }

    if (isMinus) {

        myString = (char*)realloc(myString, ((i + 1) * sizeof(char)));  //increse memory =+1 for '-' sign
        i++;
        moveArrayElementsRight(myString, i);
        *myString = '-'; //put sign at the beginning
    }
}

int main() {

    int numberToConvert = -10;
    char *numberAsString = (char*)malloc(sizeof(char));  //create empty array, with place only for '\0'
    *numberAsString = '\0';    //mark the end of array

    intToStr(numberToConvert, numberAsString);

    printf("%s", numberAsString);

    return 0;
}

Issues Concerning Allocation
It is not necessary to cast the result of a call to malloc() in C, and many consider this to be a more error-prone style. It is easier to write correctly, read, and maintain the allocation line if you write it like this:
char *numberAsString = malloc(sizeof(*numberAsString));

This way, if the type is changed at some point in the future, there is only one place you need to update, versus three places to update in your version.
I would also suggest moving the allocation from main() into the intToStr() function (which is already reallocating the memory), and returning a pointer to the allocated memory to the calling function. This would necessitate changing the function signature of intToStr() to:
char * intToStr(int myNumber, char* myString);

Then in main(), you should declare:
char *numberAsString = NULL;

The same advice given above for malloc() applies to the calls to realloc() in the function intToStr(). Additionally, realloc() returns a NULL pointer in the event of an error. By assigning the return value directly to myString you risk leaking memory by losing the pointer to the previous allocation. You should store the return value of realloc() in a temporary variable, and then assign it to myString if it is non-null.
Continuing with allocation issues, I would consider mallocing enough memory to hold any int number-string (really, I would suggest using an int32_t number-- see below) at the beginning of intToStr(). Then, before returning the pointer to this memory, you can realloc() to the size of the actual number-string, i.e.:
number_string_sz = 12;  // for int32_t plus space for `-` and `\0`

myString = malloc(number_string_sz);

...

temp = realloc(myString, strlen(myString) + 1);
if (temp) {
    myString = temp;
}

You may wish to return a NULL pointer from intToStr() if there is an allocation error, and let the caller handle the error. Of course, the caller is responsible for freeing allocated memory.
Integer Overflow
Note that it is possible that myNumber is equal to INT_MIN, and then the line myNumber *= -1; would lead to integer overflow (invoking undefined behavior for signed integer types). Consider using a fixed-width type, such as int32_t, that is passed into the function, and copy this to a larger fixed-width type (int64_t) for a working copy within the function. The reasoning here is that the widths of the integer types are implementation-dependent, and even long int is only guaranteed to be at least as wide as int. By choosing specific widths for your integer types, you can protect against integer overflow in your calculations. The int64_t type is larger than int32_t, so the smallest value which can be held by an int32_t, when stored in an int64_t, can safely be multiplied by -1.


Handling overwriting of unique Dictionary values I am working in C# to write a scheduling application for operations in a factory. I have a custom class Workday, with properties to represent a Workday for the operations in the shop. I also have a class Timeline that contains a dictionary of DateTimes as keys (using DateTime.Date to make sure we don't have two Workdays on the same date) and a Workday with the same Date as Key.Date as the value. When writing getters/setters for the Dictionary in the Timeline class, is it better to allow for overwriting a Workday, or is it preferred to clear the properties of the Workday and repopulate those values with new data?

public class Workday
{
    private Dictionary<ShopOperation, List<BOMProject>> OperationProjectPairs = new Dictionary<ShopOperation, List<BOMProject>>();
    public DateTime DayStart { get; set; }
    public DateTime DayEnd { get; set; }

    public void SetProjects(ShopOperation key, List<BOMProject> value)
    {
        if (OperationProjectPairs.ContainsKey(key))
        {
            OperationProjectPairs[key] = value;
        }
        else
        {
            OperationProjectPairs.Add(key, value);
        }
    }

    public List<BOMProject> GetProjects(ShopOperation key)
    {
        List<BOMProject> result = null;

        if (OperationProjectPairs.ContainsKey(key))
        {
            result = OperationProjectPairs[key];
        }

        return result;
    }
}

Timeline with getter that allows for full overwrite:

public class Timeline
{
    private Dictionary<DateTime, Workday> ScheduledDays = new Dictionary<DateTime, Workday>();

    public Workday GetWorkday( DateTime date )
    {
        Workday result = null;

        foreach( KeyValuePair<DateTime, Workday> pair in ScheduledDays)
        {
            if( pair.Key.Date == date.Date)
            {
                result = pair.Value;
            }
        }

        return result;
    }

    public void SetWorkday( DateTime key, Workday value)
    {
        bool scheduled = false;

        foreach( KeyValuePair<DateTime, Workday> pair in ScheduledDays)
        {
            if( pair.Key.Date == value.DayStart.Date)
            {
                ScheduledDays[key] = value;
                scheduled = true;
                break;
            }
        }

        if(!scheduled)
        {
            ScheduledDays.Add(key, value);
        }
    }
}

Timeline that will throw an exception if you try to overwrite a Workday:

public class Timeline
{
    private Dictionary<DateTime, Workday> ScheduledDays = new Dictionary<DateTime, Workday>();

    public Workday GetWorkday( DateTime date )
    {
        Workday result = null;

        foreach( KeyValuePair<DateTime, Workday> pair in ScheduledDays)
        {
            if( pair.Key.Date == date.Date)
            {
                result = pair.Value;
            }
        }

        return result;
    }

    public void SetWorkday( DateTime key, Workday value) Throws WorkdayOverwriteException
    {
        foreach( KeyValuePair<DateTime, Workday> pair in ScheduledDays)
        {
            if( pair.Key.Date == value.DayStart.Date)
            {
                throw new WorkdayOverwriteException();
            }
        }

        ScheduledDays.Add(key, value);
    }
}

As far as your question is concerned, I'm not sure either choice is best, but if you had to choose one or the other, I would let them overwrite an existing one.

Consider that right now the user has no way to override a workday with a new one directly, so they would have to call GetWorkday to get a reference to it, and then change the properties of that object.

I think the best option would be to provide an overload that takes a Boolean that allows the user to determine if they want to overwrite a value:

public void SetWorkday( Workday value, bool overwrite)

Also, you haven't provided them with any way to RemoveWorkday, which might be a helpful addition.

For code reviewing thoughts, what are the fields DayStart and DayEnd for? I thought a workday was just a single day? Seems like it should just have a Date field.

In your get workday method, you could add a break; when you find the day you're looking for (since you don't need to keep searching):

foreach( KeyValuePair<DateTime, Workday> pair in ScheduledDays)
{
    if( pair.Key.Date == date.Date)
    {
        result = pair.Value;
        break;
    }
}

In your SetWorkday method, I think you only need to pass in a Workday object, not a DateTime key. The reason is that a Workday already has a DateTime associated with it, and looking at your code, it seems like it's an easy entry point for a bug where someone passes in a key whose date is different than Workday.DayStart:

foreach( KeyValuePair<DateTime, Workday> pair in ScheduledDays)
{
    if( pair.Key.Date == value.DayStart.Date)  // <- Comparing the workday date value
    {
        ScheduledDays[key] = value;            // <- But setting it using the key value
        scheduled = true;
        break;
    }
}

Also, in a few places you are checking if a key exists, and if it does you use assign the value using the index, but if it doesn't then you call the Add method. This is unnecessary - you can use the index assignment syntax for both. This will simplify your code. Here are 3 examples of different options and the simplified syntax:

// Overwrites
public void SetWorkday(Workday value)
{
    ScheduledDays[value.DayStart.Date] = value;
}

// Allows Overwriting
public void SetWorkday(Workday value, bool overwrite)
{
    if (ScheduleDays.ContainsKey(value.DayStart.Date) && !overwrite)
    {
        throw new WorkdayOverwriteException();
    }

    ScheduledDays[value.DayStart.Date] = value;
}

// Does not allow overwriting
public void SetWorkday(Workday value)
{
    if (ScheduleDays.ContainsKey(value.DayStart.Date))
    {
        throw new WorkdayOverwriteException();
    }

    ScheduledDays[value.DayStart.Date] = value;
}


minimax of input iterator range I previously posted a question on finding the maximum value of an InputIterator range. For completeness, I thought it would be useful to post an implementation that finds the minimum & maximum values, in a similar way to std::minmax_element.

#include <iterator>
#include <algorithm>
#include <functional>
#include <utility>

namespace detail
{
    template <typename InputIt, typename Compare>
    auto minmax_input(InputIt first, InputIt last, Compare comp, std::input_iterator_tag)
    {
        using ValueType = typename std::iterator_traits<InputIt>::value_type;

        std::pair<ValueType, ValueType> result {*first, *first};

        if (++first == last) return result;

        if (comp(*first, result.first)) {
            result.first = *first;
        } else {
            result.second = *first;
        }

        while (++first != last) {
            auto prev = *first;
            if (++first == last) {
                if (comp(prev, result.first))
                    result.first = std::move(prev);
                else if (!comp(prev, result.second))
                    result.second = std::move(prev);
                break;
            } else {
                if (comp(*first, prev)) {
                    if (comp(*first, result.first))
                        result.first = *first;
                    if (!comp(prev, result.second))
                        result.second = std::move(prev);
                } else {
                    if (comp(prev, result.first))
                        result.first = std::move(prev);
                    if (!comp(*first, result.second))
                        result.second = *first;
                }
            }
        }

        return result;
    }

    template <typename ForwardIt, typename Compare>
    auto minmax_input(ForwardIt first, ForwardIt last, Compare comp, std::forward_iterator_tag)
    {
        const auto itr_result = std::minmax_element(first, last, comp);
        return std::make_pair(*itr_result.first, *itr_result.second);
    }
} // namespace detail

template <typename InputIt, typename Compare = std::less<void>>
auto minmax_input(InputIt first, InputIt last, Compare comp = Compare {})
{
    return detail::minmax_input(first, last, comp, typename std::iterator_traits<InputIt>::iterator_category {});
}

As this will presumably be used mostly on input streams, I also thought to provide this helper method:

#include <istream>

template <typename T>
auto minmax_stream(std::istream& in)
{
    return minmax_input(std::istream_iterator<T> {in}, std::istream_iterator<T> {});
}

Used like:

#include <iostream>
#include <sstream>

int main()
{
    std::istringstream ss {"0.1 0.2 0.3 0.4 0.2 0.05 0.1"};

    const auto minmax = minmax_stream<double>(ss);

    std::cout << "minmax is (" << minmax.first << ", " << minmax.second << ")" << std::endl;
}

(This is an extended (and formatted…) comment (don't like the repetition of the non-pair comparisons at the beginning and end of the sequence). Not having used C++ in a dozen years, I'm hopeless with move semantics, for one. (And not quite destined to review C++ postings…))

Let me try something about the non-pair comparisons:

template <typename InputIt, typename Compare>
auto minmax_input(InputIt first, InputIt last, Compare comp, std::input_iterator_tag)
{
    using ValueType = typename std::iterator_traits<InputIt>::value_type;

    auto const
        &minimum = *first,
        &maximum = minimum;

    while (++first != last) {
        auto &prev = *first;
        if (++first == last) { // left with half a pair: prev
            if (comp(prev, minimum))
                minimum = prev;
            else if (!comp(prev, maximum))
                maximum = prev;
            break;
        }
        if (comp(*first, prev)) {
            if (comp(*first, minimum))
                minimum = *first;
            if (!comp(prev, maximum))
                maximum = prev;
        } else {
            if (comp(prev, minimum))
                minimum = prev;
            if (!comp(*first, maximum))
                maximum = *first;
        }
    }

    return std::pair<ValueType, ValueType> {minimum, maximum};
}

(Come to think of it, I don't like the symmetrical pairs of comparisons in the loop…)

        auto min_max = std::minmax(one, *first);
        if (comp(min_max.first, minimum))
            minimum = min_max.first;
        if (!comp(min_max.second, maximum))
            maximum = min_max.second;


How can I make this CouchDB view code more "correct"? I have a fairly simple app set up with couchrest_model to capture data pulled in from vendors to couchdb for later searching and archiving. Each document has a show (the name of the vendor), a datatype (which is limited to seven or eight unique strings), and a pulled_at timestamp field.

The core method on this model needs to return a Hash where each key is a show and the value is an array of the most recent document for each datatype. Below, I have my (simplified) model code which defines this method. It works, but my code in self.most_recent_by_type_and_show() which loops over the list of _ids and calls get() on each one (thus generating n HTTP requests on each method call) will not scale.

class Snapshot < CouchRest::Model::Base
  property :show, String
  property :sources, [String]
  property :pulled_at, DateTime
  property :datatype, String
  property :data, [Hash]

  design do
    view :_most_recent_by_type_and_show, :map => <<-MAP, :reduce => <<-REDUCE
      function(doc) {
        if (doc.type == "Snapshot") {
          emit(doc.show, [doc.datatype, new Date(doc.pulled_at).valueOf()]);
        }
      }
    MAP
      function(keys, values, rereduce) {
        var maxes = {},
            out = [];

        if (!rereduce) {
          for (var i in keys) {
            var show = keys[i][0];
            var doc_id = keys[i][1];
            var datatype = values[i][0];
            var timestamp = values[i][1];

            if (typeof maxes[show] == 'undefined') { maxes[show] = {}; }
            if (typeof maxes[show][datatype] == 'undefined') { maxes[show][datatype] = [0, null]; }

            if (timestamp > maxes[show][datatype][0]) {
              maxes[show][datatype] = [timestamp, doc_id];
            }
          }
        }

        if (Object.keys(maxes).length == 0) { maxes = values; }
        for (var show in maxes) {
          for (var datatype in maxes[show]) {
            out.push(maxes[show][datatype][1]);
          }
        }

        return out;
      }
    REDUCE
  end

  class << self
    def most_recent_by_type_and_show opts = {}
      options = opts.merge({ reduce: true,  })
      raw = _most_recent_by_type_and_show(options).group.rows

      raw.reduce({}) do |hash, row|
        hash[row['key']] = row['value'].map { |id| get id }.sort_by(&:pulled_at).reverse
        hash
      end
    end
  end
end

This feels like I'm overcomplicating the view code, and I'm also not sure that my reduce function is written correctly for rereduce. I initially tried emitting [show, datatype, pulled_at] as the key and the entire doc itself as the value in the map function, but I've read that this is Bad Practice as it will cause the size of my view index to balloon over time. And for some reason, couchrest_model will not allow me to include_docs=true if I'm reducing a view, which seems to be just a limitation of the gem as I can't find anything saying this should be the case in the CouchDB docs.

Would this be better written as a different data structure altogether? Should I maybe be using a _list function? I'm not sure where to go from here.

You can use CouchRest::Database.get_bulk API:

hash[row['key']] = database.get_bulk(row['value']).sort_by(&:pulled_at).reverse


Writing waypoints to a file I have generated code which reads a file containing ways, take the individual waypoints and writes them into a new file, but only if the waypoint is not already present. At this moment my program stores the waypoint in a vector. Before the new found waypoint is written to the file the program checks if the waypoint is not already in the vector.

The program read and wrote the first 140,000 waypoint is 12 minutes, but as there are more then 13 million ways (expected 90 million waypoints) to read, examine and written this is going to take days.

I would much appreciate if anyone of you could give me a suggestion to improve the code.

struct NodeTemplate{
  string ID;
  string NodeID;
  string latitude;
  string longitude;
};
vector <NodeTemplate> NodesOut;

void split(const string& s, char c,
           vector<string>& v) {
   string::size_type i = 0;
   string::size_type j = s.find(c);

   while (j != string::npos) {
      v.push_back(s.substr(i, j-i));
      i = ++j;
      j = s.find(c, j);

      if (j == string::npos)
         v.push_back(s.substr(i, s.length()));
   }
}

int main(int argc, char *argv[])
{
      string filenameIn="ways.txt";
      ifstream myfile ((filenameIn).c_str( ));

      string fileNameNodes2="nodes2.txt";
      fstream myfile3;

      myfile3.open(fileNameNodes2.c_str(),fstream::out | fstream::trunc);
      myfile3.close();

      myfile3.open(fileNameNodes2.c_str(),fstream::out | fstream::app);

      if (myfile.is_open())
            {while ( getline (myfile,line1) )
                {

                    string s1=line1;
                    vector<string> v1;
                    split(s1, ';', v1);

                    string s2=v1[4];
                    vector<string> v2;
                    split(s2, ',', v2);
                    for (int t=0;t<v2.size();t++)
                        {
                            NodePresent=false;
                            for(int tt=0;tt<NodesOut.size();tt++)
                                {
                                  if(atoi(v2[t].c_str())==atoi((NodesOut[tt].ID).c_str()))
                                    {   NodePresent=true;
                                        break;
                                    }
                            }

                                if(!NodePresent)
                                    {
                                        stringstream ss;
                                        ss<<CounterLine;
                                        stringstream ss2;
                                        ss2<<v2[t];
                                        NewLine=ss.str()+";"+ss2.str()+"\n";
                                        myfile3.flush();
                                        myfile3<<NewLine;
                                        if(myfile3.fail()){cout<<"can't write...\n";}
                                        cout<<CounterLine<<"\r";
                                        CounterLine++;

                                        NodesOut.push_back(NodeTemplate());
                                        NodesOut[NodesOut.size()-1].ID=ss.str();
                                        NodesOut[NodesOut.size()-1].NodeID=ss2.str();

                                    }
                            }

                }
    }
  return EXIT_SUCCESS;
}

Structure

Hmm.
OK. Property bag so its OK

struct NodeTemplate{
  string ID;
  string NodeID;
  string latitude;
  string longitude;
};

But are lat/long really string? Would they not be numbers? Also ID sounds like a number type thing.

Global Variables:

vector <NodeTemplate> NodesOut;

Global variables are bad. Don't do this.
They make the code hard to test. Side affects will cause problems.

Prefer return over out parameters.

void split(const string& s, char c,
           vector<string>& v) {

Out parameters are hard to spot and maintian. Also its not very expensive to return a vector (there is no copy the compiler will elide it (assuming you enabled optimization)). Check out RVO and NRVO.

Also in C++11 and C++14 move semantics kick in and even if you can't optimize it with RVO or NRVO then it will be moved back to the calling function rather copied.

Standard functions

A lot of this work made trivial by the standard functions and some lambda's

   while (j != string::npos) {
      v.push_back(s.substr(i, j-i));
      i = ++j;
      j = s.find(c, j);

      if (j == string::npos)
         v.push_back(s.substr(i, s.length()));
   }
}

How about:

// Use the string as a stream
std::stringstream stream(s);
// Copy "Item" objets from the stream into the vector.
std::copy(std::istream_iterator<Item<c>>(stream), std::istream_iterator<Item<c>>,
          std::back_inserter(v))

Not you can simply define an Item.

template<char c>
struct Item
{
    std::string   value;
    // Converts an Item into a string
    operator std::string const& {return value;}
    // Reads one item from the stream.
    friend std::istream& operator>>(std::istream& str, Item& data)
    {
        return std::genline(str, data.value, c);
    }
}

Use constructors.

      string filenameIn="ways.txt";
      ifstream myfile ((filenameIn).c_str( ));

Much easier to write as:

      ifstream myfile("ways.txt");

Also C++11 allows you to construct the stream object with a std::string so you don't need to call c_str() if you really want to use the variable name.

I don't see the point of opening and closing the file to truncate it.

      myfile3.open(fileNameNodes2.c_str(),fstream::out | fstream::trunc);
      myfile3.close();

      myfile3.open(fileNameNodes2.c_str(),fstream::out | fstream::app);

By default a file will truncate an existing file. So all the above can be replaced by the single line:

      std::ofstream  myfile3("nodes2.txt");

This is a useless test:

      if (myfile.is_open())

I would be OK if you used it to generate an error message. But as it stands it is just protecting the while loop. You don't need to do that. The while loop will just not be entered because getline() will fail to read from an empty file.

As discussed above:

                    string s1=line1;   // usless copy.
                    vector<string> v1;
                    split(s1, ';', v1);

Much easier to write as:

                    vector<string> v1 = split(line1, ';');
                    vector<string> v2 = split(v1[4], ',');

Though personally I would probably have written an input operator that read everything directly from the stream. Rather than copy strings all over the place.

Conversion is hurting.

 if(atoi(v2[t].c_str())==atoi((NodesOut[tt].ID).c_str()))

You are converting this every time!!!!!
You should convert it once when you read it in. Conversion is really expensive. (I mean the calls atoi())

Don't manually flush the stream

 myfile3.flush();

This will cause very inefficient use of the underlying buffer. Let the stream flush itself. It is much better at the job then you are.

Better Soltion

This is how I would expect the code to be written.

class DataNode
{
     // Values
     //   With the correct types NOT string
     friend std::istream& operator>>(std::istream& str, DataNode& data)
     {
          // read from the stream into a Data Node  "data".
          // Just one object.
          return str;
     }
     friend std::ostream& operator<<(std::ostream& str, DataNode const& data)
     {
          // Write the Data Node "data" to the stream.
          return str;
     }
}

int main()
{
    std::ifstream    input("inputfile");

    DataNode   data;
    std::unordered_set<DataNode>  dataNodes;

    while(input >> data) {
    {
        if (!dataNodes.find(data))
        {
            dataNodes.insert(std::move(data));
        }
    }

    std::ofstream    output("outputfile");
    std::copy(std::begin(dataNodes), std::end(dataNodes),
              std::ostream_iterator<DataNode>(output));
}


Group list into elements with same value This is my Result class:

public class Result
{
    public DateTime Checked { get; set; }
    public bool IsWorking { get; set; }
}

Let's say I have theese results in list:

Date: ..., IsWorking = true;
Date: ..., IsWorking = true;
Date: ..., IsWorking = false;
Date: ..., IsWorking = false;
Date: ..., IsWorking = true;
Date: ..., IsWorking = false;
Date: ..., IsWorking = true;
Date: ..., IsWorking = true;

I want to split them as following:

// group 1
Date: ..., IsWorking = true;
Date: ..., IsWorking = true;

// group 2
Date: ..., IsWorking = false;
Date: ..., IsWorking = false;

// group 3
Date: ..., IsWorking = true;

// group 4
Date: ..., IsWorking = false;

// group 5
Date: ..., IsWorking = true;
Date: ..., IsWorking = true;

I've managed something like this:

public static List<List<T>> SplitByEqualProperty<T>(this IEnumerable<T> inputs, string property)
{
    List<List<T>> temp = new List<List<T>>();
    temp.Add(new List<T>());

    object previousSelector = null;
    for (int i = 0; i < inputs.Count(); i++)
    {
        var current = inputs.ElementAt(i);
        Type t = current.GetType();
        PropertyInfo prop = t.GetProperty(property);
        object currentSelector = prop.GetValue(current);

        if (previousSelector == null)
        {
            temp.LastOrDefault().Add(current);
        }
        else
        {
            if (currentSelector.Equals(previousSelector))
            {
                temp.LastOrDefault().Add(current);
            }
            else
            {
                temp.Add(new List<T>() { current });
            }
        }

        previousSelector = currentSelector;
    }

    return temp;
}

And this is working, but I don't like the code. Expecially part with string property. How can I improve that?

You've tagged this linq so you're clearly aware of Linq, but this is not a Linq-like approach. The closest approximation in Linq is GroupBy, so that can serve as a model:

Instead of string property take Func<TSource, TKey>.
Instead of List<List<T>> (which, apart from anything else, violates the principle of coding to the interface instead of the implementation) return IEnumerable<IGrouping<TKey, TSource>> (or, if you don't care about the value itself, IEnumerable<IEnumerable<TSource>>).

Also, in Linq style, favour lazy implementation with yield return.

I would advise you to rewrite from scratch with those principles in mind.

But I must address one major problem in this code, lest you repeat it in the rewrite:

    for (int i = 0; i < inputs.Count(); i++)
    {
        var current = inputs.ElementAt(i);

This is absolutely the wrong way to iterate over an IEnumerable. The right way is

    foreach (var current in inputs)
    {

Otherwise if inputs is lazy you do far more work than necessary, and potentially execute side-effects more times than you might expect.

Also,

    if (previousSelector == null)

is problematic. What if this method is used with objects which actually have null as a value of the relevant property? I think the best approach is to use the current IGrouping: either initialise that to null and test whether it's null, or initialise it to a grouping with key default(T) and replace the special case with a special case which doesn't return an empty grouping.