Swap images on click with specified image I am new to jQuery and needed a solution for the following problem:

There are 3 images in a row and when the user clicks on one of them all three pictures are hiding and instead another image shows up (depending on the click). So while the markup and CSS is quite simple and only a quick draft the jQuery was the main issue. I came up with the following solution and would love to hear some advice, reviews, critiques or similar.

I also made a JSFiddle. for better understanding.

Here is my jQuery Code with similar click-functions (this bothers me a little):

var $button = '<button id="closebtn">Close</button>';

$($button).appendTo('#panzer');

// Pic1 hide and replace
$( "#pic1" ).click(function() {
    $('#featured li img').hide( "slow");
  $( ".undercon1" ).show( "fast", function() {
            $('#closebtn').show();
  });
});
// Pic2 hide and replace
$( "#pic2" ).click(function() {
    $('#featured li img').hide( "slow");
  $( ".undercon2" ).show( "fast", function() {
        $('#closebtn').show();
  });
});
// Pic3 hide and replace
$( "#pic3" ).click(function() {
    $('#featured li img').hide( "slow");
  $( ".undercon3" ).show( "fast", function() {
        $('#closebtn').show();
  });
});

// Button shows only with Replacement Images

$('button#closebtn').click(
     function ()
     {
         $(this).hide();

         $(".undercon1").hide( "slow");
         $(".undercon2").hide( "slow");
         $(".undercon3").hide( "slow");
         $('#featured li img').show( "slow");
     }
);

Essentially I agree with @gerdi's answer, but I would do it slightly simpler.

Basically, you want to perform the same kind of action for each sample clicked:

Hide all the samples
Show the image corresponding to the sample
Show the close button

The only variable is the image to show, which must correspond to the image clicked. A good place to express this relationship is a data-* attribute, which you can put on the same tag that will receive the clicks:

...

I called it data-rel (for "related", "relationship"), and set the value to a valid DOM selector expression. This way you can rewrite the click handler in a way that you can apply to all the #feature li elements, without having to write one by one:

$('#featured li').click(function() {
    $('#featured li img').hide("slow");
    var selector = $(this).data('rel');
    $(selector).show("fast", function() {
        $('#closebtn').show();
    });
});

The shortcut $(this).data('rel') will take the value of the data-rel attribute, equivalent to $(this).attr('data-rel').

Another thing, in the click handler of the close button, to avoid the repeated similar lines for .undercon1, .undercon2, ..., you could give them all a common undercon class:

...

...

...

so that you can hide them all easily with one line:

$(".undercon").hide("slow");


Rails app query form code behind This is code of a rails app. The code has the logic used in a search form. It queries a MySQL database. Any thoughts on something that would make this unsafe or violated a rails principle?

/app/models/query.rb

class Query < ActiveRecord::Base
  attr_accessible :customer_email, :customer_first_name, :customer_last_name,
                  :max_total_amount, :min_total_amount, :order_id, :order_is_deleted,
                  :order_max_date, :order_min_date, :results_limit, :postal_code,
                  :subscribes_to_daily_emails

 def orders
   @orders ||= find_orders
 end

  #TO DO:
  #1. Pagination:
  #2. Add limit if not present when there could be too many results. DONE! Set limit to 50.
  #3. Add export feature and remove limit for export.

  private
  def find_orders

    @results_limitVar = nil

    if results_limit.nil? == true
      @results_limitVar = 50
    else
      @results_limitVar = results_limit
    end

    @subscribes_to_daily_emailsVar = nil

    if subscribes_to_daily_emails.nil? == false
      if subscribes_to_daily_emails
        @subscribes_to_daily_emailsVar = 1
      else
        @subscribes_to_daily_emailsVar = 0
      end
    end

    @order_is_deletedVar = nil

    if order_is_deleted.nil? == false
      if order_is_deleted
        @order_is_deletedVar = 1
      else
        @order_is_deletedVar = 0
      end
    end

    #Added orders explicitly because otherwise first_name, etc is ambiguous
    #since it is in multiple models.

    orders = Order.order(:order_id)
    orders = orders.where("orders.first_name LIKE ?", "%#{customer_first_name}%")     if customer_first_name.present?
    orders = orders.where("orders.last_name LIKE ?", "%#{customer_last_name}%") if customer_last_name.present?
    orders = orders.where("orders.email_address LIKE ?", "%#{customer_email}%") if customer_email.present?
    orders = orders.where("orders.order_id like ?", "%#{order_id}%") if order_id.present?
    orders = orders.where("orders.is_deleted = ?", "#{@order_is_deletedVar}") if !@order_is_deletedVar.nil?
    orders = orders.where("total_amount >= ?", min_total_amount) if min_total_amount.present?
    orders = orders.where("total_amount <= ?", max_total_amount) if max_total_amount.present?
    orders = orders.where("order_date >= ?", order_min_date) if order_min_date.present?
    orders = orders.where("order_date <= ?", order_max_date) if order_max_date.present?
    orders = orders.joins("INNER JOIN customers ON customers.customer_id = orders.customer_id AND customers.postal_code LIKE ", "'%#{postal_code}%'") if postal_code.present?
    orders = orders.joins("INNER JOIN email_preferences ON email_preferences.customer_id = orders.customer_id AND email_preferences.subscribes_to_daily_emails = ", "#{@subscribes_to_daily_emailsVar}") if !@subscribes_to_daily_emailsVar.nil?
    orders = orders.limit(@results_limitVar) if !@results_limitVar.nil?
    orders
  end

end

I think you could change your code to this :

class Query < ActiveRecord::Base
  attr_accessible :customer_email, :customer_first_name, :customer_last_name,
                  :max_total_amount, :min_total_amount, :order_id, :order_is_deleted,
                  :order_max_date, :order_min_date, :results_limit, :postal_code,
                  :subscribes_to_daily_emails

 def orders
   @orders ||= find_orders
 end

  #TO DO:
  #1. Pagination:
  #2. Add limit if not present when there could be too many results. DONE! Set limit to 50.
  #3. Add export feature and remove limit for export.

  private
  def find_orders
    @results_limitVar = results_limit || 50

    @subscribes_to_daily_emailsVar = nil

    if subscribes_to_daily_emails
      @subscribes_to_daily_emailsVar = subscribes_to_daily_emails ? 1 : 0
    end

    @order_is_deletedVar = nil

    if order_is_deleted
      @order_is_deletedVar = order_is_deleted ? 1 : 0
    end

    #Added orders explicitly because otherwise first_name, etc is ambiguous
    #since it is in multiple models.

    orders = Order.order(:order_id)
    orders = orders.where("orders.first_name LIKE ?", "%#{customer_first_name}%")     if customer_first_name
    orders = orders.where("orders.last_name LIKE ?", "%#{customer_last_name}%") if customer_last_name
    orders = orders.where("orders.email_address LIKE ?", "%#{customer_email}%") if customer_email
    orders = orders.where("orders.order_id like ?", "%#{order_id}%") if order_id
    orders = orders.where("orders.is_deleted = ?", "#{@order_is_deletedVar}") if @order_is_deletedVar.present?
    orders = orders.where("total_amount >= ?", min_total_amount) if min_total_amount
    orders = orders.where("total_amount <= ?", max_total_amount) if max_total_amount
    orders = orders.where("order_date >= ?", order_min_date) if order_min_date
    orders = orders.where("order_date <= ?", order_max_date) if order_max_date
    orders = orders.joins("INNER JOIN customers ON customers.customer_id = orders.customer_id AND customers.postal_code LIKE ", "'%#{postal_code}%'") if postal_code
    orders = orders.joins("INNER JOIN email_preferences ON email_preferences.customer_id = orders.customer_id AND email_preferences.subscribes_to_daily_emails = ", "#{@subscribes_to_daily_emailsVar}") if @subscribes_to_daily_emailsVar.present?
    orders = orders.limit(@results_limitVar)
    orders
  end

end

I refactored your if conditions. Instead of :

if var.nil? == false
if !var.nil?

It's better to write like this:

if var.present?

In your case you can simply do :

if var

For your query, please watch look at the scopes.

You can do this :

class Query < ActiveRecord::Base
  scope :first_name_like, lambda { |first_name| where("orders.first_name LIKE ?", "%#{first_name}%") }
  ...
  def find_orders
    ...
    orders = orders.first_name_like(customer_last_name)
    ...
  end
end

It's interessent for your loooooong inner join.

I think if you column is a boolean you don't need to convert the value and you could do this :

orders.where("orders.is_deleted = ?", "#{order_is_deleted}")

The last thinks. Instead of this :

@results_limitVar

Write like this (by convention) :

@results_limit_var


Setting the path to toolbarAssociatedXibName, with fallback I have some code that I've included in a project I want to release.  Here is one such block:

NSString *path = nil;
if (delegate && [delegate respondsToSelector:@selector(toolbarAssociatedXibName:)]) {
    path = [delegate toolbarAssociatedXibName:_toolbar];
    if (![[NSBundle mainBundle] pathForResource:path ofType:@"nib"]) {
        IFLog(@"Delegate responded with an invalid value (%@) for `toolbarAssociatedXibName:`. Attempting default value (%@).", path, _identifier);
        path = _identifier;
    }
} else {
    path = _identifier;
}

The equivalent pseudocode:

Variable declaration (set to null value)
If there is an option to set the variable to a custom value
    Set it to that custom value
    If the value is not valid
        Log the error
        Set the value to a specified default
Otherwise
    Set the value to a specified default

Is it possible to shorten/compact this block of code?  I have also tried combining this block into a single line of code, ending up with this:

(delegate && [delegate respondsToSelector:@selector(toolbarAssociatedXibName:)] ? !![[NSBundle mainBundle] pathForResource:[delegate toolbarAssociatedXibName:_toolbar] ofType:@"nib"] ? path = [delegate toolbarAssociatedXibName:_toolbar] : IFLog(@"Delegate responded with an invalid value (%@) for `toolbarAssociatedXibName:`. Attempting default value (%@).", [delegate toolbarAssociatedXibName:_toolbar], _identifier), path = _identifier : path = _identifier);

That does absolutely nothing for readability.  Any suggestions?

In the spirit of DRY, how about:

NSString *path = nil;
if (delegate && [delegate respondsToSelector:@selector(toolbarAssociatedXibName:)]) {
    path = [delegate toolbarAssociatedXibName:_toolbar];
    if (![[NSBundle mainBundle] pathForResource:path ofType:@"nib"]) {
        IFLog(@"Delegate responded with an invalid value (%@) for `toolbarAssociatedXibName:`. Attempting default value (%@).", path, _identifier);
        path = nil;
    }
}
if (!path) path = _identifier;

In your pseudocode:

Variable declaration (set to null value)
If there is an option to set the variable to a custom value
    Set it to that custom value
    If the value is not valid
        Log the error
        Set the value back to null
If value is null
    Set the value to a specified default


Python Method for Text Input from Project Euler Method

Frequently input for problems is given as a 1D or 2D array in a text file.

Have knocked out a method to do so, but believe something shorter, clearer and cheaper can be written:

def read_array_from_txt(path, dim, typ, sep):
    '''
    @time: O(n)
    @space: O(n)
    '''
    txt_file_object = open(path,"r")
    text = txt_file_object.readlines()
    text = text[0]
    if dim == 1:
        if typ == "int":
            text = [int(num) for num in text.split(sep)]
        elif typ == "str":
            text = [let for let in text.split(sep)]
        else:
            raise ValueError("Unknown type.")
    elif dim == 2:
        if typ == "int":
            text = [[int(num) for num in line.split(sep)] for line in text]
        if typ == "str":
            text = [[let for let in line.split(sep)] for line in text]
        else:
            raise ValueError("Unknown type.")
    else:
        raise ValueError("Unknown dimension.")
    txt_file_object.close()
    return text

An Example:

Input: encrypted_message.txt

36,22,80,0,0,4,23,25,19,17,88,4,4,19

Output:

>>> read_array_from_txt("./encrypted_message.txt", 1, "int", ",")
[36,22,80,0,0,4,23,25,19,17,88,4,4,19]

Some Other Possible Inputs:

1   37  79  164 155 32  87  39  113 15  18  78  175 140 200 4   160 97  191 100 91  20  69  198 196
2   123 134 10  141 13  12  43  47  3   177 101 179 77  182 117 116 36  103 51  154 162 128 30
3   48  123 134 109 41  17  159 49  136 16  130 141 29  176 2   190 66  153 157 70  114 65  173 104 194 54

are,in,hello,hi,ok,yes,no,is

First of all, let's make use of the with statement so that the file is closed automatically:

with open(path, 'r') as txt_file_object:
    text = txt_file_object.readlines()

With this, you don't have to call close() anymore, as the file will close automatically when you exit the with scope.

text = text[0]

You are only reading the first line of text. Is this really what you want to do?

You are using the variable text for two different things: for the input lines and for the output values. This is not very intuitive; in fact, the result can be a list of integers, so why would it be called text? Maybe result would be a better name for it.

BUT since now you don't have to close at the end of the function, you can return the result directly instead of saving it in a variable:

return [int(num) for num in text.split(sep)]

Returning will exit the with scope, so again the file will be closed automatically.

[let for let in text.split(sep)]

This selects all objects inside text.split(sep), so we can return the splitted list directly:

text.split(sep)

Similarly, there's a different way of applying a function to every item in a list, which is using map. Maybe the list comprehension feels more natural, so you can keep that if you want; still I'll show you just so you know:

# Creates a list by calling 'int' to every item in the list
list(map(int, text.split(sep))

You are repating a lot of code; you have four different results, but they are very similar, so let's try to provide a more generic way.

My concern here is handling the two possible dimensions. You are parsing the text in the same way (depending if it's int or str), but when it's two dimensions you do it for every line. So we could use Python's lambdas to decide first what type of parsing (int or str) we are doing, and then just apply it once or multiple times.

The lambda can take parameters; the only parameter we need in our case is the input text. We can't just use text directly because sometimes we want to parse the full text, but sometimes only the line:

if typ == "int":
    parse_function = lambda t: list(map(int, t.split(sep)))
elif typ == "str":
    parse_function = lambda t: list(t.split(sep))
else:
    raise ValueError("Unknown type.")

Now parse_function can be used like any other function, taking the text as input. So we can use it when deciding the dimension:

if dim == 1:
    return parse_function(text)
elif dim == 2:
    return [parse_function(line) for line in text]
else:
    raise ValueError("Unknown dimension.")

You do well in throwing exceptions for invalid input, but how is the user meant to know what possible values can be used for typ and dim? You could add that to the docstring. You should also say what the function does in the docstring.

Updated code

def read_array_from_txt(path, dim, typ, sep):
    '''
    Processes a text file as a 1D or 2D array.
    :param path: Path to the input file.
    :param dim: How many dimensions (1 or 2) the array has.
    :param typ: Whether the elements are read as 'int' or 'str'
    :param sep: The text that is used to separate between elements.
    @time: O(n)
    @space: O(n)
    '''
    with open(path,"r") as txt_file_object:
        text = txt_file_object.readlines()

        if typ == "int":
            parse_function = lambda t: list(map(int, t.split(sep)))
        elif typ == "str":
            parse_function = lambda t: list(t.split(sep))
        else:
            raise ValueError("Unknown type.")

        if dim == 1:
            return parse_function(text)
        elif dim == 2:
            return [parse_function(line) for line in text]
        else:
            raise ValueError("Unknown dimension.")


Loop to streamline pandas dataframe to_sql This code gives me what I am looking for. But I'm just thinking how I can streamline the if statements because I would be repeating myself a couple of times, and that's not really good isn't it?

import requests
import pandas
from sqlalchemy import create_engine
import os
import numpy
from selenium import webdriver
from selenium.webdriver.support.ui import WebDriverWait
from selenium.webdriver.support import expected_conditions
from selenium.webdriver.common.by import By

def _format_data_frame(dataframe_source, df_id):
"""Format the dataframe source that is retrieved from 'my_function()' and put into SQL"""

    for each_dataframe in dataframe_source: # Formatting the columns
        if "Unnamed: 2" in each_dataframe.columns:
            each_dataframe.drop(each_dataframe.index[0], inplace=True)
            each_dataframe.rename(columns={"Fare Per Ride (cent)": "Card", "Unnamed: 2": "Cash"}, inplace=True)
        if "Card Fare Per Ride (cent)" in each_dataframe.columns:
            each_dataframe.rename(columns={"Card Fare Per Ride (cent)": "Card"}, inplace=True)
        if "Card Fare (cent)" in each_dataframe.columns:
            each_dataframe.rename(columns={"Card Fare (cent)": "Card"}, inplace=True)
        if "Description" in each_dataframe.columns:
            each_dataframe.rename(columns={"Description": "Distance"}, inplace=True)

    # Each dataframe_source has a total of 5 dataframes extracted.
    # I don't need the last dataframe, and this portion is just to separate the dataframes out.
    truck_services = dataframe_source[0]
    feeder_services = dataframe_source[1]
    express_services = dataframe_source[2]
    other_services = dataframe_source[3]

    ### How can I streamline the below code?  ###

    if df_id == "df1":
        engine = create_engine("sqlite:///abc.db", echo=False)
        connection = engine.connect()
        pandas.DataFrame.to_sql(truck_services, name="Truck Services", con=engine, if_exists="append")
        pandas.DataFrame.to_sql(feeder_services, name="Feeder Services", con=engine, if_exists="append")
        pandas.DataFrame.to_sql(express_services, name="Express Services", con=engine, if_exists="append")
        pandas.DataFrame.to_sql(other_services, name="Other Services", con=engine, if_exists="append")
        connection.close()

    if df_id == "df2":
        engine = create_engine("sqlite:///defg_Fares.db", echo=False)
        connection = engine.connect()
        pandas.DataFrame.to_sql(truck_services, name="Truck Services", con=engine, if_exists="append")
        pandas.DataFrame.to_sql(feeder_services, name="Feeder Services", con=engine, if_exists="append")
        pandas.DataFrame.to_sql(express_services, name="Express Services", con=engine, if_exists="append")
        pandas.DataFrame.to_sql(other_services, name="Other Services", con=engine, if_exists="append")
        connection.close()

    if df_id == "df3":
        engine = create_engine("sqlite:///hijk_Fares.db", echo=False)
        ## Same thing
        connection = engine.connect()

    if df_id == "df4":
        engine = create_engine("sqlite:///lmno_Fares.db", echo=False)
        ## Same thing
        connection = engine.connect()

    if df_id == "df5":
        engine = create_engine("sqlite:///pqr_Fares.db", echo=False)
        ## Same thing
        connection = engine.connect()

As far as I can see the only thing changing is the URL to the DB file.

You can just define a dictionary for this:

DB_URL = {"df1": "sqlite:///abc_Fares.db",
          "df2": "sqlite:///defg_Fares.db",
          ...}

I would also define the titles of the dataframes as a list:

titles = ["Truck Services", ...]

Which you can the easily use:

engine = create_engine(DB_URL[df_id])
connection = engine.connect()
for df, title in zip(dataframe_source, titles):
    df.to_sql(title, engine, if_exists="append")
connection.close()

Note that this calls to_sql directly on the dataframes, so no need for pandas.DataFrame(df, ...).

Also note that zip will stop after the shorter iterable is exhausted. So if the list of titles only contains four titles, the fifth dataframe will not be written to the DB.

Final code:

DB_URL = {"df1": "sqlite:///abc_Fares.db",
          "df2": "sqlite:///defg_Fares.db",
          ...}

def _format_data_frame(dataframe_source, df_id):
    """Format the dataframe source that is retrieved from 'my_function()' and put into SQL"""
    column_rename = {"Fare Per Ride (cent)": "Card",
                     "Unnamed: 2": "Cash",
                     "Card Fare Per Ride (cent)": "Card",
                     "Card Fare (cent)": "Card",
                     "Description": "Distance"}

    titles = ["Truck Services", "Feeder Services", "Express Services", "Other Services"]
    engine = create_engine(DB_URL[df_id])
    connection = engine.connect()
    # Since zip stops after the shorter iterable is exhausted, this
    # leaves the fifth df out
    for df, title in zip(dataframe_source, titles):
        if "Unnamed: 2" in df.columns:
            df.drop("Unnamed: 2", axis=1, inplace=True)
        df.rename(columns=column_rename, inplace=True)
        df.to_sql(title, engine, if_exists="append")
    connection.close()

Note that I also made the if "xxx" in each_dataframe.columns faster by storing the columns once per dataframe in a set, for which membership testing is \$\mathcal{O}(1)\$.

I also made the column renaming a lot easier. The dataframe will ignore all keys in the translation dictionary for which no columns exist, so we can use one common dictionary.

You should also include your imports, right now it is not clear where create_engine comes from. You should have a look if they implemented contextmanagers, so you could do:

with engine.connect() as connection:
    for df, title in zip(dataframe_source, titles):
            df.to_sql(title, engine, if_exists="append")

Where the connection.close() is done automatically.


Roll the die... play again? Is there a way to write this code more efficiently so that it is easier to read, maybe using loops and lists?

import random
total = 0
sixes = 0
fives = 0
fours = 0
threes = 0
twos = 0
ones = 0
limit = 50
throws = 0
Total = 0

play = input("Do you want to play, 'Y' or 'N'")
if play.upper() == "Y" :
    play = True
else :
    play = False

while total < limit and play :
    roll = random.randint(1,6)
    throws +=1
    total += roll
    if roll == 6:
        sixes +=1
    elif roll == 5:
        fives +=1
    elif roll == 4:
        fours +=1
    elif roll == 3:
        threes +=1
    elif roll == 2:
        twos +=1
    elif roll == 1:
        ones +=1
    else :
        print(" try again ")

if play :
    print("Total throws : ",throws)
    print("Total : ",total)
    print("Total 6's : ",sixes)
    print("Total 5's : ",fives)
    print("Total 4's : ",fours)
    print("Total 3's : ",threes)
    print("Total 2's : ",twos)
    print("Total 1's : ",ones)
else :
    print("Yr los")

Your program is quite static,
and as you probably know that's not good.

First I would recommend changing the entire throws, ones, twos, etc logic to a list.

# Lazily initialise it with 6 zeros
throws = [0] * 6

This will now be where we hold ones, twos, etc but not throws or total.

The way you should use it is by indexing it.

if roll == 6:
    throws[5] += 1
elif roll == 5:
    throws[4] += 1
# etc

As you can probably guess, you can just index it with roll - 1.
And that is right! Below is most of your old code in a few lines.

total = 0
throws = [0] * 6
while score < limit and play:
    roll = random.randint(1, 6)
    throws[roll - 1] += 1
    total += roll

Now all there is to look at is your prints.
You can use str() to convert a number to a string.
And so you can automate most of your prints.

for side, amount = enumerate(throws, 1):
    print('Total ' + str(side) + "'s:", amount)

First enumerate will loop through the list, giving you the index and item.
Or in our case side and amount.
The above print looks kinda ugly, and so you can change the print to use str.format.

print('Total {}\'s: {}'.format(side, amount))

This replaces the first {} with the value of side, and the second {} with amount. So if side was 1, and amount was 2 it would display Total 1's: 2.

In summery, your code will look something like this:

import random

if input("Do you want to play, 'Y' or 'N'").upper() == "Y" :
    play = True
else :
    play = False

limit = 50

total = 0
throws = [0] * 6
while score < limit and play:
    roll = random.randint(1, 6)
    throws[roll - 1] += 1
    total += roll

if play :
    print("Total throws : ", sum(throws))
    print("Total : ", total)
    for side, amount in enumerate(throws, 1):
        print('Total {}\'s: {}'.format(side, amount))
else:
    print("Yr los")

To further improve your program I would recommend that you put all the 'play' functionality into a function and make your program work with any sized dice.

def play(limit, size):
    total = 0
    throws = [0] * size
    while score < limit:
        roll = random.randint(1, size)
        throws[roll - 1] += 1
        total += roll

    print("Total throws : ", sum(throws))
    print("Total : ", total)
    for side, amount in enumerate(throws, 1):
        print('Total {}\'s: {}'.format(side, amount))

if input("Do you want to play, 'Y' or 'N'").upper() == "Y" :
    play(50, 6)
    play(500, 20) # Limit = 500, dice size = 20
else:
    print("Yr los")


Using `from select`-syntax to pass through elements from a collection unmodified While working on a project with a friend of mine, I noticed the following syntax:

public IObservable<X> GetXs()
{
    return from x in xs
           select x;
}

He said that while xs is an IObservable<X> field, this is the way it is supposed to be written in Linq, and everyone does it this way. It also allows one to add where-clauses later on easily.

On the other hand, Reflector shows that a new delegate is created that is passed to the Select() function on the observable, but that function always returns just this, so it seems to me it also has a slight overhead.

So I don't agree with him and would have written it as follows:

public IObservable<X> GetXs()
{
    return xs;
}

Are there any other reasons for using the from select-syntax without filtering, selecting or ordering that I might have missed? Which syntax is better and why?

I don't personally use RX, so my answer may be wrong, but he is it anyway:

I don't know whether “everyone does it this way”, but I doubt it. The code is translated into (after LINQ query transformation and by writing extension method invocation as if it was a normal method):

return Observable.Select(xs, x => x);

Observable.Select does exactly what you would expect it to, if you know LINQ to objects: it creates a new observable, which consumes each item from xs, processes it using the delegate you gave it (which in this case just returns the item unchanged) and then produces the result.

So, using that code does have some overhead, but it will be most likely negligible for you and you shouldn't worry about it much. But it does make the code longer and more confusing (it did confuse you). And “there is a chance it will save you some typing in the future” is not a good reason to sacrifice readability, I think.


Generating all combinations and permutations of a set of symbols I am new to Python and want to have a "pythonic" coding style from the very beginning. Following is a piece of codes I wrote to generate all combinations and permutations of a set of symbols, e.g. abc or 123.  I mainly seek for suggestions on code style but comments on other aspects of codes are also welcome.

import random as rd
import math as m
def permutationByRecursion(input, s, li):
    if len(s) == len(input):
        li.append(s)
    for i in range(len(input)):
        if input[i] not in s:
            s+=input[i]
            permutationByRecursion(input, s, li)
            s=s[0:-1]

def combinationByRecursion(input, s, idx, li):
    for i in range(idx, len(input)):
        s+=input[i]
        li.append(s)
        print s, idx
        combinationByRecursion(input, s, i+1, li)
        s=s[0:-1]

def permutationByIteration(input):
    level=[input[0]]
    for i in range(1,len(input)):
        nList=[]
        for item in level:
            nList.append(item+input[i])
            for j in range(len(item)):
                nList.append(item[0:j]+input[i]+item[j:])
        level=nList
    return nList

def combinationByIteration(input):
    level=['']
    for i in range(len(input)):
        nList=[]
        for item in level:
            nList.append(item+input[i])
        level+=nList
    return level[1:]

res=[]
#permutationByRecursion('abc', '', res)
combinationByRecursion('abc', '', 0, res)
#res=permutationByIteration('12345')
print res
print combinationByIteration('abc')

Just use the standard modules :)

import itertools
for e in itertools.permutations('abc'):
    print(e)


Recursively list directories How does the following look to list the directories on a posix system? This is actually the first piece of C code that I've written where I've actually been able to use for a real-world problem!
#define _GNU_SOURCE
#include <dirent.h>
#include <string.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/syscall.h>

#define handle_error(msg) do { perror(msg); exit(EXIT_FAILURE); } while (0)

struct linux_dirent {
    unsigned long  d_ino;
    off_t          d_off;
    unsigned short d_reclen;
    char           d_name[];
};

void print_files(char* dir, FILE* out)
{
    // open the file
    int fd = open(dir, O_RDONLY | O_DIRECTORY);
    if (fd == -1) handle_error("Error opening file.\n");

    // grab a buffer to read the file data
    #define BUF_SIZE (1024*1024*1)
    char* buffer = malloc(sizeof *buffer * BUF_SIZE);
    if (buffer == NULL) handle_error("Error malloc.\n");

    // do the getdents syscall writing to buffer
    int num_read = syscall(SYS_getdents, fd, buffer, BUF_SIZE);
    if (num_read == -1) handle_error("Error getdents syscall.\n");
    close(fd);

    for (long buffer_position = 0; buffer_position < num_read;) {

        struct linux_dirent *d = (struct linux_dirent *) (buffer + buffer_position);
        char d_type = *(buffer + buffer_position + d->d_reclen - 1);

        // skip on . and .. in the listing
        if (d->d_name[0] == '.') {
            buffer_position += d->d_reclen;
            continue;
        }

        // path = dir + '/' + name
        char path[400];
        strcpy(path, dir);
        strcat(path, "/");
        strcat(path, d->d_name);

        // recursive call, as necessary
        if (d_type == DT_DIR)
             print_files(path, out);
        else if (d_type == DT_REG)
            fprintf(out, "%s\n", path);

        // advance buffer position
        buffer_position += d->d_reclen;

    }

    free(buffer);

}

int main(int argc, char *argv[])
{
    char dir[1024];
    strcpy(dir, argc > 1 ? argv[1] : ".");
    FILE *out = fopen("c-log.txt", "w");
    fprintf(out, "-------------[ START ]---------------------\n");
    print_files(dir, out);
}
```

#define handle_error(msg) do { perror(msg); exit(EXIT_FAILURE); } while (0)

There's a long-standing convention to use UPPER_CASE for preprocessor macros.  This helps draw attention to them as text replacements, which don't obey the same rules of scope as C identifiers (and which sometimes - though not in this case - expand their arguments multiple times).

void print_files(char* dir, FILE* out)

Why a const char* dir?  Surely we have no need to overwrite its contents?  Also, I'd be inclined to always write to stdout in this program - let the user apply ordinary shell plumbing if a different destination is desired.  That's easier than having a special argument in each program.
In larger programs, it's usually a bad idea to exit() from within functions, particularly if it's possible to either recover, or produce useful information regardless of the error.  As a concrete example, if we find a subdirectory we can't read, it's probably more helpful to emit a warning message to stderr but continue listing the directories the user does have permissions for.
The code using open() and syscall() is much lower level than we'd normally use for this, and tightly bound to Linux rather than POSIX as claimed.  I think opendir() and readdir() would be more suited.
Be careful with resource management and non-local exits in C.  We only reach close(fd) in the success case.  Although handle_error() currently exits the program, if we change that in future, we'll find ourselves leaking file descriptors.  Kudos for closing the descriptor before the recursive call - it's easy to leave it open until the end of the function, and you avoided that trap.

char* buffer = malloc(sizeof *buffer * BUF_SIZE);

The multiplication by sizeof *buffer is borderline pointless.  I say "borderline" in case there's a chance that we might want to change the type (but still have BUF_SIZE of them).
We don't seem to handle reading directories larger than BUF_SIZE at all - not even with an error message.

    char path[400];
    strcpy(path, dir);
    strcat(path, "/");
    strcat(path, d->d_name);

Where's that 400 come from?  Why not use the system's MAX_PATH constant?
strcpy() followed by strcat() is wasteful - that causes another traversal of the string just written, in order to find where to start writing.  In this case, we could use printf() like this:
int path_len = snprintf(path, sizeof path, "%s/%s", dir, d->d_name);

Remember to test path_len to determine whether the path was truncated at this stage.

    if (d_type == DT_DIR)
         print_files(path, out);
    else if (d_type == DT_REG)
        fprintf(out, "%s\n", path);

Why don't we print the other kinds of file?  I wouldn't expect all my symlinks, sockets, FIFOs and device files to simply disappear from the listing unless I specifically asked for that.

   fprintf(out, "-------------[ START ]---------------------\n");

Consider using fputs() when there's no conversions to be done.
As a general style point, I'd advise more vertical space for conditionals, and use of braces as a consistent rule.  E.g.
if (buffer == NULL) {
    handle_error("Error malloc.\n");
}


Recursive directory listing I'm trying to optimize performance for recursive directory listing with some exceptions:

function listdir($basedir) {
    global $root, $ignore_dirs, $ignore_files, $filetypes, $ignore_starts_with;
        if ($handle = @opendir($basedir)) {
            while (false !== ($fn = readdir($handle))) {
                if ($fn != '.' && $fn != '..') {

                    $s = true;
                    foreach ($ignore_starts_with as $pre) if (strpos($fn, $pre) === 0) $s = false;

                    $dir = $basedir."/".$fn;
                    if ($s && is_dir($dir) && !in_array($fn, $ignore_dirs)) {
                        listdir($dir);
                    } else {

                        if ($s && !in_array($fn,$ignore_files) && preg_match("/[^.\/].+\.($filetypes)$/",$dir,$fname)) printlink($fname[0]);
                    }
                }
            }
        closedir($handle);
    }
}

First of all, I'd use RecursiveDirectoryIterator, removed globals and corrected formatting.

Then used Xdebug + CacheGrind to try different versions.


C# Better structure in my code & any other critiques Any advice on how to make this code; cleaner, more effective, just overall better!
Program creates a 'post' in the console. The post has a:

message
create on date
score for 'upvotes' and 'downvotes'

Users are able to upvote or downvote the post using upvote or downvote with a simple trim and to lower applied to the string.
Any feedback is good feedback. I am currently a working programmer but being self-taught the confidence is not always there..
Want to see it in action? https://repl.it/repls/RoughGhostwhiteApplet
namespace StackOverflowPost
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Write a post!");
            string post = Console.ReadLine();
            Post newPost = new Post();
            newPost.AddPostMessage(post);

            bool val = true;
            while (val)
            {
                newPost.ShowPost();
                Console.WriteLine("What would you like to do now? \n You can 'UpVote', 'DownVote'");
                string inputData = Console.ReadLine();
                string cleanData = inputData.Trim().ToLower();

                if (cleanData == "upvote")
                {
                    newPost.Upvote();
                }
                else if (cleanData == "downvote")
                {
                    newPost.DownVote();
                }
                else
                {
                    val = false;
                }
            }
        }

        class Post
        {
            int voteScore;
            string postMessage;
            DateTime postDate;

            public Post()
            {
                this.postDate = DateTime.Now;
            }

            public void AddPostMessage(string post)
            {
                postMessage = post;
            }

            public void Upvote()
            {
                voteScore++;
            }
            public void DownVote()
            {
                voteScore--;
            }

            public void ShowPost()
            {
                Console.WriteLine("------------------------------------");
                Console.WriteLine($"Original Post Date {postDate}");
                Console.WriteLine($"User wrote: {postMessage}");
                Console.WriteLine($"Votes: {voteScore}");
                Console.WriteLine("------------------------------------");
            }

        }
    }
}

Two assumptions (because it is simplet for me with them)

Code works as it should
This code should be prepared to go "the enterprise road" (e.g. we assume many people maintaining it for a long time in the future).

Note belows are opinions only, please do not treat is as a source of truth because your company might have a different guidelines. Since it is C# code I'm trying to stick to MSFT gudielines and, where not possible, with my own preference.
Overall comments

I'm really happy that you provided a working 'ready to run' example.
I think you should start thinking about unit testing your code if you haven't already. Simple unit tests as a start. For example 'does Upvote() increases the score`'.
I really like 'make your functions as short as it is convinient and logical' approach to writing code. Here, it would mean that ideally we would split the main function to a couple of small ones. I would be very happy if you refactored it to small logical functions and posted a new question so we can pick it up from there. I would suggest something similar to the below.

Main(){
    WriteWelcomeMessage();
    var userInput = ReadUserInput();
    var newPost = CreateNewPost(userInput);
    while(val){
        DisplayPost(newPost);
        userInput = ReadUserInput();
        ExecutePostAction(userInput, newPost);
    }
}

Or something similar, I think you get the idea. This can be further split so Main has only two or one function calls.
I consider this approach an implementation of separation of concerns and single responsibility principle.

This is still early in the project but if you would proceed with this as an enterprise application I would suggest reading on dependency injection. Here you can inject classes like reader, writer, postFactory or similar. This will make the testing much simpler in the future.
I highly recommend to include some style checker (e.g. stylecop) this will ensure that you can spot more guidelines issues.

Main function

Don't couple your implementation to Console class. I personally would write a class that would encapsulate writing/reading (or maybe two classes?) so you can easily replace it with for example reading from a WPF textbox or writing to a file.

I strongly believe in descriptive variable naming, variable always should be describing its content. Below variables are IMHO missnamed:

post - should be 'postContent'
val - shouldContinue (?)
inputData - userChoice

If you would rename the variables to be descriptive, you can drop type declarations. string or bool are not giving much context. See here for more info.

Post class

I like that you encapsulated upvoting and downvoting to functions.
Is there any reason why content of the post is not passed through the constructor? This might lead to null reference exception if the user of this class would forget to call AddPostMessage.
I would add private infront of the fields
You could inject a writer class to Post in order to decouple it from Console class.
You could also inject some DateTime provider in order to unit test this class easier.
I prefer using DateTime.UtcNow instead, this makes handling multiple timezones easier.


Printing flight statuses I have a serious nesting of ifs in a helper code and I would like to making cleaner. I would like to avoid case if possible as well.

I know there is probably a more object-oriented approach to this but I can't seem to know how.

I'm flooded with stuff like this:

  def print_flight_options_status(invitation)
    if invitation.group.travel_class == 'none'
      not_allowed
    elsif invitation.refused_flight_options?
      not_needed
    elsif invitation.selected_flights?
      waiting_reservation
    elsif invitation.flight_options.empty?
      not_sent_yet
    elsif invitation.requested_more_flight_options?
      rejected
    else
      waiting_guest_input
    end
  end

  def print_event_terms_status(invitation)
    if invitation.event_terms_status.nil?
      "Aguardando"
    elsif invitation.accepted_event_terms?
      "Aceito"
    elsif invitation.rejected_event_terms?
      "Declinado"
    elsif invitation.cancelled_event_terms?
      "Cancelado"
    end.html_safe
  end

I prefer this style:

  def print_flight_options_status(invitation)
    return not_allowed         if invitation.group.travel_class == 'none'
    return not_needed          if invitation.refused_flight_options?
    return waiting_reservation if invitation.selected_flights?
    return not_sent_yet        if invitation.flight_options.empty?
    return rejected            if invitation.requested_more_flight_options?

    waiting_guest_input
  end

UPD

About second code-snippet. I think in this case you can use decorators (for example draper) for invitations, so:

def print_event_terms_status(invitation)
  InvitationDecorator.decorate(invitation).status_in_html
end

Where InvitationDecorator:

class InvitationDecorator < Draper::Base
  def status_in_html
    if invitation.event_terms_status.nil?
      "Aguardando"
    ...
  end
end


Backup Windows user's profile I do a lot of PC swaps/replacements for my role. I've decided to write a script to assist with backing up the user's profile and network drives.

I'd like to see how my script stands now. I'm still working to implement more features such as backing up saved wireless networks and printers.

#get current user's ID
$userID=$env:UserName

#####################################
### Backup user's saved documents ###
#####################################

$FoldersToCopy = @(
    'Desktop'
    'Downloads'
    'Favorites'
    'Documents'
    'Pictures'
    'Videos'
    'AppData'
    )

$SourceRoot      = "C:\Users\$User"
$DestinationRoot = "$PSScriptRoot\fileBackup\$userID\"

foreach( $Folder in $FoldersToCopy ){
    $Source      = Join-Path -Path $SourceRoot -ChildPath $Folder
    $Destination = Join-Path -Path $DestinationRoot -ChildPath $Folder

    Robocopy.exe $Source $Destination
    }

#####################################
###  Backup user's mapped drives  ###
#####################################

$mappedDrives = @()
$drives = Get-PSDrive -PSProvider FileSystem

foreach ($drive in $drives) {
    if ($drive.DisplayRoot) {
        $mappedDrives += Select-Object Name,DisplayRoot -InputObject $drive
    }
}

New-Item -ItemType Directory -Force -Path $PSScriptRoot\fileBackup\$userID\mappedDrives
$mappedDrives | Export-Csv $PSScriptRoot\fileBackup\$userID\mappedDrives\mappedDrives.csv

Your code looks pretty good to me. A few notes:

You have a bug here:

$userID=$env:UserName

$SourceRoot      = "C:\Users\$User"

You are referring to $User, which hasn't been set. I always put this at the top of my scripts to catch such bugs:

Set-StrictMode -Version Latest

Why introduce a new variable, by the way? Why not just use it directly?:

$SourceRoot      = "C:\Users\$env:UserName"

This line can be simplified for the sake of readability:

$Source      = Join-Path -Path $SourceRoot -ChildPath $Folder

To this:

$Source      = Join-Path $SourceRoot $Folder

This code would be cleaner using the pipeline:

$mappedDrives = @()

foreach ($drive in $drives) {
    if ($drive.DisplayRoot) {
        $mappedDrives += Select-Object Name,DisplayRoot -InputObject $drive
    }
}

Like this:

$mappedDrives = $drives | ? $drive.DisplayRoot | Select-Object Name, DisplayRoot

(Not tested because I don't have any mapped drives, but I believe it is correct.)

You can format it like this if you like:

$mappedDrives = $drives |
    ? $drive.DisplayRoot |
    Select-Object Name, DisplayRoot

This comment is misleading:

#####################################
###  Backup user's mapped drives  ###
#####################################

You aren't backing up the drives. You are recording the drive mappings.


Tokenizing texts from Gutenberg archive for analysis I am writing a program to analyze books from the Gutenberg archive. The program takes the title and URL and finds the text and downloads it. Then it goes through the text and tokenizes it. Here is the code:

from urllib import request
import nltk
import os.path

canon_titles = [
    'Moby Dick',
    'Great Expectations'
]
canon_urls = [
    'http://www.gutenberg.org/files/2701/2701-0.txt',  # Moby Dick
    'http://www.gutenberg.org/files/1400/1400-0.txt'
]
# Canon_beginnings is the location of where the book actually begins in
# the text file (skips headers, publisher info, etc.)
canon_beginnings = [
    28876,
    886
]
# canon endings exists just to grab a small amount of text for prototyping
canon_endings = [x + 500 for x in canon_beginnings]
canon_raw = [None] * len(canon_titles)
canon_tokens = [None] * len(canon_titles)
canon_words = [None] * len(canon_titles)
canon_words2tokens = [None] * len(canon_titles)
canon_pos = [None] * len(canon_titles)

# Now I combine all these together into a dictionary
canon_dict = {z[0]: list(z[1:]) for z in zip(canon_titles, canon_urls, canon_beginnings, canon_endings, canon_raw, canon_tokens,
                                             canon_words, canon_words2tokens, canon_pos)}

# Now I go through each title in the dict and see if I already have the text (I rerun this in Jupyter Notebook sometimes)
# And if not I grab it from online
for x in canon_dict:
    print("Working on {}".format(x))
    if canon_dict[x][3] == None:
        print("{} does not already exist, grabbing the text".format(x))
        url = canon_dict[x][0]
        response = request.urlopen(url)
        canon_text_draft = response.read().decode('utf8')
        canon_dict[x][3] = canon_text_draft[canon_dict[x][1]:canon_dict[x][2]]
    else:
        print("Already have this text, skipping")

# OK, now we'll tokenize, do parts of speech, etc.
def tokinze_text(raw_text):
    tokens = nltk.word_tokenize(raw_text)
    return tokens

# Now let's find the tokens
for x in canon_dict:
    print(canon_dict[x][3])
    canon_dict[x][4] = tokinze_text(canon_dict[x][3])

I would like to go much further with this - extracting parts of speech, rare words, etc., but I'm worried that my basic data structures are wrong. The python dictionary is feeling unwieldy as it gets larger. Should this entire thing be done in a pandas dataframe instead? Also, should the raw text be held in a separate structure from the rest? If I want to run a bunch of analysis on some of the results (like the tokenized text), will having the massive raw text slow everything down?

This is the ideal place for a class. Each book is its own object with its own method of returning its tokens. I would make a method tokens, which I would make a property that fills itself on the first call to it.

Something like this:

from urllib import request
import nltk

class Book(object):
    def __init__(self, title, url, start=0, end=-1):
        self.title = title
        self.url = url
        self.start = start
        self.end = end
        self._raw = None
        self._tokens = None
        # self.words = None
        # self.words2tokens = None
        # self.pos = None

    def __str__(self):
        return self.title

    @property
    def raw(self):
        if self._raw is None:
            response = request.urlopen(self.url)
            draft = response.read().decode('utf8')
            self._raw = draft[self.start:self.end]
        return self._raw

    @property
    def tokens(self):
        if self._tokens is None:
            self._tokens = nltk.word_tokenize(self.raw)
        return self._tokens

if __name__ == "__main__":
    books = [Book('Moby Dick', 'http://www.gutenberg.org/files/2701/2701-0.txt', 28876, 28876 + 500),
             Book('Great Expectations', 'http://www.gutenberg.org/files/1400/1400-0.txt', 886, 886 + 500)]

    for book in books:
        print book
        print book.tokens

I commented out the words, words2tokens and pos attributes as they are not currently needed.

Alternatively, if you don't want to insist on the delayed getting of the values, you can do it all already in the constructor:

class Book(object):
    def __init__(self, title, url, start=0, end=-1):
        self.title = title
        self.url = url
        self.start = start
        self.end = end
        self.raw = self.get_raw(url)
        self.tokens = nltk.word_tokenize(self.raw)
        # self.words = None
        # self.words2tokens = None
        # self.pos = None

    def __str__(self):
        return self.title

    def get_raw(self, url):
        response = request.urlopen(url)
        draft = response.read().decode('utf8')
        return draft[self.start:self.end]

Which you can use exactly the same way.


Inserting a Node in a singly linked list I'm currently writing an "insert" function for a linked list. The code works but I would like to know if there is any way I can improve this code and at the same time covering all the "special" cases.`

private class Node{
    Node next;
    int data;

    public Node(int data, Node next){
        this.data = data;
        this.next = next;
    }

    public Node(){
        this.next = null;
        this.data = 0;
    }}`

public void insert(int data) {
    if(first == null || first.item != data){
        first = new Node(data, first);
    }
    else{
        Node x = first;
        while(x.next != null && x.next.item != data){
            x.next = new Node(data, x.next);
        }
        x = x.next;
    }
}

public Node(){
      this.next = null;
      this.data = 0;
}

As used, your no-arg constructor can be reduced to Node() {} The default instance values for objects and integers is null and 0 respectively; this is redundant.

In your insert method you refer to item yet it has no reference anywhere, I'm guessing that's supposed to be data but that's conjecture. The same goes for wherever first comes from. I'm guessing the actual linkedList class keeps reference to it, but again without the context this is purely speculation.

I would suggest you ponder why linkedlists are used. When you think of the answer you might realize you want to keep a reference to tail and prev as well.


Implementing redirect in Redux middleware Let's say I have following action:

export function signIn(data) {
  return {
    type: USER_SIGN_IN,
    promise: api.post('/sign_in', data)
  }
}

and the following middleware:

export default function promiseMiddleware() {
  return next => action => {
    const { promise, type, ...rest } = action

    if (!promise) {
      return next(action)
    }

    const SUCCESS = `${type}_SUCCESS`
    const REQUEST = `${type}_REQUEST`
    const ERROR = `${type}_ERROR`

    next({ ...rest, type: REQUEST })

    return promise
      .then(res => {
        next({ ...rest, response: res.data, type: SUCCESS })
        return true
      })
      .catch(error => {
        next({ ...rest, response: error.response.data, type: ERROR })
        return false
      })
  }
}

This code is loosely based on https://github.com/reactGo/reactGo/

But what if in then callback after calling next I want to make a redirect to another path?

I did following. I passed redirect url through action:

export function signIn(data) {
  return {
    type: USER_SIGN_IN,
    promise: api.post('/sign_in', data),
    redirect: '/'
  }
}

and added another call of next method with push from react-router-redux:

import { push } from 'react-router-redux'

export default function promiseMiddleware() {
  return next => action => {
    const { promise, type, redirect, ...rest } = action

    if (!promise) {
      return next(action)
    }

    const SUCCESS = `${type}_SUCCESS`
    const REQUEST = `${type}_REQUEST`
    const ERROR = `${type}_ERROR`

    next({ ...rest, type: REQUEST })

    return promise
      .then(res => {
        next({ ...rest, response: res.data, type: SUCCESS })
        next(push(redirect))
        return true
      })
      .catch(error => {
        next({ ...rest, response: error.response.data, type: ERROR })
        return false
      })
  }
}

It seems like it works, but I'm not sure if this is a good idea or if there are some pitfalls of multiple next calls and I shouldn't do like this? Or maybe I should use dispatch method which I can get from promiseMiddleware params instead of next for redirecting?

If there are some better approaches for implementing such redirects, please, let me know.

Instead of creating your custom promiseMiddleware you could use the redux-thunk middleware which accepts a function as an action. And the function will be called with dispatch and getStore as parameters.

Imagine you would like to fetch some data.. and you want to do it as an action. like dispatch(getPost(id))

normally your actionCreator getPost would return the action object,

const getPost = (id) => {
  return {
    type: "FETCH_POST",
    id
  }
}

But when you use redux-thunk you can return a function:

const getPost = (postId) => {
  return dispatch => {
      // Now you can use dispatch inside this block, even for async calls..
      // Start fetching..
      dispatch({type: "START_LOADING_POST" }) // imagine this could change your state to {isLoading: true}

      http
       .get("from.somewhere.net/post/" + postId)
       .then((data) => dispatch({type: "POST_LOADED_SUCCESS", postId, data }))
       .catch((error) => dispatch(loadingPostFailed({postId, error})))
  }
}

I think you know how to apply the middleware to the store, so I skip it. Hope it makes it clear how you can extend your actionCreators with redux-thunk to functions and use them specially for async tasks, like promises or even with callback-based libraries.

Redux thunk was part of redux, but its now an own package, but also maintained by Dan Abramov, check out the repo: https://github.com/gaearon/redux-thunk

If you would like to save some time by writing every actionCreator by hand you could create an creator for the actionCreator :D sounds funny, but take a look at this:

const promiseActionCreator = type => {
   const
      start    = type + "_START",
      resolved = type + "_RESOLVED",
      failed   = type + "_FAILED";

   return (params) => (dispatch, getState) => {

      dispatch({type: start})

      http
          .get(params.url)
          .then(result => dispatch({type: resolved, result }))
          .catch(error => dispatch({type: failed, error }))
   }
}

// Now you can create/generate your special promise based actionCreator
const getPost = promiseActionCreator("POST_LOADER");


Keep parsing JSON response until a field becomes false using GSON I have JSON which is coming by executing a URL. I want to store all the value of id field in a List<String> by parsing through GSON. Also, if the next field is true, then that means there is another URL at the bottom of the JSON which I need to use to get the second page which will also look like this JSON response which will have ids again, so I will add those ids into the same list again.

Here is the flow:

Execute the URL and get JSON response. After, that check if the next field is true or false in that JSON response.
If it is false, then there is only one page so add all those ids into my List<String> and return.
If it is true, then there will be another URL at the bottom. Use that URL and add prefix to it and then execute it to get another JSON response and repeat the same thing until next is false and keep adding all ids into the same list.

JSON response:

{
    "status": {
        "number": "100",
        "value": "ok"
    },
    "next": true,
    "result": [{
        "type": "Process",
        "id": "1.2.3.4.5.6",
        "device": "hello"
    }, {
        "type": "Process",
        "id": "9.55.1.2.3.2",
        "device": "world"
    }, {
        "type": "Process",
        "id": "5.4.3.2.1.2",
        "device": "proc"
    }, {
        "type": "Process",
        "id": "99.11.11.2.3.4",
        "device": "oreon"
    }],
    "pagination": {
        "cursor": ["", "57d9e9d4e4b073aa2f401542", ""],
        "limit": [10000, 1000, 1000],
        "hint": 0,
        "maxFetch": 0,
        "skipEmptyPage": false
    },
    "next": {
        "method": "GET",
        "url": "/tries/next_set"
    }
}

Working code:

  public List<String> getIds() {
    List<String> ids = new ArrayList<>();
    try {
      String url = generateUrl();
      boolean hasMore = true;
      while (hasMore) {
        Optional<String> response = executeUrl(url);
        if (!response.isPresent()) {
          JsonElement jelement = new JsonParser().parse(response.get());
          JsonObject jobject = jelement.getAsJsonObject();
          hasMore = jobject.get("next").getAsBoolean();
          if (hasMore) {
            url = PREFIX + jobject.getAsJsonObject("next").get("url").getAsString();
          }
          JsonArray jarray = jobject.getAsJsonArray("result");
          for (JsonElement type : jarray) {
            ids.add(type.getAsJsonObject().get("id").getAsString());
          }
        } else {
          hasMore = false; // to avoid infinite loop if response is not present
        }
      }
    } catch (Exception ex) {
      // log error
    }
    return ids;
  }

Below is how my execute method looks like:

  private Optional<String> execute(final String url) {
    Optional<String> abcResponse = Optional.absent();
    try (AsyncHttpClient asyncHttpClient = new DefaultAsyncHttpClient()) {
      Future<Response> future = asyncHttpClient.prepareGet(url).execute();
      Response response = future.get();
      abcResponse = Optional.of(response.getResponseBody(StandardCharsets.UTF_8.displayName()));
      System.out.println(response.getResponseBody());
    } catch (IOException | InterruptedException | ExecutionException ex) {
      // logging error
    }
    return abcResponse;
  }

The first URL is always generated by calling the generateUrl() method which is different than the subsequent URL which are there in the bottom of the JSON response. I want to see if there is any better way to do the same thing.

Overall I think it is a good solution.
Exception handling
I think getIds()-method cannot handle exceptions properly in the context of any application.
Currently it makes the result look like as nothing has happened even an exception is thrown. I do not think that callers can make valid assumptions on the returned value so I suggest to encapsulate the general Exception into a RuntimeException (or some of your own). The caller of the method should know if the result is valid. If you want the caller continue to process with partial results then you can make your own exception containing the information received so far. But to abstract from errors is dangerous.
The decision of throwing exceptions is as important as catching, rethrowing, wrapping, logging and handle them.
If you throw an exception you do it because you want to indicate that the assumptions a method has are not met so it cannot fulfill its work.
If you want to rethrow an exception the purpose mostly is to intercept information.
If you wrap an exception you want to

map the Exception to an exception of YOUR domain
make the Exception signature irrelevant (RuntimeException)

An exception should be handled at a point it can be handled. And this is not neccessarily the method it occurs.
Logging an exception is the last option for exceptional cases that have no previous mentioned handle. And this is also not neccessarily the method it occurs.
Remove Optional-construct
To exit the loop you determine the existance of a next "url". That should be your single point of truth. "getResponse" is called if "hasMore" is true. But If "getResponse" gets called then it is called under the assumption that there "is" more. If the getResponse-method cannot fulfill the work it is an exception. My view is that getting an "Optional" when "hasMore" has determined before is contradictory.
Throw an exception and the same mechanism as described in "Exception handling" is used. It is transparent to the caller. The caller always will get all necessary information and can decide on its own what to do. Either the getIds()-method returns normal or an exception is thrown that contains the Ids received so far.
Ambigious next-key and semantic redundancy
Currently you have two next-keys holding different values: boolean and String. Theoretically a JsonObject should have unique keys. But any behaviour you rely on to retreive values from this key is specific to the JSON library you use. Maybe GSON does it right somehow.
Furthermore you have semantical redundancy. The existance of the next URL is sufficient information to determine if there is a next URL.
My suggestion is to get rid of the semantic redundancy as early as possible so that further algorithms work with a single source of truth. Maybe you have a preprocession of the Json String and return the JsonObject as the return value of your execute-method().
Reuse JsonParser
I think that is a little issue. But I would reuse the JsonParser-instance as I do not know if this is an expensive operation to instantiate this kind of object.
Extracting constants
There are code fragments that should be extracted to constants. the "key" String is repeated but adresses the same semantic. But also the other keys to access the json object.
Variable scope
The subjects become pedantic... You may reduce the scope of the "hasMore" variable by using a for loop rather than a while loop.
Types
You should also have a look at your GSON-Types. To determine the id you ask an JsonElement. A JsonElement may also be a JsonNull-Object (see here). Maybe you have to introduce some type-check.
Code
private static final String KEY_NEXT = "next";
private static final String KEY_URL = "url";
private static final String KEY_RESULT = "result";
private static final String KEY_ID = "id";

public List<String> getIds() {
    List<String> ids = new ArrayList<>();
    try {
        String url = generateUrl();
        final JsonParser jsonParser = new JsonParser();
        for (boolean hasMore = true; hasMore;) {
            JsonObject jobject = execute(url);
            JsonArray jarray = jobject.getAsJsonArray(KEY_RESULT);
            for (JsonElement type : jarray) {
                ids.add(type.getAsJsonObject().get(KEY_ID).getAsString());
            }
            hasMore = jobject.has(KEY_NEXT); // assumption, that only the next-key with the url is possibly present
            if (hasMore) {
                url = PREFIX + jobject.getAsJsonObject(KEY_NEXT).get(KEY_URL).getAsString();
            }
        }
    } catch (Exception ex) {
        throw new PartialResultException(ex, ids);
    }
    return ids;
}

private static class PartialResultException extends RuntimeException {

    private List<String> idsReceivedSoFar;

    public PartialResultException(Exception ex, List<String> idsReceivedSoFar) {
        super(ex);
        this.idsReceivedSoFar = idsReceivedSoFar;
    }

    public List<String> getIdsReceivedSoFar() {
        return idsReceivedSoFar;
    }

}

// Example call
public void caller() {

    List<String> ids = new ArrayList<>();

    try {
        ids.addAll(getIds());
    } catch (PartialResultException e) {
        // omit this if the caller is not interested in the ids received so far if an exception occured
        ids.addAll(e.getIdsReceivedSoFar());
    }

    // code that uses the ids
    for (String string : ids) {
        ...
    }

}

The execute-method will either wrap the occuring exceptions to a RuntimeException...
private JsonObject execute(final String url) {
    try (AsyncHttpClient asyncHttpClient = new DefaultAsyncHttpClient()) {
        Future<Response> future = asyncHttpClient.prepareGet(url).execute();
        Response response = future.get();
        final JsonParser jsonParser = new JsonParser();
        JsobObject jsonResponse = jsonParser.parse(response.getResponseBody(StandardCharsets.UTF_8.displayName())).getAsJsonObject();
        removeNextKeyBooleanValue(jsonParser);
        return jsonResponse;
    } catch (IOException | InterruptedException | ExecutionException ex) {
        throw new RuntimeException(ex);
    }
}

... or you may throw them
private JsonObject execute(final String url) throws IOException, InterruptedException, ExecutionException {
    try (AsyncHttpClient asyncHttpClient = new DefaultAsyncHttpClient()) {
        Future<Response> future = asyncHttpClient.prepareGet(url).execute();
        Response response = future.get();
        final JsonParser jsonParser = new JsonParser();
        JsobObject jsonResponse = jsonParser.parse(response.getResponseBody(StandardCharsets.UTF_8.displayName())).getAsJsonObject();
        removeNextKeyBooleanValue(jsonParser);
        return jsonResponse;
    }
}

The point is that there is no rule to catch and handle an exception where it occurs or as early as possible. The rule is: it has to be the semantic correct location under the assumptions of the involved methods.


To understand what recursion is, you must first understand recursion This is the second assignment for my CS2 course:

  Write a recursive C++ function writeLine() that writes a character    repeatedly to form a line of n characters. For example:

writeLine('*', 5)

  produces the output:

*****

  Write a recursive [C++] function writeBlock() that uses writeLine    to write m lines of n characters each. For example,

writeBlock('*', 5, 3)

  produces the output:

*****
*****
*****

  Implement and test a recursive C++ function writePyramid(). The    function should take in a character to build a pyramid with and the
  height of the pyramid (the number of lines in it). Hint: Each line in
  the pyramid should be generated by two calls to writeLine: the first
  call generates the spaces and a second call to generate the line of
  characters.
     The ␣ in the output below represents a space character. This output    is generated when the function is asked to generate a
  pyramid of    height 3 with the * character.

␣␣*
␣***
*****

Please feel free to rip the code apart!  In my opinion, I think the switch in main() could be DRYed up a bit, but I wasn't sure how.

lab2.cpp:

#include <iostream>
#include <cstdio>
#include "lab2functions.h"

char getSanitizedChar()
{
  // absorb newline character (if existant) from previous input
  if('\n' == std::cin.peek()) std::cin.ignore();
  return std::tolower(std::cin.get());
}

int main()
{
  char type = '*';
  char shape = 'l';
  int rows = 0;
  int cols = 0;
  do
  {
    std::cout << "Enter shape you would like {(L)ine, (b)lock, (t)riangle}: ";
    std::cin >> shape;

    switch(shape)
    {
    case 'l':
      std::cout << "Enter number of columns: ";
      std::cin >> cols;
      std::cout << "Enter character to build with (*): ";
      type = getSanitizedChar();
      writeLine(('\n' == type ? '*' : type), cols);
      std::endl(std::cout);
      break;
    case 'b':
      std::cout << "Enter number of rows: ";
      std::cin >> rows;
      std::cout << "Enter number of columns: ";
      std::cin >> cols;
      std::cout << "Enter character to build with (*): ";
      type = getSanitizedChar();
      writeBlock(('\n' == type ? '*' : type), rows, cols);
      break;
    case 't':
      std::cout << "Enter number of rows: ";
      std::cin >> rows;
      std::cout << "Enter character to build with (*): ";
      type = getSanitizedChar();
      writePyramid(('\n' == type ? '*' : type), rows);
      break;
    default:
      std::cout << "Invalid option";
      break;
    }
    std::cout << "\nRun a calculation again (y/N): ";  // signify n as default with capital letter
  } while ('y' == getSanitizedChar());
}

lab2functions.h:

#ifndef LAB2FUNCTIONS_H
#define LAB2FUNCTIONS_H

#include <cstdint>

void writeLine(const char, const int8_t);
void writeBlock(const char, const int8_t, const int8_t);
void writePyramid(const char, const int8_t);

#endif

lab2functions.cpp:

#include <iostream>
#include "lab2functions.h"

void writeLine(const char c, const int8_t num)
{
  if (num > 0)
  {
    std::cout << c;
    writeLine(c, num - 1);
  }
}

void writeBlock(const char c, const int8_t m, const int8_t n)
{
  if (m > 0)
  {
    writeLine(c, n);
    std::endl(std::cout);
    writeBlock(c, m - 1, n);
  }
}

void writePyramid(const char c, const int8_t h)
{
  static int count;
  count++;
  if (h > 0)
  {
    writeLine(' ', (2 * h - 1) / 2);
    writeLine(c, (2 * count - 1));
    std::endl(std::cout);
    writePyramid(c, h - 1);
  }
  count = 0;
}

Usually we write recursive functions with the exit condition all by itself:

void writeLine(const char c, const int8_t num)
{
  if (num > 0)
  {
    std::cout << c;
    writeLine(c, num - 1);
  }
}

I would normally write this as:

void writeLine(const char c, const int8_t num)
{
  if (num <= 0)
  {
      return;
  }

  std::cout << c;
  writeLine(c, num - 1);
}

No real reason apart from tradition I suppose. But it does show tail recursion more clearly. The compiler can easily optimize tail recursion into a loop for you. But it also makes it obvious to the human eye where tail recursion is.

A static member of a recursive function!!!

void writePyramid(const char c, const int8_t h)
{
  static int count;
  count++;

That's a bit strange. You normally pass all values as parameters between iterations. If you need a defaulting starting value then you can either use a wrapper function that calls your recursive function and provides the initial value. Or have a parameter that defaults to the start value but internally you increment it (I choose the second here).

void writePyramid(const char c, const int8_t h, int count = 0)
{
  if (h <= 0)
  {
    return;
  }

  writeLine(' ', (2 * h - 1) / 2);
  writeLine(c, (2 * count - 1));
  std::endl(std::cout);
  writePyramid(c, h - 1, count + 1);
}

I know that Static storage duration objects are zero initialized. But how many of your peers know this. It would have been nice to give them a hint.

static int count = 0;

Going back to your wrapper code:

All the switch statements contain the same bit of code:

  std::cout << "Enter number of rows: ";
  std::cin >> rows;
  std::cout << "Enter number of columns: ";
  std::cin >> cols;
  std::cout << "Enter character to build with (*): ";
  type = getSanitizedChar();

Keep it DRY. Either move this into a separate function (called from each case). Or move it to before the switch intirally.


Matching a set of conditions based on keywords to text I am currently working on a function that should return true or false based on a condition written in string by a user. The condition will look something like this:

((dog OR cat) AND *Horse*)

The () grouping objects and *word* making the search case insensitive.

How I approached the problem was to have a string that would replace the words by True or False and then simply do an eval() of the new string, giving us the final result.

For example ((dog OR cat) AND *Horse*) would become ((True or False) and True) if applied to the sentence "My dog is not a horse, obviously",

What I don't like about my code it is that it looks really messy, is there a better way to do this task?

import re
def text2cond(text, condition):
    """Will transform a string into a condition usable by tweet_filter"""
    # We will need to give modularity to this
    s = ""
    for token in condition.split():
        regex = None
        if token.startswith('('):
            open_par = 0
            case_sens = False
            for char in token:
                if char == "(":
                    open_par = open_par + 1
                elif char == "*":
                    case_sens = True
            if case_sens:
                regex = r"\b" + re.escape(token[open_par+1:-1]) + "\\b"
                if re.search(regex, text, re.IGNORECASE):
                    s= s + ("("*open_par + "True ")
                else:
                    s= s + ("("*open_par + "False ")
            else:
                regex = r"\b" + re.escape(token[open_par:]) + "\\b"
                if re.search(regex, text):
                    s= s + ("("*open_par + "True ")
                else:
                    s= s + ("("*open_par + "False ")
        elif token.endswith(')') and not token.startswith("*"):
            close_par = 0
            for char in token:
                if char == ")":
                    close_par = close_par + 1
            regex = r"\b" + re.escape(token[:-close_par]) + "\\b"
            if re.search(regex, text):
                s= s + ("True" + ")"*close_par + " ")
            else:
                s= s + ("False" + ")"*close_par + " ")
        elif token.lower() == "or":
            s= s + ("or ")
        elif token.lower() == "and":
            s= s + ("and ")
        elif token.lower() == "not":
            s= s + ("not ")
        elif token.startswith('*'):
            close_par = 0
            for char in token:
                if char == ")":
                    close_par = close_par + 1
            regex = r"\b" + re.escape(token[1:-1-close_par]) + "\\b"
            if re.search(regex, text, re.IGNORECASE):
                s= s + ("True" + close_par*")")
            else:
                s= s + ("False" + close_par*")")
    return s

_condition = "((dog OR cat) AND *Horse*)"
_text = "My dog is obviously not a horse"

print(text2cond(_text, _condition))
print(eval(text2cond(_text, _condition)))

1. Difficulties

The approach taken in the post is to use string operations to transform the input into a Python expression and then evaluate the result.

The trouble with this approach is that the string comes from a user, who may not enter exactly what you expect. So handling of erroneous input is vital. For example, suppose I want to know if text contains "dog". The first thing I would try is dog:

>>> eval(text2cond(_text, 'dog'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 0

    ^
SyntaxError: unexpected EOF while parsing

Ok, maybe I need parentheses? What about (dog):

>>> eval(text2cond(_text, '(dog)'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 1
    (False
          ^
SyntaxError: unexpected EOF while parsing

Ok, I suppose I have to use an operator too? I'll try (dog OR dog):

>>> eval(text2cond(_text, '(dog OR dog)'))
True

Suppose that I want case-insensitive matching of two words? I might try *(dog AND horse)*:

>>> eval(text2cond(_text, '*(dog AND horse)*'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 1, in <module>
NameError: name 'Falseand' is not defined

Ok, maybe I need more parentheses? What about (*(dog AND horse)*):

>>> eval(text2cond(_text, '(*(dog AND horse)*)'))
False

Which is misleading — what I should have written is (*dog* AND *horse*):

>>> eval(text2cond(_text, '(*dog* AND *horse*)'))
True

I think that users will find this behaviour hard to cope with. Of course you can go through the code and try to fix up these cases as users discover them. But how can you be sure you have caught everything? And even if you've caught all the cases, the SyntaxError that you get if the user makes a mistake are not likely to be very friendly.

>>> eval(text2cond(_text, '(dog AND horse OR cat)'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 1
    (True and or False)
               ^
SyntaxError: invalid syntax

The other problem is that using eval on text that comes from the user is risky — if you didn't manage to completely sanitize the input, then the user may be able to run arbitrary code.

2. The standard approach

The standard approach to this kind of problem is parsing. The idea is to break down the problem into three processing stages:

Tokenization. In this stage the input string is turned into a sequence of tokens (pieces which can't be further broken down). For example, given this input:

((dog OR cat) AND *Horse*)

the tokenizer might emit this sequence of tokens:

'(', '(', 'dog', 'OR', 'cat', ')', 'AND', '*', 'Horse', '*', ')', '<end of input>'

Parsing. In this stage the sequence of tokens is turned into a parse tree, a data structure corresponding to the syntactic structure of the input. For example, given the input above, the parser might construct the following data structure:

BinOp(
    left=BinOp(
        left='dog',
        op='OR',
        right='cat'),
    op='AND',
    right=IgnoreCase(
        body='Horse'))

Evaluation. This is easy to do by a depth-first pass over the parse tree.

There are several good reasons to solve the problem using this approach:

It's the standard approach, so other programmers will easily understand how it works.
It's efficient: each stage needs to pass over its input exactly once.
Splitting the work into steps with clearly defined inputs and outputs makes it easier to test.
The approach extends to more complicated applications, such as interpretation of programming languages.

3. Tokenization

In Python an easy way to tokenize a string is to use the finditer method on regular expressions. A little bit of care has to be taken to avoid gaps between tokens (other than whitespace, which you are allowing). This is done by having a "catch-all" alternative (\S) which matches any non-whitespace character that doesn't belong to a valid token.

import re

# Regular expression matching a token (group 1) or an error (group 2).
_TOKEN_RE = re.compile(r'\s*(?:([()*]|\w+\b)|(\S))')

# Token representing the end of the input.
TOKEN_END = '<end of input>'

def tokenize_condition(s):
    """Generate the tokens in the condition s.

    >>> list(tokenize_condition("((dog OR cat) AND *Horse*)"))
    ['(', '(', 'dog', 'OR', 'cat', ')', 'AND', '*', 'Horse', '*', ')', '<end of input>']

    """
    for match in _TOKEN_RE.finditer(s):
        token, error = match.groups()
        if error:
            raise SyntaxError("unexpected character {!r}".format(error))
        yield token
    yield TOKEN_END

4. Parsing

(In this section I'm using a simplified version of your language, lacking the NOT operator.)

First, we'll need a data structure to represent the parse tree. Here I'm going to use collections.namedtuple:

from collections import namedtuple

# Binary operation with left operand, operator and right operand.
BinOp = namedtuple('BinOp', 'left op right')

# Ignore case when matching against body.
IgnoreCase = namedtuple('IgnoreCase', 'body')

A word to be matched like dog will be represented by a string.

Second, the parser. There are lots of different techniques for writing parsers. I could use a parser generator like pyparsing, that constructs the parser from a representation of a formal grammar, but I think it's more illustrative to write a recursive descent parser by hand.

def parse_condition(tokens):
    """Parse a condition given by an iterator of tokens, and return a
    parse tree.

    >>> tokens = tokenize_condition("((dog OR cat) AND *Horse*)")
    >>> parse_condition(tokens)
    BinOp(left=BinOp(left='dog', op='OR', right='cat'), op='AND', right=IgnoreCase(body='Horse'))

    """
    token = next(tokens)           # The current token.

    def error(expected):
        # Current token failed to match, so raise syntax error.
        raise SyntaxError("Expected {} but found {!r}"
                          .format(expected, token))

    def match(*valid_tokens):
        # If the current token is found in valid_tokens, consume it
        # and return True. Otherwise, return False.
        nonlocal token
        if token in valid_tokens:
            token = next(tokens)
            return True
        else:
            return False

    def term():
        # term ::= ( binop ) | * binop * | WORD
        if match('('):
            tree = binop()
            if match(')'):
                return tree
            else:
                error("')'")
        elif match('*'):
            tree = binop()
            if match('*'):
                return IgnoreCase(tree)
            else:
                error("'*'")
        elif token in (')', 'AND', 'OR'):
            error("term")
        else:
            t = token
            match(token)
            return t

    def binop():
        # binop ::= term | term OR term | term AND term
        left = term()
        op = token
        if match('AND', 'OR'):
            right = term()
            return BinOp(left, op, right)
        else:
            return left

    tree = binop()
    if token != TOKEN_END:
        error("end of input")
    return tree

5. Evaluation

I'll leave the details of evaluating of the parse tree as an exercise. What we want is something like this:

def evaluate_condition(text, tree, ignorecase=False):
    """Return true if the expression given by the parse tree matches the text.

    >>> tokens = tokenize_condition("((dog OR cat) AND *Horse*)")
    >>> tree = parse_condition(tokens)
    >>> evaluate_condition("horse", tree)
    False
    >>> evaluate_condition("cat on a horse", tree)
    True
    >>> evaluate_condition("horse with a dog", tree)
    True

    """
    if isinstance(tree, str):
        # search for tree in text, ignoring case if ignorecase=True
    elif isinstance(tree, BinOp):
        # evalute tree.left (and maybe tree.right) recursively; apply op
    elif isinstance(tree, IgnoreCase):
        # evaluate tree.body, passing ignorecase=True
    else:
        # raise an error

6. Conclusion

The code here is substantially longer than the code in the post, but has these advantages:

It handles the difficult examples I gave in section 1.
It generates reasonable (if not great) syntax error messages.
Because you are in charge of generating the error messages (rather than delegating them to Python) you can improve them if you need to.
It is straightforward to extend this to handle more syntax features (such as the NOT operator).
You can start to think about how to improve the performance, for example if your have a node in the parse tree of the form BinOp(left=word1, op='OR', right=word2) then you could evaluate this in just one pass over the text instead of two. This kind of transformation is much easier to do here when you have a parse tree to work on.


Returning data reference with mutex lock guard struct I'm writing a multi-threaded program where some external part of the program needs access to a large piece of data that's controlled by my class. I'm trying to return a reference to this data in a thread-safe way.
class Chunk
{
public:

    Chunk(){
        chunkData = std::make_shared<vector<XMFLOAT3>>();
        chunkMutex = std::make_shared<std::mutex>();
    };

    struct LockedData
    {
        LockedData(std::vector<float> const& d, std::mutex& mtx) : data(d), lk(mtx) {};
        std::vector<XMFLOAT3> const& data;
    private:
        std::lock_guard<mutex>lk;
    };

    inline LockedData const&& RequestChunkData()const { return LockedData(*chunkData,chunkMutex); };

private:

    mutable std::shared_ptr<std::mutex> chunkMutex;
    std::shared_ptr<vector<float>> chunkData;

}

Is this over-complicated? will this actually protect my data, and am I using the rvalue references (&&) correctly?

Overview
This is not a good solution.
You are basically setting up a busy wait on the mutex.
You should be using a condition variable to give the OS the opportunity to reassign the processor to do other work.
Returning the LockedData is just making this much more complex than it needs to be. Use a get/release set of methods (but make them private) then use RAII to call get/release so that it is done correctly.
Code Review
class Chunk
{
    // I put private member variables at the top.
    // When writting the constructor I want to double check that
    // I have correctly set up all members.
    // I put all private methods at the bottom.
    // Not everybody agrees with me on this. But its what I like. :-)
    private:
        mutable std::shared_ptr<std::mutex> chunkMutex;
        std::shared_ptr<vector<float>> chunkData;

    public:

        // Use the initializer list to set up members
        Chunk(){
            // I don't think these need to be shared dynamically like this.
            // Just make them members of chunk.
            chunkData = std::make_shared<vector<XMFLOAT3>>();
            chunkMutex = std::make_shared<std::mutex>();
        };

        // don't use `inline` here (it means nothing).
        // Dont add && on the return type it does nothing.
        inline LockedData const&& RequestChunkData()const { return LockedData(*chunkData,chunkMutex); };

};

How I would do it:
class ChunkAccess;
class Chunk
{
    private:

        std::mutex              chunkMutex;
        std::condition_variable chunkCondition;
        std::vector<float>      chunkData;
        bool                    inUse;

    public:
        Chunk()
            : inUse(false)
        {}
    private:
        friend class ChunkAccess;
        void getUniqueAccesses()
        {
            std::lock_guard<mutex>  lock(chunkMutex);
            chunkCondition.wait(lock, [&inUse](){return !inUse;});
            inUse = true;
        }
        void releaseUniqueAccesses()
        {
            std::lock_guard<mutex>  lock(chunkMutex);
            inUse = false;
            chunkCondition.notify_one();
        }
};
class ChunkAccess
{
    private:
        Chunk&  chunk;
    public:
        ChunkAccess(Chunk& chunk)
            : chunk(chunk)
        {
            chunk.getUniqueAccesses();
        }
        ~ChunkAccess()
        {
            chunk.releaseUniqueAccesses();
        }
        // Don't want multiple access objects so prevent copy.
        ChunkAccess(ChunkAccess const&)           = delete;
        ChunkAccess operator=(ChunkAccess const&) = delete;
        // With a bit of tinkering you can make this moveable.

        // Simplify data accesses
        float&       operator[](std::size_t index)       {return chunk.chunkData[index];}
        float const& operator[](std::size_t index) const {return chunk.chunkData[index];}
};

int main()
{
    Chunk         data;
    ChunkAccess   dataAccess(data);

    std::cout << dataAccess[3] << "\n";
}

How I could do (Option 2)
class Chunk
{
    private:

        std::mutex              chunkMutex;
        std::vector<float>      chunkData;

    public:
        Chunk()
            : inUse(false)
        {}

        void access(std::function<void(std::vector<float>&)> const& action)
        {
            std::lock_guard<mutex>  lock(chunkMutex);
            action(chunkData);
        }
};

int main()
{
    Chunk         data;
    data.access([](std::vector<float>& value){
        std::cout << value[3] << "\n";
    });
}


Joining strings to form a URL I have to construct a URL string from smaller substrings. I'll usually have a resource url, an endpoint, a query, but you can imagine even more parts in creating a full URL.

If, for example, I have this resource URL: http://foo.com, this endpoint: bar/v1, and this query: q?var1=33,var2='abc', I expect the final URL to be:

http://foo.com/bar/v1/q?var1=33,var2='abc'

If the strings are formatted following a simple convention (for example, only have leading slashes on a substring) I could simply concatenate them together. The problem however is that these strings will be given as arguments to some library function calls, and hence it is very probable that some library users will miss the convention. It's so easy to add a trailing '/' or omit a leading '/'. So I do not want to impose a convention on this. Instead, I prefer to check and sanitise the arguments. I thought that urllib.parse.urljoin() would serve my purpose, but it does not. So I wrote a simple little method:

def slash_join(*args):
    '''
    Joins a set of strings with a slash (/) between them. Useful for creating URLs.
    If the strings already have a trailing or leading slash, it is ignored.
    Note that the python's urllib.parse.urljoin() does not offer this functionality.
    '''
    stripped_strings = []
    # strip any leading or trailing slashes
    for a in args:
        if a[0] == '/': start = 1
        else:           start = 0
        if a[-1] =='/':
            stripped_strings.append(a[start:-1])
        else:
            stripped_strings.append(a[start:])

    return '/'.join(stripped_strings)

Usage

>>> slash_join('http://foo.bar/', '/path/', '/query')
>>> 'http://foo.bar/path/query'

>>> slash_join('http://foo.bar', 'path', 'query')
>>> 'http://foo.bar/path/query'

>>> slash_join('http://foo.bar', 'path/', 'query/')
>>> 'http://foo.bar/path/query'

It works fine, but I was wondering if there is a more pythonic way of expressing this, or if I indeed missed a standard library method call that could have helped me.

You can simplify the solution if, instead of checking for a presence of the slash, you would str.strip() the slashes from both sides of each argument and then str.join() the arguments:

def slash_join(*args):
    return "/".join(arg.strip("/") for arg in args)

urljoin() unfortunately does not allow to join more than 2 url parts - it can only do a base and one more part. There is though this clever functools.reduce() usage that may help to adapt it here:

from urllib.parse import urljoin
from functools import reduce

def slash_join(*args):
    return reduce(urljoin, args).rstrip("/")

Note that I'm explicitly stripping a possible right slash but not sure if it is actually needed since the url with a trailing slash would still be valid - e.g. http://foo.com/test/.


A function which collates data from various API calls. Help on design-pattern Help on the appropriate design pattern for this.

Program details:

  A scheduled script that fetches all the data from various framework-based API's - executeSearch('webservice_name') - ,
  then, collate the data and pass it to another framework-API - saveToReportTemplate('report_template_name') -
  which persists the collated data to the database.

var Report = Report || {};

(function (R) {

    var _data1 = 0,
        _data2 = 0,
        _data3 = 0,
        _data4 = 0;

    function initData() {
        _data1 = 0;
        _data2 = 0;
        _data3 = 0;
        _data4 = 0;
    }

    function fetchData1() {
        //executeSearch is a framework API
        var result = executeSearch('transactions');

        _data1 = result.getValue('transactions.done');
    }

    function fetchData2() {
        //executeSearch is a framework API
        var result = executeSearch('company');

        _data2 = result.getValue('company.investors');
    }

    function fetchData3and4() {
        //executeSearch is a framework API
        var result = executeSearch('employee');
        _data3 =  result.getValue('employee.name');
        _data4 =  result.getValue('employee.salary');
    }

    function populateReport() {
        //framework API
        saveToReportTemplate('some_report_template', _data1, _data2, _data3, _data4);
    }

    R.generateReport = function () {
        initData();
        fetchData1();
        fetchData2();
        fetchData3and4();
    };

})(Report);

//usage
R.generateReport();

Need help/advice/criticisms on:

overall structure of the program
my usage of private variables and functions
usage of javascript's closure
is there anything I can improve?
any good design patterns for solving this kind of problem?

Note:
There is no way we can implement a new database table/view for this, because we are just interacting on external APIs.
Also, there is no SQL-based method provided for us to solve such problems.

Well, based on what I can get, here's some code that should work. Note that I use native Promises which is ES6 technology. It's handy, but I don't want to have jQuery tagging along. Use a polyfill for older browsers.

;(function(ns) {

  ns.generateReport = function(template, executionList) {

    // Because the operations can be async, we use promises instead. This allows
    // us to dictate when and what in the operation we resolve as a value.
    var promises = executionList.map(function(item) {
      return new Promise(function(resolve, reject) {

        //Assuming that everything is synchronous, the code most likely looks like this
        var result = search(item.search);
        var data = item.save.map(function(saveItem) {
          result.getValue(saveItem);
        });
        resolve(data);

        //If the operations are aync, then most likely it will look like
        search(item.search, function(result) {
          var data = item.save.map(function(saveItem) {
            result.getValue(saveItem);
          });
          resolve(data);
        });

      });
    });

    // We listen for all of the promises to resolve. When they do, the callback is
    // called with an array of resolved values from the promises we just mapped. They
    // will look like [[1],[2],[3,4]]
    Promises.all(promises).then(function(values) {

      // We concatenate the arrays inside the array using apply and concat. They'll
      // end up like [1,2,3,4]
      var params = Array.prototype.concat.apply([], values);

      // Add in the report template parameter so it'll look like ['template',1,2,3,4]
      params.unshift(template);
      saveToReportTemplate.apply(null, data);
    });

  }

// Uses an existing R or creates one
}(this.R = this.R || {}));

// Sample usage is this.
R.generateReport('report_template', [{
  search: 'transactions',
  save: ['transactions.done']
}, {
  search: 'company',
  save: ['transactions.done']
}, {
  search: 'company.investors',
  save: ['employee.name', 'employee.salary']
}]);