Read a csv file and insert into DB table I've found some resources about PHP performance optimization here and there but I don't know how to start. I send 130k CSV rows in approximatively 7min. My script read CSV file and send $batch_size row per query, while doing some string operations such as changing date to the correct format or replacing , with . for the floats.
$db = new PDO("mysql:host=$db_host;port=$db_port;charset=utf8","$db_username", "$db_password");
$csv_path = "path/to/myfile.csv";
$table = "TableName";
//Start of the query
$query="USE ".$db_name.";";

$FilePath = $csv_path;
$is_not_title_row=FALSE;//Usefull not to take the first row

$row_batch_count=0;//Counter for batches

//1 data = 1 part of the row
$number_of_data=0;
$data='';

// Open file in read-only mode
$handle = fopen($FilePath, "r");

while (($data = fgetcsv($handle, 2000, ";"  ,  chr(8))) !== FALSE) {

        $number_of_data = count($data)-1;

        // Generate MySQL query
        if($is_not_title_row)
        {
            $query .= "INSERT INTO ".$table." VALUES (";

            //We read all the data of one row
            for ($c=0; $c <= $number_of_data; $c++) {

                $data[$c]=utf8_decode($data[$c]);

                $data[$c]=str_replace('/', '-', $data[$c]);
                $data[$c]=str_replace(',', '.', $data[$c]);

                //If it's a date, we convert it to the right date format
                 if (DateTime::createFromFormat('d-m-Y', $data[$c]) !== FALSE) {
                    $data[$c] = date("Y-m-d", strtotime($data[$c]));
                    $query .= "'" . mres($data[$c]) ."',";
                //If there is nothing, we send NULL value
                else if(mres($data[$c])==NULL){
                    $query .= "NULL,";
                }
                else{
                        $query .= "'" . mres($data[$c]) ."',";
                }

                //If this is the end of the INSERT INTO we remove the comma
                if($c==$number_of_data)
                {
                    $query=substr_replace($query ,"", -1);
                }

            }
            $query .= ");";

            $row_batch_count++;

            //If we are at the end of the batch, we send the query then start the creation of another
            if($row_batch_count==$batch_size)
            {
                echo "There is ".$row_batch_count." row sent
";
                echo "

";
                $query_to_execute = $db->prepare($query);
                $query_to_execute->execute();

                $query="USE ".$db_name.";";

                $row_batch_count=0;
            }

        }

        //Usefull to remove the first row
        if(!$is_not_title_row){
            $is_not_title_row=TRUE;
        }

    }

        //Without more batches, we execute the query(=we are at the end of the file)
        $query_to_execute = $db->prepare($query);
        $query_to_execute->execute();
        $query_to_execute = NULL;

Solution
Instead of doing one INSERT INTO for each row, I've simply inserted multiple row in a single INSERT INTO.
The output query looks like that :
INSERT INTO Table VALUES (x,x,x),(x,x,x),(x,x,x),(...

The csv insertion is now 30 times faster

What I found to greatly improves the speed of insert queries, is inserting multiple rows using a single query. You seems to have found the same solution, but it is implemented in a very roundabout way.
Your code could also benefit a lot from using functions. So, let's start with the most important one: How to insert multiple rows. It could look something like this:
function insertRows($db, $table, $rows)
{
    $values    = '(' . implode(',', array_fill(0, count($rows[0]), '?')) . ')';
    $query     = 'INSERT INTO '.$table . ' VALUES ' .
                 implode(',', array_fill(0, count($rows), $values));
    $statement = $db->prepare($query);
    $index     = 1;
    foreach($rows as $row) {
        foreach($row as $value) {
            $statement->bindValue($index++, $value);
        }
    }
    $statement->execute();
}

This bit of code has only one function: Insert a certain number of rows into the database as quickly as possible. This function doesn't read data, change it in any way, but it does use parameter binding for extra security. That's probably not needed in this case, but it also doesn't hurt. No more problems with quotes.
You can insert quite a few rows using this function, somewhere around 50 to 150 rows, depending on what your input looks like. Above a certain number the speed simply won't increase anymore, so there's no point in going to extremely large number of rows.
I would create another function for changing the row data from the format you received to the format you want to insert:
function reformatRow($row)
{
    // <...put your code here...>
    return $row;
}

We can now also implement a function that does the above for multiple rows:
function reformatRows($rows)
{
    foreach ($rows as $key => $row) {
        $rows[$key] = reformatRow($row);
    }
    return $rows;
}

Functions don't have to be complex to be useful. I actually will not use this function, but I thought it was nice to show it.
All that's now left to do is to read the input, so let's do that. I will use parts of your code:
const MAX_ROWS = 100;

fgetcsv($handle, 2000, ";", chr(8)); // read header and dump it

$rows = [];
while (($row = fgetcsv($handle, 2000, ";", chr(8))) !== FALSE) {
    $rows[] = reformatRow($row);
    if (count($rows) >= MAX_ROWS) {
        insertRows($db, $table[$i], $rows);
        $rows = [];
    }
}

if (count($rows) > 0) {
    insertRows($db, $table[$i], $rows);
}

I haven't fully tested all this code.
I also don't think this code will be any quicker than yours. There's also no way for me to test any optimization, since I don't have your data, so I'm afraid I can't help you there. Reading data into a database just takes time. It is possible that fgetcsv() is not the optimal way to read a big CSV file, but I haven't tested that.
What I have tried to make clear is that by splitting your code into functional parts it become easier to understand, and more reusable. The insertRows() is, for instance, is, partly, reused from my own code. Once you've written a function like that, and it works, you can reuse it everywhere.


A simple clusterness measure of data in one dimension using Java Problem definition

Given \$X = (x_1, \dots, x_n)\$ such that \$x_1 \leq x_2 \leq \dots \leq x_n \$. Let \$x_{\min} = \min X = x_1\$, \$x_{\max} = \max X = x_n\$ and \$r = x_{\max} - x_{\min}\$. Also, let \$m = n - 1\$. Finally,
$$
\ell = \frac{r}{m}
$$
is the average gap length. The clusterness measure CM of \$X\$ is given by
$$
1 - \frac{1}{r} \sum_{i = 1}^m \min(\vert \ell - \Delta_i \vert, \ell),
$$
where \$\Delta_i = x_{i + 1} - x_i\$ is the length of the \$i\$th gap. Assuming \$r > 0\$, CM returns a value in range \$[0, 1)\$ that represents how well the points in \$X\$ are close to each other, the value of 0 denoting the situation where all the gaps are equal in length. If we define \$CM = 1\$ in case \$r = 0\$ (all \$x_i\$ are equal), the range of CM extends to \$[0, 1]\$.

Solution

package net.coderodde.datascience.measures;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;

/**
 * This class implements a method for computing clusterness measure (CM). CM
 * asks for a set of points x1, x2, ..., xn, and it returns a
 * number within [0, 1]. The
 * idea behind CM is that it returns 0 when all the data points are equal and 1
 * whenever all adjacent points are equidistant.
 *
 * @author Rodion "rodde" Efremov
 * @version 1.6 (Feb 21, 2019)
 */
public class ClusternessMeasure {

    public double computeClusternessMeasure(Collection<Double> points) {
        List<Double> pointList = new ArrayList<>(points);
        Collections.sort(pointList);

        double minimumPoint = pointList.get(0);
        double maximumPoint = pointList.get(pointList.size() - 1);
        double range = maximumPoint - minimumPoint;

        if (range == 0.0) {
            // Once here, all data points are equal and so CM must be 1.0.
            return 1.0;
        }

        double expectedDifference = range / (points.size() - 1);
        double sum = 0.0;

        for (int i = 0; i < points.size() - 1; i++) {
            double currentDifference = pointList.get(i + 1) - pointList.get(i);
            sum += Math.min(Math.abs(expectedDifference - currentDifference),
                                     expectedDifference);
        }

        return sum / range;
    }

    public static void main(String[] args) {
        ClusternessMeasure cm = new ClusternessMeasure();

        // CM = 0.0
        System.out.println(
                cm.computeClusternessMeasure(
                        Arrays.asList(1.0, 2.0, 3.0, 4.0, 5.0)));

        // CM = 1.0
        System.out.println(
                cm.computeClusternessMeasure(
                        Arrays.asList(2.0, 2.0, 2.0)));

        System.out.println(
                cm.computeClusternessMeasure(
                        Arrays.asList(1.0, 2.0, 3.0, 5.0, 4.0, 6.0, 7.1)));
    }
}

Critique request

Please tell me anything that comes to mind. I am most interested in coding conventions and naming style. Also, would like to hear comments from machine learning perspective.

In my opinion your code is perfectly fine! I just found some fussy thinks.

HTML Comments
In Robert C. Martin's book Clean Code under HTML Comments you can read

HTML in source code comments is an abomination [...]. It makes the comments hard to read in the one place where they should be easy to read — the editor/IDE. If comments are going to be extracted by some tool (like Javadoc) to appear in a Web page, then it should be the responsibility of that tool, and not the programmer, to adorn the comments with appropriate HTML.

In to places you uses the -tag. Which can be replaced by javadocs {@code}
For example from

x1, x2, ..., xn

to
{@code x1, x2, ..., xn}

Duplicated Statement
The statement points.size() - 1 occurs three times inside the method computeClusternessMeasure. Even it is only a \$O(1)\$-statement it makes it easier to read the variable indexOfLastEntry instead of points.size() - 1

An Other Argument
Currently computeClusternessMeasure takes a Collection but instead you need a List. This "transformation" has nothing to do with the algorithm and a a collection makes additional to that no sense..
Furthermore you need to sort it..
Take a First-Class-Collection as Argument
If you want to keep the flexibility you could create a First-Class-Collection.

Any class that contains a collection should contain no other member variables. Each collection gets wrapped in its own class, so now behaviors related to the collection have a home.

The method signature I want to create is double computeClusternessMeasure(SortedMessurementPoints points)
public class SortedMessurementPoints {

      private List<Double> points;

      public SortedMessurementPoints(Collection<Double> points) {
          List<Double> asList = new ArrayList(points);
          Collection.sort(asList )
          this.points = asList;
      }

      public double get(int index) {
          return points.get(index);
      }

      public int indexOfLastEntry() {
            return points.size() - 1;
      }
}

public double computeClusternessMeasure(SortedMessurementPoints points) {
    double minimumPoint = points.get(0);
    double maximumPoint = points.get(points.indexOfLastEntry());
    double range = maximumPoint - minimumPoint;

    if (range == 0.0) {
        // Once here, all data points are equal and so CM must be 1.0.
        return 1.0;
    }

    double expectedDifference = range / points.indexOfLastEntry();
    double sum = 0.0;

    for (int i = 0; i < points.indexOfLastEntry(); i++) {
        double currentDifference = points.get(i + 1) - points.get(i);
        sum += Math.min(Math.abs(expectedDifference - currentDifference),
                                     expectedDifference);
    }

    return sum / range;
}


Script that creates an active CSS on link I have a script that creates an active CSS on link, but I think there are a lot of if statements.  Can someone help me refactor this?

var link = window.location.pathname;
var currentPageName = window.location.href.match(/[^\/]+$/)[0];

var hl = $('#ih').val();

if (hl == 'True') {
    var currentPageName = 'WVD';
}

if (window.location.href.indexOf("R") > -1) {
    var currentPageName = window.location.href.match(/[^\/]+$/)[0];
}

if (window.location.href.indexOf("C") > -1) {
    var currentPageName = window.location.href.match(/[^\/]+$/)[0];
}

if (location.pathname == "/") {
    var currentPageName = 'MD';
}

$(".main-navigation ul li a").each(function () {
    var link = $(this).attr("href");
    if (link.indexOf(currentPageName) > -1) {
        $(this).addClass("active");
        $(this).css("color", "white");
        $(this).css("text-decoration", "bold");
    }
});

Please consider that using Javascript might not be the best solution. If you use something like PHP, passing a variable to the file generating the html setting which one is active is a cleaner solution.

Anyway..

First it seems that the following variable is unused and can be removed:

var link = window.location.pathname;

Then this line:

var currentPageName = window.location.href.match(/[^\/]+$/)[0];

will trigger an error if the url is root (http://www.example.com/)
and it's the same as in these ifs, which also can be combined to one:

if (window.location.href.indexOf("R") > -1) {
    var currentPageName = window.location.href.match(/[^\/]+$/)[0];
}

if (window.location.href.indexOf("C") > -1) {
    var currentPageName = window.location.href.match(/[^\/]+$/)[0];
}

which means that one of them is redundant and can be removed.

You can also remove var in all but the first  var currentPageName

An easier way to add the active effect would be to use the attribute contains selector like this:

selector = '.main-navigation ul li a[href*="'+currentPageName+'"]"'
$(selector).css({
    'color': 'white',
    'font-weight': 'bold',
}).addClass('active');

This means that the complete code could look like this:

var match = window.location.href.match(/[^\/]+$/);
var currentPageName = '';

if (location.pathname == '/') {
    currentPageName = 'MD';
} else if (match != null) {
    currentPageName = match[0];
}

var hl = $('#ih').val();

if (hl == 'True') {
    currentPageName = 'WVD';
}

selector = '.main-navigation ul li a[href*="'+currentPageName+'"]"'
$(selector).css({
    'color': 'white',
    'font-weight': 'bold',
}).addClass('active');


Interface for a transformable grid I am working on an XNA project, and since procedural generation is likely to be used, I wanted to eventually create a helper class to work with data to facilitate that.

Design Goal:

Originally, I wanted to generate and modify a heightmap in a simple but efficient manner. The most straightforward approach to handle this would be a with a float[,].

Right now, I have a Grid<T> which simply holds the data in a more convenient manner, but I want to introduce a way to transform it somehow - slicing, rotations, etc. The ability to add and multiply different heightmaps together would be beneficial as well.

My Code:

Knowing that C# is optimized for single-dimensional arrays, I decided to make a simple wrapper class that contains an internal jagged array, but have it look and behave like a 2-dimensional array.

I first made it into an interface because doing so allows me to implement it however I want, which allows for future optimizations which won't break any dependent code:

// Presents itself as a simplified 2-dimensional array
public interface IGrid<T> : IEnumerable<T>, IEnumerable
{
    Point Size { get; }
    T this[int x, int y] { get; set; }
}

And the current (simplified) implementation is:

// Functions as a jagged array under the hood
public class Grid<T> : IGrid<T>
{
    private readonly T[][] _values;

    // Implement IGrid<T>
    public Point Size { get; private set; }
    public T this[int x, int y]
    {
        get { return _values[y][x]; }
        set { _values[y][x] = value; }
    }

    // Constructor
    public Grid(int x, int y)
    {
        Size = new Point(x, y);
        _values = new T[y][];
        for (int i = 0; i < y; i++)
            _values[i] = new T[x];
    }

    // Provide IEnumerator, override ToString(), etc...
}

My approach for the transformation bits is similar:

public interface ITransformable<T>
{
    T Slice(int x, int y, int width, int height);
    T Slice(Rectangle value);
    T Transpose();
    T Flip(bool horizontal, bool vertical);
    T Rotate(bool clockwise);
}

And here is where I am stumbling. I'm not actually sure what I want to do with it!

Attach it to IGrid<T>. It's no longer about simply storing the data, but transforming it as well.
Attach it to Grid<T>. The fact that a grid can transform the data is an implementation detail.
Create a derived class Mesh<T> which does everything Grid<T> does, but also implements ITransformable<T>.
Create a derived class Mesh<T>, but remove ITransformable<T> and move its methods into this class.

Thoughts:

Since generic types do not support operator overloading (+, -, /, *), it means that if I want a Heightmap to support addition in the form of Heightmap m = a + b, it would need a specific implementation, such as public class Heightmap : Mesh<float>, which then supplies the implementation of the operators.

Based on that, my entire class chain might resemeble:

public interface IGrid<T> { ... }
public interface IMesh<T> : IGrid<T> { ... }
public class Grid<T> : IGrid<T> { ... }
public class Mesh<T> : Grid<T>, IMesh<T> { ... }
public class Heightmap : Mesh<float> { ... }

I think you should use aggregation.

It's either:

class Mesh<T> : ITransformable<T>
{
    public Mesh(IGrid<T> grid, ....)
    {
        Grid = grid;
        ......
    }

    public IGrid<T> Grid { get; private set; }
    .....
}

or:

class Transformer<T> : ITransformer<T>
{
    public T Slice(IGrid<T>, int x, int y, int width, int height)
    {
        ...
    }
    ....
}

It's hard to tell which is better without good understanding of your design (which I don't have). But generally I would prefer the second option.

Also, your IGrid implementation looks really weird. You are saying that you want to implement 2D array using 1D array, yet you use T[][] _values; in your implementation. So, what's the point? There is also a System.Windows.Size class, so I don't understand why would you want to use a Point.


Help optimizing a function to count the rows in a textarea I am writing a script to count the number of rows in a textarea, and as it will be called on every keypress I want to make sure it runs as fast as possible. I'm not aiming for a general solution: we can assume that the textarea has a monospace font and enough horizontal space for exactly 80 characters.

I've probably solved this problem in 20 different ways, and this is by far the fastest solution I've come up with:

function countRows1(txt) {
    var i, len, rows = 0;
    txt = txt.split('\n');
    len = txt.length;
    for (i = 0; i < len; i++) {
        rows += Math.ceil(txt[i].length / 80) || 1;
    }
    return rows;
}

Here's a second solution, which is slightly faster in Firefox, but otherwise between a little and 5x slower:

function countRows2(txt) {
    var i = 0, nextBreak, rows = 0;
    while ((nextBreak = txt.indexOf('\n', i)) !== -1) {
        rows += Math.ceil((nextBreak - i) / 80) || 1;
        i = nextBreak + 1;
    }
    return rows + (Math.ceil((txt.length - i) / 80) || 1);
}

The performance doesn't make any sense to me - countRows2 does the same thing as countRows1, except it doesn't create any unnecessary strings or arrays. Is indexOf with a start position just really slow, or is there some optimization only the first method is picking up on?

Finally, here's one more method, which is much slower in every browser I've tested, but beautifully concise:

function countRows3(txt) {
    return txt.match(/\n?[^\n]{1,80}|\n/g).length;
}

Here's the jsPerf I'm using to compare the three.

So I've really got three questions:

Why is countRows2 slower than countRows1?
Is there any way to optimize the regex in countRows3 so the performance is comparable to the first two?
Is there any other way to speed up this code?

ps: I can't just do something like el.scrollHeight, because I need it to shrink as text is deleted. Also, I don't want to remember the previous count and just calculate the difference, because some changes will inevitably be missed and a differential script will never right itself.

What kind of performance are you getting?  Is it acceptable.  If it's not, then I'd look for changing your algorithm, and not minor tweaks.  In particular, I'd look into NOT splitting the string and recalclating each time.  Instead keep track of the state (what line and the number of characters on that line, whether it's a paste or a delete), and do the right thing based upon that.  Adding an additional character at the end of a 5k block of text, adds one more line at most, if the last line was 1 character long, then unless the key being pressed is the enter key, it doesn't even do that.

But before doing anything, sit down and see how well your existing function works, for your typcial data, and then for some extreme data.


Using Common Table Expression and Outer queries In following query I'm use both Common Table Expression and Outer queries. Apparently both looks same to me.

If I summarize my requirement, I have employees in SalaryTrans table and I want to find out amounts to be recovered from their salaries monthly. Table Rcovery contains all recovery data.  Table Instalment contains all previously paid amounts so I want to sum all paid amounts and then it is deducted from total amount to calculate balance amount.

So what I'm doing here is in CTE, I'm getting two numeric fields called recovered_amount and total_amount. In second query I'm checking whether recovered_amount is not null and if it's not null I'm calculating the balance.

In outer query I have a criteria to filter out less than 0 balances from the result.

WITH CTE (employee_id, RID, instalment, reference, recovered_amount, total_amount) AS
         (SELECT SalaryTrans.employee_id, Recovery.RID, Recovery.amount / Recovery.duration AS instalment,
          SalaryTrans.reference, SUM(Instalment.recovered_amount) AS recovered_amount,       Recovery.amount AS total_amount
          FROM Recovery INNER JOIN
                        SalaryTrans ON Recovery.emp_id = SalaryTrans.employee_id LEFT OUTER JOIN
                        Instalment ON Recovery.RID = Instalment.recovery_id
          GROUP BY SalaryTrans.employee_id, SalaryTrans.reference, Recovery.RID, Recovery.amount / Recovery.duration, Recovery.amount)

SELECT RID, Balance, reference  FROM
      (SELECT RID,
       "Balance" = CASE WHEN recovered_amount !=NULL
                   THEN total_amount-recovered_amount
                   ELSE total_amount END ,
       reference FROM CTE) AS Result
WHERE Balance >0

This query gives intended output. But I want to know Can I do whole thing using CTE or simply should I use outer queries in this case. Also any idea to make it look better?

Could you please review this and give your feedback?

I find the query rather difficult to read due to your indentation style.  For your benefit and for the benefit of other reviewers, I've reformatted your query, changing only the whitespace:

WITH CTE (employee_id, RID, instalment, reference, recovered_amount, total_amount) AS (
    SELECT SalaryTrans.employee_id
         , Recovery.RID
         , Recovery.amount / Recovery.duration AS instalment
         , SalaryTrans.reference
         , SUM(Instalment.recovered_amount) AS recovered_amount
         , Recovery.amount AS total_amount
        FROM Recovery
            INNER JOIN SalaryTrans
                ON Recovery.emp_id = SalaryTrans.employee_id
            LEFT OUTER JOIN Instalment
                ON Recovery.RID = Instalment.recovery_id
        GROUP BY SalaryTrans.employee_id
               , SalaryTrans.reference
               , Recovery.RID
               , Recovery.amount / Recovery.duration
               , Recovery.amount
)
SELECT RID, Balance, reference
    FROM (
            SELECT RID
                 , "Balance" = CASE
                      WHEN recovered_amount != NULL THEN total_amount - recovered_amount
                      ELSE total_amount
                   END
                 , reference
                FROM CTE
        ) AS Result
    WHERE Balance > 0

In particular, note:

For anything other than a trivial SELECT, use a separate line for each column being selected.  (My use of leading commas may be controversial.  I find that it helps prevent silly mistakes when editing the query to add and remove columns.)
The indentation matches the clauses of each expression.  For example,

The big-picture structure is WITH (CTEs) SELECT (something).

Within the CTE, SELECT takes a FROM clause and a GROUP BY clause.

The FROM clause consists of two JOINs.

The main SELECT has a FROM clause and a WHERE clause.

Within the CASE, make it more obvious that the THEN is associated with WHEN.

Use a space on each side of binary operators such as !=, -, and >.


Using dplyr and pipes for logistic regression plotting I have successfully created a plot of a binomial glm using example data. https://sciences.ucf.edu/biology/d4lab/wp-content/uploads/sites/125/2018/11/parasites.txt

The predictors of the model include 3 predictors (one categorical, 2 continuous)

The code works fine but I have been wanting to try and incorporating more dplyr functions and pipes to streamline code. Ultimately, I want to make my block of code into a function that works with any model with the same type and number of predictors for a binomial glm. Are there better ways to carry out my code with more tidyverse/dplyr code?

#import parasites file

df<-parasites
m1<-glm(data=df, infected~age+weight+sex, family = "binomial")
summary(m1)
age_grid <- round(seq(min(df$age), max(df$age), length.out = 15))
weight_grid <- round(seq(min(df$weight), max(df$weight), length.out = 15))
newdat <- expand.grid(weight =weight_grid,
                      age = age_grid, sex = c("female", "male"))

pred <- predict.glm(m1, newdata = newdat, type="link", se=TRUE)
ymin <- m1$family$linkinv(pred$fit - 1.96 * pred$se.fit)
ymax <- m1$family$linkinv(pred$fit + 1.96 * pred$se.fit)
fit <- m1$family$linkinv(pred$fit)
z <- matrix(fit, length(age_grid))
ci.low <- matrix(ymin, length(age_grid))
ci.up <- matrix(ymax, length(age_grid))

x<-data.frame(pred = fit,
              low = ymin,
              high = ymax,
              newdat) %>% mutate(category=cut(age, breaks=c(0, 69, 138, 206), labels =
                                                c("0-69", "70-139", "139-206")))

x$age<-as.factor(x$age)

library(ggplot2)
finalgraph<-ggplot(data=x)+
  geom_line(aes(x = weight, y = pred, color = age))+
  geom_ribbon(aes(x = weight, ymin = low, ymax = high, fill = age), alpha = 0.1) +
  facet_grid(category~sex) +theme(panel.grid.major = element_blank(),
                                  panel.grid.minor = element_blank())+
  ylab(expression(bold(y = "Infection Probability"))) + xlab(expression(bold("Weight"))) +
  theme(legend.position = "right",strip.text.x = element_text(face = "bold", size=12),
        strip.text.y = element_text(size=10),
        axis.text.y = element_text(size=10, face = "bold"), axis.text.x = element_text(size=10),
        axis.title = element_text(size=12),
        legend.text=element_text(size=10), legend.title = element_text(size=12, face="bold"))+
  labs(linetype="Age (months)", colour="Age (months)", fill = "Age (months)")
finalgraph

Code notes:
Essentially I made a model, created a bunch of values from my predictors (age_grid, v_grid) and made all possible combinations of these values along with the categorical variable of sex using expand.grid.

Then I just used the predict.glm function to extract predicted values based off of expand.grid object. I also extracted std. errors and calculated confidence intervals (ci.up and ci. low). Then I used some dplyr functions to create a dataframe with all this information and also made a new column called category. Category breaks down one of my variables (age) into four distinct groups based of  f of breaks I decided on and labelled as decided as well.  Then I plotted all of this data using ggplot2.

Here is what I would do leaning fully into the tidyverse style to get a nice "pipelined" set of steps that are easy to wrap up into functions.

Loading tidyverse just gets some extra tools from purrr here, but I find it much more productive for data manipulation in general, compared to just using dplyr alone. That said if you can only use dplyr you can replace the purrr::map() for lapply() and purrr::keep() for Filter(), but you lose a little pipe readability.

First time posting on the sub-Exchange, so feedback is welcome.

df <- read.csv("https://sciences.ucf.edu/biology/d4lab/wp-content/uploads/sites/125/2018/11/parasites.txt", header = T)

m1 <- glm(data=df, infected ~ age + weight + sex, family = "binomial") # add spaces to variables separated by arithmetic operators
link_func <- m1$family$linkinv # maybe this could become a generic function

library(tidyverse)

# anonymous functions are quick and easy to type, my preference if only one input arg
newdat_func <- . %>% # meant to start with df
  select(weight, age) %>% # keep only column of interest
  map(~ round(seq(min(.), max(.), length.out = 15))) %>% # don't repeat yourself and call the same operation on both columns in one line
  c(list(sex = c("female", "male"))) %>% # prep a 3-element list for expand.grid to process
  expand.grid()

newdat2 <- newdat_func(df)

# fall back to traditional function format for multiple inputs
x_func <- function(model, newdata, link_func) {
  predict.glm(model, newdata = newdata, type="link", se=TRUE) %>% # obviously this only works on glm objects, you could add checks to be defensive
    keep(~ length(.) == nrow(newdata)) %>% # drop the third element that is length 1
    bind_cols() %>% # build data frame with a column from each list element
    mutate(low = fit - 1.96 * se.fit,
           high = fit + 1.96 * se.fit) %>%
    mutate_all(funs(link_func)) %>% # again don't repeat yourself
    bind_cols(newdata) %>% # bolt back on simulated predictors
    mutate(category = cut(age,
                          breaks = c(0, 69, 138, 206),
                          labels = c("0-69", "70-139", "139-206")),
           age = as.factor(age))
}

x2 <- x_func(m1, newdat2, link_func)

ggplot(data = x2, aes(x = weight)) + # always use spaces around '+' and '=', do ggplot(data = data) +
  geom_line(aes(y = fit, color = age)) +
  geom_ribbon(aes(ymin = low, ymax = high, fill = age), alpha = 0.1) + # okay is all on one line (<80 chars)
  facet_grid(category ~ sex) +
  labs(x = expression(bold("Weight")), # if a function goes beyond 1 line, split its args one per row
       y = expression(bold(y = "Infection Probability")),
       linetype = "Age (months)",
       colour = "Age (months)",
       fill = "Age (months)") +
  theme(panel.grid.major = element_blank(), # split args again
        panel.grid.minor = element_blank(),
        legend.position = "right",
        strip.text.x = element_text(face = "bold", size=12),
        strip.text.y = element_text(size=10),
        axis.text.y = element_text(size=10, face = "bold"),
        axis.text.x = element_text(size=10),
        axis.title = element_text(size=12),
        legend.text = element_text(size=10),
        legend.title = element_text(size=12, face="bold"))

Minor tidy-style adjustments everywhere are adding spaces around ~/=/+ signs and only one argument per line for multiy line calls like theme() and labs(). See more here https://style.tidyverse.org/

Obviously I went the last inch and wrapped the processing steps into functions. But I developed the sequence as an open pipe chain, adding a step and printing the result to console as I progressed. The speed of that iterative/dev workflow is why I love leveraging pipes, but I think it also makes the code easier to read. Now instead of multiple intermediate variables and repeated patterns your have two code chunks/functions that handle the two distinct phases of this model plotting problem


XMLGregorianCalendar to LocalDateTime From some generated code I get a
javax.xml.datatype.XMLGregorianCalendar and I want to convert it to a LocalDateTime without any zone-offset (UTC).
My current code accomplishes it, but I think it must be possible to acheaf the same result in a more elegant (and shorter) way.
    public static LocalDateTime xmlGregorianCalendar2LocalDateTime(XMLGregorianCalendar xgc) {
        // fix the time to UTC:
        final int offsetSeconds = xgc.toGregorianCalendar().toZonedDateTime().getOffset().getTotalSeconds();
        final LocalDateTime localDateTime = xgc.toGregorianCalendar().toZonedDateTime().toLocalDateTime(); // this simply ignores the timeZone
        return localDateTime.minusSeconds(offsetSeconds); // ajust according to the time-zone offet
    }

Something like :

xgc.toGregorianCalendar().toZonedDateTime().toLocalDateTime() ?

If you don't want to just rip off the zone information, but instead get the local time at UTC :

ZonedDateTime utcZoned = xgc.toGregorianCalendar().toZonedDateTime().withZoneSameInstant(ZoneId.of("UTC"));
LocalDateTime ldt = utcZoned.toLocalDateTime();

This answer is from the guy who has written the java.time specifications and implemented them btw : https://stackoverflow.com/questions/29767084/convert-between-localdate-and-xmlgregoriancalendar


Logging a message and the stack trace of caught exceptions Using Spring's JdbcTemplate to load a specific object by ID if exists, I have this code:

Person person = null;
try {
    person = jdbcTemplate.queryForObject(sql, new Object[]{ id }, new PersonMapper());
} catch (EmptyResultDataAccessException e) {
    LOGGER.warn("Could not find person with id " + id);
}

A Sonar rule (squid:S1166) complains about this piece of code, basically saying that logging a message is not enough, I should also log the exception. But in this case, I don't see the point. Adding the exception as a 2nd parameter to the logger will put a completely pointless stack trace in my logs that doesn't contain anything that I don't already know.

Would you agree that this is a false positive or am I missing something? Or is there another way of writing this code that complies better with Sonar or code quality analysis tools in general?

I agree it’s a false positive. Sonarqube is way too strict on this issue in my view and creates many false positives with that rule. Often, when catching a specific exception, we know exactly where it came from.

For this rule, I gave in and logged the exception on TRACE in addition to what I already logged at whatever different level. Sonarqube is satisfied, and the log stays clean unless you explicitly ask for verbose output.


Class that reads data from Excel I have a class that reads key-value pairs from an Excel file. I want to store the values in variables and access them from the class instance.

I did this by putting in the constructor the method that reads the Excel file. Is this OK? AFAIK it's not a good practice to put methods in constructors.

public class ExcelReader{
    private Map<String, String> map = null;
    private String firstname;
    private String lastname;

    public ExcelReader(){
        map = new HashMap<String, String>();
        getExcelData();
        firstname = map.get("Firstname");
        lastname= map.get("Lastname");
    }

    private void getExcelData() { //reads data from excel and puts it in map
       //...
       map.put(key,value);
    }

}

This is how I want to get the values:

ExcelReader reader = new ExcelReader();
reader.firstname;

You do have to be careful when calling methods from the constructor though. See the answer to the same question here.
But as long as you don't call overridable methods (and don't pass a reference to this) there is no problem.

The biggest issue I have with your method is the naming. By convention a method starting with getXXX() implies that it actually returns that XXX. If you would rename your method to something like initFromExcelData() for example it would be a lot clearer what the method does.

I personally would also do the assignments for first and last name inside this method. Especially if these are the only things you want to do with the excel. That way you can make the map local to that init method.


Solving Fizz Buzz using Java Problem:

Write a program that prints the numbers from 1 to 100. But for multiples of three print "Fizz" instead of the number and for the multiples of five print "Buzz". For numbers which are multiples of both three and five print "FizzBuzz".

Problem from here.
This is my code:
/*
 * Code by Clint
 */

public class FizzBuzz {
    public static void main(String[] args) {
        for (int numbers = 1; numbers <= 100; numbers++) {
            if (numbers % 3 == 0 && numbers % 5 == 0) {
                System.out.println("Fizz Buzz");
            } else if (numbers % 3 == 0) {
                System.out.println("Fizz");
            } else if (numbers % 5 == 0) {
                System.out.println("Buzz");
            } else {
                System.out.println(numbers);
            }
        }
    }
}

Here's the output:

The code looks perfect, except for the variable name numbers. This variable only holds a single number, therefore its name must be number instead.
Since the variable is only used in a very small scope, the names n or i may be acceptable as well. (Subject to personal preference.)


Basic login script for educational purposes I have created a login script to demonstrate the usage of sessions in PHP. The focus is not on security, databases or encryption. The idea is to exemplify how you can use sessions to protect pages from user access.

Key points to focus on:

Logical structure (return early, missing checks, error handling).
Compliance with PSR-1 and PSR-2.
Possible errors in the code.

<?php
  // Step 1. Start session management.
  session_start();

  // Step 2. Check if user is already logged in.
  if (isset($_SESSION['log']) && !empty($_SESSION['log'])) {
    // If true, redirect user to protected content
    header('Location: index.php');
  }

  // Step 3. Check if form was submitted through POST.
  if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    // Create array to store possible error messages.
    $errors = array();

    // Step 4. Check if username or password has been left empty by user.
    if (empty($_POST['user']) || empty($_POST['pass'])) {
      // If true, create an error message.
      $errors[] = 'Please check that you have filled in username and password.';
    }

    // Declare username and password.
    // These are only used to exemplify usage of sessions and should as standard be
    // stored in an encrypted form in a database (salt/hash).
    $user = 'admin';
    $pass = 'test';

    // Step 5. Check if username and password from form match with those declared.
    if ($_POST['user'] == $user && $_POST['pass'] == $pass) {
      // If true, create a session variable with assigned value true.
      $_SESSION['log'] = true;

      // Redirect user to protected content.
      header('Location: index.php');
    }
    else {
      // If username or password are incorrect, create an error message.
      $errors[] = 'Incorrect username or password.';
    }
  }
  else {
    // Error message if form has not been submitted through POST.
    $errors[] = 'Form has not been submitted through POST.';
  }
?>
<!DOCTYPE html>

    <meta charset="utf-8">
    <title>login.php</title>
    <link rel="stylesheet" href="style.css">

    <?php
      // Prints out error messages raised by login script
      if (count($errors) > 0) {
        foreach($errors as $e) {
          echo $e . '
';
        }
      }
    ?>
    <form method="post" action="login.php">
      <fieldset>
        <legend>Enter user information</legend>

            Username
            <input name="user" type="text">

            Password
            <input name="pass" type="password">

      </fieldset>
      <fieldset>
        <button type="submit" name="submit">Login</button>
      </fieldset>
    </form>

First, all of your indentation is half indentation: you two-spaces instead of four.

Most IDEs have proper spacing if you press TAB.

Naming:

You have variables need re-naming:

$_SESSION['log']: a log is a regular or systematic record of incidents or observations, not whether you've logged in, which has entirely different meaning, also the same applies to the following variables, log is a completely different thing than the state it represents: loggedIn or signedIn would be much better.
$pass, $_SESSION['pass'], $user and $_SESSION['user']: it's really not worth it to sacrifice four extra characters here and there, for the sake of simplification. add word and name to the end of those.

Security:

You SHOULD not be saving the password to a session variable, there are ways to break into sessions, and therefore, is an insecure way to save them longer than needed; use unset($_SESSION['password']) to remove them after processing.

Failed Globals:

if ($_SERVER['REQUEST_METHOD'] == 'POST') {
        $errors = array();

You use the $errors variable outside of the if statement, which is a no-no. Declare the variable up there with session_start().

You shouldn't be manually providing the name of the PHP form, but, rather using $_SERVER['PHP_SELF'] instead:

<form method="post">

into:

<form action=<?php echo htmlspecialchars($_SERVER['PHP_SELF']); ?> method="post">

  if (count($errors) > 0) {
    foreach($errors as $e) {
      echo $e . '
';
    }
  }

count($errors) > 0 can be simplified to: count($errors)
count($errors) can simplified to: $errors
$errors can simplified to: '' (Nothing)

Explicitly checking the length of $errors is redundant, when you have a for or foreach loop.

If there's \$0\$ elements, the foreach won't trigger.

foreach($errors as $e) {
    echo $e . '
';
}

In fact, in this instance, foreach is redundant in itself.

Using a neat function called implode, you can join an array together, without needing to iterate over its elements explicitly.

echo implode('
', $errors);

Actually,
 isn't really the best way to display multiple errors.

Consider using
 instead.

    <?php echo implode ('

', $errors); ?>

That certainly slimmed down, didn't it? When displaying content built from variables or arrays, make sure you consider how you choose to return them to the client.

        Username
        <input name="user" type="text">

        Password
        <input name="pass" type="password">

A few points to make about this block:

Why are you using
? It seems silly, and pointless. Even using
 would be pointless.
In the <input> you can use the attribute placeholder, which as the name suggests, lets you set a placeholder to show an example password, or the name of the field. Personally, I'd ditch the labels, and put username and password as placeholders. As @KevinBrown pointed out in the comments: screen readers need lables, and without them, it can cause issues, however, I'd still set placeholders.

    Username
    <input name="user" type="text">

    Password
    <input name="pass" type="password">

Into:

<input name="user" placeholder="username" type="text">
<input name="pass" placeholder="password" type="password">

You can individually space them with CSS (Recommended method),
 + inline, or
.

if (isset($_SESSION['log']) && !empty($_SESSION['log'])) {

$_SESSION['log'] is stored as a boolean, not a variable. Meaning, you can simply test it, without the need for functions like isset and !empty.

if ($_SESSION['loggedIn']){

Finally, the usage of $errors is a little superfluous.

$errors is used as an array, implying multiple errors will be added. However, only one $error should be added to the array, meaning a variable would be better.

Well, that would be the case, if you added a break, or return in the empty() check, but instead, you check if it's empty, add an error.

Then proceed to run through the rest of the code before comparing them. You already know if they're empty, they won't match the username and password given by the server, so why continue the script?

First, initialise $error (Note the lack of s) as a variable.
Then, break the function if they're empty.
Finally, return $error as a variable.

(I do realise this nullifies my previous point about displaying the errors, but, nonetheless, if you were to display errors, that's the way to do it.)


Eigenvector calculation involving three matrices Code performs well for small matrices, but gives memory error for large matrices of size 63000x93000, even when I am using server with memory of 64GB, space 100GB, swap 40GB.

# -*- coding: utf-8 -*-
from numpy import array, argmax, concatenate
import csv

def uread(Filename):
    UFILE = open(Filename, 'r')
    with UFILE:
        READER = csv.reader(UFILE)
        for i in READER:
            yield i

ufinal = uread('U.csv')
U = []
for k in ufinal:
    TEMP = []
    for j in k:
        TEMP.append(eval(j))
    U.append(TEMP)
    del TEMP
    del k

sfinal = uread('s.csv')
S = []
for s in sfinal:
    for t in s:
        S.append(eval(t))
    del t

EIGVALfinal = uread('EIGVAL.csv')
EIGVAL = []
for e in EIGVALfinal:
    for l in e:
        EIGVAL.append(eval(l))
    del l

EIGVECfinal = uread('EIGVEC.csv')
EIGVEC = []
for e in EIGVECfinal:
    for l in e:
        EIGVEC.append(eval(l))
    del l

NEW_U = []
NEW_EIGVAL = []
NEW_EIGVEC = []
for i in range(len(S)):
    if S[i] > 0.001:
        NEW_U.append(U[i])
        NEW_EIGVAL.append(EIGVAL[i])
        NEW_EIGVEC.append(EIGVEC[i])
del EIGVAL
del EIGVEC
# Select the first r columns of u corresponding to the r principle Eigenvector of
# MatrixForEigenvalues
TRANSPOSE_NEW_EIGVEC = array(NEW_EIGVEC).real.T   # Done for easy accesiblity of matrix
TRANSPOSE_MATRIX_U = array(NEW_U).real.T  # Done for easy accesiblity of matrix
FINAL_ARRAY_EIGVAL = []
FINAL_ARRAY_EIGVEC = []
FINAL_MATRIX_U = []
for i in range(len(array(NEW_EIGVAL).real)):
    j = argmax(array(NEW_EIGVAL).real)
    FINAL_ARRAY_EIGVAL.append((array(NEW_EIGVAL).real)[j])
    FINAL_ARRAY_EIGVEC.append(TRANSPOSE_NEW_EIGVEC[j])
    FINAL_MATRIX_U.append(TRANSPOSE_MATRIX_U[j])
    TRANSPOSE_NEW_EIGVEC = concatenate((TRANSPOSE_NEW_EIGVEC[:j], TRANSPOSE_NEW_EIGVEC[j+1:]))
    TRANSPOSE_MATRIX_U = concatenate((TRANSPOSE_MATRIX_U[:j], TRANSPOSE_MATRIX_U[j+1:]))
RESULT_MATRIX_U = array(FINAL_MATRIX_U).T     # This is the actual R
r_file = open('r.csv', 'w')
with r_file:
    WRITER = csv.writer(r_file)
    WRITER.writerows(RESULT_MATRIX_U)
print(RESULT_MATRIX_U)
del FINAL_MATRIX_U

Performance

Here's where all the action happens:

# Select the first r columns of u corresponding to the r principle Eigenvector of
# MatrixForEigenvalues
TRANSPOSE_NEW_EIGVEC = array(NEW_EIGVEC).real.T   # Done for easy accessibility of matrix
TRANSPOSE_MATRIX_U = array(NEW_U).real.T  # Done for easy accessibility of matrix
FINAL_ARRAY_EIGVAL = []
FINAL_ARRAY_EIGVEC = []
FINAL_MATRIX_U = []
for i in range(len(array(NEW_EIGVAL).real)):
    j = argmax(array(NEW_EIGVAL).real)
    FINAL_ARRAY_EIGVAL.append((array(NEW_EIGVAL).real)[j])
    FINAL_ARRAY_EIGVEC.append(TRANSPOSE_NEW_EIGVEC[j])
    FINAL_MATRIX_U.append(TRANSPOSE_MATRIX_U[j])
    TRANSPOSE_NEW_EIGVEC = concatenate((TRANSPOSE_NEW_EIGVEC[:j], TRANSPOSE_NEW_EIGVEC[j+1:]))
    TRANSPOSE_MATRIX_U = concatenate((TRANSPOSE_MATRIX_U[:j], TRANSPOSE_MATRIX_U[j+1:]))
RESULT_MATRIX_U = array(FINAL_MATRIX_U).T     # This is the actual R

The first thing I notice is multiple calls to array(), specifically for array(NEW_EIGVAL).real. This is inefficient; doing all the necessary conversions at the beginning and then referencing the variables would be much more efficient. Next, I get the strong suspician that j = argmax(array(NEW_EIGVAL).real) could be moved out of the loop, since array(NEW_EIGVAL).real isn't changing between iterations of the for loop. This in turn makes me strongly suspect that the line appending to FINAL_ARRAY_EIGVAL is buggy in some manner, since it's just getting the same element of NEW_EIGVALUE each time. Another thing I notice is that neither FINAL_ARRAY_EIGVAL nor FINAL_ARRAY_EIGVEC is referenced outside of the loop, making them functionally useless at the moment. If you plan to use them later, they can stay, but otherwise, they are just unnecessary cycles. Finally, concatenate is a very hacky and non idiomatic way to remove one element of an array; I would probably try to use delete() instead.

Since I'm unfamiliar with matrix eigenvector calculations, I unfortunately can't help you much more with optimization, unless I did some more research (and I probably would need some more information about your datasets). Anyway, I have some other suggestions to help you improve your Python coding style in general:

Descriptive variable names are useful

I recommend looking at PEP 8 variable naming conventions. It would improve the readability of your code.

In particular, instead of using names like U and K, I would recommend using more descriptive names. Also, instead of continually phasing names out and deleting them, it may be better to create temp variables to store the new value and then delete those, so the same variable has the same name throughout, but that's a matter of personal style (I'll address this more in the next section.)

Touch-ups

I notice a lot of del statements in your code. This can be avoided by taking advantage of the for-loop scope and not naming one use variables:

U = []
for k in uread('U.csv'):
    # ...

S = []
for s in uread('s.csv'):
    # ...

EIGVAL = []
for e in uread('EIGVAL.csv'):
    # ...

EIGVEC = []
for e in uread('EIGVEC.csv'):
    # ...

With statements can use an as clause to specify the name of the file so you don't have to define it on a separate line:

def uread(filename):
    with open(filename, 'r') as UFILE:
        READER = csv.reader(UFILE)
        for i in READER:
            yield i

List comprehensions can significantly simplify your code, and eliminate unnecessary (explicit) variable assignment:

Original:

for k in ufinal:
    TEMP = []
    for j in k:
        TEMP.append(eval(j))
    U.append(TEMP)
    del TEMP
    del k

Improved:

for k in ufinal:
    U.append([eval(i) for i in k])
    del k

Better yet, you can just turn most of your initial loops into list comprehensions:

Original:

U = []
for k in uread('U.csv'):
    U.append([eval(i) for i in k])

S = []
for s in uread('s.csv'):
    for t in s:
        S.append(eval(t))

EIGVAL = []
for e in uread('EIGVAL.csv'):
    for l in e:
        EIGVAL.append(eval(l))

EIGVEC = []
for e in uread('EIGVEC.csv'):
    for l in e:
        EIGVEC.append(eval(l))

Improved:

U = [[eval(j) for j in i] for i in uread('U.csv')]
S = [eval(j) for i in uread('s.csv') for j in i]
EIGVAL = [eval(j) for i in uread('EIGVAL.csv') for j in i]
EIGVEC = [eval(j) for i in uread('EIGVEC.csv') for j in i]

Final improved code

Warning: this is untested, so one or two things may be broken (but I've tried my very best to avoid that.) Since I don't know your datasets, I thought I'd leave testing up to you.

# -*- coding: utf-8 -*-
from numpy import array, argmax, delete
import csv

def uread(filename):
    with open(filename, 'r') as UFILE:
        READER = csv.reader(UFILE)
        for i in READER:
            yield i

U = [[eval(j) for j in i] for i in uread('U.csv')]
S = [eval(j) for i in uread('s.csv') for j in i]
EIGVAL = [eval(j) for i in uread('EIGVAL.csv') for j in i]
EIGVEC = [eval(j) for i in uread('EIGVEC.csv') for j in i]

NEW_U = []
NEW_EIGVAL = []
NEW_EIGVEC = []
for i in range(len(S)):
    if S[i] > 0.001:
        NEW_U.append(U[i])
        NEW_EIGVAL.append(EIGVAL[i])
        NEW_EIGVEC.append(EIGVEC[i])
del U
del EIGVAL
del EIGVEC
# Select the first r columns of u corresponding to the r principle Eigenvector of
# MatrixForEigenvalues
NEW_EIGVAL = array(NEW_EIGVAL).real
NEW_EIGVEC = array(NEW_EIGVEC)
TRANSPOSE_NEW_EIGVEC = NEW_EIGVEC.real.T   # Done for easy accessibility of matrix
TRANSPOSE_MATRIX_U = array(NEW_U).real.T  # Done for easy accessibility of matrix
FINAL_ARRAY_EIGVAL = []
FINAL_ARRAY_EIGVEC = []
FINAL_MATRIX_U = []
j = argmax(NEW_EIGVAL)
for i in range(len(NEW_EIGVAL)):
    FINAL_ARRAY_EIGVAL.append((NEW_EIGVAL)[j])
    FINAL_ARRAY_EIGVEC.append(TRANSPOSE_NEW_EIGVEC[j])
    FINAL_MATRIX_U.append(TRANSPOSE_MATRIX_U[j])
    TRANSPOSE_NEW_EIGVEC = delete(TRANSPOSE_NEW_EIGVEC, j)
    TRANSPOSE_MATRIX_U = delete(TRANSPOSE_MATRIX_U, j)
RESULT_MATRIX_U = array(FINAL_MATRIX_U).T     # This is the actual R
with open('r.csv', 'w') as r_file:
    WRITER = csv.writer(r_file)
    WRITER.writerows(RESULT_MATRIX_U)
print(RESULT_MATRIX_U)


Repeatedly multiplying digits until a single digit is obtained I've written code that solves a problem from codewars.com. My code passes all the tests on the codewars site. It's a simple enough problem to solve naïvely, but I would love to improve the structure.

The prompt for the problem is as follows:

  Write a function, persistence, that takes in a positive parameter num
  and returns its multiplicative persistence, which is the number of
  times you must multiply the digits in num until you reach a single
  digit.

  For example:

persistence(39) => 3  # Because 3*9 = 27, 2*7 = 14, 1*4=4
                      # and 4 has only one digit.

So my solution follows:

def get_digits(num, digits=[]):
    while num > 0:
        digits.append(num % 10)
        return get_digits(num / 10, digits)
    if num == 0:
        return digits

def multiply_all(digits, multiplier=1):
    while len(digits) > 0:
        multiplier = multiplier * digits.pop(0)
        return multiply_all(digits, multiplier)
    if len(digits) == 0:
        return multiplier

def persistence(num, count=0):
    while num >= 10:
        num = multiply_all(get_digits(num))
        count += 1
        return persistence(num, count)
    if num < 10:
        return count

I was thinking I might consolidate these three little recursive functions into one function containing all three... but I'd really like input from more seasoned programmers, so I very much appreciate you taking the time to make any suggestions.

Python 3.x bug

Did you know that your program outputs 1 for the number 39 if executed on Python 3?

This is because of the / division operator on this line:

return get_digits(num / 10, digits)

The meaning of / in Python 3.x was changed (PEP 238). Switching to // would be a quick fix.

Improving the solution

  I was thinking I might consolidate these three little recursive
  functions into one function containing all three..

What if we would get all the digits of a number by iterating over the string representation of an integer and converting every digit back to an integer - map(int, str(num)). In order to multiple the digits of a number we can use reduce() applying the operator.mul (multiplication operator) function:

from functools import reduce
from operator import mul

def persistence(number, count=0):
    # recursion base case - exit once the number is less than 10
    if number < 10:
        return count

    # get new number by multiplying digits of a number
    new_number = reduce(mul, map(int, str(number)))

    return persistence(new_number, count + 1)

Note that you don't actually need the while loop at all, since you are going recursive and have a num < 10 base case as a recursion "exit condition".

Note that you can also approach the "getting digits of a number" "mathematically" instead of converting the types back and forth:

from functools import reduce
from operator import mul

def get_digits(number):
    """Generates digits of a number."""
    while number:
        digit = number % 10

        yield digit

        # remove last digit from number (as integer)
        number //= 10

def persistence(number, count=0):
    if number < 10:
        return count

    # get new number by multiplying digits of a number
    new_number = reduce(mul, get_digits(number))

    return persistence(new_number, count + 1)

This would also be generally faster than the first version.


Checking if two strings are permutations (anagrams) in Python Practicing for my interview, I am working on some problems in Python.

This problem is to decide two strings are permutation (or anagrams) of each each other.

def is_permutation(first, second):
    if len(first) != len(second):
        return False
    first_arr = [0 for i in range(256)] # ASCII size
    second_arr = [0 for i in range(256)] # ASCII size
    for i in range(len(first)):
        first_arr[ord(first[i])] += 1
        second_arr[ord(second[i])] += 1
    if first_arr != second_arr:
        return False
    else:
        return True

I feel this code can be more efficient, but I cannot make it. Can anybody give me advice or tip so that I can improve please?

From Sumit Raj's 2015 article 10 Neat Python Tricks Beginners Should Know, which I would classify as "more pythonic":

from collections import Counter
def is_anagram(str1, str2):
    return Counter(str1) == Counter(str2)
>>> is_anagram('abcd','dbca')
True
>>> is_anagram('abcd','dbaa')
False


Validate input of month/year, otherwise set it to current month/year Is there a cleaner way of doing this? I'm using filter_input to first check for valid input, then using it again to assign the variable. Also, my condition in if/else seems quite long.

$optionsMonth = array('options'=>array('min_range'=>1, 'max_range'=>12));
$optionsYear = array('options'=>array('min_range'=>1900, 'max_range'=>9999));

if ((filter_input(INPUT_GET, 'month', FILTER_VALIDATE_INT, $optionsMonth))&&(filter_input(INPUT_GET, 'year', FILTER_VALIDATE_INT, $optionsYear))){
    $month = filter_input(INPUT_GET, 'month', FILTER_VALIDATE_INT, $optionsMonth );
    $year = filter_input(INPUT_GET, 'year', FILTER_VALIDATE_INT, $optionsYear);
} else {
    $datetime = new Datetime('NOW');
    $month = $datetime->format('m');
    $year = $datetime->format('Y');
}

You may be over thinking it.  It's actually very simple.

The only ambiguous thing is if the year is two digits.  In most cases a two digit year can be fixed like this.  I believe in your use the month and year will be all be future and nothing from the 1900s.

if (intval($optionsYear) < 100){$optionsYear  = '20' . $optionsYear;}

Then to finish up:

$date = strtotime("$optionsYear-$optionsMonth-13"); // yyyy-mm-dd
if ($date){  // test validity, will be false if invalid
  $month = date('m',$date);  // invalid, use today's month and year.
  $year = date('Y',$date);
}
else{
  $month = date('m');  // valid numbers entered
  $year = date('Y');
}

Because the day of the month is no an issue I used 13.  Not that it would but in dates the month and day can cause problems.  By using a day of 13 it cannot be confused with a month.  If a date were entered as 13/4/2015, it would not be difficult to understand 13 is a day and 4 is a month. Oct, Nov, Dec would still be ambiguous if entered as 10, 11, or 12.


Computed properties vs methods with shape classes I'd be interested in hearing what folk think are the pros and cons of the following two ways of writing classes like the following Circle, Square and Rectangle classes in Swift.

The first way has them extending a superclass (ShapeSC) with methods. The second has them implementing a protocol (ShapeP) with computed properties.
Both are tested with identical code, and give identical results.

Note: I've copied the code directly from a playground where it was working, so hopefully you'll find it error-free.

First way: subclasses extend a superclass and override "abstract" methods:

import UIKit
import Darwin

class ShapeSC {
   let π = M_PI
   var area: Double
   var perimeter: Double

   init() {
      area = 0
      perimeter = 0
   }

   func calcArea() -> Double {
      return 0
   }

   func calcPerimeter() -> Double {
      return 0
   }

class Circle: ShapeSC {
   var radius: Double

   init(radius: Double) {
      self.radius = radius
   }

   override func calcArea() -> Double {
      return π * radius * radius
   }

   override func calcPerimeter() -> Double {
      return π * 2 * radius
   }
}

class Square: ShapeSC {
   var side: Double

   init(side: Double) {
      self.side = side
   }

   override func calcArea() -> Double {
      return side * side
   }

   override func calcPerimeter() -> Double {
      return 4 * side
   }
}

class Rectangle: ShapeSC {
   var width: Double
   var height: Double

   init(width: Double, height: Double) {
      self.width = width
      self.height = height
   }

   override func calcArea() -> Double {
      return width * height
   }

   override func calcPerimeter() -> Double {
      return 2 * width + 2 * height
   }

}

let c1 = Circle(radius: 10)
c1.calcArea()
c1.calcPerimeter()

let s1 = Square(side: 10)
s1.calcArea()
s1.calcPerimeter()

let r1 = Rectangle(width: 5, height: 5)
r1.calcArea()
r1.calcPerimeter()

Second way: classes conform to a protocol and use computed properties:

import UIKit
import Darwin

protocol ShapeP {
   var area: Double { get }
   var perimeter: Double { get }
}

class Circle: ShapeP {
   let π = M_PI
   var radius: Double

   init(radius: Double) {
      self.radius = radius
   }

   var area: Double {
      return π * radius * radius
   }

   var perimeter: Double {
      return π * 2 * radius
   }
}

class Square: ShapeP {
   var side: Double

   init(side: Double) {
       self.side = side
   }

   var area: Double {
      return side * side
   }

   var perimeter: Double {
      return 4 * side
   }
}

class Rectangle: ShapeP {
   var height: Double
   var width: Double

   init(height: Double, width: Double) {
      self.height = height
      self.width = width
   }

   var area: Double {
      return width * height
   }

   var perimeter: Double {
      return 2 * width + 2 * height
   }
}

let c1 = Circle(radius: 10)
c1.area
c1.perimeter

let s1 = Square(side: 10)
s1.area
s1.perimeter

let r1 = Rectangle(height: 5, width: 5)
r1.area
r1.perimeter

Using a superclass

ShapeSC shouldn't be a superclass,
because it provides no useful implementation.
A shape with 0 area and perimeter doesn't make any sense.
Since there are no abstract classes in Swift,
ShapeSC cannot enforce sub-classes to override its methods.
So it's an outright dangerous approach,
as implementors may forget to replace the useless method with meaningful ones.

Btw, the π variable didn't belong in ShapeSC,
as it's only useful for Circle,
not by any other class in the hierarchy.

On the other hand,
it would have made sense to make Square extend Rectangle,
and inherit much of its implementation.

One small final note, I would write Rectangle's calcPerimeter method as:

   override func calcPerimeter() -> Double {
      return 2 * (width + height)
   }

Using a protocol

If you notice,
writing it using a protocol automatically cleaned up some odd elements of the superclass variant:

π is naturally defined in Circle, not somewhere else needlessly
No more 0-area 0-perimeter objects, by design

Even the code is shorter.
There are no obvious downsides either, which is in stark contrast with the superclass variant.

Using a mixed hierarchy

Consider a mixed hierarchy, using protocols and inheritance where they make sense.
For example Shape should clearly be a protocol,
but Rectangle can be a class that specialized cases like Square can inherit from.
As you explore the shape universe,
there might be other cases in the hierarchy where a protocol might make sense.


Logging the load balance I created a load balancing server at work and have to keep track of which server it adds the user to, so I wrote a little Ruby script that will do just that.

I'm looking for any critique of my work, can I do anything better at all.

Source(user information, directory information, and server information changed for security):

#!/local/usr/bin/ruby

require 'net/ssh'

@username = 'user'
@password =  nil
@host     = 'server_to_run_command_on'

def loadbalance_server_data(user)
  check = Net::SSH.start(@host, @username, :password => @password)
  cmd = "finduser #{user}" #<= Custom command that finds user info
  res = check.exec!(cmd)
  data = res
  write_data(data)
end

def write_data(data)
  File.open("path/to/log.txt", "a+"){ |s| s.puts(data) }
end

user_list = %w(user user user).each do |user|
  loadbalance_server_data(user)
end

Example of log:

Querying: servers..

Server usage report for user
        server_name:
        server_name:
        server_name:
        server_name:
        server_name:
        server_name:
        server_name:  dbus-daemon gvfsd thunderbird #<= These programs always run on this server no matter what server they are logged in on
        server_name:
        server_name:
        server_name:  dbus-daemon dbus-launch do_4000_clone_u execrulz gam_server gnome-screensav gnome-settings- gnome-vfs-daemo newphp openbox openbox-session qccm soffice soffice.bin tint2 umessage #<= The server the user is logged in on
        server_name:

Constants in UPPERCASE

@username = 'user'
@password =  nil
@host     = 'server_to_run_command_on'

should be constants, as well as:

LOGFILE = "path/to/log.txt"

Marking a variable as constant in Ruby just needs upper-casing its name.

Return value simplification

res = check.exec!(cmd)
data = res
write_data(data)

Is unnecessarily long just write:

write_data( check.exec!(cmd) )

Other then this minor remarks, the code looks as straightforward and readable as it can get.


Enumerate all items to find those that match a complex condition Currently, I have a big for..loop with many if..else inside the loop. I just wonder is there any design pattern or other approach to reduce the long code inside the for..loop. Thank in advance.

foreach ($workingTimes as $hour => $minutes) {
    foreach ($minutes as $shift) {
        $isBookable = $this->isBookabe($hour, $shift);

        if(!$isBookabe) {
            continue;
        }

        $isNotEnoughEmployee = $this->checkEmployees($hours, $shift);

        if(!$isNotEnoughEmployee) {
            continue;
        }

        $isNotEnoughResources = $this->checkResources($hour, $shift);

        if(!$isBookabe) {
            continue;
        }

        $isNotEnoughRooms = $this->checkRooms($hour, $shift);

        if(!$isNotEnoughRooms) {
            continue;
        }

        $formatted = sprintf("%s-%s", $hour, $shift);
        $timetable[$formatted] = 'Bookable';
    }
}

You can't reduce code you wrote because each condition has to be checked and you're already using variables to clarify your intent. What you can do is to move all that code out of your loop, reading that code in future you won't need to scroll over all those ifs:

foreach ($workingTimes as $hour => $minutes) {
    foreach ($minutes as $shift) {
        if (!canBookRoom($hour, $shift))
            continue;

        $formatted = sprintf("%s-%s", $hour, $shift);
        $timetable[$formatted] = 'Bookable';
    }
}

...

function canBookRoom($hour, $minute) {
    if (!isThereAvailableRoom($hour, $minute))
        return false;

    return areThereAvailableResources($hour, $minute);
}

function isThereAvailableRoom($hour, $minute) {
    if (!$this->isBookable($hours, $shift))
        return false;

    return $this->checkRooms($hour, $shift);
}

function areThereAvailableResources($hour, $minute) {
    if (!$this->checkEmployees($hour, $shift))
        return false;

    return $this->checkResources($hour, $shift);
}

What has been done

Reading your loop you're not really interested in all details about availability, there you just need to know if it's bookable or not. Pulling out a function for that makes your code easier to parse and your intent explicit. If/when you'll be interested in that details you may browse to canBookRoom() function.
canBookRoom() function itself explicitly split booking availability in two logic domains: room availability and resources availability. Each one has its own function.
areThereAvailableResources() unifies resources, from a higher level point of view both humans and materials are resources you can count.
Your local variables has been dropped, if each function name is self descriptive then you don't need them.

What should be done

Your loop is still doing many things: it searches for a bookable room inside a time range and if it finds an available time then it marks that range as bookable.

At least something should be done in a separate function. Here you have two options for your hypothetical buildTimetableForAllBookableRooms() function: refactor out function to fill timetable or add one function findAllBookableRooms() which returns all available time frames when there is a bookable room plus another function formatTimetableForBookableRoom() to perform booking, called in a foreach loop.

function findAllBookableRooms() {
    foreach ($workingTimes as $hour => $minutes) {
        foreach ($minutes as $shift) {
            if (canBookRoom($hour, $shift))
                yield sprintf("%s-%s", $hour, $shift);
        }
    }
}

function formatTimetableForBookableRoom() {
    foreach (findAllBookableRooms() as $bookableTimeFrame) {
        $timetable[$bookableTimeFrame] = 'Bookable';
    }
}

Here I used generator syntax but you may also return an array. Note that findAllBookableRooms() may also return unformatted value using yeld array($hour, $shift);.

Wherever it will be your $formatted variable has a poor name. It should describe what it contains otherwise you may simply drop it).
Another thing to do (but I don't have enough context) is to rename isBookable() and checkRooms() functions, from name alone it's not clear what they're actually doing and why/how they're different. Note I renamed isBookabe() to isBookable(), I suppose it's just a typo.


Returning the size of the largest clique in a graph This function is to return the size of the largest clique in a graph. When I run this code, I am running out of space at runtime. Also, it is insanely slow, so any speed tips would be appreciated.

int cliqueSize(graph &g, std::vector<VertexID> nodes, std::vector<EdgeID> edges)
{
    std::vector<std::vector<VertexID> > previous_cliques;

    // start with all the singular nodes...
    for (unsigned i = 0; i < nodes.size(); i++)
    {
        std::vector<VertexID> tmp;
        tmp.push_back(nodes[i]);
        previous_cliques.push_back(tmp);
    }

    for (unsigned clique_size = 1; clique_size < nodes.size(); clique_size++)
    {
        std::vector<std::vector<VertexID> > new_cliques;
        // go through all the nodes to try to add to the clique.
        for (unsigned i = 0; i < nodes.size(); i++)
        {
            // try to add node i to clique j.
            for (unsigned j = 0; j < previous_cliques.size(); j++)
            {
                // make sure node j is not already in the clique.
                if (std::find(previous_cliques[j].begin(), previous_cliques[j].end(), nodes[i]) == previous_cliques[j].end())
                {
                    std::vector<VertexID> potential_clique(previous_cliques[j]);
                    potential_clique.push_back(nodes[i]);

                    // isClique has no issues, just checks if the given graph is a clique...
                    if (isClique(g, potential_clique, edges))
                    {
                        new_cliques.push_back(potential_clique);
                    }
                }
            }
        }
        // no new cliques?
        if (new_cliques.size() == 0) { return clique_size; }
        else { previous_cliques = new_cliques; }
    }
    return nodes.size();
}

First of all, start passing things by const reference more.  You're passing both nodes and edges by value, which will make a copy.
Instead of looping through the containers by index, you should use iterators.
Pushing tmp into previous_cliques will make a copy.

The following may be faster:

std::vector<VertexID> tmp(1);
for (...) {
    tmp[0] = nodes[i];
    previous_cliques.push_back(tmp);
}

(If you're worried about scope, just wrap it in braces.)

Checking for emptiness with size() may be less efficient than with empty(); unlikely in the case of a vector, though (I think size() must be O(1)).
You're not showing us isClique, so it's hard to comment on that.  Make sure you're passing be const reference there.  Also consider not copying previous_cliques until you know you want to push it into new_cliques and instead handling nodes[i] as a special case (may be faster, may be slower).
If you're using C++11, the emplace functions are your friend.


Keep NumLock always on in Linux Intention

I intend to replace my own Bash script executing the following commands indefinitely in favor of this C++ compiled code, the reason being that the Bash script seemed very CPU ineffective.

It's not that I would think this solution is effective, nor perfect, but better than the one written in Bash for there is a great overhead over time.

The two commands are very simple:

/usr/bin/numlockx on
sleep 1

Environment

Linux Mint 18 with Cinnamon desktop environment.

Purpose

As I don't have a Num Lock indicator on my laptop, I want to ensure the Num Lock is always turned on.

xmodmap

I have tried using this utility, but it literally, excuse my wording, crippled my key mappings, which I have set up from inside Cinnamon DE.

Current C++ code

#include <cstdlib>  // in order to use system function
#include <unistd.h> // in order to use sleep  function

int main()
{

do {

    system("/usr/bin/numlockx on");

    sleep(1);

} while (true);

return 0;

}

This is compiled with g++ 5.x as follows:

g++ numlock-always-on.c -o "numlock-always-on"

I don't know much C++, but your indentation isn't great. To fix this you could try running your code through a C/C++ linter.

Your code is also having a much larger performance hit than if you told your Xorg to do this. Assuming that you only want this to work in Xorg, rather than in both the console and Xorg, you can use setxkbmap as described in the Arch wiki page Keyboard configuration in Xorg. setxkbmap comes with a couple of options to change how your keyboard works, listed under /usr/share/X11/xkb/rules/base.lst. From this list numpad:mac looks like what you want, and will change your keyboard to always use numbers whether numlock is on or not.
And so I would use:

setxkbmap -option numpad:mac


Build List from unshared items from two other Lists Ultimately, my goal is to take two lists and then generate a third consisting of items that the first two lists do NOT share. I am able to get the results I want using two foreach loops, but I am hoping to learn about some magical List tool that will do it more cleanly.

public List<string> removeMatchingItems(List<string> List1, List<string> List2)
{
    List<string> desiredList = new List<string>();

    foreach (string value in List1)
    {
        if (!List2.Contains(value))
        { desiredList.Add(value); }
    }
    foreach (string value in List2)
    {
        if (!List1.Contains(value))
        { desiredList.Add(value); }
    }
    return desiredList;
}

The operation is called the symmetric difference of List1 and List2.  You can write

return list1.Except(list2).Union(list2.Except(list1)).ToList<string>();


Find smallest subset prefixes (part 2) Here is my code for this problem. Any bugs, performance in terms of algorithm time complexity, code style advice are appreciated.

This is a continued discussion from (Find smallest subset prefixes) and since it is new code, I post a new thread.

Problem:

  Given a set of strings, return the smallest subset that contains the longest possible
  prefixes for every string.

  If the list is ['foo', 'foog', 'food', 'asdf'] return ['foo', 'asdf']

The return is foo since foo is prefix for foo (itself), prefix for foog and prefix for food (in other words, foo could "represent" longer string like foog and food). Output also contains asdf because it is not prefix for any other words in the input list, so output itself.

The empty set is not a correct answer because it does not contain the longest possible prefixes.

Source code:

from collections import defaultdict
class TrieNode:
    def __init__(self):
        self.children = defaultdict(TrieNode)
        self.isEnd = False
    def insert(self, word):
        node = self
        for w in word:
            node = node.children[w]
        node.isEnd = True
    def find_prefix(self, prefix, result):
        if self.isEnd:
            result.append(''.join(prefix[:]))
            return
        for w, child_node in self.children.items():
            prefix.append(w)
            child_node.find_prefix(prefix, result)
            prefix.pop(-1)

if __name__ == "__main__":
    words = ['foo', 'foog', 'food', 'asdf']
    root = TrieNode()
    for w in words:
        root.insert(w)
    result = []
    root.find_prefix([], result)
    print result

PEP 8

You still haven't followed PEP 8. Please:

Install (for example) Pylint.
You can run it as pylint filename.py.
If you use Vim you can install syntastic and every time you save it will run either flake8 or pylint.
There are alternatives for other text editors please install one, it will save you time in the long run.

def main

You have used if __name__ == "__main__": which is good. It is even considered more idiomatic (see this answer under "An Even Better Way") to do this:

def main():
    words = ('foo', 'foog', 'food', 'asdf')
    root = TrieNode()
    for w in words:
        root.insert(w)
    result = []
    root.find_prefix([], result)
    print result

if __name__ == "__main__":
    main()

Immutability

You don't mutate words. I would turn it into a tuple.


Change Data Capture using HIVE Could you please review my code and let me know if there is something wrong? I'm getting the expected results.

I'm trying to do change data capture using hive. We already have an existing CDC in DataStage, but we are trying to implement in HIVE now. Here is what I have done, please correct me if I'm doing something wrong.

Here is the source data:

Table name: month1

e_id, e_name, e_ad
11, john, singapore
22, jack, Melbourne
33, jessy, sydney
44, lampard, canada
55, pomer, perth

Table name2: month2

e_id, e_name, e_add
11, john, odisha
22, jack, Melbourne
44, lampard, canada
55, pomer, perth
66, rahul, mumbai

Here, the key field is the e_id, and value field is e_name and e_add:

select
    case    when cdc_codes ='Updates' Then month2s
        when cdc_codes = 'NoChange' then month1s
        when cdc_codes = 'New' then month2s
        when cdc_codes = 'Deletes' then month1s
        else 'Error' end as fin_cols
from (select
    case    when m1.e_id = m2.e_id and concat(m1.e_name,m1.e_add) = concat(m2.e_name,m2.e_add)     then  'NoChange'
            when m1.e_id = m2.e_id and  concat(m1.e_name,m1.e_add) <> concat(m2.e_name,m2.e_add) then  'Update'
            when m1.e_id is null then 'New'
            when m2.e_id is null then 'Deletes'
            else 'Error' end as cdc_codes,
        concat(m1.e_id,',',m1.e_name,',',m1.e_add) as month1s,
        concat(m2.e_id,',',m2.e_name,',',m2.e_add) as month2s
    from    month1 as m1
    full outer join
    month2 as m2
    on  m1.e_id = m2.e_id) as b1

Nitpicks

There are a few inconsistencies in your capitalization of SQL keywords. It's pretty minor. I see then and Then in some places, etc.

Naming

m1 and m2 don't make for very good aliases. Likewise e_id, e_name, e_add are not very good column names, though I realize those are probably example tables and your real schema is probably quite a bit different.

concat

Concatenation can make things confusing in SQL, in my opinion. I would encourage to see if there is a more natural way to compare data in your case statements. For example, instead of this:

case    when m1.e_id = m2.e_id and concat(m1.e_name,m1.e_add) = concat(m2.e_name,m2.e_add)

Why not try:

case    when m1.e_id = m2.e_id and m1.e_name = m2.e_name and m1.e_add = m2.e_add

This one is especially odd looking and would be easy to make a mistake:

concat(m1.e_id,',',m1.e_name,',',m1.e_add) as month1s,

CTE

I think using a common table expression instead of a subquery would make your query more elegant and easier to maintain. I also think a little more vertical space in your case statements to separate the conditions from the results helps the code read better:

with cte_cdc_codes as (
    select
    case    when m1.e_id = m2.e_id and concat(m1.e_name,m1.e_add) = concat(m2.e_name,m2.e_add)
                then  'NoChange'
            when m1.e_id = m2.e_id and  concat(m1.e_name,m1.e_add) <> concat(m2.e_name,m2.e_add)
                then  'Update'
            when m1.e_id is null
                then 'New'
            when m2.e_id is null
                then 'Deletes'
            else 'Error'
            end as cdc_codes,
        concat(m1.e_id,',',m1.e_name,',',m1.e_add) as month1s,
        concat(m2.e_id,',',m2.e_name,',',m2.e_add) as month2s
    from    month1 as m1
    full outer join
    month2 as m2
    on  m1.e_id = m2.e_id
)
select
    case    when cdc_codes ='Updates'
            Then month2s
        when cdc_codes = 'NoChange' then month1s
        when cdc_codes = 'New' then month2s
        when cdc_codes = 'Deletes' then month1s
        else 'Error' end as fin_cols
from cte_cdc_codes;


Checking whether any item in an array has a desired value for a key I want to check whether or not an object exists in an array given its key (key is not unique).

The object takes the form:

class item {
    key: string,
    value: (string|int|boolean),
    id: Guid
}

And this._items is an item array.

First approach is simply filtering the array and checking its length:

has(key) {
    return this._items.filter(item => item.key === key).length > 0;
}

Then a for loop that terminates when exists is true.

has(key) {
    let exists = false;
    for (let i = 0; i < this._items.length && !exists; i++) {
        exists = this._items[i].key === key;
    }

    return exists;
}

One more take on it, this.keys() returns an array of unique item keys:

has(key) {
    return this.keys().toString().match(new RegExp(key, 'ig')).length > 0;
}

The first one is cleaner / more readable but iterates over every item in the array. The second one doesn't read quite as well but only iterates over as many items as it needs to.

Until it needs to be optimized, is this just a preference/coding-style thing?

Easy to clean up your second suggestion, at which point, I would prefer it.

has(key) {
    for (let i = 0, len = this._items.length; i < len; i++) {
        if (key in this._items[i]) return true;
    }
    return false;
}

Note I determinethis._items.length only once with this approach vs. on every iteration.

You may consider returning index position of found item (or found item itself) here to make this more useful, because presumably you are checking for this key because you want to access it somehow. The caller would need to be aware of potential falsey behavior of 0 vs. false or similar.  You also would probably want to rename the function at this point to something more meaningful like getIndexHavingKey() or getItemHavingKey()

Of course, this is essentially what Array.find() and Array.findIndex() do.  You would just need to implement your key check check in callback.  That might look something like this:

var search = { key: 'someKey' };
// define filter callback
var keyExistsFilter = function (element) {
    // here 'this' is search object passed via find or findIndex
    return (this.key in element);
}

// get item from array based on filter
var itemWithKey = this._items.find(keyExistsFilter, search);

// get item index from array based on filter
var itemIndexWithKey = this._items.findIndex(keyExistsFilter, search);


Converting HEX values to Unicode characters I have a small bot for social network, which accepts message "unicode XXXX XXXX XXXX ...", where XXXX - HEX values which I should convert to Unicode characters. Initially command accepted only one HEX value, and it worked fine and fast. After I modified code to accept multiple values, process slowed down (1-2 seconds response vs. 4-5 seconds).

Here is my code:

def unic(msg):
    if type(msg) == list:
        msg.pop(0) # removing "unicode" word
        try:
            sym = '' # final result
            correct = [] # correct HEX values to be converted
            for code in msg:
                try:
                    chr(int(code, 16))
                    correct.append(code)
                except:
                    pass
            if correct != []:
                for code in correct:
                    sym = sym + chr(int(code, 16))
            elif correct == []:
                return c_s.get('incorrect') # returning error message from common strings list
            if sym != '':
                return sym
        except:
            return c_s.get('incorrect')

What should I change here to accelerate process? Any suggestions are welcome.

Only use try-except when you need to. Your outer try should be useless.
You should also perform chr(int(code, 16)) once, as there is no gain to running it twice, it just costs cycles.

Your output is horrible, if there is no valid input you go from explicitly silencing it to returning an error.
And if somehow you have an array of items, but they don't get added to sym you return None.
You need to pick how your function works. Either remove invalid characters, or raise errors.

Doing the above removes half your code. I'd also change except to catch certain errors, e.g. ValueError, otherwise your program is prone to mask bugs.

Strings are immutable and so sym = sym + chr(int(code, 16)) can take a very long amount of time.
Instead build a list and ''.join it.

Finally return c_s.get('incorrect') is a massive red flag, remove these and raise exceptions instead.

This can get you:

def unicode_(msg):
    new_msg = []
    for char in msg:
        try:
            char = chr(int(char, 16))
        except ValueError:
            char = '?'
        new_msg.append(char)
    return ''.join(new_msg)


More simplified way of creating page links dynamically? A little birdie suggested I bring this question here, so here it goes.

Well I have a working script (see below) but it seems quite clunky and redundant; in my defense I wrote this code many moons ago, but that's not the point. I was curious if anyone has an idea on a more efficient way of writing this code, with less loops and conditionals and, well, noise in the code.

Code in question:

private function pageLinks($num, $page = 1, $search = false, $ne = false) {
    $query = ($search) ? '&query='.$search : null;
    $by = (is_numeric($ne)) ? '&by='.$ne : null;
    $links = 'Page(s):1';
    $count = 1;
    $npp = $this->numPerPage;
    $buttons = 9;
    $half = 4;
    for($i = 1; $i <= $num; $i++) {
        if(($i%$npp) === 0) {
            $count++;
        }
    }
    if($count < $buttons) {
        for($i = 2; $i <= $count; $i++) {
            $links .= '' . $i . '';
        }
    } elseif($page <= ($half + 2)) {
        for($i = 2; $i <= $buttons; $i++) {
            $links .= '' . $i . '';
        }
        $links .= '...' . $count . '';
    } elseif($page <= ($count - ($half + 2))) {
        $links .= '...';
        for($i = $half; $i > 0; $i--) {
            $links .= '' . ($page - $i) . '';
        }
        $links .= '' . ($page - $i) . '';
        for($i = 1; $i <= $half; $i++) {
            $links .= '' . ($page + $i) . '';
        }
        $links .= '...' . $count . '';
    } else {
        $links .= '...';
        for($i = $buttons - 1; $i >= 0; $i--) {
            $links .= '' . ($count - $i) . '';
        }
    }
    return($links);
}

The method is called like so:

$links = $this->pageLinks($count, $page, $url, $ne);

And the variables are as such:

$count = total number of clients in database (int)
$page = current page to build from (int)
$url = the name or email for the search (String)
$ne = is for the search string either by name (1) or email (2) (int)

And the output is something like (as links):

Page(s):1 2 3 4 5 6 7 8 9...33

Or if you're in the middle (page 20):

Page(s):1...16 17 18 19 20 21 22 23 24...33

Now this isn't always called through a search function, hence the default values for $url and $ne, but that's not very important. My question is there a cleaner way to handle building of these links? Or am I stuck with this cluster of loops?

This:

$count = 1;
for($i = 1; $i <= $num; $i++) {
    if(($i%$npp) === 0) {
        $count++;
    }
}

is equivalent to

$count = floor($num / $npp) + 1;

As for the paging, I'd do it like this:

$from = $page - $half;
if ($from <= 2) $from = 2;
$to = $page + $half;
if ($to >= $count - 1) $to = $count - 1;

$extra = $query . $by;
$links = $this->pageLink(1, $extra);
if ($from > 2) $links .= "...";
for ($i = $from; $i <= $to; $i++)
    $links .= $this->pageLink($i, $extra);
if ($i < $count) $links .= "..."; // I use $i instead of $to because $i == $to + 1, so I save one addition
$links = $this->pageLink($count, $extra);

Here, you also need:

private function pageLink($num, $extra) {
    return '' . $num . '';
}

This was written by heart, so be weary of the possible bugs.


Linked list node implementation with maximum code reuse Currently as an exercise to practicing SOLID principles and basic data structures, I am trying to implement linked list type structures with as much code reuse as possible. Currently, I have:

package structures.linked;

public class SingleNode<T> {

    private T data;
    private SingleNode<T> next;

    public SingleNode(T data, SingleNode<T> next) {
        this.data = data;
        this.next = next;
    }

    public T getData() {
        return data;
    }

    public void setData(T data) {
        this.data = data;
    }

    public SingleNode<T> getNext() {
        return next;
    }

    public void setNext(SingleNode<T> next) {
        this.next = next;
    }

}

and...

package structures.linked;

public class DoubleNode<T> extends SingleNode<T> {

    private DoubleNode<T> prev;

    public DoubleNode(T data, DoubleNode<T> next, DoubleNode<T> prev) {
        super(data, next);
        this.prev = prev;
    }

    public DoubleNode<T> getPrev() {
        return prev;
    }

    public void setPrev(DoubleNode<T> prev) {
        this.prev = prev;
    }

    public DoubleNode<T> getNext() {
        return (DoubleNode<T>) super.getNext();
    }

    public void setNext(DoubleNode<T> next) {
        super.setNext(next);
    }
}

It seems to me that getNext() inside of DoubleNode<T> is a violation of Liskov's substitution principle. Is this the case? Is there a better way to implement this while still reusing the code in SingleNode<T> and without breaking SOLID principles?

Usually linked lists keep most methods as private and the actual nodes cannot be accessed by the public API.

If you start implementing the different methods for adding, removing, etc. you might have on singly and doubly linked lists, you will find the implementations are quite different.  So much so that I think it would be simpler to have two completely separate classes.  They could both implement the java.util.List interface.  I'm a huge fan of code reuse, but I don't think it's a good idea in this case.  Trying to share some code between singly and doubly linked list would end up making the code less readable.  This is just a guess since I have not actually tried implementing them.


Extracting data from an array of arrays I was browsing Stack Overflow recently and saw a PHP question about rearranging an array of arrays for printing. The initial data structure looked like this:

$array1 = array(
    array("type01",1995),
    array("type03",1996),
    array("type01",1995),
    array("type09",1998),
    array("type09",1995),
    array("type02",1997),
    array("type01",1995),
    array("type02",1999),
    array("type09",1995)
);

For printing, the number of occurrences of each pairing--type and year--had to be counted and a table of frequencies made with type as columns and the years as the rows, i.e.

Year    type01    type02    type03    type09
1995    3         0         0         2
1996    0         0         1         0
1997    0         1         0         0
1998    0         0         0         1
1999    0         1         0         0

(the actual table formatting was not critical)

Someone else answered the question, and used the following code:

$sorted = array();
$types = array();
array_walk($array1, function($v) use (&$sorted, &$types){
   if( !isset($sorted[$v[1]]) )
       $sorted[$v[1]] = array();
   $sorted[$v[1]][] = $v[0];
   $types[] = $v[0];
});

// filter types
$types = array_unique($types);
sort($types);

// sort year
ksort($sorted);

echo '----' . implode(' ,', $types) . "\r\n";
foreach($sorted as $year => $data)
{    echo $year . ' - ';
    $count = array_count_values($data);

   foreach($types as $type)
   {
      if( !isset($count[$type]))
      {
          echo '0,';
      } else {
          echo $count[$type].',';
      }
   }
   echo "\r\n";
}

I come from a Perl background, and I would use the following code to solve the problem:

$finalArray = array();
$years = array();

foreach ($array1 as $ix) {
    // $ix[0] is typeNN, $ix[1] is year.
    // index by type and then year.
    // the accumulator will count the number per type per year
    $finalArray[ $ix[0] ][ $ix[1] ]++;
    // keep an array of years
    $years[ $ix[1] ]++;
}

// sort arrays by key
ksort($finalArray);
ksort($years);

// print out the column headings
echo "Year\t" . implode("\t", array_keys($finalArray) ) . "\n";

// now add the data
foreach (array_keys($years) as $y) {
    echo $y;
    // for each type, check the frequency for the year
    // print '0' if the year does not exist
    foreach ($finalArray as $type => $year) {
        if (array_key_exists($y, $year)) {
            echo "\t" . $year[$y];
        } else {
            echo "\t0";
        }
    }
    echo "\n";
}

Granted, the two pieces of code format the table slightly differently, but is one approach "better"--more efficient, more in line with standard PHP coding practices--than the other?

I'm also aware that my code generates notices about undefined index/offsets because I'm used to Perl's autovivification of data structures. Is this something to worry about or not?

In terms of the original code I find it a bit more complex than it needs to be. Once you get to the point where code will exist for a long time or be used by other people a cleaner/simpler solution with similar attributes is often a better bet.

Anyway, I created a solution which is very similar to your own but there are a few differences that might be worth highlighting.

$years = array();
$types = array();
$ytx   = array();

foreach ($array1 as $item)
{
  list($type,$year) = $item;

  $years[$year] = $year;
  $types[$type] = $type;

  if ( !isset($ytx[$year][$type]) )
    $ytx[$year][$type]=0;
  $ytx[$year][$type]++;
}

ksort($years);
ksort($types);

I'm big on simplification and variables are relatively cheap. So, above there are arrays for the years and types as well as new variable collecting the count of year/type instances.

For sorting purposes the year and type keys are set to the year or type while the (dummy) value used is the same which allows simple foreach use later.

echo "
Year";
foreach ($types as $type)
  echo "\t$type";
echo "
\n";

foreach ($years as $year)
{
  echo "$year";
  foreach ($types as $type)
  {
    $val = 0;
    if ( isset($ytx[$year][$type]) )
      $val = $ytx[$year][$type];
    echo "\t$val";
  }
  echo "
\n";
}
echo "
\n";

The above is also very similar. However, you'll  notice I avoided the else statement by setting a variable to zero. There doesn't seem to be a need to be overly concerned for efficiency so, to me, avoiding the else is just a little bit cleaner -- the same reason I often avoid the brackets for single line if statements though some will not.

A few other general review notes:

Strive for meaningful variable names. I tend to us shorter names than many folks do these days (such as $ytx) but in the context of the code I find them meaningful.
I didn't find a need to use all kinds of array related functions. If we are walking through the array anyway things like array_count_values, implode, array_unique, array_key_exists and so on aren't necessarily adding much value. Someone maintaining the code in the future may not be very familiar with PHP's array functions.

In short, I'd prefer your solution to the original. However, if it was being created for inclusion on a project of mine there is no way I'd accept it with the undefined index notices.

Other than that, get into the habit of putting time into what I call simplification. The total project effort can often be significantly reduced with design choices and in terms of long term maintenance simpler code is faster to modify and less risky to work with. If you don't think about simplification during smaller efforts it is not going to be second nature when you get into larger projects.

Of course, your view of simplicity may not coincide with mine which is fine as long as you don't work for me. ;)