Shredding XML into an SQL table I have a few "given" things:

An SQL Server 2012 table with certain structure
Data provided as XML with certain structure

My code has to shred that XML into a row in the table. I'm looking to get feedback on how I'm getting the data for each column from the XML variable.

Here's a realistic yet minimal repro of my scenario:

-- Table structure is "a given", i.e. I *have to* work with this:
CREATE TABLE #Person
(
    Id INT IDENTITY(1,1),
    First_Name NVARCHAR(100),
    Last_Name NVARCHAR(100),
    Two_Char_Nationality VARCHAR(2),
    InsertedAt DATETIME -- My code has to manually insert the timestamp here (cannot create a default constraint)
);

-- Xml structure is "a given", i.e. I *have to* work with this:
DECLARE @data XML = '<Person>
  <FirstName>John</FirstName>
  <Surname>Doe</Surname>
  <Nationality>
    <Fullname>Netherlands</Fullname>
    <IsoAlpha2>NL</IsoAlpha2>
  </Nationality>
</Person>';

-- Looking to get feedback on this:
INSERT INTO #Person
(
    First_Name,
    Last_Name,
    Two_Char_Nationality,
    InsertedAt
)
VALUES
(
    (SELECT @data.value('(/Person/FirstName)[1]', 'NVARCHAR(100)')),
    (SELECT @data.value('(/Person/Surname)[1]', 'NVARCHAR(100)')),
    (SELECT @data.value('(/Person/Nationality/IsoAlpha2)[1]', 'VARCHAR(2)')),
    GETDATE()
);

-- For testing purposes only:
SELECT * FROM #Person
DROP TABLE #Person

As you can see I'm doing many sub-select-queries in the VALUES(...) part, which doesn't feel all too smart.

In reality, there's about 10 - 20 columns in my #Person table, and sometimes my XML is slightly more nested, but I'm trying to focus on the basics first.

Any recommendations on the INSERT statement?

INSERT INTO #Person
(
    First_Name,
    Last_Name,
    Two_Char_Nationality,
    InsertedAt
)
SELECT
    tab.p.value('FirstName[1]', 'NVARCHAR(100)'),
    tab.p.value('Surname[1]', 'NVARCHAR(100)'),
    tab.p.value('(Nationality/IsoAlpha2)[1]', 'VARCHAR(2)'),
    GETDATE()
FROM
    @data.nodes('/Person') tab(p);

SELECT or Query nodes in hierarchial or nested XML


Java Slick 2D Collision Detection Methodology I have a collision detection for terrain setup (non-terrain is handled with a quadtree I implemented apart from this) and I was wondering how others have done it compared to mine and what I could learn from people's critiques here.

My map is stored in a 2D array of "GameSquare"s

public class GameSquare extends GameObject {
    private char tileLetter = 'W';
    private int row, col;
}
public boolean isPassable() {
    return tileLetter != 'W';
}

The row and col are the index in the 2D array.
The char corresponds to the graphic, my render checks the char and draws a Wall(W), Floor(O), or Door(X)
The Player
My player class gets track of the player in pixels. The speed and X,Y positions are all in exact pixels. I divide those measures by the width or height of the tile graphics I use and truncate it; in order to find the corresponding array cell that the player is in. In order to do collision detection I wrote a method that takes a cardinal direction, the players current collision box, and a distance which is usually their speed.
The cardinal direction is an enum that has methods for the adjX and adjY.
For example; East would return adjX() = 1 and adjY() = 0 whereas
SouthEast would return adjX() = 1 and adjY() = 1 <-- Positive because this is the movement in a 2D array.

The Collision Check

public int canMove(Direction dir, Rectangle collisionBox, int distance) {
    int goodToGo = 0;
    // Check each 'step' of our attempted movement from 1 - distance(ie speed)
    for(int i = 1; i <= distance; i++) {
        // Assemble suggested new position bounds to check, ie corners of suggested movement
        HashSet<Vector2f> cornerPoints = new HashSet<Vector2f>();
        cornerPoints.add( new Vector2f(collisionBox.getMinX()+dir.adjX()*i, collisionBox.getMinY()+dir.adjY()*i) );
        cornerPoints.add( new Vector2f(collisionBox.getMaxX()+dir.adjX()*i, collisionBox.getMinY()+dir.adjY()*i) );
        cornerPoints.add( new Vector2f(collisionBox.getMinX()+dir.adjX()*i, collisionBox.getMaxY()+dir.adjY()*i) );
        cornerPoints.add( new Vector2f(collisionBox.getMaxX()+dir.adjX()*i, collisionBox.getMaxY()+dir.adjY()*i) );

        // Check each corner of this step
        for( Vector2f point : cornerPoints ) {
            // Out of bounds check
            if( point.y < 0 || point.y > this.dungeonHeight*ratioRow ||
                    point.x < 0 || point.x > this.dungeonWidth*ratioCol ) {
                return goodToGo;
            }

            // Figure out what map square this point falls on
            int checkX = (int) (point.x / ratioCol);
            int checkY = (int) (point.y / ratioRow);

            // If that's a wall square... no go, we're done here, return the number of steps that
            // were OKed so far
            if( this.dungeon[checkY][checkX].getTileLetter('W') ) {
                return goodToGo;
            }
        }
        // If this step, i, didn't have any conflicts on any of the proposed new corners, then
        // increment our ongoing count of how many steps are OK to take
        goodToGo++;
    }
    return goodToGo;
}

I know blocks of code are kind of a bitch but any feedback or just shared attempts at 2D collision detection in Slick/Java are very appreciated.

That really looks quite slow.

First of, don't use HashSet in these situations. A HashSet is a HashMap with "blind" values - like using the key as a value, too. They guarantee uniqueness and fast access (as everyone knows!), but for a set of four objects in a tight and often called loop, you are really hurting performance - the creation overhead is far too high and lookup speed isn't that great.

Next, you should probably change your usage pattern. Don't store distances in int if everything else is float. Same goes the other way around - the conversion between int and float is not free and you should avoid it in code that has to be really fast.

It looks like you never use fractional values and always integers - even in your floats - so you probaly think you can get away with them. They are used as painting positions for the tiles anyway.

Still, even for pixel-precise games, as soon as you change to float, everything starts to get smoother. Because you get sub-pixel movements, which wouldn't be possible for int. You can't display them, but they still affect percieved smoothness. I'd go for float, always keep positional data as floats and only convert to int when I need it to draw stuff.

Next, you can use a vastly better collision check method.

Vastly better in most cases as least. If every object is rectangular, you'd be ok with your way - but there's a lot of conditionals involved and you end up with code like this:

if (!(a.maxY < b.minY || a.minY > b.maxY)) blocked = true; ....

The other (better) way:

Use bounding circles. For each object, you store float x, y, boundsRadius; - the center and the distance to the entities point with the largest distance from the center.

Then... pythagoras (a*a + b*b = c*c)! For entities e1 and e2:

public static boolean maybeCollision(Entity e1, Entity e2) {
    float xDistance = e1.x - e2.x,
          yDistance = e1.y - e2.y,
          rSum      = e1.boundsRadius + e2.boundsRadius
    ;
    return xDistance * xDistance + yDistance * yDistance < rSum * rSum;
}

It's like calculating the distance - Math.sqrt(dx * dx + dy * dy) - but we don't use square root (which is on the slow side) and square the sum of the radius instead. You can use float or int - it doesn't matter as long as there is no overflow. Just change the local variables.

I really love how it's totally independend of the orientation of the objects towards each other - you don't need any ifs, the square of the distance automatically makes the result positive and it just works.

Here, I call the method maybeCollision (and that's a bad name, as it's not a verb. Still fits!) because you still have to check per pixel - it sees anything inside of the circle as a collision. But it's lightning fast and you have to check with bounding boxes, too (as long as you don't write Tetris or a round-based Minecraft)...


Secure row insertion using a position-based prepared statement I have the following function to insert a new row into a table:

/**
 * Insert row into the designated table
 *
 * Function to add a new row into the designated table
 *
 * @param str $table            Table name to insert the row
 * @param arr $data             Array with values
 *
 * @return int row              Last inserted ID
 */
public function database_add_row($table, $data) {

    try {
        $fields = '';
        foreach ($data as $field) {
            $fields.= '?,';
        }
        $fields = substr($fields,0,-1);
        $sql = "INSERT INTO ".$table." VALUES(".$fields.")";
        $sth = $this->database->prepare($sql);
        $sth->execute($data);
        return $this->database->lastInsertId();

    }
    catch(PDOException $e) {
        throw new userman_Exception("
ups!

". $e->getMessage());
    }

}

I'm counting the values in the array to prepare the query, and adding placeholders for each position, then subtracting the last comma from the generated string. This avoids using the field names, which leads to a table agnostic function,

Considerations:

All columns values are passed with the $data array.
The function is designed to insert on any table with any number of columns.

My question is:

Is my method adequate, leading to a secure row insertion?

As noted in your other question, that exception handling completely masks the true exception.  It adds no value yet detracts.  There's no reason to do that.  Just allow the exception to throw out of the function.

As for 'security', assuming you properly control the table name, it is safe.  (A better way to phrase that may be that SQL injection is not possible as long as you control $table.)

I might consider writing it a bit differently though:

public function database_add_row($table, $data) {
    $placeholders = implode(', ', array_fill(0, count($data), '?'));
    $sql = "INSERT INTO {$table} VALUES ({$placeholders})";
    $stmt = $this->database->prepare($sql);
    if ($stmt->execute($data)) {
        return $this->database->lastInsertId();
    } else {
        return false;
    }
}

I also might consider using associative arrays.  That would allow you to specify columns (which could be useful if you had default-valued columns or auto incrementing columns).

public function database_add_row($table, $data) {
    $cols = array_keys($data);
    $columns = implode(', ', $cols);
    $placeholders = implode(', ', array_map(function($c) { return ":{$c}"; }, $cols));
    $sql = "INSERT INTO {$table} ({$columns}) VALUES ({$placeholders})";
    $stmt = $this->database->prepare($sql);
    if ($stmt->execute($data)) {
        return $this->database->lastInsertId();
    } else {
        return false;
    }
}

Then you would use it like:

$obj->database_add_row("table1", array('column1' => 'val1', 'column2' => 'val2'));

Depending on how far you plan to go with your DB abstractions, you might want to consider a DBAL or ORM library.  The Doctrine Project has quite widely used versions of both of those.


Take a number and return English language representation Like many, I started out with procedural programming.  Of course, when learning a functional language, old habits may die hard, so I wrote a fairly trivial little thing which takes an integer and returns a list of the english-language representation.

A few days later, I re-wrote it to try to take advantage of tail recursion in the main order-of-magnitude function; I freely admit that no attempt has been made to add tail-recursion to the calculation of numbers less than 1000, but since those have a fixed maximum depth of 3 (and a maximum of 5 calls in any case) I opted not to worry about it for now.

Any notes and criticisms, please lob them at me, that I may learn to do things in a less procedural way.

-module(titoa).
-export([itoa/1]).

itoa(0)                    -> "zero";
itoa(N) when is_float(N)   -> no_float_support;
itoa(N) when N < 0         -> "negative " ++ itoa(abs(N));
itoa(N) when is_integer(N) -> itoa_render(N);
itoa(_)                    -> severe_error.

itoa_render(N) when N >= 1100, N < 10000, N rem 100 == 0, N rem 1000 /= 0 ->
    itoa_render(N, ["", "hundred"], 100, []);
itoa_render(N) when N >= 1000 ->
    itoa_render(N,["",
        "thousand","million","billion","trillion","quadrillion","quintillion",
        "sextillion","septillion","octillion","nontillion","dectillion"
        ],1000, []
    );
itoa_render(N) when N >= 100, N rem 100 == 0 ->
    itoa_render(N div 100) ++ " hundred";
itoa_render(N) when N >= 100 ->
    Hun_diff = N rem 100,
    itoa_render(N - Hun_diff) ++ [32 | itoa_render(Hun_diff)];
itoa_render(N) when N > 19, N rem 10 == 0 ->
    lists:nth(N div 10 - 1, [
        "twenty","thirty","forty","fifty","sixty","seventy","eighty","ninety"
    ]);
itoa_render(N) when N > 19 ->
    Ten_diff = N rem 10,
    itoa_render(N - Ten_diff) ++ "-" ++ itoa_render(Ten_diff);
itoa_render(N) when N > 0 ->
    lists:nth(N, [
        "one","two","three","four","five","six","seven","eight","nine",
        "ten","eleven","twelve","thirteen","fourteen","fifteen","sixteen",
        "seventeen","eighteen","nineteen"
    ]);
itoa_render(_) -> []. % 0

itoa_render(0, _, _, After) -> After;
itoa_render(_, [], _, _) -> overflow;
itoa_render(N, [Magnitude | Remaining_Magnitudes], Factor, After) ->
    This_OOM = itoa_render(N rem Factor),
    This_Rep = if
        This_OOM  == [] -> [];
        Magnitude == [] -> This_OOM;
        true            -> This_OOM ++ [32 | Magnitude] ++ if
            After     == [] -> [];
            true            -> [32]
        end
    end,
    itoa_render(N div Factor, Remaining_Magnitudes, Factor, This_Rep ++ After).

At first glance, you're trying to be a bit cute here and it's making your code more complex than necessary.  For example, why not handle 11 and 12 as part of the teens and why not just type out the teen and decade names in full rather than having extra logic to append a not-quite common suffix ("teen" and "ty")?

For n < 1000 you have the following rules (let <n> mean "n in words", % mean modulo, and // mean integer division):

if 100 <= n then <n // 100> " hundred" ++
                 (if n % 100 == 0 then "" else " and " ++ <n % 100>)
if 20 <= n then  {"", "twenty", ..., "ninety"}[n // 10] ++
                 (if n % 10 == 0 then "" else " " ++ <n % 10>)
otherwise        {"zero", "one", "two", ..., "ten", "eleven", ..., "nineteen"}[n]

Now you only need to deal with the orders of magnitude ("thousand", "million", etc.).


Caching large object in multithreading environment I'm having to dabble with caching and multithreading (thread per request), and I am an absolute beginner in that area, so any help would be appreciated.

My project requirements are:

Cache one single large object that has ether interval refresh or refresh from user
Because retrieving object data is very time consuming make it thread-safe
When retrieving object data return "old data" until new data is available
Optimize it

public enum DbCachedObject {

INSTANCE;

private final CountDownLatch initLock = new CountDownLatch(1);
private final Object refreshLock = new Object();
private final AtomicReference<CachedObject> cachedInstance = new AtomicReference<CachedObject>();
private final AtomicLong lastUpdate = new AtomicLong();
private volatile boolean refreshing;
private long cachePeriodMs = 1000L;

public CachedObject get() {

    CachedObject o = cachedInstance.get();
    if (o == null || isCacheOutdated()) {
        updateCache();
        if (o == null) {
            awaitInit();
            o = cachedInstance.get();
        }
    }
    return o;
}

public void refresh() {
    updateCache();
}

private boolean isCacheOutdated() {
    return (System.currentTimeMillis() - lastUpdate.get() > cachePeriodMs);
}

private void updateCache() {

    synchronized (refreshLock) {
        // prevent users from refreshing while an update is already in progress
        if (refreshing) {
            return;
        }
        refreshing = true;
        // prevent other threads from calling this method again
        lastUpdate.set(System.currentTimeMillis());
    }
    new Thread() {
        @Override
        public void run() {
            try {
                cachedInstance.set(getFromDb());
                // set the 'real' last update time
                lastUpdate.set(System.currentTimeMillis());
                initLock.countDown();
            } finally {
                // make sure refreshing is set to false, even in case of error
                refreshing = false;
            }
        }
    }.start();
}

private boolean awaitInit() {

    boolean initialized = false;
    try {
        // assume cache-period is longer as the time it takes to create the cached object
        initialized = initLock.await(cachePeriodMs, TimeUnit.MILLISECONDS);
    } catch (Exception e) {
        e.printStackTrace();
    }
    return initialized;
}

private CachedObject getFromDb() {
    // dummy call, no db is involved
    return new CachedObject();
}

public long getCachePeriodMs() {
    return cachePeriodMs;
}
}

I surely dislike your mixing of various mean synchronization. Using synchronized and Atomic* and also volatile together looks very strange. However, it looks like you got it right (a deeper analysis would be needed).

private volatile boolean refreshing;

You need no volatile if you access it inside the synchronized block only.

private long cachePeriodMs = 1000L;

I'd suggest cachePeriodMillis. It's only slightly longer and there's a precedent.

I guess, I'd drop the latch, define a non-anonymous Runnable for getting from the DB and let the first interested thread execute it directly. The others would wait on the corresponding synchronized block's entry (and then leave immediately).

I'd probably also aggregate CachedObject and lastUpdate into one object as they belong together. This might be a problem w.r.t "prevent other threads from calling this method again", but it should be possible to eliminate the need for this.

This way I might get a simpler design which I could trust more.


Quick sort implementation Please look the code below

#include<stdio.h>
void quicksort(int *,int ,int);
int partition1(int *,int ,int);
void swap1(int *,int *);

int main()
{

int low = 0;
int length;
int i;
int a[100];
int high;

printf("Enter  the length of array\n");
scanf("%d",&length);

high=length-1;

printf("Enter the array elememts\n");

for(i=0;i<=high;i++)
{
 scanf("%d",&a[i]);
}

quicksort(a,low,high);

printf("The sorted array\n");

for(i=low;i<=high;i++)
{
  printf("%d--",a[i]);
}

 printf("\n");
 return 0;
}

void quicksort(int a[],int low,int high)
{
   int p;

  if(low<high)
  {
     p=partition1(a,low,high);
     quicksort(a,low,p-1);
     quicksort(a,p+1,high);
  }
}

 int partition1(int a[],int low,int high)
{
   int pivot=a[high];
   int i=low-1;
   int j=low;

   for(j=low;j<high;j++)
  {
      if(a[j]<a[high])
      {
         i++;
         swap1(&a[i],&a[j]);
      }
  }
        swap1(&a[i+1],&a[high]);
        return i+1;
}

  void swap1(int* a,int* b)
 {
   int  temp;
   temp=*a;
   *a=*b;
   *b=temp;
 }

This is the working program of quick sort.Can anyone tell me the way to make this program much better?

  please do the necessary changes  required.

Coding conventions

Have at least one empty line after all #includes.
Have one space character before and after every binary operator. For example, a + 1 instead of a+1. That makes your code easier to read.
Have one space after each for keyword: not for(i=1;i<10;i++), but rather  for (i = 1; i < 10; i++). (Same applies to if.)
Fix your indentation:

for(j=low;j<high;j++)
  {
      if(a[j]<a[high])
      {
         i++;
         swap1(&a[i],&a[j]);
      }
  }

to

for(j=low;j<high;j++)
{
    if(a[j]<a[high])
    {
        i++;
        swap1(&a[i], &a[j]);
    }
}

Instead of writing &a[index] you can write simply a + index.
Whenever printing a simple (non-formated) standard output, use puts instead of printf. (Also, note that puts automatically adds the new line character after the argument string.)

All the above points are relevant to readability of your source code.

Summa summarum

All in all, I had this in mind:

#include<stdio.h>

void quicksort(int*, int, int);
int partition(int*, int, int);
void swap(int*, int*);

int main()
{
    int low = 0;
    int length;
    int i;
    int a[100];
    int high;

    puts("Enter the length of array:");
    scanf("%d", &length);

    high = length - 1;

    puts("Enter the array elements:");

    for (i = 0; i <= high; i++)
    {
        scanf("%d", &a[i]);
    }

    quicksort(a, low, high);

    puts("The sorted array:");

    for (i = low; i <= high; i++)
    {
        printf("%d ",a[i]);
    }

    puts("");
    return 0;
}

void quicksort(int a[],int low,int high)
{
    int p;

    if (low < high)
    {
        p = partition(a, low, high);
        quicksort(a, low, p - 1);
        quicksort(a, p + 1, high);
    }
}

int partition(int* a, int low, int high)
{
    int pivot = a[high];
    int i = low;
    int j;

    for (j = low; j < high; ++j)
    {
        if (a[j] <= pivot)
        {
            swap(a + i++, a + j);
        }
    }

    swap(a + i, a + high);
    return i;
}

void swap(int* a, int* b)
{
    int temp = *a;
    *a = *b;
    *b = temp;
}

Hope that helps.


Wrapping my head around generators I am going on with refactoring my code base while migrating from Python 2 to Python 3. I am using generators to make reusable components this time as I am more comfortable with them this time then last time. But there are times when I am stuck at their use. Like below example. These two are essentially same with one infinitely generating and other with a limit.

Is there a way to make these as one function? Normally an extra optional parameter as limit will do the trick but with generators I am not sure how to use them. Is there some way to do that?

def fibonacci_inf(a, b):
    """Lazily generates Fibonacci numbers Infinitely"""
    while True:
        yield b
        a, b = b, a + b

def fibonacci(a, b, num):
    """Lazily generates Fibonacci numbers"""
    while b < num:
        yield b
        a, b = b, a + b

You could just implement the infinite generator, and use itertools.takewhile to limit it:

>>> list(itertools.takewhile(lambda x : x < 8, fibonacci_inf(0,1)))
[1, 1, 2, 3, 5]


Shortest and longest city name and alphabetical order Problem

  Query the two cities in STATION with the shortest and longest CITY
  names, as well as their respective lengths (i.e.: number of characters
  in the name). If there is more than one smallest or largest city,
  choose the one that comes first when ordered alphabetically.

  Sample Input

  Let's say that CITY only has four entries: DEF, ABC, PQRS and WXY

  Sample Output

  ABC 3 PQRS 4

Can I get shorter code in MySQL and also an optimal Oracle query? There's no way to have only one sub-query for both min and max, is there?

select city, char_length(city) from STATION
 where city = (select min(city) from STATION
     where char_length(city) = (select min(char_length(city)) from STATION))

 or city = (select min(city) from STATION
     where char_length(city) = (select max(char_length(city)) from STATION));

Formatting

Your SQL statement formatting is not very good. I would encourage you to use a free tool like sql-format.com (or one of the many others) to format your SQL queries in a more readable way. This is your original query, with better formatting (I indented the subqueries by hand because the tool didn't).

SELECT
    city,
    CHAR_LENGTH(city)
FROM STATION
WHERE city = (
    SELECT
        MIN(city)
    FROM STATION
    WHERE CHAR_LENGTH(city) = (
        SELECT
            MIN(CHAR_LENGTH(city))
        FROM STATION
    )
)
OR city = (
    SELECT
        MIN(city)
    FROM STATION
    WHERE CHAR_LENGTH(city) = (
        SELECT
            MAX(CHAR_LENGTH(city))
        FROM STATION
    )
);

Now we can see much more easily how deeply nested queries are.

Variables

You could make the code simpler by using a few user-defined variables:

SET @MinCityLen = (SELECT MIN(CHAR_LENGTH(city)) FROM STATION);
SET @MaxCityLen = (SELECT MAX(CHAR_LENGTH(city)) FROM STATION);
/* Query below only to demonstrate the variables */
SELECT
  '@MinCityLen' AS `VariableName`,
  @MinCityLen AS `Value`
UNION
SELECT
  '@MaxCityLen',
  @MaxCityLen;

Which returns this:

VariableName  Value
@MinCityLen   3
@MaxCityLen   4

This will abstract away one level of nesting and make the query simpler to understand. Note that the @ symbol is just a convention, it is not needed as far as MySQL syntax goes. Some database systems do require the @ symbol though, the most famous being Microsoft SQL Server.

Shorter code != Better code

Sometimes, better code is longer rather than shorter. This is often the case with SQL. Longer code can be better formatted, better documented, better abstracted, better structured. In my personal SQL experience, short queries, unless they are very simple, are often pretty bad. Yours is not a case of a very simple query.

This is what I came up with. I also have a link on sqlfiddle

# find shortest city name
SET @MinCityLen = (SELECT MIN(CHAR_LENGTH(city)) FROM STATION);
# find longest city name
SET @MaxCityLen = (SELECT MAX(CHAR_LENGTH(city)) FROM STATION);

SELECT
    city,
    CHAR_LENGTH(city)
FROM
    STATION
WHERE
    # find shortest city name sorted alphabetically
    city = (
        SELECT
            city
        FROM STATION
        WHERE CHAR_LENGTH(city) = @MinCityLen
        ORDER BY city ASC
        LIMIT 1
    )
    # find longest city name sorted alphabetically
    OR city = (
        SELECT
            MIN(city)
        FROM STATION
        WHERE CHAR_LENGTH(city) = @MaxCityLen
        ORDER BY city ASC
        LIMIT 1
    );


Retrieving my public IP address with the javascript Fetch API (no jquery, no json) Note for future reference and google searches
The list of urls for IP retrieving will very likely need continuous adjustments in the future.
// Few services that should return the IP as plain text,
// properly configured to accept CORS
const IP_URLs = [
  'https://api.ipify.org/',
  'https://ipecho.net/plain',
  'https://api.kwelo.com/v1/network/ip-address/my',
  'https://myexternalip.com/raw',
];

Original question
This code is for retrieving the public IP address. I wanted something that can automatically switch between a few services in case the some of them are down. Since I do not use jquery in my project, I preferred the Fetch API aiming at keeping the code as light as possible. I was not able to find anything similar in the web, so here we go!
I am mostly concerned about correctness, but since I consider myself a beginner in JS, also readability, style, indentation are all very relevant. Should I have used async/await?
// Few services that should return the IP as plain text, properly configured to accept CORS
const IP_URLs = [
  'https://api.ipify.org/',
  'https://ipecho.net/plain',
  'https://api.kwelo.com/v1/network/ip-address/my',
  'https://myexternalip.com/raw',
];

// Log my public IP in the console
get_ip(IP_URLs, console.log);

// Trampoline that deep copies the urls array to later allow for pop() and recursion
function get_ip(urls, callback) {
  get_ip_impl(urls.slice(), callback);
}

function get_ip_impl(urls, callback) {
  if (urls.length > 0) {
    let url = urls.pop();
    let init = {
      method: 'GET', // *GET, POST, PUT, DELETE, etc.
      mode: 'cors', // no-cors, *cors, same-origin
      cache: 'no-store', // *default, no-cache, reload, force-cache, only-if-cached
      credentials: 'same-origin', // include, *same-origin, omit
      headers: {
        'Content-Type': 'application/text'
      },
      redirect: 'follow', // manual, *follow, error
      referrerPolicy: 'no-referrer', // no-referrer, *no-referrer-when-downgrade, origin, origin-when-cross-origin, same-origin, strict-origin, strict-origin-when-cross-origin, unsafe-url
    };
    fetch( url, init).then(
      response => {
        response.text().then(
          txt   => { if (isValidIP(txt)) {
                       callback(txt);
                     } else {
                       console.log("Invalid IP: "+url+" --> "+txt);
                       get_ip_impl(urls, callback);
                     }
                   },
          notxt => { console.log("text(): "+url+" --> "+notxt);
                     get_ip_impl(urls, callback);
                   }
        )
      },
      reason => { console.log("Fetch: "+url+" --> "+reason);
                  get_ip_impl(urls, callback); }
    );
  } else {
    callback("No more URLs");
  }
}

function isValidIP(txt) {
  return /^(?!0)(?!.*\.$)((1?\d?\d|25[0-5]|2[0-4]\d)(\.|$)){4}$/.test(txt);
}

One of the main reasons Promises were created was to avoid the ugly nesting of callbacks when multiple asynchronous actions need to occur. For example, rather than

fn1(dataToSend, dataForFn2 => {
  fn2(dataForFn2, dataForFn3 => {
    fn3(dataForFn3, processedData => {
      console.log(processedData);
    });
  });
});

If the functions returned Promises instead of accepting callbacks, the above could be done with

fn1(dataToSend)
  .then(fn2)
  .then(fn3)
  .then(processedData => {
    console.log(processedData);
  });

The Promise-as-callback antipattern is when you nest .thens in other .thens or pass around callbacks rather than chaining another .then onto the previous Promise. This is the trap you're falling into here. Since fetch returns a Promise itself, it would be best to return that Promise and allow consumers of this script to use it. That is, it would be really convenient (and appropriate) if consumers to be able to do something like

get_ip(IP_URLs)
  .then(console.log)
  .catch(handleErrors);

Another related nesting issue you might consider is, instead of

if (goodCondition) {
  // lots of code
} else {
  callback("No more URLs");
}

Rather than having to keep track of which brackets correspond to what upper condition, it might be preferable to return early instead:

if (!goodCondition) {
  callback("No more URLs");
  return;
}
// lots of code

But if you return the Promises instead of using callbacks, this should cease to be an issue.

The init object you're constructing seems strangely complicated. Many of the properties you're passing are the default ones already (like method: 'get', redirect: 'follow') or useless (credentials: 'same-origin'). Unless you're deliberately trying to affect some things with the object, it would be simpler and less confusing to omit it entirely (the script looks to work just fine without it, none of the properties look useful).

You have a lot of fail-handlers. If you really want to exhaustively identify what's OK and what isn't, it would be good to also check if the response is OK too:

return fetch(url).then(
  response => {
    if (!response.ok) {
      console.log('Response was not OK:', response.status);
      return getIPFromNextSite(urls);
    }

But unless you're actively analyzing exactly which stage is working and what isn't, and really need all those .fail handlers, you might consider avoiding them and .catching errors at the end instead:

function getIPFromNextSite(urls) {
  if (!urls.length) {
    // Send control flow to the consuming .catch block:
    throw new Error('No more URLs');
  }
  const url = urls.pop();
  return fetch(url)
    .then(response => response.text())
    .then((txt) => {
      if (isValidIP(txt)) {
        // Send control flow to the consuming .then block:
        return txt;
      } else {
        return getIPFromNextSite(urls);
      }
    })
    .catch((err) => {
      return getIPFromNextSite(urls);
    });
}

It's only a comment in your code, but .slice does not deep copy arrays - it only shallow copies them. (Since the array here contains immutable strings, a shallow copy is fine anyway.)

If you want to follow the standard Javascript naming conventions, you could consider using camelCase for functions and most variables by default.

When declaring variables, best to always use const. Using let warns readers of the code that you may reassign the variable name, which results in more cognitive overhead (and is confusing if there isn't actually any chance of the name being reassigned).

In full, this is how I would do it:

const IP_URLs = [
  'https://api.ipify.org/',
  'https://ipecho.net/plain',
  'https://api.kwelo.com/v1/network/ip-address/my',
  'https://myexternalip.com/raw',
];
function getIPFromNextSite(urls) {
  if (!urls.length) {
    // Send control flow to the consuming .catch block:
    throw new Error('No more URLs');
  }
  const url = urls.pop();
  return fetch(url)
    .then(response => response.text())
    .then((txt) => {
      if (isValidIP(txt)) {
        // Send control flow to the consuming .then block:
        return txt;
      } else {
        return getIPFromNextSite(urls);
      }
    })
    .catch((err) => {
      return getIPFromNextSite(urls);
    });
}
function isValidIP(txt) {
  return /^(?!0)(?!.*\.$)((1?\d?\d|25[0-5]|2[0-4]\d)(\.|$)){4}$/.test(txt);
}
const getIP = urls => getIPFromNextSite(urls.slice());

getIP(IP_URLs).then(console.log);

  Should I have used async/await?

You could. If you need to exhaustively check every error path, it would require a lot of ugly try/catch boilerplate, but if you just want a result, it looks moderately cleaner than the version above:

const IP_URLs = [
  'https://api.ipify.org/',
  'https://ipecho.net/plain',
  'https://api.kwelo.com/v1/network/ip-address/my',
  'https://myexternalip.com/raw',
];
async function getIPFromNextSite(urls) {
  for (const url of urls) {
    try {
      const response = await fetch(url);
      const text = await response.text();
      if (isValidIP(text)) {
        // Send control flow to the consuming .then block:
        return text;
      }
    } catch(e) {
      // Don't do anything, just continue on to next iteration
    }
  }
  // Send control flow to the consuming .catch block:
  throw new Error('No more URLs');
}
function isValidIP(txt) {
  return /^(?!0)(?!.*\.$)((1?\d?\d|25[0-5]|2[0-4]\d)(\.|$)){4}$/.test(txt);
}
const getIP = urls => getIPFromNextSite(urls);

getIP(IP_URLs).then(console.log);


Optimizing for data import in Neo4j using py2neo Here is my code for importing from a .csv to a neo4j graph using py2neo and cypher statements. I've noticed that it slows down significantly the bigger the graph gets. It takes several seconds just to parse ~10,000 lines. I'm curious to see if there are any glaring mistakes or improvements to be made.

Before doing any of the import, I created indexes for every node and property, based off of this blog.

The machine I'm working on is windows 7 with 65gb RAM, so it's certainly fast enough to handle a large graph. The upload of a 20 million line csv took weeks, so there has to be something that can be improved on.

I am currently stuck with using windows, I can't switch over to Linux. I also can't use Jexp's batch importer unfortunately.

import csv
import sys
import os
from py2neo import neo4j,node, rel, cypher
import time

def main():

    f = "C:file_path/file.csv"

    graph_db = neo4j.GraphDatabaseService("http://localhost:7474/db/data/")

    with open(f, 'r+') as in_file:
        reader = csv.reader(in_file, delimiter = ',')
        next(reader, None)  # skip headers
        batch = neo4j.WriteBatch(graph_db)

        try:
           i = 0
           j = 0
            for row in reader:
                if row:
                    if (i == 10000):
                        print j, "processed"
                        i = 0
                    i += 1
                    j += 1
                    character = strip(row[0])
                    first_name = strip(row[1])
                    last_name = strip(row[2])
                    actor = strip(row[3])
                    character_birth = strip(row[4])
                    character_death = strip(row[5])
                    allegiance = strip(row[6])
                    house = strip(row[7])
                    territory = strip(row[8])
                    region = strip(row[9])

                    query = neo4j.CypherQuery(graph_db,
                    """CYPHER 2.0
                       merge (character:Character {Character: {a}, First_Name:{b}, Last_Name:{c}, Actor:{d}, Birth:{e}, Death:{f}})
                       merge (house:House{House:{g}, Allegiance:{h}})
                       merge (territory:Territory {Territory: {i}})
                       merge (region:Region {Region: {j}})
                       merge (character)-[:Of_House{House:{k}}]-(house)-[:Is_From]->(territory)-[:Is_In]->(region)
                       """)
                    result = query.execute(a = character, b = first_name, c = last_name, d = actor, e = character_birth, f = character_death, g = house, h = allegiance, i = territory, j = region, k = house)

    except Exception as e:
        print e, row, reader.line_num #print the line number to fix any data errors in file

def strip(string): return''.join([c if 0 < ord(c) < 128 else ' ' for c in string]) #removes non utf-8 chars from string within cell

if __name__ == '__main__':
    start=time.time()
    main()
    end = time.time() - start
    print "Time to complete:", end

Here is the updated solution with batching. I found that the request times out of the batch size is much larger than 1000, unfortunately and I'm not sure what causes that:

import csv
from py2neo import Graph
import time

def main():

    f = "C:file_path/file.csv"
    graph = Graph("http://localhost:7474/db/data/")
    with open(f, 'r+') as in_file:

        reader = csv.reader(in_file, delimiter=',')
        next(reader, None)
        batch = graph.cypher.begin()

        try:
            i = 0;
            j = 0;
            for row in reader:
                if row:
                    character = strip(row[0])
                    first_name = strip(row[1])
                    last_name = strip(row[2])
                    actor = strip(row[3])
                    character_birth = strip(row[4])
                    character_death = strip(row[5])
                    allegiance = strip(row[6])
                    house = strip(row[7])
                    territory = strip(row[8])
                    region = strip(row[9])
                    query = """
                        merge (character:Character {Character: {a}, First_Name:{b}, Last_Name:{c}, Actor:{d}, Birth:{e}, Death:{f}})
                        merge (house:House{House:{g}, Allegiance:{h}})
                        merge (territory:Territory {Territory: {i}})
                        merge (region:Region {Region: {j}})
                        merge (character)-[:Of_House{House:{k}}]-(house)-[:Is_From]->(territory)-[:Is_In]->(region)
                    """
                    batch.append(query, {"a":character, "b": first_name, "c": last_name, "d":actor, "e":character_birth, "f":character_death, "g":house, "h": allegiance, "i":territory, "j":region, "k":house})
                    i += 1
                    j += 1
                batch.process()

                if (i == 1000): #submits a batch every 1000 lines read
                    batch.commit()
                    print j, "lines processed"
                    i = 0
                    batch = graph.cypher.begin()
            else: batch.commit() #submits remainder of lines read
            print j, "lines processed"

        except Exception as e:
            print e, row, reader.line_num

def strip(string): return''.join([c if 0 < ord(c) < 128 else ' ' for c in string]) #removes non utf-8 chars from string within cell

if __name__ == '__main__':
    start = time.time()
    main()
    end = time.time() - start
    print "Time to complete:", end


Project Euler #10 in Cython I'm trying to teach myself some Cython. To do so, I use Project Euler #10:

  The sum of the primes below 10 is 2 + 3 + 5 + 7 = 17.
    2 Find the sum of all the primes below two million.

My Cython code is:

from libcpp.vector cimport vector
from libcpp cimport bool

SOLL = 142913828922

def run(unsigned long top = 2000000):

    cpdef unsigned long ist = 2
    cdef vector[bool] pos
    pos.resize(top, True)
    top //= 2

    # reduced_prime is (prime - 1)/2, since all primes (except 2) are uneven
    cpdef unsigned long reduced_prime, ind, prime

    for reduced_prime in range(1, top):
        if pos[reduced_prime]:
            prime = reduced_prime*2 + 1

            for ind in range(prime + reduced_prime, top, prime):
                pos[ind] = False

            ist += prime

    return ist

which needs about .19s to run. The following C++ code only needs .01s:

#include <vector>
#include <iostream>

int main() {
    unsigned long top = 2000000;
    unsigned long ist = 2;

    std::vector<bool> pos(top, 1);
    top /= 2;

    for (unsigned long reduced_prime = 1; reduced_prime < top; reduced_prime++) {
        if (pos[reduced_prime]) {
            unsigned long prime = reduced_prime*2 + 1;

            for (unsigned long ind = prime + reduced_prime; ind < top; ind += prime) {
                pos[ind] = 0;
            }

            ist += prime;
        }
    }

   std::cout << ist << std::endl;
}

Therefore I'm under the impression my Cython code is suboptimal.

For the off-chance it matters, Cython code was compiled with:

  cython src/e010.pyx -3 --cplus -o src/e010.cpp
  g++ -shared -pthread -fPIC -fwrapv -O2 -Wall -fno-strict-aliasing -I/usr/include/python3.2mu src/e010.cpp -o src/e010.so

C++ code was compiled by:

  g++ e10.cpp -O2 -o e10

Timings are taken with time on Ubuntu 12.04.

Change the inner loop to use old Pyrex syntax:

        for ind from prime + reduced_prime <= ind < top by prime:
            pos[ind] = False

Apparently Cython still does not optimize range loops to pure C when step size is a variable: ticket #546.


Bash script that lowercases files I have the following bash script that:

Finds all files with .cfc and .cfm extension and converts them to lowercase
Stores the relative file paths to those files (filenames.txt)
Chops those files to get only the name, excluding the extension (files.txt)
Loops through 1500 files checking for references to any of the other 1500 files and converts those to lower case

#!/bin/bash
# Search for references to JS function is all .cfm and .cfc functions
# Prompt to make sure
while true; do
    read -p "All .cfc and .cfm files in the theradoc/ directory and lower directories will be converted. Do you wish to contiue? (y/n)" yn
case $yn in
    [Yy]* ) make install; break;;
    [Nn]* ) exit;;
    * ) echo "Please answer yes or no.";;
esac
done
echo "Renaming files..."
for f in `find theradoc/ -d -name '*.cfc'`;
    do mv -v $f `echo $f | tr '[A-Z]' '[a-z]'`
done
for f in `find theradoc/ -d -name '*.cfm'`;
    do mv -v $f `echo $f | tr '[A-Z]' '[a-z]'`
done
echo "Indexing file names..."
find theradoc/ -d -name '*.cfc' > filenames.txt
find theradoc/ -d -name '*.cfm' >> filenames.txt
echo "Editing file names..."
sed 's/theradoc.*\///g' filenames.txt > tmp.txt
sed 's/\.cf.*//g' tmp.txt > files.txt
rm tmp.txt
echo "Searching all files..."
a=($(wc filenames.txt))
lines=${a[0]}
count=0
while read fn; do
    echo "$fn | $count/$lines finished..."
    while read f; do
        perl -pi -e "s/$f/$f/gi" "$fn"
    done < files.txt
    count=$((count+1))
done < filenames.txt

Runtime: 4 hours

Hardware: MacBook 16GB RAM

I would definitely like to decrease the runtime of this as it may be needed to run on other systems with the same files.

In my testing, constructing one Perl script and running it repeatedly is much faster (0.5s versus 3.6s) then running a new Perl instance for each replacement:

while read f; do
    echo "s/$f/$f/gi;"
done < files.txt > s.pl

while read fn; do
    perl -pi s.pl "$fn"
    echo "$fn | $count/$lines finished..."
    count=$((count+1))
done < filenames.txt

rm s.pl

But it seems even faster (0.05s) to rewrite the whole thing to Perl:

#! /usr/bin/perl
use warnings;
use strict;

use File::Find;

my $dir = 'theradoc2';

my %change;

find(sub {
    return unless -f;
    undef $change{$_};
    rename $_, lc $_;
}, $dir);

my $regex = join '|',
            map quotemeta,
            sort { length $b <=> length $a }
            keys %change;
find(sub {
    return unless -f;
    my $file = $_;
    open my $IN, '<', $file or die $!;
    open my $OUT, '>', "$file.new" or die $!;
    while (<$IN>) {
        s/($regex)/\L$1/g;
        print {$OUT} $_;
    }
    close $OUT or die $!;
    unlink $file or die $!;
    rename "$file.new", $file or die $!;
}, $dir);


zip_longest without fillvalue in typescript I would like to create a function like zip_lognest in python and if the array size is uneven, I do not want to filled any value. following is my code and example

const aFixture = [
  { name: 'name_a', value: 'value_a'},
  { name: 'name_b', value: 'value_b'},
  { name: 'name_c', value: 'value_c'},
  { name: 'name_d', value: 'value_d'},
];
const bFixture = [
  { name: 'name_1', value: 'value_1'},
  { name: 'name_2', value: 'value_2'},
];
const cFixture = [
  { name: 'name_x', value: 'value_x'},
  { name: 'name_y', value: 'value_y'},
  { name: 'name_z', value: 'value_z'},
];
export function zipLongest(...arrays: Array<any>) {
  const length = Math.max(...arrays.map((arr) => arr.length));
  const tempArray = Array.from({length}, (_, index) =>
    arrays.map((array) => (array.length - 1 >= index ? array[index] : undefined))
  )
  return tempArray.map((array) => array.filter((x: any) => x !== undefined))
}

const myarray =
  zipLongest(
    aFixture,
    bFixture,
    cFixture,
  )
console.log(myarray);
//[ [ { name: 'name_a', value: 'value_a' },
//    { name: 'name_1', value: 'value_1' },
//    { name: 'name_x', value: 'value_x' } ],
//  [ { name: 'name_b', value: 'value_b' },
//    { name: 'name_2', value: 'value_2' },
//    { name: 'name_y', value: 'value_y' } ],
//  [ { name: 'name_c', value: 'value_c' },
//    { name: 'name_z', value: 'value_z' } ],
//  [ { name: 'name_d', value: 'value_d' }, ]
//]

How about this? You iterate through each array only once.

function zipLongest(...arrays: Array<any>) {
    const result:any[] = [];
    arrays.forEach((arr) => {
      arr.forEach((val: any, i: number) => {
        if (i === result.length) result.push([]);
            result[i].push(val);
      })
    });
    return result;
}

"use strict";
const aFixture = [
    { name: 'name_a', value: 'value_a' },
    { name: 'name_b', value: 'value_b' },
    { name: 'name_c', value: 'value_c' },
    { name: 'name_d', value: 'value_d' },
];
const bFixture = [
    { name: 'name_1', value: 'value_1' },
    { name: 'name_2', value: 'value_2' },
];
const cFixture = [
    { name: 'name_x', value: 'value_x' },
    { name: 'name_y', value: 'value_y' },
    { name: 'name_z', value: 'value_z' },
];
function zipLongest(...arrays) {
    const result = [];
    arrays.forEach((arr) => {
        for (var i = 0; i < arr.length; i++) {
            if (i >= result.length) {
                result.push([arr[i]]);
            }
            else {
                result[i] = result[i].concat(arr[i]);
            }
        }
    });
    return result;
}
const myarray = zipLongest(aFixture, bFixture, cFixture);
console.log(myarray);


Convert a number into a different base and return as string I wanted to write a function that would take a long long int argument as well as a base and it would convert that number into an equivalent number in a different base, and return the result as a string (er, char array). For example, the call,

convertBase(9, 5)

should return "14\0". For negative values, the returned string should simply have a negative sign, '-' in front.

The code is as follows:

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

#define MIN_BASE 2
#define MAX_BASE 36
#define MAX_LLI_REP 63

char toCharacter(int v)
{
    v = abs(v);
    return (v < 10)
                ? ('0' + v)
                : ('a' + v - 10);
}

char * convertBase(long long int value, int base)
{
    if (base < MIN_BASE || base > MAX_BASE) {
        fprintf(stderr, "The base must be within [%d, %d].\n", MIN_BASE, MAX_BASE);
        return NULL;
    }
    char c[MAX_LLI_REP];
    int i, j = 0;
    long long int quotient;
    for (i = 0, quotient = value; quotient != 0; i++, quotient /= base)
    {
        c[i] = toCharacter(quotient % base);
    }
    int negative = value < 0;
    char * result = (char *) malloc(sizeof(char) * (i + 1 + negative));
    if (negative) {
        result[0] = '-';
        j = 1;
    }
    while (i) {
        result[j++] = c[(i--) - 1];
    }
    result[j] = '\0';
    return result;
}

I've ran a variety of test cases, such as both LLONG_MIN and LLONG_MAX from limits.h, in various bases, as well as trying to convert a decimal number into a decimal number (expecting the same answer). It passed in all of these cases. However, I was wondering if additional sets of eyes could spot potential errors. Also, any stylistic or performance improvement suggestions would be appreciated.

Off by one

Your MAX_LLI_REP is 63, but since you are accepting long long inputs, you could have a 64 character representation if you pass in LONG_LONG_MIN which is 0x8000000000000000 and in binary would be 10000...000 which is one 1 and 63 0's.

When I ran your program passing in LONG_LONG_MIN (or -9223372036854775808), the first character in the output which should be a 1 was a random character instead, because it was the character that fell out of bounds and got overwritten by something else.  For example, I got:

-h000000000000000000000000000000000000000000000000000000000000000

When I set MAX_LLI_REP to 64, the problem went away.

Simplified expression

This expression:

result[j++] = c[(i--) - 1];

Could be simplified to:

result[j++] = c[--i];

Modified interface

Currently, you allocate a string and return it from your function.  This creates some potential awkwardness because someone has to free that string later on.

It might be nicer to pass in a buffer to your function and have the function fill it in instead.  The buffer size can be documented to be a minimum of 66 bytes, or you could pass in a buffer length argument as well and have the function fill up to the buffer length.

Alternatively, you could create a static buffer in your function and return a pointer to it.  This has the drawback that you must use the return value before you call the function again.  Some C library functions such as ctime() do this, so it isn't unprecedented.


Python algorithm for identification of low-level jets (meteorology) A low-level jet is a local wind speed maximum near the surface. Baas (2009) defined a low-level jet as follows:

  the lowest maximum of the wind speed profile in the lowest 500 m of the atmosphere that is at least both 2 m/s and 25% faster than the next minimum above (...) A minimum is neglected if the wind speed above that minimum increases less than 1 m/s before decreasing again to values lower than the wind speed of that minimum. When no minumum is present, the lowest value of the wind speed profile above the jet is taken as a minimum.

I have a 2D numpy array that contains wind speed values at several measurement heights and times. I've written an algorithm to identify for each time step whether the wind profile obeys this criterion. However, I think my code can be improved. Right now I'm using nested loops and a lot of if statements. Also I need to catch an index error because I need to check whether the 'next' value in the array is lower or higher.

I was hoping to get some feedback from more experienced programmers. Here's my code:

def findLLJ(wspd_matrix, time, hgt):
  '''Find low-level jets according to Peter Baas' algorithm
     input: wind speed matrix with time as first axis and height as second axis
            time and height arrays
     output: jet index array, jet height array, jet speed array
     use: jetidx,jethgt,jetspd = findLLJ(wsp_matrix,time,height)'''
  #Allocate output arrays:
  jet = np.zeros(time.shape,dtype=bool)
  jetspd = np.zeros(time.shape)
  jethgt = np.zeros(time.shape)
  #Shape of input array:
  ntime,nlev = wspd_matrix.shape
  #Consistency check:
  if not (ntime == len(time) and nlev == len(hgt)):
    raise Exception("Shapes of input arrays are not consistent")
  #Loop over all wind profiles at given times
  for t,prof in enumerate(wspd_matrix):
    maxflg = False
    minflg = False
    maxspd = 0.
    #Loop over all vertical points in the profile
    for i,x in enumerate(prof):
      #Find out whether vertical point is a maximum
      if np.any(x>(prof[i:]+2.5)) and np.any(x>1.25*prof[i:]) and x>maxspd:
        maxflg = True
        maxind = i
        maxspd = x
        print 'Maximum found at time',t,'and level',i,'with speed',x,'m/s'
      #Find out whether vertical point is a minimum
      try:
        if x<0.8*maxspd and x<maxspd-2.5 and x<prof[i+1] and not prof[i+2]<x:
          minflg = True
          minind = i
          minspd = x
          print 'Minimum found at time',t,'and level',i,'with speed',x,'m/s'
      except IndexError:
        if i == nlev-2 and x<0.8*maxspd and x<maxspd-2.5 and x<(prof[nlev-1]-1):
          minflg = True
          minind = i
          minspd = x
          print 'Minimum found at time',t,'and level',i,'with speed',x,'m/s'
        else:
          minflg = True
          minind = nlev-1
          minspd = prof[nlev-1] # this should be changed to minumum of the profile, but that might return NaN, need to find an update for that.
          print 'No minumum found, using lowest value from profile'
      #Determine wheter a LLJ occured
      if maxflg and minflg:
        print 'Low-level jet found at time',t,'and height',hgt[maxind],'m with speed',maxspd,'m/s and minimum at',hgt[minind],'m with speed',minspd,'m/s'
        jet[t] = True
        jetspd[t] = maxspd
        jethgt[t] = hgt[maxind]
        break
      else:
        continue
  if maxflg and not minflg:
    print 'probably something wrong with NaNs'
  #End of loop, next profile...
  return jet,jethgt,jetspd

Naming the variables

wspd_matrix would be much easier to read if it were called wind_speed_matrix
jetspd would be much easier to read if it were called jet_speed
jethgt would be much easier to read if it were called jet_height

Remove the numeric values in the code

0.8 and 2.5 appear quite often in your code. It is hard for someone without a domain knowledge to know what they stand for. And if you have to change them for some reason, this is error-prone. Why not just naming them (max_multiplier = 0.8 per example) at the beginning of the function ?

Redundant condition

if x<0.8*maxspd and x<maxspd-2.5

appears twice in your code. It represents a clear mathematical function :

if x<min(0.8*maxspd , maxspd-2.5)

which is much easier to read (and picture mentally)

Redundant code

Something needs to be done about this three prints :

print 'Maximum found at time',t,'and level',i,'with speed',x,'m/s'
print 'Minimum found at time',t,'and level',i,'with speed',x,'m/s'
print 'Minimum found at time',t,'and level',i,'with speed',x,'m/s'

Maybe a

print_local_optimum_found(type_optimum, at_time, at_level, at_speed)

Check the input

if maxflg and not minflg:
    print 'probably something wrong with NaNs'

Why waiting until the end of the program to realize something wrong happened ? If you know NAs cause trouble, just check the presence of NAs before

The algorithm itself

It is hard to suggest improvements now with every piece of code in the same function. If you fix all this, such improvements will become much more readable!


Storing customers' orders I'm developing a shop system and using this method to store the customers with their respective orders:

try
{
    $connection = new PDO("mysql:host={$HOST};dbname={$DB_NAME}", $USERNAME, $PASS);
}

$connection->beginTransaction();
$sql = "INSERT INTO orders (customer_id, customer_name, order_value, order_date)
        VALUES (?, ?, ?, ?)";

$query = $connection->prepare($sql);
$query->execute(array
(
    $user_id,
    $user['user_name'],
    $order_value,
    $date
));

$id_of_respective_order = $connection->lastInsertId();

$sql = "INSERT INTO purchased_products (order_id, product_name, product_price, quantity)
        VALUES (?, ?, ?, ?)";

$query = $connection->prepare($sql);

foreach($_SESSION['cart'] as $product)
{
    $query->execute(array
    (
        $id_of_respective_order,
        $product['product_name'],
        $product['product_price'],
        $product['quantity']
    ));
}

$connection->commit();

By using this, later I can show all the products that belongs to their respective customers by using the Foreign Key order_id.

My question is simple: is this secure? If many people buy at the same time can I have problems? Is there a possibility of a product being directed to the wrong customer? Can I trust in this method?

My question is simple: is this secure?

It's safe from SQL injection attacks.

  If many people buy at the same time can I have problems?

Since you insert the order and the cart contents in a transaction, the table will be locked until you have inserted everything, preventing other users from placing their orders. So the transaction can become a bottleneck. However, this is the right way of doing it, if it's important for you to avoid partial orders.

  Is there a possibility of a product being directed to the wrong customer?

One thing is not clear, what happens if the first query fails. You didn't check for errors, though I would guess that in that case you won't get a meaningful insert ID (would have to check the docs). In any case, you should check for errors after all insert operations and handle gracefully.

Other than that, the transaction guarantees that order IDs won't get mixed up. Even without the transaction, probably it wouldn't happen, but I'd have to dig into the docs for that.

  Can I trust in this method?

Yes, keeping in mind the potential bottleneck and other considerations above.


Check source contain replace string I created this function  it is looking for the html tag  and replacing it with the ^value. It seems complicated wanted to know if there is a better way to do something like this.

  Will you review the syntax, structure, and logic of my code. It can be tested using the main.

Output

dsfsadfsad 58 dsfsadfsad
dsfsadfsad 5^8  dsfsadfsad

Code

namespace Replacestring
{
    class Program
    {
        static void Main(string[] args)
        {

            string source = "dsfsadfsad 58 dsfsadfsad";
            Console.WriteLine(source);
            source = superscriptRule(source);
            Console.WriteLine(source);

        }

        protected static string superscriptRule(string source)
        {
            if (!string.IsNullOrEmpty(source) == true)
            {
                if (source.Contains(""))
                {
                    Match m = Regex.Match(source, @"\s*(.+?)\s*", RegexOptions.IgnoreCase);
                    if (m.Success)
                    {
                        int i = 0;
                        string x = m.Groups[1].Value;
                        bool result = int.TryParse(x, out i);
                        if (result == true)
                        {
                            string replacestring = string.Format("{0}", x);
                            string newstring = string.Format("^{0} ", x);
                            source = source.Replace(replacestring, newstring);
                            return source;
                        }
                        else
                        {
                            return source;
                        }
                    }
                    return source;
                }

            }
            return source;

        }
    }
}

Method names should be named using PascalCase casing.
An if condition is just a boolean expression hence if (!string.IsNullOrEmpty(source) == true) is a little bit over the top. Either write

if (!string.IsNullOrEmpty(source))
{

}

or

if (string.IsNullOrEmpty(source)) { return source; }

which saves one level of indentation.
Why is the method protected ? Doesn't make sense to me.
The name of that method sounds rather like a class/object name than a method name. Methodnames should contain a verb or verb-phrase.

Instead of the Regex stuff and string.Contains() etc your method could just look like so

private static string SuperscriptRule(string source)
{
    if (string.IsNullOrEmpty(source)) { return source; }

    return source.Replace("<sup>", "^").Replace("<\sup>", string.Empty);
}


Python function to fill a list based on index number I wrote this function to fill a list with object based on the desired index they should have.

Here the func:

def fill(lst, index, add, fillvalue=None):
    '''Fills a list with the add value based on desired index'''
    '''Let the value in place if present'''
    for n in range(index):
        try:
            if lst[n]:
                pass
        except Exception:
            lst.append(fillvalue)
    try:
        if lst[index]:
            pass
        else:
            lst[index]=add
    except Exception:
        if index==len(lst):
            #This if check probably not mandatory
            lst.append(add)
    return lst

edit: here's an usage example that would for example happen in a for loop

>>> my_list = [1, 7, 14]
>>> fill(my_list, 5, 6)
[1, 7, 14, None, None, 6]
>>> fill(my_list, 6, 12)
[1, 7, 14, None, None, 6, 12]
>>> fill(my_list, 3, 25)
[1, 7, 14, 25, None, 6, 12]
>>> fill(my_list, 4, 18)
[1, 7, 14, 25, 18, 6, 12]

Note:

>>> my_list = [1, 7, 14]
>>> fill(my_list, 1, 6)
#returns the list not modified, my_list[1] is already taken
[1, 7, 14]

It works good for my purpose (matrix generating). My matrixes are generated with many "for loops" passes, because the data I convert to is unordered. This is why the "None" elements are important in order to fill them out on a subsequent loop pass.

My questions are:

I'm wondering if I made everything as simple as possible? Or if there
were a lib, that would do the same job?
For now the matrixes aren't so long but I guess, if I go further
in my development, they will become big (1000+ lines). Should
I somehow better use itertools?
I also somehow thought of first using a dict {index: value} and
convert it to a list when filled up. Would it be better practice?

Edit: Now that I pasted the code in here I notice the arg name "add" is blued. My Vim-editor didn't told me that... Is it a name already taken by the python core?

if lst[n] is not sufficient to determine if the value at index n is None, 0 will also not pass the test. Use if lst[n] is not None or if lst[n] is None, depending on which one you want.
The whole first loop is just trying to determine if you need to extend the list, but you could just do that:

lst.extend([fill_value] * (index - len(lst) + 1))

If the list is already long enough this does nothing, as a list multiplied by an integer smaller than one is the empty list, and extending with an empty list does nothing. It even works when you use negative indices.
This also guarantees that the index exists, so you don't need the try..except anymore. But if you use try and except, always try to catch the narrowest exception possible. This way you make sure you don't catch an unexpected exception. In this case it would be an IndexError instead of the more generic Exception.
The convention in Python is for functions to only do one of two things: either do not mutate the inputs and return a new output or mutate (some of) the input and return None (potentially implicitly). Your function falls into the second category.

With these changes your function could be a lot shorter:

def fill(lst, index, value, fill_value=None):
    """Place the `value` at index `index`,
    but only if the value already there is `None`.
    Extends the list with `fill_value`, if necessary.
    """
    # No-op if list long enough or index negative
    lst.extend([fill_value] * (index - len(lst) + 1))
    if lst[index] is None:
        lst[index] = value

my_list = [1, 7, 14]
# my_list = [1, 7, 14]
fill(my_list, 5, 6)
# my_list = [1, 7, 14, None, None, 6]
fill(my_list, 6, 12)
# my_list = [1, 7, 14, None, None, 6, 12]
fill(my_list, 3, 25)
# my_list = [1, 7, 14, 25, None, 6, 12]
fill(my_list, 4, 18)
# my_list = [1, 7, 14, 25, 18, 6, 12]

Note that you will not directly see the result anymore in an interactive session, since the list is no longer returned. But that's a feature, not a bug, as mentioned above.

Of course, if you need to do this multiple times directly after each other, you might want to define a function that needs to extend only once:

def fill_multiple(lst, indices, values, fill_value=None):
    lst.extend([fill_value] * (max(indices) - len(lst) + 1))
    for index, value in zip(indices, values):
        if lst[index] is None:
            lst[index] = value

Then you can use e.g a dictionary:

my_list = [1, 7, 14]
updates = {5: 6, 6: 12, 3: 25, 4: 18}
fill_multiple(my_list, updates.keys(), updates.values())
# my_list = [1, 7, 14, 25, 18, 6, 12]

I kept the indices and values separate so you can also just pass lists or anything else, not just a mapping. Depending on your usecase you might want to pass the dictionary to the function instead.


Hibernate transaction advice I'm new to Hibernate so I need some advice/direction on doing Transactions.

I have a DAO like

public class MyDao extends HibernateDaoSupport implements IMyDao {

 @Override
 public Foo getFoo(int id) {

  return (Foo)getHibernateTemplate().load(Foo.class, id);

 }
}

With this setup (using HibernateDaoSupport), will Hibernate/Spring handle transactions for me? Some of the examples I see say yes, others show using

Transaction tx = getHibernateTemplate().getSessionFactory().getCurrentSession().beginTransaction();

to get a Transaction in every DAO method.

Right now I'm assuming my minimal DAO is correct but I want to do a "larger" transaction that includes a couple of Hibernate calls in one method. Would I use the manual Transaction method there? Do I have to use it everywhere? Thanks.

In general, you should never handle transactions in dao code, as you never know where the transaction boundary is. You should let whoever wants to control the transactions control them.

Code that starts and commits a transaction in every dao method is likely to be problematic.

Btw: not very keen at all on 'dao' but as you use it, so will i.


Text-based Mario and Luigi objects I have this project about using OOP in python. How to fully make use of OOP and use __init__(self)?

class Parent:
    name = "Player"
    lives = 3
    #display player running
    def run(self, name):
        self.name = name
        print "%s is running." %self.name

    #player will start to jump against enemy
    def jump(self, name):
        self.name = name
        print "%s jumps over enemy. TOINK TOINK" % self.name

    #player will start to grow
    def grow(self, name):
        self.name = name
        print "%s sarting to Grow" %self.name

For player1

#Mario
class Child1(Parent):
    name = "Mario"
    #special skill
    def blazingFire(self, name):
        self.name = name
        print "%s used his blazingFire" %self.name

For player 2

#Luigi
class Child2(Parent):
    name = "Luigi"
    #special Skill
    def blazingWind(self,name):
        self.name = name
        print "%s used his blazingWind" %self.name

Calling object

player = Parent()
player1 = Child1()
player2 = Child2()

#mario's turn
player1.run(mario)
player1.jump(mario)
player1.grow(mario)

Am I doing the right thing or there's a better way for this?

This is clearly Python2. To have better inheritance in Python2 you have to inherit from object.
Inheriting from object is classed as a new-style class, and is better than old ones.

class Parent(object):
    pass

class Child1(Parent): # No need to inherit object again.
    pass

You don't use __init__...
In your example code it may be bad to use it, since you are adding spells anyway.
And you're overwriting self.name in your class anyway, so it's quite pointless.

But lets say you wanted to have a default name for Child1,
and you could assign it to something else,
so you don't have to supply the name on every function.
You can do almost exactly the same as you are doing on every function.

def Parent(object):
    def __init__(self, name="Player", lives=3):
        self.name = name
        self.lives = lives

    def run(self):
        print "%s is running." % self.name

As for your print statements you should use str.format rather than the old 'sprintf'-like %.
There are quirks with % and str.format is a lot stronger.

print "{} is running.".format(self.name)

The above is exactly the same as your above prints, but I'm sure at one point you will want to print the name and the lives.
You can change the contents of the {}'s to allow you to use the name instead.
Which allows for greater readability, as now we know '[player name] is running'.

print "{0.name} is running.".format(self)

Why stop there you can show lives to!

print "{0.name} has {0.lives} live(s).".format(self)

As just saying 'use new style classes they are better than old style ones' isn't a compelling reason.
There is something that you can do with new-style classes, which is calling it's parents functions.
For example if we wanted to have the default name for Child1 as Luigi, you can do the following:

class Child1(Parent):
    def __init__(name="Luigi", **args):
        super(Child1, self).__init__(name=name, **args)

It's one good thing about new style classes, which get's improved in Python3 to:

class Child1(Parent):
    def __init__(name="Luigi", **args):
        super().__init__(name=name, **args)

This will now change the 'calling object' code to:

player1 = Child1('Jay')
player2 = Child1('Joe')

player1.run()
player1.jump()
player1.grow()
player2.run()

Here is Parent and Child1 with the above changes, note I removed all the self.name = name's.

class Parent(object):
    def __init__(name="Player", lives=3):
        self.name = name
        self.lives = lives

    def run(self):
        print "{0.name} is running.".format(self)

    def jump(self):
        print "{0.name} jumps over enemy. TOINK TOINK".format(self)

    def grow(self):
        print "{0.name} sarting to Grow".format(self)

class Child1(Parent):
    def __init__(name="Luigi", **args):
        super(Child1, self).__init__(name=name, **args)

    def blazing_fire(self):
        print "{0.name} used his blazingFire".format(self)


Styling navigationController per viewController The design of my application consists of multiple viewControllers which all have different styling applied to them. The following code is used in every viewController to style the viewController:

override func viewWillAppear(_ animated: Bool) {
        super.viewWillAppear(animated)

        self.title = "PROFILE"

        self.navigationController?.view.backgroundColor = .white
        self.navigationController?.navigationBar.tintColor = self.view.tintColor

        navigationController?.navigationBar.barStyle = .default

        tabBarController?.tabBar.isTranslucent = false
        tabBarController?.tabBar.backgroundColor = .white
        tabBarController?.tabBar.tintColor = .black
    }

    override func viewWillDisappear(_ animated: Bool) {
        super.viewWillDisappear(animated)

        self.navigationController?.navigationBar.tintColor = .white
        self.navigationController?.view.backgroundColor = UIColor.clear
        self.navigationController?.navigationBar.barStyle = .blackTranslucent

        self.navigationController?.navigationBar.shadowImage = UIImage()
    }

Some properties change per VC, for example the backgroundColor of the VC. This code is copy pasted in every VC I make with small changes, which causes a lot of duplicate code etc. Is there a design pattern which allows me to create the styling of every VC more conveniently?

extension UIViewController   create  a navigation bar Method call anywhere  from ViewController  in your project.navigation bar will appear   and  add more parameter  color or other relevant change you want to achieved .

for example

    import UIKit

    extension UIViewController {

        func setupNavigationBar(title: String) {
            // back button without title
            //self.navigationController?.navigationBar.topItem?.title = ""

            //back button color
            //self.navigationController?.navigationBar.tintColor = UIColor.white

            //set titile
             self.navigationItem.title =  title

            //set text color & font size
            //self.navigationController?.navigationBar.titleTextAttributes = [NSForegroundColorAttributeName:UIColor.init(red: 251/255, green: 251/255, blue: 251/255, alpha: 1) , NSFontAttributeName:UIFont.systemFont(ofSize: 19)]

            //set background color without gradian effect
            //self.navigationController?.navigationBar.barTintColor = UIColor.init(red: 134/255, green: 145/255, blue: 152/255, alpha: 1)

            //show right button
            let rightButton = UIBarButtonItem(image: #imageLiteral(resourceName: "Menu"), style: .plain, target: self, action: #selector(menu))

            //right Bar Button Item tint color
            //self.navigationItem.rightBarButtonItem?.tintColor = UIColor.init(red: 219/255, green: 219/255, blue: 219/255, alpha: 1)

            //show the Menu button item
            self.navigationItem.rightBarButtonItem = rightButton

            //show bar button item tint color
            //self.navigationItem.rightBarButtonItem?.tintColor = UIColor.init(red: 219/255, green: 219/255, blue: 219/255, alpha: 1)

        }

        func menu(){
            print("showSlideOutMane fire ")
        }

    }


Validation before sending mail in PHP In my initial problem posted on SO, whenever anyone accessed the Mail.php on the server, it used to send an empty email to the $to. To avoid this I came up with a solution.
require 'PHPMailer/PHPMailerAutoload.php';

    $yourName = $_POST['yourName'];
    $sender  = $_POST['emailID'];
    $subject  = $_POST['subject'];
    $message  = $_POST['message'];
    $to = 'email@email.com';

        if(empty($yourName) || empty($sender) || empty($subject) || empty($message) || empty($message))
        {
            echo "Fields are empty";
        }
        else
        {
                $mail = new PHPMailer;
                //$mail->SMTPDebug = 2;
                $mail->isSMTP();
                $mail->Host = 'smtp.gmail.com';
                $mail->SMTPAuth = true;
                $mail->Username = 'gmailUser';
                $mail->Password = 'gmailPassword';
                $mail->SMTPSecure = 'tls';
                $mail->Port = 587;

                $mail->setFrom($sender,$yourName);
                $mail->addAddress($to);
                $mail->addReplyTo($sender);

                $mail->isHTML(true);

                $mail->Subject = $subject;
                $mail->Body    = "From: ". $sender. "
" ." Name: ". $yourName. "
". " Message Body " .$message;
                $mail->AltBody = "From: ". $sender. "
" ." Name: ". $yourName. "
". " Message Body " .$message;

                if(!$mail->send())
                {
                    echo 'Message could not be sent.';
                    echo 'Mailer Error: ' . $mail->ErrorInfo;
                }
                else
                {
                    echo "Message has been sent....You're being redirected.....";
                }
        }

This fix, basically allows the user to interact with Mail.php, but Mail.php doesn't sends a null email to $to
Now hours later, I find out that this is a very bad approach.

I would like to know how a professional would solve this problem?
Any good approach which I could use to efficiently optimize the code?

Validate your fields individually so you can provide the client side with better error messages (assuming this is an AJAX script)...

$reqd_params = array('yourName', 'emailID', 'subject', 'message');
foreach($reqd_params as $param)
    !empty($_REQUEST[$param]) || die("$param parameter is required.");

The AltBody paramter is supposed to be the plaintext version of the regular body, yours has html in it. You need to do wrap it in striptags and nl2br like so... striptags(nl2br($my_html_message))

Avoid returning plain text from an AJAX script too. Set your properties in an array and return it in JSON so Javascript can read it easier.

for example:

header("content-type: application/json");
echo json_encode(array(
    "success" => true,
    "message" => "email_sent"
));

Edit.. I said nl2br but what u need is the opposite of that..


Using std::vectors to store textures I need to write a TextureManager class which stores Texture objects; the problem is I'm using an std::map<Texture *, const char*> map to store Texture objects using a key.

Searching elements through a map is not ideal because of performance issues (I'm writing a C++ library, so performance is important).

I've had the idea to use an std::vector<Texture*>. In this case, I need to use an unsigned int. But after several tries, I still cannot implement it.

Here is the .h:

class CTextureManager
{
public:
    CTextureManager(SDL_Renderer *pRenderer);
    void SetRenderer(SDL_Renderer *pRenderer);

    ~CTextureManager();

    // Load File function
    CTexture *LoadFromFile(std::string name, std::string filename);

    bool UnLoad(std::string name);

    CTexture *GetTexture(std::string name);

    void ClearAll();

protected:
    std::map<std::string, CTexture *> m_mapTexture;
    SDL_Renderer *m_pRenderer;

};

and the .cpp:

CTextureManager::CTextureManager(SDL_Renderer *pRenderer)
{
    m_pRenderer = pRenderer;
}

CTextureManager::~CTextureManager()
{
    ClearAll();

    if (m_pFont != nullptr)
    {
        TTF_CloseFont(m_pFont);
    }
}

void CTextureManager::ClearAll()
{
    for (std::map<std::string, CTexture *>::iterator it = m_mapTexture.begin(); it != m_mapTexture.end(); ++it)
    {
        delete (it->second);
        (it->second) = nullptr;
    }
    m_mapTexture.clear();
}

CTexture * CTextureManager::GetTexture(std::string name)
{
    return m_mapTexture[name];
}

bool CTextureManager::UnLoad(std::string name)
{
    CTexture *pTempTexture = m_mapTexture[name];

    if (!pTempTexture)
        return false;

    delete pTempTexture;
    pTempTexture = nullptr;
    m_mapTexture.erase(name);
    return true;
}

CTexture * CTextureManager::LoadFromFile(std::string name, std::string filename)
{
    CTexture *pTexture = m_mapTexture[name];

    if (pTexture == nullptr)
    {
        pTexture = new CTexture();
        pTexture->SetRenderer(m_pRenderer);

        m_mapTexture[name] = pTexture;
    }

    if (!pTexture->LoadFromFile(filename))
        return nullptr;

    return pTexture;
}

void CTextureManager::SetRenderer(SDL_Renderer *pRenderer)
{
    m_pRenderer = pRenderer;
}

The interface is badly designed:

Are you allowed to pass nullptr here?
Is the object taking ownership of the pointer?

CTextureManager(SDL_Renderer *pRenderer);

Are you allowed to change the renderer?
Are you allowed to replace it with nullptr?
Is the ownership being passed to the CTextureManager object?

void SetRenderer(SDL_Renderer *pRenderer);

Passing name and filename by value. This is causing a copy. This is inefficient and not required. Pass by const reference.
The returned value:
Is it allowed to be nullptr?
Who owns the pointer?

CTexture *LoadFromFile(std::string name, std::string filename);

Another pass by value.
What does the return value indicate?

bool UnLoad(std::string name);

Pass by value again.
Who owns the returned object?

CTexture *GetTexture(std::string name);

You are storing pointers.

    std::map<std::string, CTexture *> m_mapTexture;
    SDL_Renderer *m_pRenderer;

Which means your class is doing both resource management and business logic. A class should do one or the other (search for Separation of Concerns). Split your resource management into its own class. Then use this inside your texture manager.

Ownership semantics.

Your Major problem is you are not correctly indicating the ownership semantics of the objects.

Ownership is all about who is responsible for deleting a dynamically allocated object and is one of the core principles of C++ (the one that raises C++ above C in terms of memory management). If you don't define these semantics well in the interface then people have to actually dig into the implementation to understand how the ownership is working so that they can correctly write their code.

If people are digging into your code to understand the ownership semantics they are now depending on implementation details of your code. This makes your code very brittle to any future changes and tightly couples any code using your class.

Return by reference to indicate that ownership is not being transferred.
Pass by reference to indicate that you are not accepting ownership.
Use smart pointers to pass unique/shared ownership objects around.
Never pass by pointer across a public interface

its fine to use internally but it should not be part of your public interface to other code.

Prefer to use the constructors initialization list.

TextureManager::CTextureManager(SDL_Renderer *pRenderer)
{
    m_pRenderer = pRenderer;
}

// When you have members that have constructors.
// These will be called before the body of the constructor is
// entered.
//
// By initializing in the body of the code you
// are first constructing the object into the default state
// the updating that state. It is better to call the constructor
// directly with the appropriate parameters.
//
// Also reference must be done in the initialization list.
// So prefer to do all initialization in the initialization list
// for consistency.
TextureManager::CTextureManager(SDL_Renderer *pRenderer)
    : m_pRenderer(pRenderer)
{}

Destructor

You destructor is not virtual.

CTextureManager::~CTextureManager()

This means you are not expecting to have any sub types of the object. Which is fine. But you should keep it in mind.

ERROR

    if (m_pFont != nullptr)
    {
        TTF_CloseFont(m_pFont);
    }

m_pFont is not a member of the class. So I expect this would not compile.

Clear All

    for (std::map<std::string, CTexture *>::iterator it = m_mapTexture.begin(); it != m_mapTexture.end(); ++it)

   // Prefer to use `auto` for types you don't care about.
   // Also prefer to use the std::begin() and std::end() rather than
   // calling the methods directly. This will allow you to change the
   // container type in the future without having to change the code.
    for (auto it = std::begin(m_mapTexture); it != std::end(m_mapTexture); ++it)

   // Even better would be to use the new range based for
   for(auto& value: m_mapTexture)

   // Or potentially an algorithm:
   std::for_each(td::begin(m_mapTexture), std::end(m_mapTexture), /* ACTION */);

Don't see any need to set this to nullptr:

        (it->second) = nullptr;

When you are finished you clear the container. So the values no longer exist.

GetTexture

On std::map the operator[] will insert a value into the container if it does not exist. I don't think that is what you actually want.

    return m_mapTexture[name];

UnLoad

Again with the insert.

    CTexture *pTempTexture = m_mapTexture[name];

Don't see the need for this test.

    if (!pTempTexture)
        return false;

Deleting a nullptr is fine.

    delete pTempTexture;

This does nothing useful (as it is about to go out of scope).

    pTempTexture = nullptr;

Since you inserted the name you may want to always delete it (even if the pointer is null).

    m_mapTexture.erase(name);

Just return the pointer here:

    return true;

The pointer will get converted to the correct bool. If you found nothing then it will be false. If you found something it will be true.

LoadFromFile

So your object is always a CTexture never anything derived from it.

        pTexture = new CTexture();

Since your texture is a single type I don't see the need to store pointers in the container. Just store the object directly.

Seems like the only reason for the texture manager to have a renderer is to make this call. Seems strange. Why not pass the renderer into the LoadFromFile() method?

        pTexture->SetRenderer(m_pRenderer);

You have just dynamically allocated and placed into your structure a CTecture. You may return a nullptr now. But a call to GetTexture() is going to return the previously created texture object (that failed to load). On a load failure you may want to remove this object from the structure.

    if (!pTexture->LoadFromFile(filename))
        return nullptr;

This is how I would do it:

// I am guessing a bit because I have not read the SDL documentation.
struct SLDTextureDeleter
{
    void operator()(SDL_Texture* p){SDL_DestroyTexture(p);}
};
class CTextureManager;
class CTexture
{
        // Constructor is private so it can only be used by CTextureManager
        friend class CTextureManager;
        CTexture(std::string const& filename, CRenderer& renderer)
        {
            // Create Texture and load from File.
            // Failure to load leaves the texture pointer as nullptr.
            // The TextureManager tests this by calling ok() to validate
            // that the texture was loaded correctly.

        }
        bool ok() const {return texture.get();}
    public:
        // Allow texture moving
        CTexture(CTexture&& move)
            : texture(std::move(move.texture))
        {}
        CTexture& operator=(CTexture&& move)
        {
            using std::swap;
            swap(texture, move.texture);
        }
        // But disable Copy (as it is manager by CTextureManager)
        CTexture(CTexture const&)            = delete;
        CTexture& operator=(CTexture const&) = delete;
    private:
        std::auto_ptr<SDL_Texture, SLDTextureDeleter>   texture;
};
class CTextureManager
{
  public:
    bool      loadFromFile(std::string const& name, std::string const& filename, CRenderer& renderer);
    bool      hasTexture(std::string const& name) const;
    CTexture& getTexture(std::string const& name) const;

    void clearAll();
    void unLoad(std::string const& name);

  private:
    std::map<std::string, CTexture> mapTextures;
};

bool CTextureManager::loadFromFile(std::string const& name, std::string const& filename, CRenderer& renderer)
{
    CTexture  tmp(filename, renderer);
    bool result = tmp.ok();
    if (result)
    {
        mapTextures.emplace(name, std::move(tmp));
    }
    return result;
}
bool CTextureManager::hasTexture(std::string const& name) const
{
    auto find =   mapTextures.find(name);
    return find != mapTextures.end();
}
CTexture& CTextureManager::getTexture(std::string const& name) const;
{
    // Note: UB if loadFromFile() failed to return true.
    //          or if hasTexture() returned false.
    //          user is supposed to check before use.
    auto find =   mapTextures.find(name);
    return find->second;
}

void CTextureManager::clearAll()
{
    mapTextures.clear();
}
void CTextureManager::unLoad(std::string const& name)
{
    mapTextures.erase(name);
}


Binary search tree methods I have implemented a binary search tree with the methods search, insert, and delete. I want to know if this is the best way to code them or if there are any other way by which I can reduce the code size or redundancies in code, if any.

#include <bits/stdc++.h>
using namespace std;

struct node{
    int val;
    node* left;
    node* right;
    node(){
        left=right=NULL;
    };
    node(int val){
        this->val = val;
        left=right=NULL;
    }
};

struct bTree{
    node* root;
    void insert(int val);
    node* search(int val);
    void deleteVal(int val);
    node* getParent(int val);
    bTree(){
        root = NULL;
    }
    bTree(int val){
        root = new node(val);
    }
};

void bTree::insert(int val){
    if(!root){
        root = new node(val);
        return;
    }
    node* temp = root;
    node* parent = root;
    while(temp){
        parent = temp;
        if(val >= temp->val){temp = temp->right;}
        else{ temp = temp->left;}
    }
    if(val >= parent->val){parent->right = new node(val);}
    else{parent->left = new node(val);}
    return;
}

node* bTree::search(int val){
    node* temp = root;
    while(temp && (temp->val!=val)){
        if(val > temp->val){temp = temp->right;}
        else if(val < temp->val){temp = temp-> left;}
    }
    return temp;
}

node* bTree::getParent(int val){
    if(!root || (root->val == val)){return NULL;}
    node* temp = root;
    node* parent = NULL;
    while(temp){
        if(temp->val == val) return parent;
        if(val < temp->val) {parent = temp; temp= temp->left;}
        if(val > temp->val) {parent = temp; temp= temp->right;}
    }
    return NULL;
}

void bTree::deleteVal(int val){
    node* temp = search(val);
    if(!temp){return;}
    if(!(temp->left) && !(temp->right)){
        if(temp==root){
            root = NULL;
            delete temp;
        }
        else{
            node* parent = getParent(val);
            if(parent->right==temp){parent->right=NULL; delete temp;}
            else{parent->left=NULL; delete temp;}
        }
    }
    else if(!(temp->left) && (temp->right)){
        if(temp==root){
            root = root->right;
            delete temp;
        }
        else{
            node* parent = getParent(val);
            if(parent->right==temp){parent->right=temp->right; delete temp;}
            else{parent->left = temp->right; delete temp;}
        }
    }
    else if((temp->left) && !(temp->right)){
        if(temp==root){
            root = root->left;
            delete temp;
        }
        else{
            node* parent = getParent(val);
            if(parent->right==temp){parent->right = temp->left; delete temp;}
            else{parent->left = temp->left; delete temp;}
        }
    }
    else{
        node* minNode =temp->right;
        while(minNode->left){
            minNode = minNode->left;
        }
        int valStoring = minNode->val;
        deleteVal(minNode->val);
        temp->val = valStoring;
    }
    return;
}

int main(){
    bTree a;
}

Namespace
#include <bits/stdc++.h>
using namespace std;

The first line supposedly inserts all includes your code needs but haven't specifically included. The second line saves you a lot of typing by not having to insert std:: in front of all relevant methods and types.
Please don't use those!
Read Why is “using namespace std;” considered bad practice? on Stack Overflow for the full read, but the short version is it's a major maintainability risk and allows function conflicts to arise.
The first line combined with the second line makes it even worse, since now you are no longer in control about how your code executes. You should tell the compiler what to do, not the other way around.
You current style not only hurts maintainability, it also hurts your understanding of how the language works. Practices like these lead to programmers not understanding how to make the best of namespaces.
Style
left=right=NULL;`

This hurts readability and maintainability. It's clear you're setting something to NULL, but not why. You're probably resetting something. However, you're resetting both left and right at the same time, on the same line. If you need to reset multiple things at the same time, you usually implement a function doing so.
if(val < temp->val) {parent = temp; temp= temp->left;}`

All your if statements look like that. The more accepted (and more readable) style of bracketing in C++ goes like this:
if(val < temp->val)
{
    parent = temp;
    temp= temp->left;
}

Don't be afraid of using newlines.
int main(){
    bTree a;
}

You declare a bTree named a. You're not doing anything with it, just creating something. If you would do something with it, you'd notice a isn't a good name for a bTree. a.insert(val) doesn't tell me anything, except you're inserting something into a. In a bigger project, you'll encounter more variable names and all those names should be descriptive.
Finally:
The size of code in C++ is usually irrelevant. Space is cheap nowadays and saving a couple of bytes in the source won't bring you anywhere with compiled languages. Only the size of the resulting binary may be relevant, especially if you're working on embedded systems.


How to implement Publish-subscribe pattern in PHP? I have two classes, Database and Logger. Database is responsible for handling SQL queries, and Logger logs messages to an external file. Whenever the database executes an SQL query, I want Logger to log it. Right now my implementation looks something like this:

    class Database {
        public function query($sql) {
            Logger::log($sql);

            // ...
        }
    }

    class Logger {
        public static function log($msg) {
            // ...
        }
    }

But I feel that this is bad practice because Database shouldn't know about Logger. So instead I was thinking of creating a publisher-subscriber model (or something similar) where Database keeps a list of subscribers and has a method to subscribe to events, then it alerts subscribers when an event is fired (eg: when query is executed). I'm thinking something like this:

    class Database {
        $subscribers = array();

        public function subscribe($something) {
            $subscribers.push($something);
        }

        private function alert_subscribers($msg, $event_type) {
            foreach ($s as $subscribers) {
                // Alert all subscribers, somehow
            }
        }

        public function query($sql) {
            alert_subscribers($sql, SQL_QUERY_EXECUTED);

            // ...
        }
    }

    class Logger {
        public static function log($msg) {
            // ...
        }
    }

I want it to have multiple event types, so basically different groups are alerted when different events happen. I also don't know how the Logger should subscribe to Database under this design.

How should I go about implementing this? I'm not sure how to start.

There is nothing wrong with having the loggers everywhere. If you clutter all your classes with some kind of event notification it would look worst :)

But, Logger should not be called with a static method. You can create a pool of Loggers and your call will look like:

$log=Logger::get("Database");
$log->info("foo");

The big advantage is that you can put a configuration file in your project and define the log level of every logger independently.

According your questions you have really big plans. You want to write a template engine, a logging framework, an ORM and I guess somewhere in between your real application. Trust me when I tell you, you will fail. (I and most likely everybody else here tried it already :) ). For all your components there are mature frameworks out there. Find the one you like!!


Readability and cyclomatic complexity of database transaction code I consider these two options exactly the same. I prefer the first one but I'm not sure if it's better than the second option in terms of cyclomatic complexity or readability.

Assuming that this is not pure Ruby and I can call to the try method.

existing_transaction = database.find_transaction_by_id(transaction.id)
database.create_transaction(transaction) unless existing_transaction
database.update_transaction(transaction) unless existing_transaction.try(:processed)

existing_transaction = database.find_transaction_by_id(transaction.id)
if existing_transaction
  unless existing_transaction.processed
    database.update_transaction(transaction)
  end
else
  database.create_transaction(transaction)
end

It's obviously subjective, but IMHO inline conditionals make code harder to understand, indentation is very important. So I'd definitely take the second approach, but writing it differently to use only one indentation level:

existing_transaction = database.find_transaction_by_id(transaction.id)
if !existing_transaction
  database.create_transaction(transaction)
elsif !existing_transaction.processed
  database.update_transaction(transaction)
end