Preventing SQL injection without using prepared statements I'm learning OOP and totally new to this way of coding. I've always scripted PHP the procedurial way. Now I've written a working class, which creates a database connection and has the method to create a query which is impossible to SQL-inject by hexing the non-integer data. (I know how to use prepared statements, but i just don't want to use them because there is a slight performance penalty in my case)

Note: When hexing data the right way, its not possible to inject.Have a look at Zaffy's answer.

My questions:

What could I've done better?
Is it 'wrong' to use this class because its not good OOP (besides its function), or does it not really matter, because it works as how i want it to work? (nobody else is going to maintain the script)

And I must say, I've read multiple tutorials, but it's really difficult to fully understand howto write OOP style if you've never done before!

databaseconnection.class.php

<?php
class DatabaseConnection {
    private $data, $sql, $conn, $host, $user, $pass, $dtbs;

    public function __construct($host, $user, $pass, $dtbs) {
        $this->host = $host;
        $this->user = $user;
        $this->pass = $pass;
        $this->dtbs = $dtbs;

        $this->conn = new mysqli($this->host, $this->user, $this->pass, $this->dtbs);
        if ($this->conn) {
            return $this->conn;
        } else {
            return false;
        }
    }

    public function query($sql, $data) {
        $this->data = $data;
        $this->sql = $sql;
        foreach ($data as $val) {
            if (strpos($this->sql, "'%i'") !== false || strpos($this->sql, "'%s'") !== false || strpos($this->sql, '"%i"') !== false || strpos($this->sql, '"%i"') !== false) {
                echo "SQL incorrect: There can't be any quotes around the parameters, because this function does that automaticly for you";
                exit();
            }
            $pos = strpos($this->sql, '%');
            $type = substr($this->sql, $pos, 2);
            if ($type == '%i') {
                if (is_int($val)) {
                    $this->sql = substr_replace($this->sql, $val, $pos, 2);
                } else {
                    echo "Parameter incorrect: Data is not an integer value";
                    exit();
                }
            } elseif ($type == '%s') {
                if (!is_int($val)) {
                    $this->sql = substr_replace($this->sql, "UNHEX('".bin2hex($val)."')", $pos, 2);
                } else {
                    echo "Parameter incorrect: Data is an integer value";
                    exit();
                }
            } else {
                echo "Parameter incorrect: Parameter %i for integer of %s for all other types of data";
                exit();
            }
        }

        $this->query = $this->conn->query($this->sql);
        return $this->query;

    }

    public function fetch($query) {
        return $query->fetch_assoc();
    }

    public function num_rows($query) {
        return $query->num_rows;
    }

}
?>

index.php

<?php
function __autoload($class){
  require('classes/' . strtolower($class) . '.class.php');
}
require('config.php');

$sql = new DatabaseConnection(DB_HOST, DB_USER, DB_PASS, DB_DTBS);
// The DB_xxxx constants are defined in config.php
?>
<!doctype html>

<meta charset="utf-8">
<title>OOP - Object Orientated Programming</title>

<?php
$data = array(1, "Brian o'Reilly");
$query = $sql->query("SELECT * FROM users WHERE id = %i AND name = %s", $data);
if ($sql->num_rows($query) > 0) {
    while ($r = $sql->fetch($query)) {
        echo '
User ' . $r['name'] . ' has ID ' . $r['id'] . '
';
    }
} else {
    echo "
No results
";
}
?>

never return from constructor
avoid having new statements in the constructor
on error, throw an exception instead of echo + exit
% is used in MySQL's LIKE syntax
you should refactor your DatabaseConnection::query() method - it's too high cyclomatic complexity
constants are global immutable state (it's not as bad as mutable globals, but still a bad idea)
you should be using spl_autoload_register instead of the archaic __autoload
you are mixing HTML with logic .. bad idea, try this approach instead
don't use ".class.php" in filenames, because then you will be screwed, when you start adding interfaces (which one would want to autoload too)


Is my code for a forum page vulnerable for sql injections? This is the backend code for the page where you upload a post. I have already tried to do a sql attack on this page, but it didn't work.

<?php
include_once 'includes1/dbh.inc.php';
session_start();
//*skåd ifall man är inloggad *//
if (isset($_POST['topic_submit'])) {
    $newFileName = "annons"; //Här ska egentligen vara $_POST['topic_title'];//
    if (empty($newFileName)) {
        $newFileName = "annons";
    } else {
        $newFileName = strtolower(str_replace(" ", "-", $newFileName));
    }
        $cid = $_POST['cid'];
        $title = $_POST['topic_title'];
        $content = $_POST['topic_content'];
        $creator = $_SESSION['UserName'];
        $imageNamn = $_POST['filenamn'];
        $imageMail = $_POST['filemail'];
        $imageNummer = $_POST['filenummer'];
        $imagePris = $_POST['filepris'];
        $imagestad = $_POST['filestad'];

    $file = $_FILES['file'];

    $fileName = $file["name"];
    $fileType = $file["type"];
    $fileTempName = $file["tmp_name"];
    $fileError = $file["error"];
    $fileSize = $file["size"];

    $fileExt = explode(".", $fileName);
    $fileActualExt = strtolower(end($fileExt));

    $allowed = array("jpg", "jpeg", "png", "image/jpg");

    if (in_array($fileActualExt, $allowed)) {
        if ($fileError === 0) {
          if ($fileSize < 2000000000) {
            $imageFullName = $newFileName . "." . uniqid("", true) . "." . $fileActualExt;
            $fileDestination = "img/gallery/" . $imageFullName;

            include_once 'includes1/dbh.inc.php';

            if (empty($title) || empty($content) || empty($imageNamn) || empty($imageMail) || empty($imagestad) || empty($imagePris)) {
                header("Location: ../index.php?upload=empty");
                exit();
            } else {
                $sql2 = "SELECT * FROM gallery;";
                $stmt = mysqli_stmt_init($conn);
                if (!mysqli_stmt_prepare($stmt, $sql2)) {
                    echo "SQL statement failed!";
                } else {
                    mysqli_stmt_execute($stmt);
                    $result2 = mysqli_stmt_get_result($stmt);
                    $rowCount = mysqli_num_rows($result2);

                    $sql = "INSERT INTO topics (category_id, topic_title, topic_creator, topic_date, topic_reply_date, imgFullNameGallery, topic_pris) VALUES ('".$cid."', '".$title."', '".$creator."', now(), now(), '".$imageFullName."', '".$imagePris."')";
                    $result = mysqli_query($conn, $sql);
                    $new_topic_id = mysqli_insert_id($conn);
                    $sql2 = "INSERT INTO gallery (category_id, topic_id, post_creator, titleGallery, descGallery, namnGallery, emailGallery, nummerGallery, prisGallery, stad, post_date, imgFullNameGallery)
                     VALUES ('".$cid."', '".$new_topic_id."', '".$creator."', '".$title."', '".$content."', '".$imageNamn."', '".$imageMail."', '".$imageNummer."', '".$imagePris."', '".$imagestad."', now(), '".$imageFullName."')";
                    $result2 = mysqli_query($conn, $sql2);
                    $sql3 = "UPDATE categories SET last_post_date=now(), last_user_posted='".$creator."' WHERE id='".$cid."' LIMIT 1";
                    $result3 = mysqli_query($conn, $sql3);
                    if (!mysqli_stmt_prepare($stmt, $sql2)) {
                        echo "SQL statement failed!";
                    } else {
                        mysqli_stmt_bind_param($stmt, "sssssssssss", $cid, $new_topic_id, $creator, $title, $content, $imageNamn, $imageMail, $imageNummer, $imagePris, $imagestad, $imageFullName);
                        move_uploaded_file($fileTempName, $fileDestination);

                        header("location: view_topic.php?cid=".$cid."&tid=".$new_topic_id);
                    }
                }
            }
        } else {
        echo "File size is too big!";
        exit();
        }
    } else {
    echo "You had an error!";
    exit();
    }
} else {
    echo "You need to upload a proper file type!";
    exit();
}
}
?>

Yes, it is.
For some reason you put everything upside-down: where you don't need a prepared statement (for a constant query) you are using it, but where a prepared statement is mandatory - for a query that uses variables - you don't.
But I would say, it is not the main problem of this code. The main problem is this code being a total mess. There are two blocks of code that do absolutely nothing useful, one of them even fails, but you don't notice it.
I would review only a database interaction part
1. Useless code
The following block of code does absolutely nothing
            $sql2 = "SELECT * FROM gallery;";
            $stmt = mysqli_stmt_init($conn);
            if (!mysqli_stmt_prepare($stmt, $sql2)) {
                echo "SQL statement failed!";
            } else {
                mysqli_stmt_execute($stmt);
                $result2 = mysqli_stmt_get_result($stmt);
                $rowCount = mysqli_num_rows($result2);

it takes you $rowCount which is nowhere used. You can safely take this code away.
Another block,
                if (!mysqli_stmt_prepare($stmt, $sql2)) {
                    echo "SQL statement failed!";
                } else {
                    mysqli_stmt_bind_param($stmt, "sssssssssss", $cid, $new_topic_id, $creator, $title, $content, $imageNamn, $imageMail, $imageNummer, $imagePris, $imagestad, $imageFullName);

is also doing nothing useful (as you are already executed this query), and even fails on the bind_param part, because of the flawed
2. Error reporting
There are two things you are doing wrong in regard of error reporting

using procedural interface that silently fails on errors
checking the function results manually

To fix these issues, you have to use OOP syntax and add a specific command to your mysqli connection code,
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

(just for reference, here is a canonical example for mysqli connect I wrote).
After that, all MySQL errors will pop up automatically, therefore you won't have to check every function's result manually.
3. SQL injection.
Finally to the SQL injection. The rules are simple:

if your query doesn't accept any variables, then you don't have to run it using a prepared statement. Use a regular query() instead.
if your query accepts any variables, then it must be executed using prepared statement

Now you can see that if SELECT * FROM gallery had any meaning, you would have run it using query(), not a prepared statement. But as it is just useless, we won't run this query at all.
All other queries must be run using prepared statements:
$sql = "INSERT INTO topics (category_id, topic_title, topic_creator, topic_date, topic_reply_date, imgFullNameGallery, topic_pris)
        VALUES (?,?,?,now(), now(),?,?)";
$stmt = $conn->prepare($sql);
$stmt->bind_param("sssss", $cid, $title, $creator, $imageFullName, $imagePris);
$stmt->execute();
$new_topic_id = mysqli_insert_id($conn);

$sql = "INSERT INTO gallery (category_id, topic_id, post_creator, titleGallery, descGallery, namnGallery, emailGallery, nummerGallery, prisGallery, stad, post_date, imgFullNameGallery)
         VALUES (?,?,?,?,?,?,?,?,?,?,?)";
$stmt = $conn->prepare($sql);
$stmt->bind_param("sssssssssss", $cid, $new_topic_id, $creator, $title, $content, $imageNamn, $imageMail, $imageNummer, $imagePris, $imagestad, $imageFullName);
$stmt->execute();

$sql = "UPDATE categories SET last_post_date=now(), last_user_posted=? WHERE id=? LIMIT 1";
$stmt = $conn->prepare($sql);
$stmt->bind_param("ss", $creator, $cid);
$stmt->execute();

move_uploaded_file($fileTempName, $fileDestination);
header("location: view_topic.php?cid=".$cid."&tid=".$new_topic_id);


Prime number generator in C I have to print numbers between two limits n and m, t times.
I created t variable that stores a number of test cases.
Outer for loop iterates for every test cases.
Inner for loop prints primes from m to n.
Code
#include <stdio.h>
#include <stdlib.h>

int is_prime(int);

int main(void) {
    int t, m, n;
    scanf("%d", &t);
    for (int i = 0; i < t; i++) {
        scanf("%d %d", &m, &n);
        for (int j = m; j <= n; j++) {
            if (is_prime(j)) {
                printf("%d\n", j);
            }
        }
        if (i < t - 1) printf("\n");
    }

    return 0;
}

int is_prime(int num) {
    if (num <= 1) return 0;
    for (int i = 2; i * i <= num; i++) {
        if (num % i == 0) {
            return 0;
        }
    }
    return 1;
}

Problem: http://www.spoj.com/problems/PRIME1/
Can I have review?

The only even prime number is two, so with a little extra coding you can ignore even numbers in the for loop.  For even more speed use a sieve, though maybe save that for when you get a 'TLE' failure.

A more formal code layout would be good as well, your if statements need expanding to include {...}.  More vertical space (blank lines) a well as explanatory comments help.  When you come back to your old code six months later you will thank yourself.

I prefer to set 0 and 1 as FALSE and TRUE to make code easier to read YMMV.

int is_prime(int num) {
  if (num <= 1) {
    return FALSE;
  }

  // Even numbers.
  if (num % 2 == 0) {
    return num == 2;  // 2 is the only even prime.
  }

  // Odd numbers.
  // Start loop at 3 and step 2 to skip even divisors.
  for (int i = 3; i * i <= num; i += 2) {
    if (num % i == 0) {
        return FALSE;
    }
  }
  return TRUE;
}


R Container for Multiple data.frame with a Brief Description of the Content of the data.frame For a project I have a large dataset with a multitude of variables from different questionnaires. Not all variables are required for all analyses.
So I created a preprocessing script, in which subsets of variables (with and without abbreviations) are created. However it gets confusing pretty fast.
For convenience I decided to create a index_list which holds all data.frames as well as a data.frame called index_df which holds the name of the respective data.frame as well as a brief description of each subversion of the dataset.
######################## Preparation / Loading #####################
# Clean Out Global Environment
rm(list=ls())

# Detach all unnecessary pacakges
pacman::p_unload()

# Load Required Libraries
pacman::p_load(dplyr, tidyr, gridExtra, conflicted)

# Load Data
#source("00_Preprocess.R")
#create simulation data instead
sub_data <- data.frame(x=c(2,3,5,1,6),y=c(20,30,10,302,5))
uv_newScale <- data.frame(item1=c(2,3,5,1,6),item2=c(3,5,1,3,2))

# Resolving conflicted Namepsaces
conflict_prefer("filter", "dplyr")

# Creating an Index
index_list <- list("sub_data"=sub_data,
                   "uv_newScale"=uv_newScale
                   )

index_df <- data.frame("Data.Frame"=c("sub_data",
                                        "uv_newScale"),
                       "Description"=c("Contains all sumscales + sociodemographics, names abbreviated",
                                       "Only sum scores for the UV Scale"))

I am wondering if there is a more efficient way to do so. Like saving the data.frames together with the description in one container?

One approach
If the description is to serve only as a metadata, R allows you to add any number of metadata to objects with attributes feature. you can add it as an attribute for the data.frame object.
attr(sub_data, "Description") <- "Contains all sumscales + sociodemographics, names abbreviated"

# Description now shows in the data frame structure
str(sub_data)
# 'data.frame': 5 obs. of  2 variables:
#  $ x: num  2 3 5 1 6
# $ y: num  20 30 10 302 5
# - attr(*, "Description")= chr "Contains all sumscales + sociodemographics, names # abbreviated"

#You can access the Description only
attributes(sub_data)$Description
# [1] "Contains all sumscales + sociodemographics, names abbreviated"

However, custom attributes come with limitations.They are not persistent when you perform certain operations on objects such as subsetting. Here is an example using same object with the new Description attribute we just added. If we subset the data, the custom attribute will be lost.
sub2_data <- sub_data[,"x", drop = FALSE]

attributes(sub2_data)$Description
# NULL

Alternative approach
You can use the same idea of creating a container within container. However, instead of creating a list that contains data frames, you can create a data.frame within a data.frame. This makes it easier to access and manipulate. You can access the inner data frame by adding second $
# Assigning new column `data` to hold data frames
index_df$data <- index_list

# We can access Description
index_df$Description
# [1] "Contains all sumscales + sociodemographics, names abbreviated"
# [2] "Only sum scores for the UV Scale"

# Accessing Data
index_df$data$sub_data
#  x   y
#1 2  20
#2 3  30
#3 5  10
#4 1 302
#5 6   5

str(index_df)
# 'data.frame': 2 obs. of  3 variables:
#  $ Data.Frame : chr  "sub_data" "uv_newScale"
# $ Description: chr  "Contains all sumscales + sociodemographics, names abbreviated" "Only sum scores for the UV Scale"
# $ data       :List of 2
#  ..$ sub_data   :'data.frame':    5 obs. of  2 variables:
#  .. ..$ x: num  2 3 5 1 6
#  .. ..$ y: num  20 30 10 302 5
#  ..$ uv_newScale:'data.frame':    5 obs. of  2 variables:
#  .. ..$ item1: num  2 3 5 1 6
#  .. ..$ item2: num  3 5 1 3 2

Efficiency
The first approach is more efficient than the second in terms of memory footprint. Here is a comparison between objects sizes in bytes.
# Creating one data frame within one data frame for comparison
list_df <- list(sub_data=sub_data)
dfs <- data.frame("Data.Frame"="sub_data",
             "Description"="Contains all sumscales + sociodemographics, names abbreviated")

dfs$data <- list_df

# Adding attributes
attr(sub_data, "Description") <- "Contains all sumscales + sociodemographics, names abbreviated"

# Memory Size in bytes
object.size(dfs)
# 2376 bytes
object.size(sub_data)
# 1224 bytes


Writing a generic performance efficient isEmpty method which can check for null or emptiness I am writing a utility method which can check for empty and null string, or collection or an object or any general types -

public static boolean isEmpty(Object obj) {
    if (obj == null)
        return true;
    if (obj instanceof Collection)
        return ((Collection<?>) obj).size() == 0;

    // is below line expensive?
    final String s = String.valueOf(obj).trim();

    return s.length() == 0 || s.equalsIgnoreCase("null");
}

How can I make my above method efficient, since above isEmpty method will be called multiple times from the application which is very performance critical?

I am suspecting below line will be expensive because of heavy toString methods and it will create temporary garbage as well that might cause GC and slow down the performance?

final String s = String.valueOf(obj).trim();

If I need to check for map null or empty, should I keep both collection isEmpty and Map isEmpty method both or Collection isEmpty method will be fine for that?

public static void main(String[] args) {

    Map<String, String> hello = new HashMap<String, String>();
    System.out.println(isEmpty(hello));

    Map<String, HashMap<Integer, String>> primary = new HashMap<String, HashMap<Integer, String>>();
    System.out.println(isEmpty(primary));

}

public static boolean isEmpty(Collection<?> value) {
    return value == null || value.isEmpty();
}

public static boolean isEmpty(Map<?, ?> value) {
    return value == null || value.isEmpty();
}

Method overloading can make your implementations more efficient and cleaner:

public static boolean isEmpty(Collection obj) {
    return obj == null || obj.isEmpty();
}

public static boolean isEmpty(String string) {
    return string == null || string.trim().isEmpty();
}

public static boolean isEmpty(Object obj) {
    return obj == null || obj.toString().trim().isEmpty();
}

The Collection version is as efficient as possible.

The String version would be more efficient without the trimming. It would be best to trim your strings as soon you see them, long before they reach this call. If you can review the callers and make sure that the strings are always trimmed at their origins, then you can remove .trim() for best performance.

The Object version can be inefficient, depending on the toString implementation of the objects that will be passed to it, and because of the trimming.

I removed the comparison with null from there, because it seems pointless to me. I mean, a class whose toString method says "null" would seem very very odd.

In any case, you don't really want the Object version to be called, at all. Most importantly because it probably won't even work. Take for example an empty Map. Its toString method returns the string {}, which won't match your conditions of emptiness. (For this type you should definitely add isEmpty(Map<?, ?> map) to benefit from its isEmpty method.)

If performance is so critical, then add more overloaded implementations for all other types that you care about, for example:

public static boolean isEmpty(Something obj) {
    return obj == null || obj.isEmpty();
}

Finally, especially when something is so important, you definitely want to unit test it, for example:

@Test
public void testEmptyObject() {
    assertTrue(isEmpty((Object) null));
    assertFalse(isEmpty(new Object()));
}

@Test
public void testEmptyString() {
    assertFalse(isEmpty("hello"));
    assertTrue(isEmpty(""));
    assertTrue(isEmpty(" "));
    assertTrue(isEmpty((Object) null));
}

@Test
public void testEmptySet() {
    assertFalse(isEmpty(new HashSet<String>(Arrays.asList("hello"))));
    assertTrue(isEmpty(new HashSet<String>()));
}

@Test
public void testEmptyMap() {
    Map<String, String> map = new HashMap<String, String>();
    assertTrue(isEmpty(map));
    map.put("hello", "hi");
    assertFalse(isEmpty(map));
}


Mysql left joins last row I am making a query in mysql to import users in our marketing system and need for this a lot of data like for example :

Date of last order
Date of last cheque created
...

This are all left joins where I just want the last result with conditions. For the moment I do just a select for this field but I don't know if this is the best option. Here you have the query that I'm currently usign:

select u.first_name, u.last_name, u.email, u.birthdate, u.id, u.sex, c.name, u.facebook_id, u.created_at, u.last_active, u.referrer_id,
IF(wants_email = 0, (select created_at from wants_email_logs where value = 0 and u.id = user_id order by id desc limit 1), NULL ) as DATEUNJOIN,
(select max(o.created_at) from orders o where o.user_id = u.id and o.state in ('paid','processing', 'shipped', 'completed')) as DATE_LAST_ORDER,
(select max(c.created_at) from cheques c where c.user_id = u.id and c.spent = 0) as cheque_date from users u
left join countries c on u.country_id = c.id ;

Can you use temp tables?  I think that's generally the best idea... using a lot of subqueries can get nasty real quick.  Here's my version:

(I'm not able to test it so use it at your own risk.  Also if you use temp tables, you'll still probably want to define your datatypes in the CREATE TABLE sections)

DROP TABLE IF EXISTS temp_date_unjoin;

CREATE TABLE temp_date_unjoin
SELECT
    user_id,
    MAX(created_at) AS date_unjoin
FROM wants_email_logs
WHERE value = 0
GROUP BY user_id;

DROP TABLE IF EXISTS temp_date_last_order;

CREATE TABLE temp_date_last_order
SELECT
    user_id,
    MAX(created_at) AS date_last_order
FROM orders o
WHERE state IN ('paid', 'processing', 'shipped', 'completed')
GROUP BY user_id;

DROP TABLE IF EXISTS temp_cheque_date;

CREATE TABLE temp_cheque_date
SELECT
    user_id,
    MAX(created_at) AS cheque_date
FROM cheques
GROUP BY user_id;

SELECT
    u.first_name,
    u.last_name,
    u.email,
    u.birthdate,
    u.id,
    u.sex,
    c.name,
    u.facebook_id,
    u.created_at,
    u.last_active,
    u.referrer_id,
    du.date_unjoin,
    lo.date_last_order,
    cd.cheque_date
FROM
    users u
    LEFT JOIN temp_date_unjoin du
        ON du.user_id = u.id
    LEFT JOIN temp_date_last_order lo
        ON lo.user_id = u.id
    LEFT JOIN temp_cheque_date cd
        ON cd.user_id = u.id
    LEFT JOIN countries c
        ON u.country_id = c.id
;


Get network interfaces on Linux I wrote this in order to get a list of network interfaces on Linux. In the past, I have used publicly available methods from, say, ActiveState, which uses ioctl and was annoying to get working under Python 3. This version simply uses regular expressions on the output of ifconfig.

The documentation style used here follows the numpy documentation guide.

#!/usr/bin/env python
# -*- coding: utf-8 -*-
"""
Get a list of network interfaces on Linux.

This code is compatible with Python versions 2 and 3.

"""
from collections import namedtuple
import re
import subprocess

def get_interfaces(external=False, ip=False):
    """
    Get a list of network interfaces on Linux.

    To access the MAC address and/or the IP address, set the relevant keyword
    arguments to True.

    Parameters
    ----------
    external : bool, optional
        Only show external interfaces, and ignore virtual (e.g. loopback)
        devices, and return their MAC addresses.
    ip : bool, optional
        Only show interfaces which are UP and have an IP address, and return
        their IPv4 addresses.

    Returns
    -------
    interfaces
        list of str containing the interface name by default, or list of
        namedtuple containing `name`, `mac`, and `ip` as requested.

    Examples
    --------
    >>> print(get_interfaces())
    ['eth0', 'lo', 'wlan0']
    >>> print(get_interfaces(external=True))
    [Interface(name='eth0', mac='a0:b1:c2:d3:e4:f5'), Interface(name='wlan0', ma
    c='f5:e4:d3:c2:b1:a0')]
    >>> print(get_interfaces(ip=True))
    [Interface(name='lo', ip='127.0.0.1'), Interface(name='wlan0', ip='192.168.1
    1.2')]
    >>> print(get_interfaces(external=True, ip=True))
    [Interface(name='wlan0', mac='f5:e4:d3:c2:b1:a0', ip='192.168.11.2')]

    """
    name_pattern = "^(\w+)\s"
    mac_pattern = ".*?HWaddr[ ]([0-9A-Fa-f:]{17})" if external else ""
    ip_pattern = ".*?\n\s+inet[ ]addr:((?:\d+\.){3}\d+)" if ip else ""
    pattern = re.compile("".join((name_pattern, mac_pattern, ip_pattern)),
                         flags=re.MULTILINE)

    ifconfig = subprocess.check_output("ifconfig").decode()
    interfaces = pattern.findall(ifconfig)
    if external or ip:
        Interface = namedtuple("Interface", "name {mac} {ip}".format(
            mac="mac" if external else "",
            ip="ip" if ip else ""))
        return [Interface(*interface) for interface in interfaces]
    else:
        return interfaces

if __name__ == "__main__":
    interfaces = get_interfaces(external=True, ip=True)
    for interface in interfaces:
        print("{name}: {ip}".format(name=interface.name, ip=interface.ip))

That is a huge docstring, it's longer than the code itself. You should read Python's PEP about how to write good docstrings.

For a start a docstring shouldn't contain examples, you can put that as a  regular comment in the script if you want. And you don't need to be so detailed and explicit about the type and optional nature of your parameters. I would take some information out of your parameters and returns sections and just fold it back into your opening section. Docstrings should be relatively concise and mostly add context and explanation.

A user can easily see the default parameters get_interfaces(external=False, ip=False), which reveals that they're optional and booleans. All you need to explain is what setting them will affect.


Reading Data from XML and storing them into SQL Database I am developing a C# Windows forms application. In that, I am reading data from XML which is coming from a web API and storing that XML data into SQL database. I am able to achieve this. But my only concern is, In Production environment, this application will be in service and there will a lot of data to synchronize from XML to SQL Database. So, can anyone review my code and tell me whether it is an efficient way or not. Any sugggestions are welcome.

Because it's the first time I am working on the API and even in coding, I am a fresher.

I have done all to achieve my requirement. I would like a code review from experts.

public void save_vendor_info()
    {
    SqlConnection con = new SqlConnection(@"server=localhost;Database=TEST;integrated security=true");
    con.Open();
    DataTable dt = new DataTable();
    XmlDocument doc = new XmlDocument();
    doc.Load("https://testdata/api/vendordata");

    try
    {
        XmlNode node = doc.DocumentElement.ChildNodes.Cast<XmlNode>().ToList()[0];
        foreach (XmlNode column in node.ChildNodes)
        {
            dt.Columns.Add(column.Name, typeof(String));
        }

        XmlNode Filas = doc.DocumentElement;
        foreach (XmlNode Fila in Filas.ChildNodes)
        {
            List<string> Valores = Fila.ChildNodes.Cast<XmlNode>().ToList().Select(x => x.InnerText).ToList();

           SqlCommand cmd = new SqlCommand(("IF NOT EXISTS (Select Vendorcode From Vendors where Vendorcode = @Vendorcode) INSERT INTO Vendors VALUES (@Vendorcode, @STCD3, @Name1, @Name2, @Name3, @street1, @street2, @street3, @city1, @city2, @city3, @state1, @state2, @state3, @zip1, @zip2, @zip3, @countrycode, @BussinessUnitId, @VATID, @nationalVATID, @IBAN, @BankAccount, @BankCode)"), con);
            cmd.CommandType = CommandType.Text;
            cmd.Parameters.AddWithValue("@Vendorcode", Valores[0]);
            cmd.Parameters.AddWithValue("@STCD3", Valores[1]);
            cmd.Parameters.AddWithValue("@Name1", Valores[2]);
            cmd.Parameters.AddWithValue("@Name2", Valores[3]);
            cmd.Parameters.AddWithValue("@Name3", Valores[4]);
            cmd.Parameters.AddWithValue("@street1", Valores[5]);
            cmd.Parameters.AddWithValue("@street2", Valores[6]);
            cmd.Parameters.AddWithValue("@street3", Valores[7]);
            cmd.Parameters.AddWithValue("@city1", Valores[8]);
            cmd.Parameters.AddWithValue("@city2", Valores[9]);
            cmd.Parameters.AddWithValue("@city3", Valores[10]);
            cmd.Parameters.AddWithValue("@state1", Valores[11]);
            cmd.Parameters.AddWithValue("@state2", Valores[12]);
            cmd.Parameters.AddWithValue("@state3", Valores[13]);
            cmd.Parameters.AddWithValue("@zip1", Valores[14]);
            cmd.Parameters.AddWithValue("@zip2", Valores[15]);
            cmd.Parameters.AddWithValue("@zip3", Valores[16]);
            cmd.Parameters.AddWithValue("@countrycode", Valores[17]);
            cmd.Parameters.AddWithValue("@BussinessUnitId", Valores[18]);
            cmd.Parameters.AddWithValue("@VATID", Valores[19]);
            cmd.Parameters.AddWithValue("@nationalVATID", Valores[20]);
            cmd.Parameters.AddWithValue("@IBAN", Valores[21]);
            cmd.Parameters.AddWithValue("@BankAccount", Valores[22]);
            cmd.Parameters.AddWithValue("@BankCode", Valores[23]);

            cmd.ExecuteNonQuery();
        }
    }
    catch (Exception ex)
    {

    }
}

Review

Single responsibility principle
Method save_vendor_info (which we'll rename later) does data retrieval from an external source, xml to db mapping, and SQL command execution on a database. I would expect a method with the name Save* to only store data, in this case in the database. I would also suggest to split up loading the input data and saving to the database in separate methods. A mediator method could then call both methods and ensures the data is mapped correctly.

Resource management
You don't dispose of resources that you create. SqlConnection and SqlCommand implement the IDisposable interface. You should make sure to dispose these instances (SQL Disposable Pattern Example). You already open the connection before retrieving the web data. This is too soon. Keep the scope of a connection lifetime to a minimum.

Exception Handling
Definining an empty catch block is really bad practice. You don't know whether something went wrong. At least, log exceptions. Think about which specific exceptions you want to handle and which ones need to be propagated up the stack. Even in sand-box mode, where you don't want exceptions to reach the caller, you should return a boolean to the caller whether the call succeeded.

Naming & Style Conventions

Try to avoid underscores in method names and use PascalCase. Prefer SaveVenderInfo over save_vendor_info. As indicated earlier on, I would use this method name only for the part where you save the data to the database.
Use var when the type is inferred and don't use abbreviations for variable names: Prefer var dataTable = new DataTable(); over DataTable dt = new DataTable();.
Read SQL connection string information from a config file rather than hardcoding it. This gives better maintainability for different environments.
Be consistent in placement of curly braces. Either append an opening curly brace after the method with a white space between, or place the opening curly brace on the next line at the same indentation of the member declaration. Don't indent a curly brace on the next line, this leads to deeply indented code.


Finding users with the highest Following vs. Friends ratio I am trying to find the users who have the highest "Following vs. Friends" ratio in my Twitter timeline.

I have a SQLite database of the users in my Twitter timeline and each column contains data found in the user section of the tweet JSON.

Is there a better way that I could write this search query?

Why does it return zero when the user has a higher friends count than followers count?

Is there a way to shorten this search query?

SELECT CAST(followers_count AS DECIMAL)/ CAST(friends_count AS DECIMAL), screen_name FROM following
ORDER BY CAST(
    CAST(followers_count AS DECIMAL)/ CAST(friends_count AS DECIMAL)
AS INT)
DESC LIMIT 25;

Your code is not working well, and the SQL statement is not formatted nicely either.

Reformatting your code in to logical sections produces.

SELECT CAST(followers_count AS DECIMAL)/ CAST(friends_count AS DECIMAL),
       screen_name
FROM following
ORDER BY CAST(CAST(followers_count AS DECIMAL)/ CAST(friends_count AS DECIMAL) AS INT) DESC
LIMIT 25;

SQLite is notorious for doing calculations based on the wrong affinity of the data values.

I immediately suspected that the SQL was doing integer division on the values even though you are casting them to decimal.

I put some examples together as an SQLFiddle here, and I cast the values to float instead, then used the SQLite round() function to get things decent. Doing floating-point division is what solves the zero-value division before.

This is the SQL I ended up with:

SELECT ROUND(CAST(followers_count AS float) / CAST(friends_count AS float), 2) as ratio,
       screen_name
FROM following
ORDER BY CAST(followers_count AS float) / CAST(friends_count AS float) DESC
LIMIT 25;


Toggling the background color of a page using JavaScript closures This question is to see if I can get some input on the "design pattern" I tried to implement here. I'm just learning about closure in JavaScript and I think I'm starting to get it. I'm wondering if the way I wrote this code is or is not terrible stylistically.

The code in question is attempting to answer a challenge given in the JavaScript: The Hard Parts (by Will Sentance) on Frontend Masters. Based on the challenge, it seems I went above and beyond as the solution provided, when run, doesn't work properly.

The challenge:

  Write a function changeColor that when invoked will first check if the
  current page background color is "rgb(221, 238, 255)". If it is, it
  changes the color to "rgb(255, 238, 221)". If it isn't, it sets the
  color to "rgb(221, 238, 255)".

  Add a click event listener to button #1 above (it has an id of "activate"). On click, the button should log to the console "clicked
  1". It should also set up a click event listener on button #2 (id of
  "color"). That listener should log to console "clicked 2" and then
  call the changeColor function you just created.

  Clear the console and hit the 'Run with JS' button. Look at what code has run by analyzing the console. Then try to change the
  background color by clicking button #2. What needs to happen for the
  button to work?

Based on the wording of the challenge it seemed like the solution that was hinted at was (in pseudo-ish code):

// Provided solution to the question above
// Assume all necessary HTML is in place

activationButton.addEventListener("click", () => {
  console.log("clicked activation button")
  colorChangerButton.addEventListener("click", () => {
    console.log("clicked color changer button")
    changeTheColor()
  })
})

The obvious problem with the code above is that a new event listener will be added to the colorChangerButton every time you click the activationButton. This was where I started thinking about trying to use a closure to keep track of a state variable so that I could make sure that the event listener only gets added to the second button once. Also, this is the solution provided in the solution set for the challenge.

To that end, this is what I came up with (please assume that the correct HTML is there, etc):

(function () {

  let clickCount = 0

  function changeColor() {
    if (document.body.style.backgroundColor === "rgb(221, 238, 255)") {
      document.body.style.backgroundColor = "rgb(255, 238, 221)"
    } else {
      document.body.style.backgroundColor = "rgb(221, 238, 255)"
    }
  }

  function activateButton2() {
    const btn2 = document.querySelector('#color') // Appropriate to grab button here?
    btn2.addEventListener("click", () => {
      console.log("clicked #2")
      changeColor()
    })
  }

  const btn1 = document.querySelector('#activate') // Appropriate to have variable declaration here?
  btn1.addEventListener("click", () => {
    clickCount++
    if (clickCount === 1) {
      activateButton2()
    }
    console.log("clicked #1")
    return
  })
  // Initially had an un-necessary return statement here
})()

The code above - that I wrote - is working how I'd expect it to - the color change functionality and the console logs are firing as I would expect.

One question I had about my implementation was about containing the button variables inside the IIFE. I feel like I see a lot of front-end scripts putting all the query selectors at the top of the script - is there a reason to do that vs. including them within a function like this? If I understand closure correctly, once the event handlers are created there is a persistent reference to everything in the IIFE, so I can fire this function and the buttons and their handlers persist and behave as expected.

It also seems like I could probably use a boolean to track the "clicked" status of the first button, and a switch statement for checking the background color of the page.

Any other pointers anyone wants to offer?

This is a partial answer, that only addresses style, but I prefer something like this for the changeColor function.

I'm not thrilled with activateColorButton, but it works.

This is much more readable and maintainable to me:

const changeColor = e => {
    const lightOrange = "rgb(221, 238, 255)"
    const lightBlue = "rgb(255, 238, 221)"
    const {style} = document.body
    style.backgroundColor = style.backgroundColor === lightOrange
        ? lightBlue
        : lightOrange
}

const activateColorButton = e => {
    const btnColor = document.querySelector('#color');
    btnColor.removeEventListener('click', changeColor);
    btnColor.addEventListener('click', changeColor);
}

Some Explanation:

TLDR

Use good variable names
Avoid duplication
Use whitespace effectively
Prepare assignments for future abstraction

/TLDR

In changeColor, I have eliminated as much duplication as possible by assigning variables with names that are deliberately chosen to help the code to describe what it does without additional comments.

Moving the variable names to the top of the function allows them to be extended, changed, or even abstracted out to another function or data structure entirely.

The repeated reference to document.body.style.backgroundColor takes up half of a line of code, is very distracting, hard to read, hard to reason about, and unnecessary. style.backgroundColor could not be reduced to simply backgroundColor. This was as far down as it would go without losing functionality.

I personally find the compactness of the tertiary if statement to be more readable than the explicit if statement and especially prefer it when either assigning or returning one of 2 choices.

I renamed activateButton2 to activateColorButton b/c it more clearly expresses its purpose.

In activateColorButton, I chose to remove the click handler if it's already there and reassign it, rather than have confusing 'flag' variables lying around. I'm not thrilled with it b/c, ideally, it would check to see if the click handler was already assigned via the DOM directly, but I was too lazy to implement that here, which seems ok in this case.


Checking whether given array is sorted by divide-and-conquer I've written a code that I try to use divide and conquer approach to determine if the given array is sorted. I wonder whether I apply the approach accurately.

public static boolean isSorted(List<Integer> arr, int start, int end) {
    if (end - start == 1) // base case to compare two elements
        return arr.get(end) > arr.get(start);

    int middle = (end + start) >>> 1; // division by two
    boolean leftPart = isSorted(arr, start, middle);
    boolean rightPart = isSorted(arr, middle, end);
    return leftPart && rightPart;
}

To use call,

isSorted(list, 0, list.size() - 1)

It looks like your are doing it mostly right.   You have problems with length zero and length 1 arrays, but you should be able to fix those pretty quick.

You may be doing more work than necessary.  If an array is not sorted, you might find leftPart is false, but you unconditionally go on to determine the value of rightPart anyway, despite it not mattering.  The simplest way to avoid that is to combine that recursive calls and the && operation.  Ie:

return isSorted(arr, start, middle) && isSorted(arr, middle, end);

Lastly, if the array contains duplicates, can it still be considered sorted?  You return false for [1, 2, 2, 3].


Translate strings from an XML attribute into corresponding constants in my module I have a non-trivial list comprehension in Python.  My goal is to take a whitespace-delimited list of strings from an XML attribute — which may not exist — and look up the corresponding constant in my module — which may not exist — and create a set of the results.

What's the accepted way to format such a beast?  They all make me want to vomit a little.

    self.types = frozenset(
        v for v in (
            getattr(objects_module,enum_name,None)
            for enum_name in element.attrib.get('object-types', '').split())
        if v)

    self.types = frozenset(
        v
        for v in (getattr(objects_module,enum_name,None)
            for enum_name in element.attrib.get('object-types', '').split())
           if v)

    self.types = frozenset(
        v for v in (
          getattr(objects_module,enum_name,None)
          for enum_name in element.attrib.get('object-types', '').split()
        ) if v
    )

    self.types = frozenset(v for v in (getattr(objects_module,enum_name,None) for enum_name in element.attrib.get('object-types', '').split()) if v)

Well, that's more or less of a personal preference. I'd just adapt any of those to fit my existing code.

For the sake of example, I'd also consider the below example:

self.types = frozenset(v for v in (getattr(objects_module, enum_name, None) for enum_name in
                                   element.attrib.get('object-types', '').split()) if v)

That's just how PyCharm suggested it. But as I said, is more or less a mater of preference. If you want, from what you showed, I'd go with the first one.

You might also consider the following:

split_element = element.attrib.get('object-types', '').split()

self.types = frozenset(v for v in (getattr(objects_module, enum_name, None) for enum_name in split_element) if v)

Which is exactly as your last solution. I've just created an extra variable which will hold the split() part. Now that that respects the PEP8 requirement about the length of a line, I like it.


Scala graph coloring i have solved this problem https://leetcode.com/problems/find-eventual-safe-states/submissions/

import scala.collection.mutable

object Solution {
  def eventualSafeNodes(graph: Array[Array[Int]]): List[Int] = {
    val seen = mutable.HashSet[Int]()
    val safeNodes = mutable.ArrayBuffer[Int]()

    val  color_of = mutable.HashMap[Int, Int]().withDefaultValue(0)

    def is_safe(node: Int): Boolean = {
      color_of(node) = 1
      for (neb <- graph(node)) {
        if (color_of(neb) == 1) {
          return false;
        } else if (color_of(neb) == 0) {
          if (!is_safe(neb)) {
            return false;
          }
        }
      }
      color_of(node) = 2
      return true
    }
    for (i <- 0 until graph.size) {
      if (color_of(i) == 2) {
        safeNodes.append(i)
      } else if (color_of(i) == 0) {
        if (is_safe(i)) {
          safeNodes.append(i)
        }
      }
    }
    return safeNodes.toList
  }
}

I would love some pointers on idiomatic code!

A few observations.

comments - These online code challenges don't tend to encourage code comments because, really, who's ever going to see it? After completing the challenge you will probably never return to it. Still, if it's going to be offered for code review you might pretty it up to make things easier for the reviewers.

unused - val seen is never referenced.

magic numbers - So 2 is "safe" and 1 is "unsafe" and ... This calls out for an enumeration. Then you could if (color_of(i) == Color.SAFE), and .withDefaultValue(Color.UNKNOWN), etc.

0 until graph.size - Use graph.indices instead.

return - Idiomatic Scala almost never uses return. In your case it's understandable since there are a couple of places where you want the iterations to terminate ASAP. Scala has a few different options for achieving that. One of the most common is using recursion.

if..else - In Scala the if..else construct is an expression, with a return value, and not a statement. For this, and other reasons, idiomatic Scala seldom invokes an if without a concluding else.

mutables - Perhaps the most obvious anti-Scala practice in the code is the use of mutable data structures. This isn't surprising since there is often a slight performance penalty when adhering to FP principles of immutability. Many of these online code challenges are much more difficult, if not impossible, without mutation.

So, can this be solved without mutation? I don't know. I came pretty close by using only the mutability inherent in the received Array.

def eventualSafeNodes(graph: Array[Array[Int]]): List[Int] = {

  def isSafe(xs :Array[Int], seen :Set[Int]) :Boolean =
    xs.isEmpty || xs.forall(x => !seen(x) && isSafe(graph(x), seen+x))

  graph.indices.foldRight(List.empty[Int]){ case (x, acc) =>
    if (isSafe(graph(x), Set(x))) {
      graph(x) = Array.empty[Int]  //leave marker - safe
      x :: acc                     //save index
    } else {
      graph(x) = Array(x)          //leave marker - unsafe
      acc                          //accumulator unchanged
    }
  }
}


Calculate expiration date based on server data This code yet is not really optimize and I know that . But for now this is all I have.

What this code does is that it computes the expiration date of an account.

By the way I need to do the swapping workaround because the date that the server gives to me are inverted so I need to do a lil workaround about that.

    /// <summary>
    /// Add revalidation text and how many days the agents account will expire
    /// </summary>
    public async void RevalidateAccount()
    {
        //Get Stored Persistent Data of Date
        string _expDateFromPersistent = await SecureStorage.GetAsync("ExpirationDate");

        //Check if empty or not
        //because if it is empty we need to hide the validation text first
        //then show it after relogging in
        if (!string.IsNullOrEmpty(_expDateFromPersistent))
        {
            IsVisibleValidationText = true;
            //Convert string date to DateTime
            DateTime expDate = Convert.ToDateTime(_expDateFromPersistent);
            DateTime todayDate = DateTime.Now;
            //Change Format
            string _dateToday = todayDate.ToString("yyyy/MM/dd", CultureInfo.InvariantCulture);

            //Swap workaround
            string day = expDate.Day.ToString();
            string month = expDate.Month.ToString();
            string year = expDate.Year.ToString();
            string _expirationDate = year + "/" + day + "/" + month;
            //Calculation
            TimeSpan expirationDate = DateTime.Parse(_expirationDate) - DateTime.Parse(_dateToday);
            //Business Rule: 7 days prior to 30 days show the revalidation text
            if (expirationDate.Days <= 7)
            {
                if (IsVisibleValidationText)
                {
                    //Formatted Text for validation text
                    Revalidate = new FormattedString();
                    Revalidate.Spans.Add(new Span { Text = "You have ", ForegroundColor = Color.FromHex(_textColor), FontSize = 15 });
                    Revalidate.Spans.Add(new Span { Text = $"{expirationDate.Days} days", ForegroundColor = Color.FromHex(_textColor), FontSize = 15, FontAttributes = FontAttributes.Bold });
                    Revalidate.Spans.Add(new Span { Text = " left before you need \n to re-validate your login credentials.", ForegroundColor = Color.FromHex(_textColor), FontSize = 15 });
                    await _pageDialogService.DisplayAlertAsync("NOTICE", "You have " + expirationDate.Days + " days left before you need to re-validate your login credentials.", "OK");
                }
            }
        }
        else
        {
            IsVisibleValidationText = false;
        }
    }

What my code does is that it gets the data(Expiration Date) from the server then I just need to differ the DateTime.Now and the date(expiration date) from the server to have an expiration date.

General naming guidelines suggest that an async method should end its name with "Async", so the method name should be RevalidateAccountAsync.

Some would argue that a underscore prefix should never be used for naming of any variables.  For those that do allow underscore prefixes, it should only be for class-level variables and never used for variables local to a given method.

When comparing or manipulating 2 DateTime objects, the proper way to do that is to be sure that both have the same Kind.  Oddly enough, your code does that since you jump through so many hoops to create new DateTime objects with Kind of Unspecified.

expirationDate is a misleading name since it is not a date.  It is a TimeSpan.

After checking expirationDate.Days <= 7, there is no need to check IsVisibleValidateText since it will always be true at that point.

Your code looks to be twice as long as it needs to be because of all the manipulation of DateTime and string.  For example, to get today's date should only be 1 line:

DateTime todayDate = DateTime.Today; \\Kind is Local

expDate could use DateTime.Parse or Convert.ToDateTime.  However, you should also set it's Kind because as it is it will be Unspecified unless the string contains any time zone offset.  Example setting it to Local:

DateTime expDate = DateTime.SpecifyKind(Convert.ToDateTime(_expDateFromPersistent), DateTimeKind.Local);

The alternative would be to keep expDate as Unspecified but then make todayDate also Unspecified.

DateTime todayDate = DateTime.SpecifyKind(DateTime.Today, DateTimeKind.Unspecified);

Since it looks like you are interested in dates starting a midnight, skip the slower string manipulation and instead use the DateTime.Date property.  See help at this link.  Example:

TimeSpan expirationDate = expDate.Date - todayDate;

Which gets rid of all the slower and unneeded string variables, both dates above have the same Kind, and both are set to midnight.


python httplib snippet I've written this snippet to extract a review request status from review-board in python. It works well but i'm very new to python and i would like to know if i could write this any better.
Should i put conn.close in it's own try statement ?

This script is supposed to run as part of a post-commit hook in mercurial.
If the review-board server is not responding i still want to be able to commit without any issues. If the commit is non existent then an empty string is ok.

import httplib
import json

def getReviewStatus(reviewboardUrl, id):
    try:
        conn = httplib.HTTPConnection(reviewboardUrl, 80)
        headers = {
            'Accept': 'application/json',
            'Content-Type': 'application/json; charset=UTF-8'
        }
        conn.request("GET", "/api/review-requests/%s/" % id, headers=headers)
        return json.loads(conn.getresponse().read())['review_request']['status']
    except:
        return ""
    finally:
        conn.close()

Looks pretty good to me. Regarding conn.close(), definitely don't put this in its own try statement, because if there's a problem with the earlier code, the connection might not get closed. I know you're not using Python 3, but ss an alternative, you might consider using the with statement and let Python handle the closing for you. See http://docs.python.org/2/tutorial/inputoutput.html#methods-of-file-objects (end of section) and http://docs.python.org/2/library/contextlib.html#contextlib.closing

Your code might look like this:

import httplib
import json
from contextlib import closing

def getReviewStatus(reviewboardUrl, id):
    try:
        with closing(httplib.HTTPConnection(reviewboardUrl, 80)) as conn:
            headers = {
                'Accept': 'application/json',
                'Content-Type': 'application/json; charset=UTF-8'
            }
            conn.request("GET", "/api/review-requests/%s/" % id, headers=headers)
            return json.loads(conn.getresponse().read())['review_request']['status']
    except:
        return ""

I find it's good to get into the habit of using with to open resources so don't have to worry about forgetting to close the connection.


Generic method for checking database integrity on soft delete I have written this generic method to check whether a object has a dependent items or not:

public void ItemDependant<TEntity>(Guid id,
    Expression<Func<TEntity, bool>> condition,
    Expression<Func<TEntity, string>> select) where TEntity : BaseModel
{
    var foundItems = _dbContext.Set<TEntity>().Where(x => x.IsDeleted == false).Where(condition);
    var isAny = foundItems.Any();

    if (isAny)
        throw new Exception($"You must first delete these dependent items: {foundItems.Select(select).ToList().Aggregate((i, j) => i + ", " + j)}");
}

Now in each delete method in our services we call this method this way:

public override void Delete(Guid id)
{
    // This entity has only one dependent item (ItemB)
    // It means that this entity is being used by ItemB
    // So we can't delete it from the database
    // We should first delete ItemB
    ItemDependant<ItemB>(id, m => m.ItemAId == id, s => s.Title);

    base.Delete(id);
}

It works like a charm But I'm wondering if there's a better way for doing this.

Update: There's also another problem with this approach, If the entity has several dependent entities, consumer the API must know about them and add them in order:

public override void Delete(Guid id)
{
    ItemDependant<ItemB>(id, m => m.ItemAId == id, s => s.Title);
    ItemDependant<ItemC>(id, m => m.ItemAId == id, s => s.Title);
    ItemDependant<ItemD>(id, m => m.ItemAId == id, s => s.Title);
    ItemDependant<ItemE>(id, m => m.ItemAId == id, s => s.Title);
    ItemDependant<ItemF>(id, m => m.ItemAId == id, s => s.Title);

    base.Delete(id);
}

One thing that comes in my mind is that, I should have only one ItemDependent(.... call, then this method must inspects all of the dependencies then at the end gives the user several error messages. I think this solution is better, Isn't it?

I don't think this is the route you should travel. As an assumption of me, you will, if any dependent Items are found, show a message somehow which shows all the dependent items in a messagebox/label so the user knows which items should/need to be deleted first.

IMO this isn't really userfriendly because the user will need to delete each item individually and will have to remember the items to be deleted. If one of the subitems has dependent items as well another message will be shown which results in another delete marathon...and so on.

Maybe it would be better, if a to be deleted item has subitems to provide a possibility to delete all items at all by showing some kind of form where the user can decide if all items should be deleted or not.

You should add a method which returns the dependent items, if there aren't Any(), go on and delete the desired item. If ther are Any(), return them to the user asking if all of these should deleted as well.

Now comming to review the code in question

Omitting braces {} although they might be optional is a dangerous decision. This can lead to bugs which are hard to find. I would like to encouracge you to always use them.
usually I tend to use well named variables myself, but if it is obvious I wouldn't bother with them, resulting in less lines of code. What I mean here is the isAny. Having isAny won't give you much/any benefit in this small method. I would just write

public void ItemDependant<TEntity>(Guid id,
    Expression<Func<TEntity, bool>> condition,
    Expression<Func<TEntity, string>> select) where TEntity : BaseModel
{
    var foundItems = _dbContext.Set<TEntity>().Where(x => x.IsDeleted == false).Where(condition);

    if (foundItems.Any())
    {
        throw new Exception($"You must first delete these dependent items: {foundItems.Select(select).ToList().Aggregate((i, j) => i + ", " + j)}");
    }
}

but I would use a variable to store the result of foundItems.Select(select).ToList().Aggregate((i, j) => i + ", " + j)} because it will increase the readability (IMO). Using C# 6 features is a good thing but you could do it to much.
By looking at the condition parameter at the call to ItemDependant() inside the Delete() method I wonder why you didn't place the x => x.IsDeleted == false in there as well like so

ItemDependant<ItemB>(id, m => !m.IsDeleted && m.ItemAId == id, s => s.Title);

resulting in ItemDependant looking like so

public void ItemDependant<TEntity>(Guid id,
    Expression<Func<TEntity, bool>> condition,
    Expression<Func<TEntity, string>> select) where TEntity : BaseModel
{
    var foundItems = _dbContext.Set<TEntity>().Where(condition);

    if (foundItems.Any())
    {
        throw new Exception($"You must first delete these dependent items: {foundItems.Select(select).ToList().Aggregate((i, j) => i + ", " + j)}");
    }
}


Importing someone's Twitter feed I used some Java code to work out how often MPs and Senators cited their sources in tweets.  The results are at here and the GitHub for the whole project is here, and the Java code I use to download the MPs statements from Twitter is below. I'm looking for any feedback on style, possible errors, and anything else you can think of. I'm intending to be running this code quite regularly and I'd like to check I've NOT left something silly in...

import java.util.Map;

import twitter4j.Paging;
import twitter4j.RateLimitStatus;
import twitter4j.ResponseList;
import twitter4j.Status;
import twitter4j.Twitter;
import twitter4j.TwitterException;
import twitter4j.User;

public class ProcessMp {

    public static Twitter twitter;

    public static void main(String[] args) throws TwitterException, InterruptedException {
        twitter = TwitApplicationFactory.getjoereddingtonTwitter();
        String username = args[0];
        System.out.println(username);
        User user = twitter.showUser(username.replace("@", ""));
        // ht: from http://www.devmanuals.com/tutorials/java/gettingUserId.html
        checkUser(user);
    }

    private static void checkUser(User user) throws TwitterException, InterruptedException {
        int withNumberAndRef = 0;
        int tweets = 0;
        int withNumber = 0;

        for (int i = 1; i < 10; i++) {
            ResponseList<Status> temp = twitter.getUserTimeline(user.getId(), new Paging(i, 100));
            for (Status status : temp) {
                if (status.getText().startsWith("RT")) {
                    continue;
                }
                System.out.println(status.getText());
                tweets++;
                if (status.getText().matches(".* \\d+,*\\d* .*")) {
                    withNumber++;
                    if (status.getText().matches(".*http.*")) {
                        withNumberAndRef++;
                    }
                }
            }
            waitUntilICanMakeAnotherCall();
        }

        System.out.print("XX," + user.getScreenName() + ",");
        System.out.print(user.getName() + ",");
        System.out.print(tweets + ",");
        System.out.print(withNumber + ",");
        System.out.println(withNumberAndRef);

    }

    protected static void waitUntilICanMakeAnotherCall() throws TwitterException, InterruptedException {
        {
            Map<String, RateLimitStatus> temp = twitter.getRateLimitStatus();
            RateLimitStatus temp2 = temp.get("/statuses/retweets/:id");
            if (temp2.getRemaining() == 0) {
                Thread.sleep((temp2.getSecondsUntilReset() + 5) * 1000);
                return;
            }
            int secondstosleep = 1 + temp2.getSecondsUntilReset() / temp2.getRemaining();
            Thread.sleep(secondstosleep * 1000);
        }
    }

}

Main is just a driver program. Separate it from the implementation class.
If the called function throws an exception , someone has to handle it. Check who should do it and log some helpful message.
Variable name temp , temp2.
Could we guarantee that user!=null or temp!=null or it may not contain a null status i.e check for user!=null , temp!=null and status!=null.
No hard-coded value.
Make the twit extraction implementation generic for any pageSize, any user, i.e take that as input parameters or member variable and feed on it.
Since the Twit extraction API may be called by anyone, so have the username correction logic there itself.
Limit the printlns for debugging purpose only.


Ternary search tree implementation in python 3 I have implemented a Ternary search tree. It works well. But if you think something needs to be improved, say it. This code was tested in Python 3.7.4.

class Node(object):

    def __init__(self, character):
        self.character = character
        self.leftNode = None
        self.middleNode = None
        self.rightNode = None
        self.value = 0

class TST(object):

    def __init__(self):
        self.rootNode = None

    def put(self,key,value):
        self.rootNode = self.putItem(self.rootNode, key, value, 0)

    def putItem(self, node, key, value, index):

        c = key[index]

        if node == None:
            node = Node(c)

        if c < node.character:
            node.leftNode = self.putItem(node.leftNode, key, value, index)
        elif c > node.character:
            node.rightNode = self.putItem(node.rightNode, key, value, index)
        elif index < len(key) - 1:
            node.middleNode = self.putItem(node.middleNode, key, value, index+1)
        else:
            node.value = value

        return node

    def get(self, key):

        node = self.getItem(self.rootNode, key, 0)

        if node == None:
            return -1

        return node.value

    def getItem(self, node, key, index):

        if node == None:
            return None

        c = key[index]

        if c < node.character:
            return self.getItem(node.leftNode, key, index)
        elif c > node.character:
            return self.getItem(node.rightNode, key, index)
        elif index < len(key) - 1:
            return self.getItem(node.middleNode, key, index+1)
        else:
            return node

if __name__ == "__main__":

    tst = TST()

    tst.put("apple",100)
    tst.put("orange",200)

    print( tst.get("orange") )

Data structure-wise, it seems okay, and does what you're after.

Code-style wise, I see a few things:

It's not quite PEP8, the coding style. Whilst you can read it, which is always good, there are tools to automatize the process, such as flake8, black. Most likely, your editor will have a plugin.

This is include things like underscore_names in lieu of camelCasing.

Note that this standard is a living document, and gets updated once in a while.
None is a singleton in Python, and thus you can use the is keyword instead.
Python relies on its id() function to get a hash of an object, and compare them.
As None is a singleton, the hash is the same.

So your line if node == None: could be if node is None.
Going further this way, None evaluates to False in a test, so you could go if not node: (edit: as discussed with @Gloweye, it's indeed clearer and preferred to use the first variant).
Python returns None when no value is being explicitly returned, whether you have no return in a function, or do a blank return.

These two lines taken together could then be:

if node is None:
    return

and would be considered idiomatic.

Lastly, in Python3 (which you ought to be using), class definition have been revamped. The way you do it:

class TST(object):

is now:

class TST:

If you were inheriting from something, though, it would indeed be:

class TST(ParentClass):

Maybe what you could try next is to implement unit tests with unittest or pytest and see whether your structure holds on with more convoluted cases.


Counting slides shift in javascript object Adding Dynamic Slides to a slider used by a client shifts its index, need to account for that so i wrote this simple code, but i think it's pretty lame, is there any way to improve it and accomplish the same output?

var slides = {
  1: {
    content: "",
    group: 1
  },
  2: {
    group: 1
  },
  3: {
    content: "",
    group: 1
  },
  4: {
    content: "",
    group: 2
  },
  5: {
    content: "",
    group: 3
  },
  6: {
    content: "",
    group: 3
  },
  7: {
    content: "",
    group: 3
  },
  8: {
    content: "",
    group: 4
  },
  9: {
    content: "",
    group: 4
  },
  10: {
    content: "",
    group: 4
  },
  11: {
    content: "",
    group: 5
  },
  12: {
    content: "",
    group: 5
  }
};

var shiftCount = [],
  shiftCounter = 0,
  lastgroup, lastcount;
for (var slideIndex in slides) {
  var group = parseInt(slides[slideIndex].group, 10);
  if (group === 1) { //no shift caused by 1st group
    shiftCount.push(0);
  } else {
    if (group !== lastgroup) { //stay on current group.
      lastcount = shiftCount.length;
      lastgroup = group;
      shiftCount.push(shiftCount.length);
    } else { //fill rest of array with repeated count and match length.
      shiftCount.push(lastcount);
    }
  }
}
console.log(shiftCount);

Thanks!

You never use the shiftcounter variable, so feel free to remove it.

You don't care about the keys in the object you're iterating over, you only care about the objects at each key - so, it would be more appropriate to iterate just over the values, rather than the keys. Use Object.values instead of for..in.

The .group property is already an integer in the dataset, so there's no need to call parseInt with it. Since you want to extract the group property into a variable named group, you can do this more concisely with destructuring.

Rather than hard-coding the action to take when the group is 1 (the first), you can initialize lastGroup to 1 so that the //fill rest of array with repeated count and match length section will take care of the logic you need - it'll be more DRY.

const shiftCount = [];
let lastgroup = 1, // initial group
  lastcount = 0; // initial count
for (const { group } of Object.values(slides)) {
  if (group !== lastgroup) {
    lastcount = shiftCount.length;
    lastgroup = group;
    shiftCount.push(lastcount);
  } else { //fill rest of array with repeated count and match length.
    shiftCount.push(lastcount);
  }
}

const slides = {
  1: {
    content: "",
    group: 1
  },
  2: {
    group: 1
  },
  3: {
    content: "",
    group: 1
  },
  4: {
    content: "",
    group: 2
  },
  5: {
    content: "",
    group: 3
  },
  6: {
    content: "",
    group: 3
  },
  7: {
    content: "",
    group: 3
  },
  8: {
    content: "",
    group: 4
  },
  9: {
    content: "",
    group: 4
  },
  10: {
    content: "",
    group: 4
  },
  11: {
    content: "",
    group: 5
  },
  12: {
    content: "",
    group: 5
  }
};

const shiftCount = [];
let lastgroup = 1,
  lastcount = 0;
for (const { group } of Object.values(slides)) {
  if (group !== lastgroup) {
    lastcount = shiftCount.length;
    lastgroup = group;
    shiftCount.push(lastcount);
  } else { //fill rest of array with repeated count and match length.
    shiftCount.push(lastcount);
  }
}
console.log(shiftCount);


Fix base64 data URI scripts function So I noticed Chrome has quirky behaviour when it encounters script tags whose src is a base64 value. I decided to write a quick jQuery method that is supposed to work around it:

jQuery.extend({

    /**
     * Takes a script decodes the base64 src, puts it into the body of the script tag,
     * then puts it in whatever parent specified.
     *
     * @requires https://plugins.jquery.com/base64/
     *
     * @param  {Object} script The script tag that should be manipulated
     * @param  {Object} parent The parent element to append the final script tag to.
     *
     * @return {Object}        The script tag in question
     */
    importBase64Script : function ( script, parent ) {

        // Check for base64 library
        if ( typeof $.base64 === 'undefined' )
            throw 'No $.base64 found!';

        // Sanitize our script var
        // Normalize our script object
        script = ( script instanceof jQuery ? script : $(script) );

        // Check if it is a script tag
        if ( script[0].tagName !== "SCRIPT" )
            throw "Not a script tag";

        // Set default parent value
        parent = parent || $('head');
        // Normalize our parent var
        parent = ( parent instanceof jQuery ? parent : $(parent) );

        // We're gonna extract the base64 value
        var re        = /data:[a-z]+\/[a-z]+;base64,([0-9a-zA-Z\=\+]+)/,
        base64Content = script.prop('src').match(re)[1],
        scriptContent = $.base64.decode( base64Content );

        // Drop the decoded javascript into the contents of the script tag
        script.html( scriptContent );
        // Clear src value
        script.prop('src','');

        // Append it to the parent
        parent.append(script);

        return script;

    }
});

I tested a few of the conditions on JsPerf to see which is better performance wise. Granted, I didn't do a full sweep on every browser.

Any suggestions that anybody could make?

Watch out!

jQuery is full of secrets. html method is not just a simple wrapper around innerHTML property.

Possible fail condition:

var src = `if ("<x/>" !== "<" + "x" + "/" + ">") {
  console.log("Math gone wrong.");
}`;

$("<script>").html(src).appendTo(document.head)
<!-- latest jQuery -->
<script src="https://code.jquery.com/jquery-3.3.1.min.js"></script>

Let's see what exactly is happening here:

console.log($("<i>").html("<x/>")[0].innerHTML)
<!-- latest jQuery -->
<script src="https://code.jquery.com/jquery-3.3.1.min.js"></script>

jQuery html method replaces <x/> with <x></x>. I guess it is perfectly valid for some HTML, but it fails with our JavaScript input.

The π robbery

Let's try to exploit this with malicious input and steal some valuable data:

var src = `var username = "Kevin <x\";alert(Math.PI);\"/> Mitnick";`;

$("<script>").html(src).appendTo(document.head);
<!-- latest jQuery -->
<script src="https://code.jquery.com/jquery-3.3.1.min.js"></script>

I suggest you to carefully check jQuery html source code.

Also you may replace it with plain old innerHTML.


AutoDictionary for objects with a key property The normal dictiory separates the key from the value... but what if the key is a part of the item it stores? It then forces you to enter the same things twice, once for the key and agian for the item. I never liked it so I thought I write really simple class that takes this unecessary step away a makes life easier again by allowing you to specify a key selector like the ToDictionary extension does.

public class AutoDictionary<TKey, TItem> : ICollection<TItem>, IEnumerable<TItem>
{
    private readonly Func<TItem, TKey> _keySelector;

    private readonly IDictionary<TKey, TItem> _items;

    public AutoDictionary(Func<TItem, TKey> keySelector)
    {
        _keySelector = keySelector;
        _items = new Dictionary<TKey, TItem>();
    }

    public AutoDictionary(Func<TItem, TKey> keySelector, IEqualityComparer<TKey> equalityComparer)
    : this(keySelector)
    {
        _items = new Dictionary<TKey, TItem>(equalityComparer);
    }

    public AutoDictionary(Func<TItem, TKey> keySelector, IDictionary<TKey, TItem> other)
    : this(keySelector)
    {
        _keySelector = keySelector;
        _items = new Dictionary<TKey, TItem>(other);
    }

    public TItem this[TKey key]
    {
        get { return _items[key]; }
        set { _items[key] = value; }
    }

    public int Count => _items.Count;

    public bool IsReadOnly => false;

    public void Add(TItem item) => _items.Add(_keySelector(item), item);

    public void Clear() => _items.Clear();

    public bool TryGetItem(TKey key, out TItem item) => _items.TryGetValue(key, out item);

    public bool Contains(TItem item) => _items.ContainsKey(_keySelector(item));

    public void CopyTo(TItem[] array, int arrayIndex) { }

    public bool Remove(TItem item) => _items.Remove(_keySelector(item));

    public IEnumerator<TItem> GetEnumerator() => _items.Values.GetEnumerator();

    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

I wanted to make all methods use lambdas but the constructor doesn't allow this :-(

Usage:

class Foo
{
    public string Name { get; set; }
    public int Bar { get; set;}
    public double Baz { get; set;}
}

var autoDic = new AutoDictionary<string, Foo>(x => x.Name)
{
    new Foo { Name = "foo1", Bar = 1 },
    new Foo { Name = "foo2", Bar = 2 },
    //new Foo { Name = "foo1", Bar = 1 }, // bam!
};

var foo1 = autoDic["foo1"];

I guess I've reinvented the wheel, haven't I? ;-)

The single most surprising thing that jumps at me is that an AutoDictionary doesn't implement IDictionary, and therefore can't be used as such. This is a rather major oversight, enough to be a showstopper in deciding whether or not I would use this data structure or not.

Short of implementing and exposing an Add(object, object) method, this AutoDictionary is simply not a dictionary as far as client code is concerned - it's a [keyed] collection, or an enumerable, but not a dictionary.

I would definitely try to find a way to implement the generic IDictionary<TKey,TValue> interface on top of IDictionary, too.

How? Well you already have a TKey generic type parameter, so a generic dictionary implementation would definitely be easy. The non-generic interface could validate the type of the key parameter (e.g. the first object in the Add(object, object) method) and throw some ArgumentException if the type can't be cast to TKey.

I'd also add a constructor that intakes the IDictionary dependency:

public AutoDictionary(IDictionary<TKey, TItem> source, Func<TItem, TKey> keySelector)
    : this(keySelector)
{
    _items = source;
}

Accepting a generic dictionary constrained with TKey would make the _items assignment compile and guaranteed to work, but would would still need to validate that the existing keys are those returned by the keySelector, and throw an exception otherwise.

Why is the second generic type parameter named TItem, when every IDictionary reference in the framework names it TValue? That's another rather surprising detail.

This is bad:

public void CopyTo(TItem[] array, int arrayIndex) { }

The method should at least throw a NotImplementedException. This no-op member is part of the interface, the contract you've signed when you decided to implement it: IMO leaving it unimplemented breaks the Liskov Substitution Principle, which means your implementation cannot be used in place of any other ICollection implementation. It should populate the array parameter with all values in stored in _items.

The class isn't sealed, which means a new class could be derived from it. Consider implementing the interface members as virtual, so their implementation could be overridden in inherited types - or outright seal the class.


Parallel factorial algorithm using std::thread This code calculates the factorial of a number on multiple threads. My issue: it is only a little bit faster than the sequential version of it (and I think I know why, I just can't find a way to solve this).

I use boost::multiprecision::cpp_int so the limits of default integers are not a problem, the size of integers is only limited by memory.

Only showing the relevant parts:

// ... other includes ...
#include <boost/multiprecision/cpp_int.hpp>

#define THREAD_COUNT 4
std::atomic<int> thread_num(1);         // global variable

// stuff...

void threaded_factorial(unsigned long long int num, boost::multiprecision::cpp_int& bigInt)
{
    int threadid = thread_num++;     // thread_num is atomic, so this is safe
    boost::multiprecision::cpp_int N = 1;
    for (unsigned long long int i = threadid; i <= num; i = i + THREAD_COUNT)
    {
        N *=(i);

    }

    std::lock_guard<std::mutex> lock(mu);      // race condition --> mutex needed
    bigInt *= N;
}

// more stuff ...

And the call of the function:

// ...

boost::multiprecision::cpp_int result = 1;
std::thread workers[THREAD_COUNT];

for (int i = 0; i < THREAD_COUNT; ++i)
{
    workers[i] = std::thread(threaded_factorial, num, std::ref(result));
}
for (int i = 0; i < THREAD_COUNT; ++i)
{
    workers[i].join();
}

// ...

The results seem correct, but as I said, this is not much faster than sequential code.

For example. The calculation of the factorial of 325253 took

67586 ms on 4 threads
76226 ms on a single thread

That is some really poor performance.

The reason, I think is that the for cycle in the threaded_factorial function roughly takes the same amount of time for each thread to complete, so when the std::mutex mu is locked, (THREAD_COUNT-1) threads have to wait for the one which locked the mutex.

This way, most of the work (by far the largest multiplications) is happening in a sequential manner, so the algorithm is really slow.

How can I work around this issue and make this work efficiently?

Firstly, some issues with API usage. Using raw std::threads is generally not the way you want to go with this sort of thing: prefer to use std::async. This also means you don't need to pass in by reference for updates, as it can return a value. The other big win from this is that you don't need to lock and perform an update in the thread that is running the calculation; this can be done independently.

Firstly, let's modify the threaded_factorial function:

constexpr static auto threads = 4U;
constexpr static auto test = 325253U;

namespace mp = boost::multiprecision;

mp::cpp_int thread_fact(unsigned num, int start)
{
    mp::cpp_int n = start;
    for (auto i = start + threads; i <= num; i += threads) {
        n *= i;
    }
    return n;
}

To call this, we setup some arrays for the std::futures that will be returned, as well as an array of partial results.

std::array<std::future<mp::cpp_int>, threads> futures;
std::array<mp::cpp_int, threads> results;
for (auto i = 1; i <= threads; ++i) {
    futures[i - 1] = std::async(std::launch::async, thread_fact, test, i);
}
for (auto i = 0; i < threads; ++i) {
    results[i] = futures[i].get();
}

Now, the step where you combine these is actually pretty expensive. Multiplying two numbers that are of this magnitude will be time consuming; let's launch the multiplications in separate threads as well:

std::future<mp::cpp_int> x = std::async([&results]() -> mp::cpp_int { return results[0] * results[1]; });
std::future<mp::cpp_int> y = std::async([&results]() -> mp::cpp_int { return results[2] * results[3]; });
auto x_val = x.get();
auto y_val = y.get();
auto z = x_val * y_val;

Making these changes, this runs in a bit under 10 seconds for me. In fact, from the profile graph, most of that time is spent doing the combining.

Others have pointed out that further algorithmic improvements are possible if you need more speed.


Creating a page number array for pagination I have written a small function to generate an array of page numbers for displaying a pagination. The inputs to the function is an active page number p, and the max page number. The function will return an array of 5 possible page numbers available near the active page.

Example input, output:

pages(1,10): [1,2,3,4,5]

pages(10,10): [6,7,8,9,10]

pages(5,10): [3,4,5,6,7]

The function is:

def pages(p,max):
    a=[p]
    while len(a)<5 and not (a[0]==1 and a[-1]==max):
        if a[0]>1:
            a.insert(0,a[0]-1)
        if a[-1]<max:
            a.append(a[-1]+1)
    return a

Is there anything to do to improve this code, or is there any other simpler methods to achieve this?

1. Review

The function needs a docstring to explain what it does, with maybe some examples. The explanation in your post is very good: this is exactly the kind of thing that should go into the docstring.
The number 5 is arbitrary, so I would make it a parameter to the function.
The implementation takes time proportional to the number of entries in the page range. It would be more elegant to compute the endpoints of the range directly, taking constant time.

2. Revised code

def page_range(page, last, span=5):
    """Return a range of page numbers around page, containing span pages
    (if possible). Page numbers run from 1 to last.

    >>> list(page_range(2, 10))
    [1, 2, 3, 4, 5]
    >>> list(page_range(4, 10))
    [2, 3, 4, 5, 6]
    >>> list(page_range(9, 10))
    [6, 7, 8, 9, 10]

    """
    return range(max(min(page - (span - 1) // 2, last - span + 1), 1),
                 min(max(page + span // 2, span), last) + 1)


C++ console based - "Word Guessing" game I have made a console game in C++ which is called "Word Guessing", and according to the compiler, it is using about 542 KB of memory.

I am mainly wondering if it's possible to decrease the memory usage some more, and if possible, how it can be improved.

#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include <algorithm>

using namespace std;

// Function Declaration
string getRandWord();
void checkLetter(char&, string&, int&, string&);

// main function
int main() {

    // str Variables
    string RandomWord = getRandWord();
    string hiddenWord = "";

    // int Variables
    unsigned long int _length_ = RandomWord.length();
    int chances = int(_length_)+1;
    char newLetter;

    // fill varialbe with characters
    hiddenWord.append((_length_),'_');

    cout << "The Word is "<< _length_ <<" letters long." << endl;

    // game loop
    while(chances > 0){
        // request (char) input
        cin >> newLetter;
        checkLetter(newLetter, RandomWord, chances, hiddenWord);

        // check if player won
        if(hiddenWord == RandomWord){
            cout << "Congratulations! You won the game." << endl;
            break;
        }
        // decrease try chances
        chances--;
    }

    return 0;
}

// Functions

void checkLetter(char &newLetter, string &randstr, int &strlen, string &hiddenWord){
    // initiate iterator it
    string::iterator it;

    // iterate trough string
    for(auto it = randstr.begin(); it != randstr.end();++it){
        // validate letter mach
        if(*it == newLetter){
            // replace letter
            hiddenWord.at(it - randstr.begin()) = newLetter;
        }
    }
    cout << hiddenWord << endl;
}

string getRandWord(){
    string filePath = "/Users/nedimkanat/XCODE/testcpp/testcpp/";

    // constants
    enum sizes {
        COUNTER = 0,
        ARRAY_SIZE = 5
    };

    // set seed
    srand((unsigned)time(NULL));

    // get random int between 0 and 5
    int randint = rand() % ARRAY_SIZE;

    // str to store each line from file
    string str;

    // array to store 5 (random) words
    vector<string> arr;

    // initialize file object & open file
    ifstream file(filePath+"words.txt");
    int counter = COUNTER;

    // loop trough file
    if (file.is_open()){
        while (getline(file,str) && counter < ARRAY_SIZE){
            arr.push_back(str);
            counter++;
        }
        file.close();
    } else {
        cout << "File is not open" << endl;
    }

    // send away random word
    if(arr.empty()){
        cout << "CANCER" << endl;
    }
    return arr.at(randint);
}

Well, there is much to improve with your code, so let's begin at the beginning:

You should avoid using namespace std; It's a plague and just biding its time to bite you later.
Why is “using namespace std;” considered bad practice?
You might want to rework your logic regarding the tries. It's somewhat cruel to let the user go on guessing after he already cannot win anymore. And it's somewhat pointless if he can ask for every single character.
You should const-qualify what you can. Doing so makes logic-errors compile-errors, which can be caught by the compiler.
Use auto where you can. Remember that repetition is error-prone and everything you don't write nobody ever has to understand and check. Also known as the "Don't repeat yourself" (DRY) principle.
In that vein, I would also reorder your functions so you don't have to provide forward-declarations.
Consider everything but main() for static linkage, as it is never used outside the TU.
I'm sorry, but at best your comments are a blatant violation of DRY. Never repeat the code in the comments, write about why the code does something surprising or at least not quite straight-forward.
Consider consistent casing. Most of your variables begin with lower-case, why not all? Also, consider consistently having one space around binary operators.
There's nothing quite as useless as explicitly requesting something which will happen implicitly anyway. Aside from immediately afterwards undoing it.

std::string hiddenWord = "";
unsigned long int _length_ = randomWord.length();
hiddenWord.append((_length_),'_');
// Has the same effect as
std::string(randomWord.length(), '_');

Avoid beginning and/or ending names with an underscore. Even if you know the rules, it's error-prone and ugly.
Casts, function-style or not, are a last resort. And if you must use them, consider using the new-style casts as they are more prominent and explicitly name your intent.
Anyway, just use the right type.
Don't use std::endl unless you really need a manual flush. First, it's way uglier than a simple newline '\n', second, it kills performance as flushing is costly, and third, it's superfluous.
The standard input- and output-streams std::cin and std::cout are coupled so using the latter flushes the former if used with an interactive console.
If you want to do something a specific number of times, use a for-loop, that's what it's for.
Always test whether input succeeded. A pipe for example could be closed prematurely, or a file run out.
Also, you might want to re-try until you get something.
I wonder why checkLetter doesn't return whether now everything is revealed.
return 0; is implicit for main() in C++ and C99+.
rand and srand is quite a bad random-number-generator. Still, if you insist on using it, consider initializing it directly in main() to make sure you never inadvertently initialize it twice. And especially to make sure you don't repeatedly re-initialize it before using it again.
Take a look at <random> for better RNGs.
There's absolutely no reason to limit yourself to the first five words in the input-file. Or to fail if the file contains less words.
All error output should be send to the error stream, not standard output. That's what it's there for.
Consider accepting the input-filename as an argument, instead of hardcoding it. You might even give it a default.
Extract the task of reading the opening and reading the file. std::istream_iterator might come in handy for it.
Why does checkLetter() accept everything by modifiable reference? hiddenWord should be, sure, but randstr (which should be named randomWord for consistency) should be a constant reference, newLetter a value and strlen is superfluous.
That function also has two variables named it, the first one should be summarily removed.

Modified code:

#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include <algorithm>
#include <iterator>
#include <string_view>

static bool checkLetter(
    char newLetter,
    std::string_view randomWord,
    std::string &hiddenWord
) {
    for(auto it = randomWord.begin(); it != randomWord.end(); ++it)
        if(*it == newLetter)
            hiddenWord.at(it - randomWord.begin()) = newLetter;
    std::cout << hiddenWord << '\n';
    return randomWord == hiddenWord;
}

static auto readFile(const char* filename) {
    std::ifstream file(filename);
    using isi = std::istream_iterator<std::string>;
    std::vector<std::string> arr{isi(file), isi()};
    return arr;
}

static void removeBadEntries(std::vector<std::string>& arr) {
    std::sort(arr.begin(), arr.end());
    arr.resize(std::remove_if(arr.begin(), arr.end(),
        [last = std::string_view()](auto& x) mutable{
            for(auto y : x)
                if(!isgraph((unsigned char)y))
                    return true;
            if(last == x)
                return true;
            last = x;
            return false;
        }) - arr.begin());
}

static auto getRandWord(const char* filename) {
    auto arr = readFile(filename);
    removeBadEntries(arr);
    std::cout << "Choosing from " << arr.size() << " unique valid entries.\n";

    return arr.empty() ? std::string("CANCER") : std::move(arr[rand() % arr.size()]);
}

int main(int argc, char* argv[]) {
    std::setlocale(LC_ALL, "");
    srand((unsigned)time(NULL));

    const auto randomWord = getRandWord(argc > 1
        ? argv[1]
        : "/Users/nedimkanat/XCODE/testcpp/testcpp/words.txt");
    std::string hiddenWord(randomWord.length(), '_');

    std::cout << "The Word is " << randomWord.length() << " letters long.\n";

    for(auto chances = randomWord.length() + 1; chances > 0; --chances){
        char c;
        do {
            if(!(std::cin >> c)) {
                std::cerr << "Error reading a character from stdin.\n";
                return EXIT_FAILURE;
            }
        } while(!std::isgraph((unsigned char)c));

        if(checkLetter(c, randomWord, hiddenWord)) {
            std::cout << "Congratulations! You won the game.\n";
            break;
        }
    }
}


Query the context or resulting list I was just getting some products using entity framework and was wondering which would be better to do when getting the results, or if there is no difference.

Option 1 - Query the context directly

return dbContext
                    .PPCProducts
                    .Include(p => p.Attributes)
                    .Include(p => p.ChildProducts)
                    .Where(p => p.Attributes.FirstOrDefault(a => a.AttributeName.IndexOf(attributeName, StringComparison.InvariantCultureIgnoreCase) > -1) != null)
                    .OrderBy(p => p.ProductName)
                    .ToList();

Option 2 - Query the cache

// cache the products
var products = dbContext
   .PPCProducts
   .Include(p => p.Attributes)
   .Include(p => p.ChildProducts);
CacheHelper.Add("products", products, CacheDuration.Hours3);

// query the cache
return ((List<Product>)HttpContext.Current.Cache["products"]).Where(p => p.Attributes.FirstOrDefault(a => a.AttributeName.IndexOf(attributeName, StringComparison.InvariantCultureIgnoreCase) > -1) != null)
                    .OrderBy(p => p.ProductName)
                    .ToList();

There are a few potential issues here, speed and accuracy in particular.
Speed
The cache is often faster than querying a value directly because of computation - you need to input, process, calculate, and output values.  For example, when you search the web, the servers return results based on cached data so they don't have to literally scan every website, read keywords, process data, etc.  Reading from the cache is as simple as getting the final value with no computation involved.
Accuracy
Querying a cache can return obsolete data.  Let's say you have a product priced at $X, and change the price for a sale.  If you don't update the cache, you will return outdated prices to your customers.  Querying the original data bypasses the intermediate value, always ensuring the value is correct.


Deploy folder to desired path - PowerShell function I am still learning PowerShell and I would like your opinion on my folder deployment PowerShell function. It basically check if a defined folder exists and creates it if it does not and writes it actions to log and console.
<#
.SYNOPSIS
Creates folder if it does not already exists.
.DESCRIPTION
This function check if defined transfer folder exists and if not it creates it on remote computer.
.PARAMETER Path
Full path of the folder.
.PARAMETER Cancel
If Cancel parameter set to true the folder deployment is canceled. This is used in pipeline when it is important to skip this
operation if last operation failed.
.EXAMPLE
Deploy-Folder -Path 'D:\Folder\Folder'
#>
function Deploy-Folder {
    param (
        [Parameter(Position = 0, Mandatory, ValueFromPipeline, ValueFromPipelineByPropertyName)]
        [String]
        $Path,

        [Parameter(Position = 1, ValueFromPipeline, ValueFromPipelineByPropertyName)]
        [boolean]
        $Cancel = $false
    )

    begin {
        Import-Module '.\Write-Log.psm1'
    }

    process {
        if(-not $Cancel) {
            if((Test-Path $Path) -eq $true) {
                $Message = "Successfully accessed " + $Path + " folder"
                $OperationResult  = 'Success'
            }
            else {
                try {
                    New-Item -Path $Path -ItemType "Directory"
                }
                catch {
                    $Message = "Failed to create " + $Path + " folder `n" + $_.Exception
                    $OperationResult  = 'Fail'
                }

                if((Test-Path $Path) -eq $true) {
                    $Message = "Successfully created " + $Path + " folder"
                    $OperationResult  = 'Success'
                }
            }
        }
        else {
            $Message = "Canceled " + $Path + " folder deployment"
            $OperationResult  = 'Success'
        }

        Write-Log -OperationResult $OperationResult -Message $message

        if($OperationResult -ne 'Fail') {
            $Cancel = $false
        }
        else {
            $Cancel = $true
        }

        New-Object -TypeName psobject -Property @{
            Cancel = $Cancel
        }
    }
}

The code is very cleanly written, and its easy to see what is going on.
The verb you are using for the command is not officially 'supported'. Every command should stick to the official list of verbs, that can be checked with get-verb.  would suggest New-Folder
I don't know why you go through this much code to create a folder when you could append a -force to the back end of the new-item command. I guess if you have a system that required you to log every instance where one would create a folder, this would make sense.
If you have a system where you do commands to an external module, i would suggest you look into the #require tag in powershell, instead of having import-module at the start of your command
you can easily cast to a psobject just by doing [PSCustomObject]@{cancel=$cancel}.
use [CmdletBinding()] at the start of your function to get powershell standard switches (-verbose, -debug, -erroraction etc..). just put it between the function definition line and the param definition line (line 2)


Making a deck of cards in Python I want to have a deck of cards that looks like this:

['H1', 'H2', 'H3', 'H4', 'H5', 'H6'...]

For that I wrote the following code:

    import itertools
    kind = 'HCDS'
    value = ['1','2','3','4','5','6','7','8','9','10','J','Q','K','A']
    temp = list(itertools.product(kind,value))

    deck = []
    for i in range(0, len(temp)):
        (a,b) = temp[i]
        deck.append(a+b)
    print(deck)

I was wondering if it could be done easier without the use of this temp variable or the for loop.

You could just use str.join in a list comprehension and create deck directly:

import itertools
kind = 'HCDS'
value = ['2','3','4','5','6','7','8','9','10','J','Q','K','A']
deck = ["".join(card) for card in itertools.product(kind,value)]

Note that a regular French card set starts at 2, there is no 1.