function for PDO queries and return result - tested - any tips? I write a function using PHP and MySQL, to get PDO connection, query and array of placeholders to execute query and return result.
I test this function and it has good response when result for SELECT has empty result or has error; do you see any problems, or have tips to say about this?
Do you have any tips about this to test or check?
Function and test code
function db_query_pdo( PDO $pd, $q,$ar )
    {

//      $pd Pdo connection object, $q query string ,  $ar array of placeholders
      try
       {
            $stmt = $pd->prepare($q);
            $x=$stmt->execute($ar);
            $rs=$stmt->fetchAll();

            if(!empty($rslt))
                return $rslt;
            else
                return $x;
        }
        catch (Exception $ex)
        {
            echo 'Query failed';
//            exit(); uncomment this lines if needed in future

//    echo $ex->getCode();
//    echo $ex->getMessage();
//    throw $ex;
        }

    }
// TEST CODE
$pdo_cnctn=db_con_pdo(); // is a pdo connection function that worked correctly and return connection object
    $id=133;
    $id2=77;

$query='SELECT * FROM `users` WHERE u_id=:uid OR u_id=:uid2;
';
$vals=[
    'uid' => $id,
    'uid2' => $id2
];

    $res= db_query_pdo($pdo_cnctn,$query,$vals);
//$res[] = $stmt->fetch();
    if(!is_bool($res))
    {
        var_dump($res);
    }
    else
    {
       // $$pdo_cnctn->rollBack();
        echo 'Not Found';
        goto  end_point;
    }

    //some codes
    end_point:

First of all, it's a very good thing that you came to the idea of such a function. Sadly, but out of many people asking questions on Stack Overflow, only a handful ever come to such an idea. It means you're a natural born programmer, with an eye for the sub-optimal code and the desire to make it better.
The regular routine with prepared statements is not that optimal indeed, you need two function calls instead of one. Some drivers already have such a function to execute the prepared query in one go, pg_query_params() for example. But for PDO we have to create one of our own.
The function
However, the code for such a function should be much, much simper. Basically it's just three lines
function db_query_pdo( PDO $pd, $q, $ar )
{
    $stmt = $pd->prepare($q);
    $stmt->execute($ar);
    return $stmt;
}

surprisingly, that's all you really need. The rest is either harmful, superfluous or can be achieved by other means.
$query='SELECT * FROM `users` WHERE u_id=:uid OR u_id=:uid2';
$vals=[
    'uid' => $id,
    'uid2' => $id2
];

$res = db_query_pdo($pdo_cnctn,$query,$vals)->fetchAll();
if($res)
{
    var_dump($res);
}
else
{
    echo 'Not Found';
}

See, the calling code is almost the same, but the function became
a) much cleaner
b) much more versatile, as you can chain any PDOStatement method to it, not limited to the return value of fetchAll()
Say, you've got a delete query and want to know whether it deleted any rows? No problemo!
if (db_query_pdo($pdo_cnctn,"DELETE FROM t WHERE id=?",[1])->rowCount()) {
    echo "deleted";
}

or you want a fetchAll() but with different modifier:
$ids = db_query_pdo($pdo_cnctn,"SELECT id FROM t WHERE id<?",[10] )->fetchAll(PDO::FETCH_COLUMN);

or you want to get a single value
$exists = db_query_pdo($pdo_cnctn,"SELECT 1 FROM t WHERE email=?",[$email] )->fetchColumn();

etc., etc., etc.
The error reporting
To be honest, all that try catch business in your function is completely off the track. Especially that commented out section, "uncomment this lines if needed in future". Seriously? Are you really going to comment these lines back and forth? That's not how a good program should be written. It shouldn't require anything like this.
Besides, this approach is based on the two wrong notions which, sadly, are extremely popular among PHP folks:

"an error must be thrown only in some circumstances". That's but a grave delusion. A programmer desperately needs the error message no matter that. An error must be thrown every time, no exceptions. As a matter of fact, in the production environment the error message is much more important than in the dev. Without the error message a programmer will never have an idea what's going wrong
"a database function should decide what to do with its errors". This is a funny one. There is nothing special in the database interactions. But for some reason PHP users diligently wrap their database functions in a distinct handling code. At the same time completely neglecting every single other line in the code. It just makes no sense. A database error is no different from a filesystem error. And as you don't wrap every include in a distinct try catch, you should never do it for a db function either.

All the error handling business should occur elsewhere. You can read more about error reporting basics from my article.


Array with enumerated elements Java program uses the following construction: (provided mainly for context)

public class Utils {
  public static final byte PROVIDER_FOO = 0;
  public static final byte PROVIDER_MOO = 1;
  public static final byte PROVIDER_BAR = 2;
  public static final byte PROVIDER_FUR = 3;
  public static final byte PROVIDER_BOO = 4;
  public static final byte PROVIDER_ANOTHER_FOO = 5;
  public static final byte PROVIDER_YET_ANOTHER = 6;

  public static String [] PROVIDERS = {"FOO", "MOO", "BAR", "FUR", "BOO",
    "PROVIDER_ANOTHER_FOO", "PROVIDER_YET_ANOTHER"};

  ...

}

So, each element of array has a number. And each array index in range 0..6 has text representation. Text representation is using in outer world: reading configs, printing results. Number representation is using in internal tasks. So, there a problem to transform text representation to number and vice versa. Number is transferring to string as following:

// someObject.getProviderNumeric() returns numeric value for someObj's provider
String strProvider = Utils.PROVIDERS[someObj.getProviderNumeric()]

String is transferring to number like this:

int getNumericByText(String strProvider) {
  for (int i = 0; i < Utils.PROVIDER_YET_ANOTHER; ++i) {
    if (strProvider.equals(Utils.PROVIDERS[i])) {
      return i;
  }
  return -1;
}

It looks pretty bulky. What is the best way to rewrite?

In my opinion, what you need is an enum.

Use an enum and I believe what you are looking for will be provided for you.

public enum Utils {
    PROVIDER_FOO, PROVIDER_MOO, PROVIDER_BAR, PROVIDER_FUR,
    PROVIDER_BOO, PROVIDER_ANOTHER_FOO, PROVIDER_YET_ANOTHER;
}

String to int:

Utils.valueOf(str).ordinal();

int to String:

Utils.values()[intValue].name();

Or if you want to be a bit more advanced, I suggest reading this answer of mine


Counting the number of alerts by type and partition from a JSON object Here is my problem, I receive these kind of json data and I don't know in advance the keys (unknownProperty#) but the different objects in this data array have the same keys. What I need to do is to calculate the number of each level of alert (these levels are fixed).
Here is what my end result should look like

var result = { unknownProperty1: { unknown: 0, ok: 2, warning: 0, ko: 1 },
  unknownProperty2: { unknown: 0, ok: 0, warning: 2, ko: 1 },
  unknownProperty3: { unknown: 3, ok: 0, warning: 0, ko: 0 },
  unknownProperty4: { unknown: 0, ok: 0, warning: 3, ko: 0 },
  unknownProperty5: { unknown: 0, ok: 0, warning: 2, ko: 1 } }

My script is working but I would like to know if there is any way to optimize/clean it with map/reduce/filter javascript function

const data = [
  {
    unknownProperty1: { alert: "ok", unusedItem1: "something" },
    unknownProperty2: { alert: "warning", unusedItem1: "something" },
    unknownProperty3: { alert: "unknown", unusedItem1: "something" },
    unknownProperty4: { alert: "warning", unusedItem1: "something" },
    unknownProperty5: { alert: "ko", unusedItem1: "something" }
  },
  {
    unknownProperty1: { alert: "ok", unusedItem1: "something" },
    unknownProperty2: { alert: "warning", unusedItem1: "something" },
    unknownProperty3: { alert: "unknown", unusedItem1: "something" },
    unknownProperty4: { alert: "warning", unusedItem1: "something" },
    unknownProperty5: { alert: "warning", unusedItem1: "something" }
  },
  {
    unknownProperty1: { alert: "ko", unusedItem1: "something" },
    unknownProperty2: { alert: "ko", unusedItem1: "something" },
    unknownProperty3: { alert: "unknown", unusedItem1: "something" },
    unknownProperty4: { alert: "warning", unusedItem1: "something" },
    unknownProperty5: { alert: "warning", unusedItem1: "something" }
  }
];

var result = new Object();
var dataKeys = data.map(function(element) {
  return Object.keys(element);
});
dataKeys[0].map(function(element) {
  result[element] = { unknown: 0, ok: 0, warning: 0, ko: 0 };
});
//At this point the result is like what we expect but no calculation has been made yet

//We then increment the right values
data.map(function(element) {
  for (var prop in element) {
    if (Object.prototype.hasOwnProperty.call(element, prop)) {
      // element correspond to the current object
      // prop corresponds to the name of the key, for example 'unknownProperty1'
      switch (element[prop].alert) {
        case "ok":
          result[prop].ok++;
          break;
        case "warning":
          result[prop].warning++;
          break;
        case "unknown":
          result[prop].unknown++;
          break;
        case "ko":
          result[prop].ko++;
          break;

        default:
          break;
      }
    }
  }
});
console.log(result);

Compact Code

If you're looking for more compact code, you can start by avoiding redundant function declarations:

var dataKeys = data.map(function(element) {
  return Object.keys(element);
});

var dataKeys = data.map(Object.keys);

And a compact notation for an empty object.

var result = new Object();

var result = {};

Pivot Table

What you're doing is pivotting data from rows to columns. You are correct to mention reduce as this method could help us out here. It's possible to leave out the switch case if we just loop through the entries and aggregate the results.

var pivotTable = data.reduce((result, item) => {
  Object.entries(item).forEach(([key, val]) => {
    if (result[key] === undefined) {
      result[key] = { unknown: 0, ok: 0, warning: 0, ko: 0 };
    }
    result[key][val.alert]++;
  });
  return result;
}, {});

console.log(pivotTable);


Method to return a collection of objects, filtered by a string I have a Vehicle object which has a few properties such as Make, Model, Price, etc.

I also have a VehicleCollection object which derives from List<Vehicle which has a few custom methods, one of which is the following...

class VehicleCollection : List<Vehicle>
{
    public VehicleCollection GetVehicles(string _searchTerm = "")
    {
        // return the entire collection if no search term is provided
        if (_searchTerm.Length == 0)
            return this;

        var matchingVehicles = new VehicleCollection();
        foreach(var vehicle in this) // loop through all vehicles
        {
            PropertyInfo[] propInfos = vehicle.GetType().GetProperties(); // all properties of the current vehicle
            for (int i = 0; i < propInfos.Length; i++)
            {
                var propVal = propInfos[i].GetValue(vehicle); // the value of the current property of the current vehicle

                // check aganst value of a strings or the values within a string array
                // ignores case
                if (propVal is string propertyValueStr && propertyValueStr.IndexOf(_searchTerm, StringComparison.CurrentCultureIgnoreCase) >= 0
                    || propVal is string[] propValueArr && propValueArr.Any(x => x.IndexOf(_searchTerm, StringComparison.CurrentCultureIgnoreCase) >= 0))
                {
                    matchingVehicles.Add(vehicle);
                }
            }
        }

        return matchingVehicles;
    }
}

This returns a collection of vehicles, of which each vehicle must have a property whose value contains the supplied _searchTerm parameter.

Is this the best way to do this? Can this be achieved using LINQ at all? Can this be done without reflection?

Edit:
At the request of a comment, here is the Vehicle Class, with a few unnecessary methods omitted.

public class Vehicle
{
    public string ID { get; private set; }
    public string Make { get; private set; }
    public string Model { get; private set; }
    internal decimal Value { get; private set; }
    public string Price { get { return string.Format("{0:C}", Value); } }
    public VehicleColourEnum Colour { get; private set; }
    internal StatusEnum Status { get; private set; }
    public string State
    {
        get
        {
            switch (Status)
            {
                case StatusEnum.ComingSoon:
                    return "Coming Soon";
                case StatusEnum.HasDeposit:
                    return "Has Deposit";
                case StatusEnum.InStock:
                    return "In Stock";
                case StatusEnum.Sold:
                    return "Sold";
                case StatusEnum.Unknown:
                default:
                    return "Unknown";
            }
        }
    }

    public Vehicle(string _make, string _model, decimal _value, VehicleColourEnum _colour = VehicleColourEnum.None, StatusEnum _status = StatusEnum.Unknown)
    {
        Make = _make;
        Model = _model;
        Value = _value;
        Colour = _colour;
        Status = _status;
        ID = Guid.NewGuid().ToString("N");
    }
}

I'll ignore your naming conventions/etc as I know that's not what you're asking about.

Since the VehicleCollection object already inherits from IEnumerable through the List<> inheritence, you can query the collection as a list through the 'this' keyword.

That means you could write up your search method like:

public VehicleCollection GetVehicles(string searchTerm = "")
{
    // Null and white-space check for sanity
    if(string.IsNullOrWhiteSpace(searchTerm))
    // We're assuming if the search term is bad, we return everything.
    // This isn't standard practice - usually we return nothing
        return this;

    var matches = new VehicleCollection();
    matches.AddRange(
        this.Where(v=>v.ID.Equals(searchTerm) ||
                    v.Make.Equals(searchTerm) ||
                    v.Model.Equals(searchTerm)
    // I personally prefer to be explicit in what I'm selecting, but it's your choice
        ).Select(v=>v));

    // inline IF because I'm lazy
    // again, we're returning everything if there are no matches,
    // which isn't standard practice.
    return matches.Any()? matches: this;
}

Hope that helps!


Extracting the first two initials of a person's name I've come up with a hackish approach to get my XSL function to only return the first 2 initials of a person. I think there should be a more efficient way to do this though. The replace specifically is what I think I should be able to get rid of, looking through docs though I can't figure out how to correctly iterate with the split.

So 5 examples of my XML:

<name>
    <surname>Test I</surname>
    <given-names>Chris</given-names>
</name>
<name>
    <surname>Test II</surname>
    <given-names>Chris Chris</given-names>
</name>
<name>
    <surname>Test III</surname>
    <given-names>Chris Eat Meat</given-names>
</name>
<name>
    <surname>Test IV</surname>
    <given-names>Chris-Eat Meat</given-names>
</name>
<name>
    <surname>Test V</surname>
    <given-names>Chris-Eat-Meat</given-names>
</name>

My function call:

<xsl:value-of select="pull_it:get_initials(name/given-names/text())"/> <!-- name/given-names -->

and my inefficient XSL function:

<xsl:function name="pull_it:get_initials">
    <xsl:param name="names"/>
    <xsl:value-of select="
            replace(string-join(
                for $token in tokenize($names,'(-|\s+)')[string-length(.) > 0]
                    return concat(substring($token,1,1),'.'),
                ''), '(\w\.\w?\.?).*', '$1')
                    "/>
</xsl:function>

So with the first two XML blocks the expected output/return is:

C.

and

C. C.

The remaining three should return (as expected, no M. because we only want the first 2 initials)

C. E.

but I thought there would be a way to stop the iteration. For example my PHP just has:

$abbreviated_firstnames = array();
$firstnames = mb_split('(\s+|-)', html_entity_decode($this->firstname,ENT_QUOTES,'UTF-8'));
$intial_count = 0;
foreach ($firstnames as $firstname) {
    $intial_count++;
    $firstinit = mb_substr($firstname,0,1,'UTF-8');
    if ($firstinit) {
        $abbreviated_firstnames[] = $firstinit . '.';
        if($intial_count >= 2) {
            break; // <---- we got 2 matches stop NOW
        }
    }
}
return implode(' ',$abbreviated_firstnames);

I think you should be able to get rid of the replace() by testing the position of the token in an additional predicate.

You should also be able to remove the string-join() (unless you don't want spaces between the initials).

You can also remove the string-length() test by adjusting the regex for -.

New function based on above suggestions:

<xsl:function name="pull_it:get_initials">
  <xsl:param name="names"/>
  <xsl:value-of select="
    for $token in tokenize($names,'(-+|\s+)')[2 >= position()]
    return concat(substring($token,1,1),'.')
    "/>
</xsl:function>

Complete example (used Saxon-HE 9.5 to test)...

XML Input (Added test VI)

<doc>
    <name>
        <surname>Test I</surname>
        <given-names>Chris</given-names>
    </name>
    <name>
        <surname>Test II</surname>
        <given-names>Chris Chris</given-names>
    </name>
    <name>
        <surname>Test III</surname>
        <given-names>Chris Eat Meat</given-names>
    </name>
    <name>
        <surname>Test IV</surname>
        <given-names>Chris-Eat Meat</given-names>
    </name>
    <name>
        <surname>Test V</surname>
        <given-names>Chris-Eat-Meat</given-names>
    </name>
    <name>
        <surname>Test VI</surname>
        <given-names>Chris---Eat----Meat</given-names>
    </name>
</doc>

XSLT 2.0

<xsl:stylesheet version="2.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
  xmlns:pull_it="pull_it"
  exclude-result-prefixes="pull_it">
  <xsl:output indent="yes"/>
  <xsl:strip-space elements="*"/>

  <xsl:function name="pull_it:get_initials">
    <xsl:param name="names"/>
    <xsl:value-of select="
      for $token in tokenize($names,'(-+|\s+)')[2 >= position()]
      return concat(substring($token,1,1),'.')
      "/>
  </xsl:function>

  <xsl:template match="@*|node()">
    <xsl:copy>
      <xsl:apply-templates select="@*|node()"/>
    </xsl:copy>
  </xsl:template>

  <xsl:template match="name">
    <xsl:copy>
      <xsl:apply-templates select="@*|node()"/>
      <initials><xsl:value-of select="pull_it:get_initials(given-names)"/></initials>
    </xsl:copy>
  </xsl:template>

</xsl:stylesheet>

XML Output

<doc>
   <name>
      <surname>Test I</surname>
      <given-names>Chris</given-names>
      <initials>C.</initials>
   </name>
   <name>
      <surname>Test II</surname>
      <given-names>Chris Chris</given-names>
      <initials>C. C.</initials>
   </name>
   <name>
      <surname>Test III</surname>
      <given-names>Chris Eat Meat</given-names>
      <initials>C. E.</initials>
   </name>
   <name>
      <surname>Test IV</surname>
      <given-names>Chris-Eat Meat</given-names>
      <initials>C. E.</initials>
   </name>
   <name>
      <surname>Test V</surname>
      <given-names>Chris-Eat-Meat</given-names>
      <initials>C. E.</initials>
   </name>
   <name>
      <surname>Test V</surname>
      <given-names>Chris---Eat----Meat</given-names>
      <initials>C. E.</initials>
   </name>
</doc>


Float string to integer string I have a float in the String strFloat.  I wanted to convert it to a rounded integer also stored in a String.  This looks awful to me, so please offer some suggestions for improvement.

String strInteger = new Integer(Float.valueOf(strFloat).intValue()).toString()

Instead of creating a new Integer and throwing it away, use String.valueOf() directly.

String strInteger = String.valueOf(Float.valueOf(strFloat).intValue())


Removing exact instances of elements in one list from another Basically, here's the problem statement:

  Given an IEnumerable<T> source and an IEnumerable<T> exceptions, return an IEnumerable<T> result which contains all of source, without exceptions, only omitting exact instances of a value. I.e.: if a value appears twice in source, but once in exceptions, then result will have that value exactly one time.

Let's take an example:

Exception list:

{ 2, 2, 3, 4, 5}

Source list:

{ 2, 2, 2, 6 }

Result:

{ 2, 6 }

Code:

public static IEnumerable<T> ExceptExact<T>(this IEnumerable<T> source, IEnumerable<T> exceptions)
{
    var tExceptions = new List<T>();
    tExceptions.AddRange(exceptions);

    var result = new List<T>();

    foreach (var el in source)
    {
        if (tExceptions.Contains(el))
        {
            tExceptions.RemoveAt(tExceptions.IndexOf(el));
        }
        else
        {
            result.Add(el);
        }
    }

    return result;
}

Usage:

var a = new List<int> { 2, 3, 5, 4, 2 };
var b = new List<int> { 2, 2, 2, 6 };

var c = b.ExceptExact(a);
var d = a.ExceptExact(b);

Console.WriteLine("A: { " + string.Join(", ", a) + " }");
Console.WriteLine("B: { " + string.Join(", ", b) + " }");
Console.WriteLine("C: { " + string.Join(", ", c) + " }");
Console.WriteLine("D: { " + string.Join(", ", d) + " }");

Output:

A: { 2, 3, 5, 4, 2 }
B: { 2, 2, 2, 6 }
C: { 2, 6 }
D: { 3, 5, 4 }

Additional to the valid point from Rick Davin:

1) You can drop the Contains because IndexOf returns -1 if the item is not contained.

2) You could use yield to get rid of the additional list

public static IEnumerable<T> ExceptExact<T>(this IEnumerable<T> source, IEnumerable<T> exceptions)
{
    var tExceptions = exceptions.ToList();

    foreach (var el in source)
    {
        var index = tExceptions.IndexOf(el);
        if (index >= 0)
        {
            tExceptions.RemoveAt(index);
            continue;
        }
        yield return el;
    }
}

After taking a look to MSDN (List.Remove), I figured out that there is even a simpler way:

  Removes the first occurrence of a specific object from the List.

  Returns true if item is successfully removed; otherwise, false.This method
  also returns false if item was not found in the List.

public static IEnumerable<T> ExceptExact<T>(this IEnumerable<T> source, IEnumerable<T> exceptions)
{
    var tExceptions = exceptions.ToList();
    return source.Where(el => !tExceptions.Remove(el));
}


Optimize subquery that searches xml The following query lists all domains from multiple companies in which a value occurs in an XML set. This is filtered to the most recent occurrence within a provided date range.

On large databases the query has performance problems. Is there a more efficient method to retrieve the most recent query results within the provided date range?

SELECT Company.Name,
       Domain.Name,
       Collection.CollectedOn
FROM Collection
INNER JOIN Domain ON Domain.DomainID = Collection.DomainID
INNER JOIN Company ON Domain.CompanyID = Company.CompanyID
WHERE CollectedOn = (SELECT MAX(CollectedOn)
                     FROM Collection
                     INNER JOIN QResult ON QResult.CollectionID = Collection.CollectionID
                     INNER JOIN QVersion ON QVersion.QVersionID = QResult.QVersionID
                     INNER JOIN QLibrary ON QLibrary.QueryID = QVersion.QueryID
                     WHERE Collection.DomainID = Domain.DomainID
                            AND CollectedOn >= :CollectedAfter
                            AND CollectedOn <= :CollectedBefore
                            AND QLibrary.Name LIKE 'Some_Table_Name_That_Only_Occurs_Once_or_Twice'
                            AND QResult.QResults.exist('/DataSet/some_table/name/text()[contains(., ''Error_value_here'')]') = 1)

If having a visual representation of the tables would be helpful I can post requested tables.

Explanation and information:

A company can have multiple domains.
A domain can have multiple collections. For the purpose of this query, collections are essentially a grouping of previously run queries executed on external (other companies) machines.
There are around 100 companies. Each company generally has 1-10 Domains. There are hundreds of collections for each domain.
QLibrary is a small table. The name I'm searching for with the LIKE param only occurs a few times. Some of the tables I'm retrieving have additional characters concatenated to the searched for name. Thus using LIKE instead of strict equality comparison.
The QResult is a large set of XML. I'm not sure how expensive this is to search in an SQL statement. If this could significantly contribute to a performance problem, I can search through this in a different language after the query is returned.  I've tried to restrict this based on QLibrary.Name. Most of the tables searched in this query should be around 5MB.

Tables (Edit)

Company Table:
╔═══════════╦═══════╗
║ CompanyID ║ Name  ║
╠═══════════╬═══════╣
║         1 ║ name1 ║
║         2 ║ name2 ║
╚═══════════╩═══════╝

Domain Table:
╔══════════╦═══════════╦═══════╗
║ DomainID ║ CompanyID ║ Name  ║
╠══════════╬═══════════╬═══════╣
║        3 ║         1 ║ name1 ║
║        4 ║         1 ║ name2 ║
║        6 ║         4 ║ name3 ║
╚══════════╩═══════════╩═══════╝

Collection Table:
╔══════════════╦══════════╦═════════════╗
║ CollectionID ║ DomainID ║ CollectedOn ║
╠══════════════╬══════════╬═════════════╣
║            1 ║        1 ║ 2016-01-03  ║
║            2 ║        1 ║ 2016-05-11  ║
║            3 ║        2 ║ 2015-09-04  ║
╚══════════════╩══════════╩═════════════╝

QResult Table:
╔════════════╦══════════════╦══════════════════╗
║ QVersionID ║ CollectionID ║     QResult      ║
╠════════════╬══════════════╬══════════════════╣
║          1 ║            1 ║ <bunch><of><xml> ║
║          2 ║            1 ║ <other><xml>     ║
╚════════════╩══════════════╩══════════════════╝

QVersion Table:
╔════════════╦═════════╗
║ QVersionID ║ QueryID ║
╠════════════╬═════════╣
║          1 ║       1 ║
║          2 ║       2 ║
║          3 ║       2 ║
╚════════════╩═════════╝

QLibrary Table:
╔═════════╦═════════════╗
║ QueryID ║  QueryName  ║
╠═════════╬═════════════╣
║       1 ║ some_name_1 ║
║       2 ║ some_name_2 ║
╚═════════╩═════════════╝

2 suggestions:

do you really need a LIKE, can't you use an equality?
can't you try to first filter the XML and then (but still in the same query) going up with the needed joins to retrieve what you need ?

Also your performances obviously depend on the size of various tables and on the available indexes. Your RDBM should be able to let you know which indexes it will use for your above query and based on that you may find you are missing some. Filtering on dates, xml, and LIKE patterns has a cost.


Printing letters in order like a pyramid There is this hackerrank question, where we need a rhombus of letters. As to my opinion it can be viewed as some fractal, or similar logic.

Well I solved the question, but I am obviously disappointed with the code I come up with. Actually when I started writing I knew how I would end-up and I was ready to ask a question here.

I am reluctant to share this code of mine but it is necessary, if I am to improve.

size = int(raw_input())
alphabet = "abcdefghijklmnopqrstuvwxyz"
def draw(size):
    shape = "" # The answer
    tmp_size = size # defining a temporary size variable to help with the solution
    tmp_size -= 1
    while tmp_size+1:
        hyphen_count = (size-tmp_size) *2 - 2 # Needed hyphen count for the left-right of new_line
        hypens = hyphen_count*"-" # Hyphens to be added

        # The new line, the strange ranges I used was to get the correct letters in place. Probably needs simplifying.
        new_line = "-".join([alphabet[abs(num)] for num in range(1-size,tmp_size-size+1) + range(size-tmp_size-1, size)])

        # Draw one line if it is the first line
        if tmp_size == size - 1:
            shape = shape + new_line
        # Draw to both top and bottom if it is not the first line.
        else:
            shape = hypens + new_line + hypens + "\n" + shape + "\n" + hypens + new_line + hypens
        tmp_size -= 1
    return shape
print draw(size)

I added many comments for easier evaluation of this badly-written code. So what I ask is:

Dear users with algorithm experience, please help me find a better method if not the code itself. (Maybe a recursion ?)
Dear programmers, please help me implement better python.
Dear users that know everything, please teach me everything.

I propose a different method!
If you generate the center, and the size of the shape you can do the entire thing easily with str.format.

>>> '{0:-^16}'.format('yo')
'-------yo-------'

Simple.

The biggest problem is getting the diamond shape.
But this can be done with ease.
For the amount of different letters you want per line it goes:

1, 2, 3, 4, 3, 2, 1

Where the character you want goes:

3, 2, 1, 0, 1, 2, 3

If you make a range that goes like the first, you can change it to the second by subtracting from 4 each time.

I'll define this range as:

def piramid_range(size):
    return range(1, size) + range(size, 0, -1)

A quick run of size 4 gives us the correct output.
And subtracting from 4 each time gives us the correct output too.

>>> piramid_range(4)
[1, 2, 3, 4, 3, 2, 1]
>>> [4 - i for i in piramid_range(4)]
[3, 2, 1, 0, 1, 2, 3]

and so adding this all together, with two comprehensions makes for a nice small function:

def draw(size):
    fmt = '{{0:-^{size}}}'.format(size=4 * size - 3)
    return '\n'.join(
        fmt.format('-'.join([alphabet[size - s] for s in piramid_range(inner_size)]))
        for inner_size in piramid_range(size)
    )

And finally, as I think your separator character is quite ugly, I think adding the ability to change it would be nice. This is just replacing its two occurrences.

def draw(size, char='-'):
    fmt = '{{0:{char}^{size}}}'.format(char=char, size=4 * size - 3)
    return '\n'.join(
        fmt.format(char.join([alphabet[size - s] for s in piramid_range(inner_size)]))
        for inner_size in piramid_range(size)
    )

print draw(4)
print
print draw(4, char=' ')

Which would have the following output:

------d------
----d-c-d----
--d-c-b-c-d--
d-c-b-a-b-c-d
--d-c-b-c-d--
----d-c-d----
------d------

      d
    d c d
  d c b c d
d c b a b c d
  d c b c d
    d c d
      d


Reading numbers from console until input isn't a number I want to read N numbers (Nmax = 100) from console into a list. N is not known, but the first input that is not a number may break the reading process... However the solution should be as simple as possible, so I just read 100 lines:

let numbers = [1..100]
        |> List.map (fun x -> Int32.TryParse(Console.ReadLine()))
        |> List.filter (fun (isNum, num) -> isNum)
        |> List.map (fun (isNum, num) -> num)

Is there a simpler (less code) solution?

Your code does not read numbers from the "console until input isn't a number", but reads 100 inputs strings from the console and returns those that can be converted to integers.

If you want to read numbers from the console "until input isn't a number", you could do something like this:

let numbers1 max = seq {for x in 1..max do yield Int32.TryParse(Console.ReadLine()) }
                  |> Seq.takeWhile (fun (b, x) -> b)
                  |> Seq.map (fun (b, x) -> x)

or

let numbers2 max = seq {for x in 1..max do yield Int32.TryParse(Console.ReadLine()) }
                    |> Seq.takeWhile (fun (b, x) -> b)
                    |> Seq.map (fun (b, x) -> x)
                    |> Seq.toList

if you want to defer the return of each input to after the last valid input has been entered.


C++ delegate implementation with member functions I have been implementing a delegate class with C++11 that supports class member functions. I am interested in knowing if there are any potential problems with this implementation. My main concern is the private Key struct of the Delegate class. Can I always rely on the key being correct in all situations?  Meaning, can I always identify certain function of certain object with it?

In there I am storing 3 things:

hash of the member function pointer
pointer to the object
std::function wrapping the actual function

The hash is calculated from the function pointer with the getHash() function. I've originally used only that as key, but I run into a problem with inherited classes.  I've gotten the same hash for the inherited function with 2 different classes inheriting from same base class. For that reason, I've also stored the pointer to the actual object, using these 2 as key I am able to identify certain entry in the list of stored keys. I'd be really interested to know any potential issues with this implementation. This compiles with VS2013, and I am hoping it will also work with Clang, but I haven't tried it out yet.

template <typename...Params>
class Delegate {
private:

typedef std::function < void(Params...)> FunctionType;
struct Key
{
    size_t        m_funcHash;
    void*         m_object;
    FunctionType  m_func;

    Key(void* object, size_t funcHash, FunctionType func) : m_object(object), m_funcHash(funcHash), m_func(func)
    {}

    bool operator==(const Key& other) const {
        return other.m_object == m_object && other.m_funcHash == m_funcHash;
    }
};

std::vector <Key> m_functions;

public:
template <typename Class>
void connect(Class& obj, void (Class::*func) (Params...) )
{

    std::function < void (Class, Params...) >  f = func;
    FunctionType fun = [&obj, f](Params... params) { f(obj, params...); };
    size_t hash = getHash(func);
    m_functions.push_back(Key( &obj, hash, fun));
}

template <typename Class>
size_t getHash(void (Class::*func) (Params...)) {
    const char *ptrptr = static_cast<const char*>(static_cast<const void*>(&func));
    int size = sizeof (func);
    std::string str_rep(ptrptr, size);
    std::hash<std::string> strHasher;
    return strHasher(str_rep);
}

template <typename Class>
void disconnect( Class& obj, void (Class::*func) (Params...)  ) {
    size_t hash = getHash(func);

    for (unsigned int i = 0; i < m_functions.size(); ++i) {
        auto key = m_functions[i];
        if (key.m_funcHash == hash && key.m_object == &obj) {
            m_functions.erase(m_functions.begin() + i);
            --i;
        }
    }
}

template <typename ... Args>
void operator() (Args...args)
{
    for (auto& key : m_functions) {
        key.m_func (args...);
    }
}
};

class BaseClass {

public:
    virtual void print(const std::string& str) = 0;
};

class A : public BaseClass {
public:
    void print(const std::string& str) {
        std::cout << " Class A : Print [" << str << "]\n";
    }

};

class B : public BaseClass {
public:
    void print(const std::string& str) {
        std::cout << " Class B : Print [" << str << "]\n";
    }
};

int _tmain(int argc, _TCHAR* argv[])
{

    A a;
    B b;

    Delegate <const std::string&> delegate;

    delegate.connect(a, &A::print);
    delegate.connect(b, &B::print);

    delegate("hello");
    delegate("world");

    delegate.disconnect(a, &A::print);
    delegate("bye world");

    delegate.disconnect(b, &B::print);
    delegate("nobody there.");  // should not print anything

    std::cin.ignore();

    return 0;
}

Fatal bugs:

A big issue is the fact that you take a copy of the object in the function in this line:

    std::function < void (Class, Params...) >  f = func;

This needs to be Class& at the very least. Otherwise, invocations of handlers would operate on copies of the intended objects. See demonstration here: Live On Coliru with debug information showing copies are being made.

Conceptual issues:

you are using a hash for equality. This is a conceptual flaw: hashes can have collisions. Hashes are used to organize large domains into a smaller set of buckets.

  this could really be considered a fatal bug, as disconnect might disconnect unrelated handlers here

you're wrapping your member-function pointers in a std::function. Twice. This is gonna be very bad for anything performance-sensitive
you are abusing std::function for generic calleables. Instead, use perfect storage on user-supplied calleables (why bother whether it's std::mem_fun_ref_t, a bind-expression, a lambda or indeed a function pointer?). This way

you don't incur the overhead of type erasure and virtual dispatch that comes with std::function, unless your situation requires it
you don't have to bother with the annoying details that you have to, now

you don't need to treat member functions differently to begin with (std::bind to the rescue)
you don't need to rely on implementation defined implementation of pointer-to-member-function

you're using implementation defined behaviour to get a hash of a pointer-to-member-function (see previous bullet)
your less-than generic solution works for pointer-to-member-function only. This you already knew. But, you may not have realized, it doesn't work for const or volatile qualified member functions.
your Delegate class tries to "guess" identity of registered handlers. In fact, the registering party should be responsible for tracking exactly which registration it manages. A cookie-based design would be most suitable here, and certainly takes all the implementation defined behaviour out of the equation (as well as immediately making it clear what happens when the same handler gets registered multiple times). See the "more drastic rewrite" below.

Style, Efficiency

Your Key class isn't the Key. It's the collection Entry (Key + Function).
Your Key defines operator==. An object that's equatable necessitates linear search to do a lookup. Instead, define a weak total ordering (using operator<) so you can use tree-like data-structures. See below for a sample that uses std::tie() to quickly implement both relational operators (the first version).
Also, need to use Key::operator== instead of just comparing m_object and m_funcHash again (encapsulation)
Your Key stores the object, but the object is also part of FunctionType implicitely, because you capture it by reference. You should probably remove the redundancy.
Your disconnect lookup is ... not very effective:

linear search? really?
no early out? (it's unclear how you really want repeatedly registered callbacks to be handled, but I'd say

 del.connect(some_handler);
 del.connect(some_handler);
 del.disconnect(some_handler); // disconnect 1 of the two
 del.disconnect(some_handler); // disconnect the other

would be the principle of least surprise)
your i-- invokes unsigned integer wraparound. Now, this is not undefined behaviour, but it's bad style IMO. You could use

for(unsigned int i = 0; i < m_functions.size(); ) {
    auto key = m_functions[i];
    if(key.m_funcHash == hash && key.m_object == &obj) {
        m_functions.erase(m_functions.begin() + i);
    } else
    {
        ++i;
    }
}

instead.
the loop crucially depends on m_functions.size() being evaluated each iteration. This is inefficient and error-prone

  Guideline: Whenever you see a loop that became... non-trivial and obscure like this, it's a sure sign you need to use an (existing) algorithm

Fixing just these elements: just use a better datastructure, like

struct Key {
    void*         m_object;
    size_t        m_funcHash;
    // no ctor: let just just be an aggregate

    bool operator==(const Key& other) const { return key() == other.key(); }
    bool operator< (const Key& other) const { return key() <  other.key(); }
  private:
    // trick to make it trivial to consistently implement relational operators:
    std::key<void* const&, size_t const&> key() const { return std::tie(m_object, m_funcHash); }
};

std::multimap<Key, FunctionType> m_functions;

The whole disconnect function becomes trivial:

template <typename Class>
void disconnect(Class& obj, void (Class::*func)(Params...)) {
    m_functions.erase(Key { &obj, getHash(func) });
}

Fixed version(s)

Here's a version that uses the multimap approach (fixing just the style/efficiency issues) Live On Coliru

A more drastic rewrite

would look like this: Live On Coliru

This version alters the design so most (if not all) the problems are sidestepped. It uses boost's stable_vector to do a cookie-based design. This moves the burden to track handler identities on the registering parties (where it belongs after all).

#include <functional>
#include <iostream>
#include <boost/container/stable_vector.hpp>

template <typename... Params>
class Delegate {
private:
    typedef std::function<void(Params...)> Handler;
    typedef boost::container::stable_vector<Handler> Vector;
    Vector m_handlers;
public:
    typedef typename Vector::const_iterator cookie;

    cookie connect(Handler&& func) {
        m_handlers.push_back(std::move(func));
        return m_handlers.begin() + m_handlers.size() - 1;
    }

    template <typename... BindArgs, typename Sfinae = typename std::enable_if<(sizeof...(BindArgs)>1), void>::type>
    cookie connect(BindArgs&&... args) {
        return connect(Handler(std::bind(std::forward<BindArgs>(args)...)));
    }

    void disconnect(cookie which) {
        m_handlers.erase(which);
    }

    template <typename ... Args> void operator()(Args...args) {
        for(auto const& handler : m_handlers)
            handler(args...);
    }
};

//////////////////////////////////
// demonstration
#define CV volatile const
struct BaseClass {
    virtual void print(const std::string& str) CV = 0;
};

struct A : BaseClass {
    A() {}
    void print(const std::string& str) CV { std::cout << " Class A : Print [" << str << "]\n"; }
};

struct B : BaseClass {
    B() {}
    void print(const std::string& str) CV { std::cout << " Class B : Print [" << str << "]\n"; }
};

using namespace std::placeholders;

int main() {
    Delegate <const std::string&> delegate;
    A CV a;
    B CV b;
    auto acookie = delegate.connect(&A::print, std::ref(a), _1);
    auto bcookie = delegate.connect(&B::print, std::ref(b), _1);

    delegate("hello");
    delegate("world");

    delegate.disconnect(acookie);
    delegate("bye world");

    delegate.disconnect(bcookie);
    delegate("nobody there.");  // should not print anything
}


Vulkan and Opengl swapable rendering backend I'm working on a multi render library. The point of it is to have a swapable backend at runtime to render the same item.

The renderer is given to the window (which is a wrapper over SDL2). Then the window is in charge of drawing a IDrawable, which is a virtual class implemented in a namespace for each backend.
The implementation of IDrawable in each namespace got the same tag as the backend, for preventing rendering a drawable with the wrong backend.

Here's the code. I would appreciate some reviews on the architecture; what's good and what's wrong.

Thanks!

#include <string>
#include <iostream>

enum Tag
{
  VK,
  GL
};

class IDrawable
{
    public:
        virtual void draw() = 0;
        virtual Tag getTag() const = 0;
};

namespace gl
{

class D : public IDrawable
{
    public:
        void draw()
        {
            std::cout << "Drawable gl::D\n";
        }

        Tag getTag() const { return tag; };
    private:
        Tag tag = Tag::GL;
};

}

namespace vk
{

class D : public IDrawable
{
    public:
        void draw()
        {
            std::cout << "Drawable vk::D\n";
        }

        Tag getTag() const { return tag; };
    private:
        Tag tag = Tag::VK;
};

}

class Backend
{
    public:
        virtual void render(IDrawable * d) = 0;
};

class Opengl : public Backend
{
    public:
        void render(IDrawable * d)
        {
            std::cout << "Backend gl\n";
            d->draw();
        }
        Tag getTag() { return tag; };
    private:
        Tag tag = Tag::GL;
};

class Vulkan : public Backend
{
    public:
        void render(IDrawable * d)
        {
            std::cout << "Backend vk\n";
            d->draw();
        }
        Tag getTag() { return tag; };
    private:
        Tag tag = Tag::VK;
};

template <typename T>
class Renderer
{
    static_assert(std::is_base_of<Backend, T>::value, "Backend not valid");
    public:
        void draw(IDrawable * d)
        {
            if(backend.getTag() == d->getTag())
            {
                backend.render(d);
            }
            else
            {
                std::cout << "Cannot render\n";
            }
        }

        T backend;
};

template <typename T>
class Window
{
    public:
        Window(Renderer<T> r) : rend(r) {}

        void draw(IDrawable * d)
        {
            rend.draw(d);
        }

    private:
        Renderer<T> rend;
};

int main()
{
    Renderer<Opengl> a;
    Renderer<Vulkan> b;

    Window w(a);
    Window k(b);

    gl::D d1;
    vk::D d2;

    w.draw(&d1);
    k.draw(&d2);
    w.draw(&d2); // Should fail

    return 0;
}

Live example

First of all: OpenGL and Vulkan work very differently in some parts. In the following i assume you are already familiar with vulkan.

IDrawable

There would probably some problems arise with the current design. You cannot just render something like that in vulkan, a vulkan drawable cannot implement the draw() function without that you maintain any global state (which you hopefully don't want, one of the strengths of vulkan is that you don't need that anymore and therefore gain things like threadsafety and a more reliable api performance). You would at least need a command buffer to record into, but this would change the meaning of the draw interface on different backends (immediate rendering on opengl vs recording on vulkan), which is bad. Additionally, in this case every drawable would change the bound pipeline (another thing that can be optimized using vulkan). In the end it would maybe make sense to handle drawing entirely in the renderer and maybe only expose things like material and vertices from the drawable.
Every drawable could e.g. own a Buffer object which is an interface that might be a vulkan or an opengl buffer and managed by the renderer (those are the parts where one could really nicely abstract opengl and vulkan).
In the end this also drives away the need for this rather smelly tag-based polymorphism pattern, and allows to model things in a easier, more natural way (which is usually a good sign). When you render using e.g. vulkan there should not be any opengl drawables around (should there? not a sign if there could be...) so there is no need to use this tag checking.

Templates

I am not sure your use of templates is optimal. In this case (although you just write the type Window in both cases) the both Windows are actually totally different types and therefore could not be changed at runtime. This means if you want to change the renderer you have to change the code
somewhere which is probably not what you want (The same for the Renderer class).
It would make more sense to have something like virtual (or even abstract) Renderer and Window interfaces that are then implemented by {Vulkan, GL}Window/Renderer. But doing this for renderer already drives away the need to do this for window at all, which is (at least for me) code smell anyways, the Window class should not changed dependent on the renderer used. In the case described you would need the Backend class anymore. You could of course use the Backend and the Render class but then, at least don't use templates (and I am not sure if there is enough to abstract from the both rendering apis that would justify a separate Renderer class).

Modern C++

You use C++17 features (and compile using this flag) so i assume you want to write modern C++. If so use the (great) features modern C++ gives you:

Use enum class Tag instead of enum Tag for type safety and less name pollution
Use the override keyword for functions that implement virtual ones
Use references where a nullptr is not a valid argument. Unless Renderer::draw(nullptr) has a special meaning and is therefore handled (which would proably be bad interface design) you should use a reference. Same for Window and Backend.
Window(Renderer) copies the renderer which is probably a bad idea. The renderer will be a heavy class which lots of state (which should probably not be copyable in the first place), use move semantics in a case like this. Additionally it would be nice to separate Window and Renderer, prefer to build a Renderer on top of a Window and not the other way around (how should the Renderer know to which Window it should render?). A draw method makes much sense in a Renderer but not so much in a Window (although there are many Window abstractions like this, this will lead to design issues later).

Also: No matter in which stage code is, don't name your classes something like D. And IMO Vulkan isn't a really good name either, choose something like VulkanBackend instead (same of OpenGL).

Those were just some first things that came to my mind (there are of course also things done right in your code i did not mention), there is probably more to say about it. An abstraction like this is really hard to build and (as a small - maybe opinionated - side note) it probably makes more sense to focus on one of the both api's if you do not absolutely need both since they are really different (at least in some, critical places like the rendering - plan global calls vs command buffer recording).


Prepend one value to a list 'x' number of times I want to add a value multiple times at the beginning of a list, what I have is...

internal static class Helper
{
  public static void Prepend(this IList<double> source, double value, int count)
  {
    for (var i = 0; i < count; ++i)
    {
      source.Insert( 0, value );
    }
  }
}

Then use it as follow

// create
var mylist = new List(){ ... };  // create the list
mylist.Prepend( 0, 10 );  // prepend 0 to the list

But I wonder if there isn't a more efficient way of doing the same thing?

Is there a reason you use IList? because List has the InsertRange method which uses Array.Copy amongst other optimizations.

If we need to support IList,  we don't want to depend upon implementation details and IList.Insert sounds like your best option.

Another approach is you could check whether the actual implementation is List and use the InsertRange method or fallback to Insert in a loop.

Example of adding the items using InsertRange

var items = Enumerable.Repeat(0, 10);
source.InsertRange(0, items);


Codility binary gap solution using indices I am seeking a review of my codility solution.

Whilst the problem is fairly simple, my approach differs to many solutions already on this site. My submission passes 100% of the automated correctness tests.

I am looking for feedback regarding the design of my solution, and any comments in relation to how this submission might be graded by a human.

Problem description:

  A binary gap within a positive integer N is any maximal sequence of
  consecutive zeros that is surrounded by ones at both ends in the
  binary representation of N.

  For example, number 9 has binary representation 1001 and contains a
  binary gap of length 2. The number 529 has binary representation
  1000010001 and contains two binary gaps: one of length 4 and one of
  length 3. The number 20 has binary representation 10100 and contains
  one binary gap of length 1. The number 15 has binary representation
  1111 and has no binary gaps. The number 32 has binary representation
  100000 and has no binary gaps.

  Write a function:

  def solution(N)

  that, given a positive integer N, returns the length of its longest
  binary gap. The function should return 0 if N doesn't contain a binary
  gap.

  For example, given N = 1041 the function should return 5, because N
  has binary representation 10000010001 and so its longest binary gap is
  of length 5. Given N = 32 the function should return 0, because N has
  binary representation '100000' and thus no binary gaps.

  Write an efficient algorithm for the following assumptions:

  N is an integer within the range [1..2,147,483,647].

Solution:

def solution(N):

    bit_array = [int(bit) for bit in '{0:08b}'.format(N)]
    indices = [bit for bit, x in enumerate(bit_array) if x == 1]

    if len(indices) < 2:
        return 0

    lengths = [end - beg for beg, end in zip(indices, indices[1:])]

    return max(lengths) - 1

Converting from string to int in your first comprehension isn't needed as you're comparing to a literal anyway.
You can merge the first two comprehensions.
you don't need to return early, as you can default max to 1.
You shouldn't need the 8 in your format, as it shouldn't matter if there's padding or not, as it'll be filtered anyway.
From Python 3.6 you can use an f-string.

def solution(N):
    indices = [bit for bit, x in enumerate(f'{N:0b}') if x == '1']
    lengths = (end - beg for beg, end in zip(indices, indices[1:]))
    return max(lengths, default=1) - 1


PHP echo GET to change a CSS theme file I have the follow code to change between different CSS theme files, I would like some reviews on it because I am sure it can be improved to be safer.

 <head>
    <?php
    $theme = $_GET['theme'];
    $choice = preg_replace('/[^A-Za-z0-9\-]/', '', $theme);
    if (empty($choice)){
echo '<link rel"stylesheet" type="text/css" href="css/default.css" />';}
    elseif (file_exists("./css/" . $choice . ".css"))
{echo '<link rel"stylesheet" type="text/css" href="css/'.$theme.'.css" />';}
    else {echo '<link rel"stylesheet" type="text/css" href="css/default.css" />';}
    exit();
    ?>
    <!-- other elements in head -->
    </head>

The functions are:

If GET string empty set CSS default file
If GET non existing CSS file set CSS default file
If GET existing CSS file them we use the echo GET.css file

Security

It's a little odd that you clean the file for the file_exists check, but not for the echoing.

So I can inject test"><script>alert(1);</script>, and if the file testscriptalert1script exists the alert is executed. This isn't an attack (well, at least as long as users can't upload their own custom theme), but still seems odd.

Up to version 5.xx of PHP, it was possible to inject a null byte to bypass file_exists, but that should be caught by your filter, so it shouldn't enable the attack described above, or any other attacks (such as injecting validFile\0" href="evil" foo=" and hoping there are browsers which choose the second href attribute if two are present).

So I didn't find any vulnerabilities, but I still don't like your approach all that much. At the very least I would echo the replaced file name instead of the original. I would actually prefer the approach suggested by @76484 (hardcode and use a switch or use an array and then check if the filename exists in it), because using whitelists is always safer than using blacklists.

tl;dr: I couldn't find a vulnerability, but would still use a different approach.

Misc

your indentations, newlines and spacing seem rather random, which makes your code a bit hard to read.
if you exit in the head, your website will be quite boring.
it should be rel="stylesheet".
choice isn't a good variable name. Something like cssFileName would be much clearer.
you repeat the inclusion of the default CSS file. Either extract that to a function, or just remove the empty check (assuming that .css doesn't exist).
but then you still have the inclusion code for CSS in general three times.

So your code would be cleaner like this (but still using a blacklist instead of a whitelist for security):

<?php
$cssFileName = $_GET['theme'];
$cssFileName = preg_replace('/[^A-Za-z0-9\-]/', '', $cssFileName);

if (empty($cssFileName) || !file_exists("./css/" . $cssFileName . ".css")) {
    $cssFileName = "default";
}

echo '<link rel"stylesheet" type="text/css" href="css/' . $cssFileName . '.css" />';


WMSAuth implementation I have to implement WMSAuth in C#: http://wmsauth.org/examples

// The following function was copied from http://wmsauth.org/examples
public static string BuildProtectedURLWithValidity(string password, string media_url, string ip, int valid)
{
    string result = null;
    DateTime cur_date = DateTime.Now;
    TimeZone localzone = TimeZone.CurrentTimeZone;

    DateTime localTime = localzone.ToUniversalTime(cur_date);

    string date_time = localTime.ToString(new CultureInfo("en-us"));

    Int32 Valid = valid;
    string to_be_hashed = ip + password + date_time + Valid.ToString();

    byte[] to_be_hashed_byte_array = new byte[to_be_hashed.Length];

    int i = 0;
    foreach (char cur_char in to_be_hashed)
    {
        to_be_hashed_byte_array[i++] = (byte)cur_char;
    }

    byte[] hash = (new MD5CryptoServiceProvider()).ComputeHash(to_be_hashed_byte_array);

    string md5_signature = Convert.ToBase64String(hash);

    result = media_url + "?server_time=" + date_time + "&hash_value=" + md5_signature + "&validminutes=" + Valid.ToString();
    return (result);
}

The problem is that, although I believe the code above might work for every possible input, it seems to be done without much knowledge of C#, and I have a big concern with the Encoding in the byte to char casting.

So I changed the code to this:

private static string BuildProtectedURLWithValidity(string wmsAuthPassword, string mediaUrl, string clientIp, int validMinutes)
{
    string dateTimeString = DateTime.UtcNow.ToString(new CultureInfo("en-us"));
    string validMinutesString = validMinutes.ToString();

    string stringToBeHashed = clientIp + wmsAuthPassword + dateTimeString + validMinutesString;
    string hashValue;
    using (MD5CryptoServiceProvider md5Provider = new MD5CryptoServiceProvider())
    {
        byte[] bytesToBeHashed = Encoding.UTF8.GetBytes(stringToBeHashed);
        byte[] hashBytes = md5Provider.ComputeHash(bytesToBeHashed);
        hashValue = Convert.ToBase64String(hashBytes);
    }

    string result = mediaUrl + "?server_time=" + dateTimeString + "&hash_value=" + hashValue + "&validminutes=" + validMinutesString;
    return result;
}

But my real concern is that with my attempt to follow the company's naming standards and improve the code readability I might be introducing a bug in already tested code. Would you use the version posted in the the WMSAuth page?

Note: It has to work with the .NET Framework 1.1

Well, the problem is that the example code provided effectively does a % 256 on the input characters. So it's possible that your version and the example code yield two different values for the same input.

However you said I believe the code above might work for every possible input - I take it then that password will never contain any characters where the wrap-around will play a role? If that's the case then your version should be equivalent.

If you are paranoid then replace Encoding.UTF8.GetBytes in your code with the same loop as used in the example but leave your other refactorings intact.

In fact I would refactor the result into:

string.Format("{0}?server_time={1}&hash_value={2}&validminutes={3}", mediaUrl, dateTimeString, hashValue, validMinutes);

Which means you could also get rid of the validMinutesString.


Simple backtracking code Is this good or can it be improved? The program should backtrack the two variables until this logic operation is true: a & b.

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

bool perm(bool, bool);

int main(){

    bool x = false;
    bool y = false;

/*  RECURSIVE SOLUTION  */

    if(perm(x,y) == true){
        puts("DONE\n");
    }

    puts("\n\n");

/* ITERATIVE SOLUTION*/

    for(int cnt = 0; cnt < 2; cnt++){
        x = cnt;
        for(int cnt_2 = 0; cnt_2 < 2; cnt_2++){
            y = cnt_2;
            if( (x & y) == true){
                printf("%d ^ %d =  TRUE\n", x,y);
                puts("DONE");
                return 0;
            }
            printf("%d ^ %d =  FALSE\n", x,y);
        }
    }

    puts("NOTHING");

    return 0;
}

bool perm(bool x, bool y){

    const bool c_x = x;
    const bool c_y = y;

    printf("%d ^ %d  = %d\n", c_x, c_y, c_x&c_y);

    if(x == true){ return x;}
    if(y == true){ return y;}

    perm(y+1,x);

    return ( (x & y) == true) ? true : perm(x+1,y);
}

I find this code rather odd. I think there are a number of problems you can work out, which I'll outline below.

Functions

You put the recursive version into a function called perm() which is a good idea. I would do the same thing with the iterative version. This way your main() function becomes more clear. I'd rename the recursive one to recursive_perm() and make the new one iterative_perm(). Then your main() becomes:

int main(){

    bool x = false;
    bool y = false;

    if(recursive_perm(x,y) == true){
        puts("DONE\n");
    }

    puts("\n\n");

    x = false;
    y = false;
    if(iterative_perm(x,y) == true) {
        puts("DONE\n");
    }

    puts("NOTHING");

    return 0;
}

Now you don't need the comments saying "RECURSIVE SOLUTION" and "ITERATIVE SOLUTION" because it's obvious from the names.

Fix Your Recursion

You recursive version doesn't do the same thing as your iterative version. In fact, it does a lot of really weird things. Why are you copying the input variables into const members only to print them out? printf() takes its arguments by value, so printing them won't change their values locally.

Also, why are you printing logical XOR when you're performing logical AND? The ^ operator is XOR in C-based languages. Your print statement tells the user that the value of x exclusive-or-ed with y is equal to the value of x logically anded with y.

Next, why are you calling perm(y+1,x); before the return statement? You don't do anything with its return value, so none of the work it does is at all useful. You throw out the results (though it does have the side effect of printing out some stuff).

What do these incorrect lines accomplish?

if(x == true){ return x;}
if(y == true){ return y;}

They appear to check if only 1 of the values is true and then return true, ignoring the other parameter. Therefore it is not actually calculating the permutations that you want it to calculate.

Doing addition on bool values is not easy to read. I would not recommend doing that in the future. You can easily flip a boolean value from true to false by prepending it with the ! operator. Either that or change the function to take int instead of bool as you've done with the iterative version.

Testing bool

In general, it's considered better form to directly test a bool variable in an if statement rather than comparing it to true or false by doing this:

bool x = true;
bool y = false;
if (x) {
    // x was true, do something here
}
if (!y) {
    // y was false, do something here
}

What Does the Return Value Represent?

What does the return value of your perm() function represent? If all you want to do is iterate through all possible permutations, it's not clear to me that it needs to return a value. Reading this, I would assume that if it's functioning correctly, it iterated all the values, otherwise it wouldn't have returned yet. If you want to know which combination returned true, you should probably return a struct containing 2 values showing the final combination.


How could I have made this Powershell script that executes robocopy code better I am trying to get better at Powershell and was wondering if I could get some advice on how to make this code more efficient/cleaner/anything.

This code is intended to launch three batch windows running robocopy at the same time with unique log names.

I looked into using a foreach loop but wasn't sure how I could do that with so many unique variables. Thinking back, I probably could have imported each source, destination and log name into a CSV though but this may have been cleaner/better because it's all in one file instead.

# sets the files to exclude
$defaultDirExclusionSet = '"$Recycle.Bin" "System Volume Information"'

# setting the filename perameter so that we have some timestamps.
$filedate = "$(get-date -format 'yyyy-MM-dd-HH-MM')"  # we can remove the HH and MM later... have this on now for testing.

#options for robocopy
$RoboOptions = "/MIR /COPY:DATSOU /ZB /R:1 /W:5 /XD $defaultDirExclusionSet /NP /TEE"

#main fileshares
$source = "\\server1\D$\Shares"
$destination = "\\server2\D$"
$logname = "fileshare"

#Citrix User Store Shares
$source1 = "\\server1\D$\CitrixUserStore"
$destination1 = "\\server2\E$\CitrixUserStore"
$logname1 = "ctxusrshare"

#Citrix User Redir Shares
$source2 = "\\server1\D$\CitrixUserRedir"
$destination2 = "\\server2\E$\CitrixUserRedir"
$logname2 = "ctxusrredir"

##### launch all at once.  fix the first line before using.

Start-Process robocopy.exe -ArgumentList "$($source) $($destination) $($RoboOptions.split(' ')) /log+:c:\robocopylogs\$($logname)_$($filedate).txt"
Start-Process robocopy.exe -ArgumentList "$($source1) $($destination1) $($RoboOptions.split(' ')) /log+:c:\robocopylogs\$($logname1)_$($filedate).txt"
Start-Process robocopy.exe -ArgumentList "$($source2) $($destination2) $($RoboOptions.split(' ')) /log+:c:\robocopylogs\$($logname2)_$($filedate).txt"

If you don't want an external csv file,
use an internal one - simulated with a here string and ConvertFrom-Csv
in a date format specifier upper case MM are for month, minutes are lower case mm
the following script has several variable names shorted to keep line length down.
the final command is only echoed / commented out.

## Q:\Test\2019\08\23\sf_226703.ps1
# sets the files to exclude
$defaultDirExclusionSet = '"$Recycle.Bin" "System Volume Information"'

#options for robocopy
$RoboOpts = "/MIR /COPY:DATSOU /ZB /R:1 /W:5 /XD $defaultDirExclusionSet /NP /TEE"

# simulate external csv with a here string and ConvertFrom-Csv
$RCJobs = @'
desc,src,dst,log
"main fileshares","\\server1\D$\Shares","\\server2\D$","fileshare"
"Citrix User Store Shares","\\server1\D$\CitrixUserStore","\\server2\E$\CitrixUserStore","ctxusrshare"
"Citrix User Redir Shares","\\server1\D$\CitrixUserRedir","\\server2\E$\CitrixUserRedir","ctxusrredir"
'@ -split '\r?\n' | ConvertFrom-Csv

$RCJobs | Format-Table -Auto

##### launch all at once.  fix the first line before using.
foreach($J in $RCJobs){
    Write-Host -Fore Green "Starting job $($J.desc)"
    $Args = '"{0}" "{1}" {2} /log+:"C:\robocopylogs\{3}_{4:yyyy-MM-dd-HH-mm}.txt"' -f `
           $J.src,$J.dst,$RoboOpts,$J.log,(Get-Date)
    "Start-Process robocopy.exe -ArgumentList $Args"
    #Start-Process robocopy.exe -ArgumentList $Args
}

Sample output without executing RoboCopy.

> Q:\Test\2019\08\23\sf_226703.ps1

desc                     src                          dst                          log
----                     ---                          ---                          ---
main fileshares          \\server1\D$\Shares          \\server2\D$                 fileshare
Citrix User Store Shares \\server1\D$\CitrixUserStore \\server2\E$\CitrixUserStore ctxusrshare
Citrix User Redir Shares \\server1\D$\CitrixUserRedir \\server2\E$\CitrixUserRedir ctxusrredir

Starting job main fileshares
Start-Process robocopy.exe -ArgumentList "\\server1\D$\Shares" "\\server2\D$" /MIR /COPY:DATSOU /ZB /R:1 /W:5 /XD "$Recycle.Bin" "System Volume Information" /NP /TEE /log+:"C:\robocopylogs\fileshare_2019-08-23-20-59.txt"
Starting job Citrix User Store Shares
Start-Process robocopy.exe -ArgumentList "\\server1\D$\CitrixUserStore" "\\server2\E$\CitrixUserStore" /MIR /COPY:DATSOU /ZB /R:1 /W:5 /XD "$Recycle.Bin" "System Volume Information" /NP /TEE /log+:"C:\robocopylogs\ctxusrshare_2019-08-23-20-59.txt"
Starting job Citrix User Redir Shares
Start-Process robocopy.exe -ArgumentList "\\server1\D$\CitrixUserRedir" "\\server2\E$\CitrixUserRedir" /MIR /COPY:DATSOU /ZB /R:1 /W:5 /XD "$Recycle.Bin" "System Volume Information" /NP /TEE /log+:"C:\robocopylogs\ctxusrredir_2019-08-23-20-59.txt"


LLVM Pass that renames all functions Below is an LLVM pass that I've written to rename all functions in an LLVM bitcode file. I would appreciate tips on what is well done and what is poorly done in the code below. In particular, is there a better way to go about writing LLVM passes. Bear in mind that I'm a novice at programming and so would also appreciate it if you could direct me to any resources that would be useful in improving my C++ code.

#include "llvm/Pass.h"
#include "llvm/IR/Function.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/Analysis/TargetLibraryInfo.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/Module.h"
#include "llvm/IR/Type.h"
#include "llvm/IR/TypeFinder.h"
#include "llvm/Transforms/IPO.h"

using namespace llvm;

namespace {

  struct FunctionRenamer : public ModulePass {
    static char ID; // Pass identification
    FunctionRenamer() : ModulePass(ID) {
      initializeFunctionRenamerPass(*PassRegistry::getPassRegistry());
    }

    void getAnalysisUsage(AnalysisUsage &AU) const override {
      AU.addRequired<TargetLibraryInfoWrapperPass>();
      AU.setPreservesAll();
    }

    bool runOnModule(Module &M) override {
      // Rename all functions
      const TargetLibraryInfo &TLI =
        getAnalysis<TargetLibraryInforWrapperPass>().getTLI();
      for (auto &F : M) {
        StringRef Name = F.getName();
        LibFunc Tmp;
        // Leave library functions alone because their presence or absence
        // could affect the behaviour of other passes.
        if (Name.startswith("llvm.") || (!Name.empty() && Name[0] == 1) ||
            TLI.getLibFunc(F, Tmp))
          continue;

        F.setName(Name + "_2");
      }
      return true;
    }
  };
}

char FunctionRenamer::ID = 0;
INITIALIZE_PASS_BEGIN(FunctionRenamer, "functionrenamer",
                      "Rename all functions", false, false);
INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass);
INITIALIZE_PASS_END(FunctionRenamer, "functionrenamer",
                    "Rename all functions", false, false);
// ===-------------------------------------------------------==//
//
// Function Renamer - Renames all functions
//
ModulePass *llvm::createFunctionRenamerPass() {
  return new FunctionRenamer();
}

TargetLibraryInforWrapperPass seems to have one r too many.
Try using auto a bit more extensively. Almost always auto
Did you really mean '\1' or should that be '1' in the following line? Either way, use a character-literal to make it obvious it's not an error.

if (Name.startswith("llvm.") || (!Name.empty() && Name[0] == 1) ||
                                                             ^

The same applies to using namespace llvm; as to using namespace std;:
Don't do it. Read "Why is “using namespace std” considered bad practice?" for why.


Memory leak in Java's stack implementation Recently I was asked to implement a stack specifically for storing 'int' type data and find memory leaks. Isn't the garbage collector responsible for releasing all the memory? What are the possible memory leaks and how do I handle them?

import java.util.NoSuchElementException;
public class MyStack {

    private int stack[];
    private int top = -1;

    private MyStack() {

    }

    MyStack(int capacity) {
        stack = new int[capacity];
    }

    public void push(int data) {
        if(top == (stack.length-1))
            throw new IllegalStateException("Stack is full.");
        top++;
        stack[top] = data;
    }

    public int pop() {
        if(top == -1)
            throw new NoSuchElementException("Stack is empty.");
        int data = stack[top];
        top--;
        return data;
    }

    public int peek() {
        if(top == -1)
            throw new NoSuchElementException("Stack is empty.");
        return stack[top];
    }

    public int size() {
        return top + 1;
    }

    public boolean isEmpty() {
        if(top == -1)
            return true;
        return false;
    }

    public static void main(String args[]) {
        MyStack stack = new MyStack(5);
        stack.push(1);
        stack.push(2);
        stack.push(3);
        stack.push(4);
        stack.push(5);
        //stack.push(6);
        System.out.println("capacity::" + stack.size());
        System.out.println("top::" + stack.peek());
        System.out.println("pop::" + stack.pop());
        System.out.println("top::" + stack.peek());
        System.out.println("pop::" + stack.pop());
        System.out.println("pop::" + stack.pop());
        System.out.println("pop::" + stack.pop());
        System.out.println("pop::" + stack.pop());
        System.out.println("isEmpty::" + stack.isEmpty());
        //System.out.println(stack.pop());
    }
}

Memory leak

No, there's no memory leak in your program.
If you changed the underlying data type from int to an object type,
then you would have, loosely speaking, a memory leak in this method:

public int pop() {
    if(top == -1)
        throw new NoSuchElementException("Stack is empty.");
    int data = stack[top];
    top--;
    return data;
}

If stack was of type Object[], then the element you popped, would still be referenced. Even though it's no longer intended to be used, the popped element would stay referenced in there, until the array slot is overwritten. The remedy is simple, just null out the unused array slot:

public int pop() {
    if(top == -1)
        throw new NoSuchElementException("Stack is empty.");
    int data = stack[top];
    stack[top] = null;
    top--;
    return data;
}

This doesn't apply to your solution. There is nothing to "null out" (null is not a valid value for an int), there's nothing to be garbage collected.

And it would be a memory leak only loosely speaking. Due to the objects not garbage collected, you might experience reduced performance due to the increased memory footprint, and paging to disk (if you run out of memory).

Usability

You made the parameterless constructor private, and thus forbidden. At the minimum, you should add a comment to the constructor body to explain why it is forbidden.

And really, why is it forbidden? Users of this stack are forced to think about a suitable capacity, but very often there is no good way to anticipate the needed capacity, and then users will end up picking something arbitrary. Granted, anything that you pick will be arbitrary too, but at least you don't make your users think.

Supporting other types

Although your specs are asking to store int values, you could make your implementation work with any type, at negligible cost. So I urge you to do that. If the assignment requires a usage without type parameters, you could create a wrapper class that specializes on the desired type, for example:

class MyStack<T> {
    // ...
}

public class MyIntStack extends MyStack<Integer> {}

Another "cost" of course of the generalization is that you would have to watch out for memory leaks, and null out the popped element as explained earlier.


Function that convert decimal to binary I have a function that converts an integer to its binary representation. I am wondering if there is any way I can improve the function.

 public List<string> Conversion(int x)
    {
        var bitConversion = new List<string>();
        var result = x;
        while (result >= 0)
        {
            if (result == 0)
            {
                bitConversion.Add("0");
                break;
            }
            bitConversion.Add((result % 2).ToString(CultureInfo.InvariantCulture));
            result = result / 2;

        }
       bitConversion.Reverse();

       return bitConversion;
    }

One major improvement would be to just use what is supplied through .NET:

Convert.ToString(int, 2);

where int is your supplied argument.

Convert.ToString Method (Int32, Int32)

  Converts the value of a 32-bit signed integer to its equivalent string representation in a specified base.

Note that this returns you a string value like 10000110100001, which is the representation of a binary number. In your code you store them as string representations and in separate entries in your collection, which is not the way a number should be stored. It's like creating an array with values "1" and "2" to represent the number 12.

If this is your intention then you can always work towards that of course but it might be an indication that something else is wrong for needing it like that.

However if you want to stay with your own implementation, there are a few things you could change around:

public List<string> Conversion2(int x)
{
   var bitConversion = new List<string>();
   while (x >= 0)
   {
       if (x == 0)
       {
           bitConversion.Add("0");
           break;
       }
       bitConversion.Add((x % 2).ToString(CultureInfo.InvariantCulture));
       x /=  2;
   }
  bitConversion.Reverse();
  return bitConversion;
}

Remove the unnecessary result variable
Contract x = x / 2 to x /= 2 (compound assignment operator)


Comparing and assigning strings Would you please run through this code and provide your comments?

void quote_container::allocate(string *i_string, string *i_value){
    string temp_name = *i_string;
    double temp_value;

    // Convert the input string to upper case.
    stringToUpper(temp_name);

    if (temp_name =="OPEN"){
        temp_value = atof( (*i_value).c_str() );
        open = (float)temp_value;
    }
    else if (temp_name =="DAYHIGH") {
        temp_value = atof( (*i_value).c_str() );
        high  = (float)temp_value;
    }
    else if (temp_name =="DAYLOW") {
        temp_value = atof( (*i_value).c_str() );
        low  = (float)temp_value;
    }
    else if (temp_name =="LOW52") {
        temp_value = atof( (*i_value).c_str() );
        year_low  = (float)temp_value;
    }
    else if (temp_name =="HIGH52") {
        temp_value = atof( (*i_value).c_str() );
        year_high  = (float)temp_value;
    }
    else if (temp_name =="PRICEBANDLOWER") {
        temp_value = atof( (*i_value).c_str() );
        lower_price_band  = (float)temp_value;
    }
    else if (temp_name =="PRICEBANDUPPER") {
        temp_value = atof( (*i_value).c_str() );
        upper_price_band  = (float)temp_value;
    }
    else if (temp_name =="TOTALBUYQUANTITY"){
        temp_value = atof( (*i_value).c_str() );
        tbq  = temp_value;
    }
    else if (temp_name =="TOTALSELLQUANTITY") {
        temp_value = atof( (*i_value).c_str() );
        tsq  = temp_value;
    }
    else if (temp_name == "BUYQUANTITY5" || temp_name == "BUYQUANTITY4" || temp_name == "BUYQUANTITY3"||
        temp_name == "BUYQUANTITY2"|| temp_name == "BUYQUANTITY1") {
        temp_value = atof( (*i_value).c_str() );
        tbq2  = tbq2 + temp_value;
    }
    else if (temp_name == "SELLQUANTITY5" || temp_name == "SELLQUANTITY4" || temp_name == "SELLQUANTITY3"||
        temp_name == "SELLQUANTITY2" || temp_name == "SELLQUANTITY1")  {
            temp_value = atof( (*i_value).c_str() );
        tsq2  = (float)tsq2 + temp_value;
    }
    else if (temp_name == "BUYPRICE1" || temp_name == "BUYPRICE2" || temp_name == "BUYPRICE3"||
        temp_name == "BUYPRICE4" || temp_name == "BUYPRICE5") {
            temp_value = atof( (*i_value).c_str() );
        avg_buy_price  = avg_buy_price + (float)temp_value ;
    }
    else if (temp_name == "SELLPRICE5" || temp_name == "SELLPRICE4" || temp_name == "SELLPRICE3"||
        temp_name == "SELLPRICE2" || temp_name == "SELLPRICE1") {
            temp_value = atof( (*i_value).c_str() );
        avg_sell_price  =  tsq2 + (float)temp_value;
    }
    else if (temp_name == "FACEVALUE") {
        temp_value = atof( (*i_value).c_str() );
        face_value = (float)temp_value;
    }
    else if (temp_name == "PREVIOUSCLOSE") {
        temp_value = atof( (*i_value).c_str() );
        prev_close = (float)temp_value;
    }
    else if (temp_name == "LASTPRICE") {
        temp_value = atof( (*i_value).c_str() );
        last_price = (float)temp_value;
    }
    else if (temp_name == "TOTALTRADEDVOLUME") {
        temp_value = atof( (*i_value).c_str() );
        ttv = (float)temp_value;
    }
    else
        cout <<"\n\t Ignoring --"<<temp_name<<"---"<<*i_value;

}

Also

void quote_container::stringToUpper(string &s)
{
   for(unsigned int l = 0; l < s.length(); l++)
    s[l] = toupper(s[l]);
}

Could the above code be improved in any way?

Lets start with the simple one:

void quote_container::stringToUpper(string &s)
{
   for(unsigned int l = 0; l < s.length(); l++)
    s[l] = toupper(s[l]);
}

How about:

std::transform(s.begin(), e.end(), s.begin(), ::toupper);

Knowledge of the standard library is very useful.

Never pass anything as a pointer (this is C++ not C).

void quote_container::allocate(string *i_string, string *i_value){

I see no testing of i_string or i_value for NULL so they can't be NULL so there is no need to pass them as pointers. So you should be passing them as references. Since neither value is modified by the function you should also probably be passing them as const to prevent accidental modification of the original value.

void quote_container::allocate(string const& iString, string const& iValue){

Don't explicitly cast values.

open = (float)temp_value;
 //    ^^^^^^^

This is code smell. If you must cast (and usually you should not) then use one of the C++ cast operators (there should never be a need to use a C cast operator (as above)). But in this case it looks like it is not needed. If you don't cast the same conversion will happen anyway.

In this case

avg_buy_price  = avg_buy_price + (float)temp_value ;

You are getting worse results. Let the compiler to the cast for you. If you had let the compiler do the cast for you with:

avg_buy_price  = avg_buy_price + temp_value;

// Then the compiler would have generated this code
avg_buy_price  = static_cast<float>(static_cast<double>(avg_buy_price) + temp_value);

This way you would have retained a tiny bit more accuracy during the addition (which may have mattered). Before you cast it back to float.

As mentioned by "The Dr" (MrSmith) above factor out any common code. But I would do it differently. Put all your conditions in a map to make the code more readable. You actually have two types of assignment. The first is simple assignment while the second is accumulation. So we break these two types of action into their own maps

std::map<std::string, float*> assign= {
    {"OPEN",             &open},
    {"DAYHIGH",          &high},
    {"DAYLOW"            &low},
    {"LOW52",            &year_low},
    {"HIGH52",            &year_high},
    {"PRICEBANDLOWER",    &lower_price_band},
    {"PRICEBANDUPPER",    &upper_price_band},
    {"TOTALBUYQUANTITY",  &tbq},
    {"TOTALSELLQUANTITY", &tsq},
    {"FACEVALUE",         &face_value},
    {"PREVIOUSCLOSE",     &prev_close},
    {"LASTPRICE",         &last_price},
    {"TOTALTRADEDVOLUME", &ttv}
  };
std::map<std::string, float*> accum = {
  {"BUYQUANTITY5",   &tbq2},
  {"BUYQUANTITY4",   &tbq2},
  {"BUYQUANTITY3",   &tbq2},
  {"BUYQUANTITY2",   &tbq2},
  {"BUYQUANTITY1",   &tbq2},
  {"SELLQUANTITY5",  &tsq2},
  {"SELLQUANTITY4",  &tsq2},
  {"SELLQUANTITY3",  &tsq2},
  {"SELLQUANTITY2",  &tsq2},
  {"SELLQUANTITY1",  &tsq2},
  {"BUYPRICE1",      &avg_buy_price},
  {"BUYPRICE2",      &avg_buy_price},
  {"BUYPRICE3",      &avg_buy_price},
  {"BUYPRICE4",      &avg_buy_price},
  {"BUYPRICE5"       &avg_buy_price},
  {"SELLPRICE5",     avg_sell_price}, /*avg_sell_price  =  tsq2 + (float)temp_value;*/
  {"SELLPRICE4",     avg_sell_price}, /* I assumed this was a copy paste mistake you */
  {"SELLPRICE3",     avg_sell_price}, /* made. If it is not then this array is slightly */
  {"SELLPRICE2",     avg_sell_price}, /* More complex for SELL (but not much) */
  {"SELLPRICE1",     avg_sell_price}
 };

Now that we have all the data stored in maps the code is highly simplified.

 std::map<std::string,float*>::const_iterator find;
 find = assign.find(iString);
 if (find != assign.end())
 {
     (*find->second) = atof( iValue.c_str() );
 }
 else
 {
     find = accum.find(iString);
     if (find != accum.end())
     {
         (*find->second) += atof( iValue.c_str() );
     }
     else
     {
         std::cout << "\n\t Ignoring --" << iString << "---" << iValue;
     }
 }

You can compreses the two maps into a single map (and thus reduce the code here) by using a functor or class abstraction as described by (@utnapistim) in his answer. Personally I would go with using std::function rather than building my own class hierarchy (but that's a you say potato I say tomato argument and down to personal preference).


Project Euler #12: Highly divisible triangular number I want to receive advice on my code. It is too slow... The problem I am trying to solve is:

  What is the value of the first triangle number to have over five hundred divisors?

import math
import numpy as np
import copy
from operator import add
memoi_table = {}

#check whether the x is prime or not
#to do a prime factorization
def prime_finder(x):
    cnt = 0
    a = math.sqrt(x)
    sqrt_number = int(math.floor(a))
    for i in range(2,sqrt_number+1):
        if x % i == 0:
            cnt = cnt +1
            break
    if cnt == 0:
        return True
    else:
        return False

#Do a prime_factorization
#Do a dynamic programing with memoization table
#Do a recursive method to cover the triangular number
#ex)Triangular number = 1,3,6,10,15,21,28
#When we treat 10, we couldn't utilize dynamic programming => Recursive method
def prime_factorization(x):
    global memoi_table
    memoi_table[x] = {}

    if prime_finder(x) == True:
        memoi_table[x][x] = 1

    for i in range(2,math.floor(math.sqrt(x)+1)):
        if (prime_finder(i) == True) and (x % i == 0):
            new_x = x / i
            if not new_x in memoi_table.keys():
                prime_factorization(new_x)
                #Recursive way

            if new_x in memoi_table.keys():
                memoi_table[x] = memoi_table[new_x].copy()
                #to shallow copy
            if i in memoi_table[x].keys():
                memoi_table[x][i] = memoi_table[x][i] + 1
            else:
                memoi_table[x][i] = 1
    return (x,memoi_table[x])
#ex) 20 => return (20,(2:2,5:1))

# To calculate the number of divisor based on prime factorization
def find_number_of_divisor(x):
    global memoi_table
    number_of_divisor = 1
    for key in memoi_table[x].keys():
        number_of_divisor = number_of_divisor * (memoi_table[x][key]+1)
    return number_of_divisor

#Let's do it
def main():
    global memoi_table
    n = 0
    while True:
        n = n + 1
        Triangular_number = n * (n + 1) / 2
        prime_factorization(Triangular_number)
        num_divisor = find_number_of_divisor(Triangular_number)

        if num_divisor > 500:
            return (Triangular_number,num_divisor)
            break

print(main())

The Algorithm

Let's just look at your top level algorithm:

n = 0
while True:
    n = n + 1
    Triangular_number = n * (n + 1) / 2
    prime_factorization(Triangular_number)
    num_divisor = find_number_of_divisor(Triangular_number)

Factoring is hard. It's a difficult problem in the best of cases. You are taking a difficult problem and making it harder. You are starting with n * (n + 1) / 2 - that is already a partial factorization. You are then throwing out that information and factoring the whole number. It is much easier to factor 7 * 4 than it is to factor 28, but you are choosing to do the latter.

It would be much more efficient to do something like:

factor1 = prime_factorization(n/2 if n % 2 == 0 else n)
factor2 = prime_factorization(n+1 if n % 2 == 0 else (n+1)/2)
full_factorization = merge(factor1, factor2)

If you memoize prime_factorization, then you're only doing a little bit of work each iteration. Let's consider just going from the 49th triangular number to the 50th and assume we have a very good memoized prime factorization (more on this later). Factoring the full number:

(1275)
--> 3 * (425)
--> 3 * 5 * (85)
--> 3 * 5 * 5 * (17)

None of 1275, 425, or 85 were in the table. 17 was the first number we would have found. Consider the alternative, we are instead finding the prime factorization of:

(25)
--> memoized as 5*5 (already done when finding the 24th triangular number)
(51)
--> 3 * (17)

And 17 is memoized already (from the 16th triangular number). So we did one division. That time difference adds up. The full solution I present later in this answer takes just under 1.0s (when run 10x) if I factor each factor independently, but 4.46s when I factor the whole number. That is a lot slower.

And when you find yourself doing a counting loop, you should really use itertools.count() so that the main loop becomes:

for n in itertools.count(start=1):
    factors1 = prime_factorization(n/2 if n % 2 == 0 else n)
    factors2 = prime_factorization(n+1 if n % 2 == 0 else (n+1)/2)
    full_factorization = merge_factors(factors1, factors2)

    if num_factors(full_factorization) >= 500:
        return n * (n+1) / 2

Memoization

Now, you're kind of memoizing, but then not really, since the first thing you do is wipe memoi_table[x]. If should definitely start with:

def prime_factorization(x):
    if x in memoi_table:
        return memoi_table[x]

    ...

In any event, it's simpler to just write a decorator:

def memoize(f):
    cache = {}
    def wrapper(*args):
        if not args in cache:
            cache[args] = f(*args)
        return cache[args]
    return wrapper

@memoize
def prime_factorization(x):
    ...

Prime Finder

If you do your loop correctly, you don't actually need prime_finder(i). What does that function do? It looks over all the numbers smaller than i... but we're already doing that in prime_factorization! You're just doubling work for no reason. The whole body should just be something like:

for i in range(2,math.floor(math.sqrt(x)+1)):
    if x%i == 0:
        old_factorization = prime_factorization(x / i).copy()
        old_factorization[i] += 1
        return old_factorization

# still here? we didn't find a factor, must be prime
res = collections.defaultdict(int)
res[x] = 1
return res

That's it.

Putting it all together

All we need is merging two dictionaries, which is an easy loop:

def merge_factors(f1, f2):
    res = f1.copy()
    for k, v in f2.items():
        res[k] += v
    return res

And finding the number of factors, which is just a product:

def num_factors(factorization):
    product = 1
    for v in factorization.values():
        product *= (v+1)
    return product

The times based on finding the first triangular number with at least X factors:

      mine      OP
  5   0.0005s   0.0007s
 10   0.0009s   0.0041s
 25   0.0039s   0.0860s
100   0.0256s   6.0304s
500   0.9849s   ???????


K&R 1-23 Remove all C comments From K&R exercise 1-23,

  Write a program to remove all comments from a C program. Don't forget
  to handle quoted strings and character constants properly. C comments
  don't nest.

My solution,

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

#define MAXLEN  10000   // max input length

int      get_input(char input[], int maxlen);
char * no_comments(char input[], int len);

_Bool comment_start(char input[], int idx);
_Bool   comment_end(char input[], int idx, char *comments, int cdx);

_Bool quote_start(char input[], int idx);
_Bool   quote_end(char input[], int idx);

int main(void)
{
    char input[MAXLEN];
    int len = get_input(input, MAXLEN);
    char *scrubbed = no_comments(input, len);

    printf("%s", scrubbed);
    free(scrubbed);
}

int get_input(char input[], int maxlen)
{
    int i;
    char c;

    for (i = 0; i < maxlen-1 && (c = getchar()) != EOF; i++) {
        input[i] = c;
    }
    input[++i] = '\0';

    return i;
}

char * no_comments(char input[], int len)
{
    char *scrubbed = (char *) malloc((len+1) * sizeof(char));
    int sdx = 0;    // index

    char *comments = (char *) malloc((len+1) * sizeof(char));
    comments[0] = '\0';
    int cdx = 0;    // index

    _Bool quoted = 0;

    for (int idx = 0; idx <= len; idx++) {
        if (strlen(comments) == 0) { // outside comment

            if (!quoted && comment_start(input, idx)) {
                comments[cdx++] = input[idx];
            } else {
                scrubbed[sdx++] = input[idx];
            }

            if (quoted && quote_end(input, idx)) {
                quoted = 0;
            } else if (!quoted && quote_start(input, idx)) {
                quoted = 1;
            }

        } else { // inside comment

            if (comment_end(input, idx, comments, cdx)) {
                comments[0] = '\0'; // exit comment
                cdx = 0;
            } else {
                comments[cdx++] = input[idx];
            }
        }
    }
    free(comments);

    return scrubbed;
}

_Bool comment_start(char input[], int idx)
{
    char nxt = input[idx+1];

    return input[idx] == '/' && (nxt == '*' || nxt == '/');
}

_Bool comment_end(char input[], int idx, char *comments, int cdx)
{
    char prev = comments[cdx-1];
    char cur  = input[idx];
    char nxt  = input[idx+1];

    _Bool new_style = (cdx == 1 && cur == '/') || (cdx > 1 && comments[1] == '/');
    _Bool old_style = (cdx == 1 && cur == '*') || (cdx > 1 && comments[1] == '*');

    return (new_style && nxt == '\n') || (old_style && cdx > 2 && prev == '*' && cur == '/');
}

_Bool quote_start(char input[], int idx)
{
    return input[idx] == '"' && input[idx-1] != '\'';
}

_Bool quote_end(char input[], int idx)
{
    return input[idx] == '"' && input[idx-1] != '\\';
}

I recommend using Valgrind as the first step of the self-code review. Here it only reports one error when compiled with gcc on Linux:

 $ gcc -O2 main.c -o main -std=c99 -Wall -Wextra  -g
 $ valgrind  --track-origins=yes ./main
 (...)
 ==24100== Conditional jump or move depends on uninitialised value(s)
==24100==    at 0x4009B4: comment_start (main.c:85)
==24100==    by 0x4009B4: no_comments (main.c:54)
==24100==    by 0x40062E: main (main.c:21)
==24100==  Uninitialised value was created by a stack allocation
==24100==    at 0x400620: main (main.c:20)
==24100==
==24100== Conditional jump or move depends on uninitialised value(s)
==24100==    at 0x4009CA: quote_start (main.c:102)
==24100==    by 0x4009CA: no_comments (main.c:62)
==24100==    by 0x40062E: main (main.c:21)
==24100==  Uninitialised value was created by a stack allocation
==24100==    at 0x400620: main (main.c:20)
==24100==
==24100== Conditional jump or move depends on uninitialised value(s)
==24100==    at 0x518C1B2: vfprintf (in /lib64/libc-2.21.so)
==24100==    by 0x51928E8: printf (in /lib64/libc-2.21.so)
==24100==    by 0x400640: main (main.c:23)
==24100==  Uninitialised value was created by a stack allocation
==24100==    at 0x400620: main (main.c:20)
=

The problem lies inside get_input(), exactly in this line:

input[++i] = '\0';

Add printing value of i after each iteration and after the pre-incrementation. You will see something like this:

i == 0
i == 1
i == 2
i == 3
i == 4
i == 5
i == 6
i == 7
i == 8
i == 9
i == 11

Position 10 is not set. However, later on in no_comments() you have a loop:

for (int idx = 0; idx <= len; idx++) {
    if (strlen(comments) == 0) { // outside comment

        if (!quoted && comment_start(input, idx)) {

Input array and index are passed to comment_start() and read there:

return input[idx] == '/' && (nxt == '*' || nxt == '/');

At some point you will read an element of input array that has not been initialized. It's an undefined behavior in C and it's quite dangerous.


Swap a word in the DOM from an array of words I have question about my code. It should swap words from an array into an HTML Element and it does so like expected. However, I was wondering about the style of my code. I would like to get some feedback on how to improve the code.

Javascript

const words = [
    "Webdesigner",
    "Frontend Entwickler",
    "Webentwickler",
    "eine Flitzpiepe"
];

const interval = 2000;

function swapWords(words, interval){

    const swappingWord = document.getElementById("swappingWord");
    let i = 0;

    setInterval(function(){

        swappingWord.textContent = words[i].toUpperCase();
        i++;

        if(i == words.length){
            i = 0;
        }
    }, interval)
}
swapWords(words, interval)

HTML

WEBDESIGNER

There is no way to remove the behavior. This is usually bad design.
Since there is no way to remove behavior, swapWords() as a function is unneeded: you will never call it twice. It would only make sense defining it as a function if it could be called on different elements.
The initial state of the cycle is undefined and/or inconsistent: since the first swap only occurs after 2s, until then the initial content of the span will be displayed (WEBDESIGNER). Then it will be swapped with... WEBDESIGNER. Either leave the span empty and explicitly call a swap round on initialisation, or push the initial contents of the span on top of the stack of words. Otherwise it's not a cycle, it's a "weird start, then a cycle".
if(i == words.length){ - ok, you defined words as const so this is safe. But it's a bad habit to develop to write code like this: if words was variable between rounds, as it got trimmed from, say, 6 items to 4, and i would be 5, the condition would not be triggered anymore, and also not checked before access. So it would just go on to infinity. The responsible way to write code is always update and bound-check any state vars in a synchronous matter, which means BEFORE using them (i.e. before swappingWord.textContent = words[i].toUpperCase();), not at the end of the round for the next async round. Also always use >=words.length not ==words.length, as a coding habit. Always test for valid/invalid condition, not edge condition. In case you decide later to make words list variable, the == condition will automatically introduce a bug. >= won't. The remarks of this point don't qualify as bad code (it works as long as you don't change initial assumptions), it's just bad style. Get rid of it before you get used to it :)