Recursive file copy function I created this function and I'm unsure if it is sufficient for "real world use", because all code snippets on the net I found for recursive copy functions used functions like

$hDir = opendir($sSrc)

which I don't think are necessary. That's why I like to have my func checked by some PHP professionals:

It recursively copies directories.

Params:

$sSrc  Path of directory, of which the content is to be copied
$sDst  Path of directory, into which the content of $sSrc is to be
copied. Non existing directories are (recursively) created.
$blnKeepPerm  Optional argument specifying if destination objects
must have the same permission as their source objects

Returns:  True only if ALL elements are copied, otherwise false.

function rcopy ($sSrc, $sDst, $blnKeepPerm = 0) {
  if (! is_dir($sSrc)) return false;
  $sSrc = (substr($sSrc,-1)=='/' ? substr($sSrc,0,-1) : $sSrc);
  $sDst = (substr($sDst,-1)=='/' ? substr($sDst,0,-1) : $sDst);
  $blnRetval = true;
  if (! is_dir($sDst)) {
    if ($blnKeepPerm) {
      if (! @mkdir($sDst, fileperms($sSrc), true)) return false;
    } else {
      if (! @mkdir($sDst), (0777-umask()), true) return false;
    }
  }
  foreach (scandir($sSrc) as $sObj) {
    if ($sObj != '.' && $sObj != '..') {
      if (is_dir("$sSrc/$sObj")) {
        // the function returns true only if copying ALL of the
        // files/dirs succeeded. To prevent overwriting of $blnRetVal
        // by a later rcopy-call, the return values here are ANDed:
        $blnRetval = $blnRetval & rcopy ("$sSrc/$sObj", "$sDst/$sObj", $blnKeepPerm);
      } else {
        if (@copy ("$sSrc/$sObj", "$sDst/$sObj")) {
          if ($blnKeepPerm) {
            @chmod ("$sDst/$sObj", fileperms("$sSrc/$sObj"));
          }
        } else  $blnRetval = false;
      }
    }
  }
  return $blnRetval;
}

What do you think? What could/should be made better?

1: You can replace:

$sSrc = (substr($sSrc,-1)=='/' ? substr($sSrc,0,-1) : $sSrc);
$sDst = (substr($sDst,-1)=='/' ? substr($sDst,0,-1) : $sDst);

with:

$sSrc = rtrim($sSrc,'/');
$sDst = rtrim($sDst,'/');

2: I would prefer the use of more normal variable names. There's no reason to use $blnKeepPerm if you can be clearer $copyPerms.

3: There is some repetition in your code.

4: The use of $blnRetval leads you to execute stuff after a failure.

In all there's really nothing wrong with this function, but it could be written more clearly. I'll give it a try:

function copyDir($sourceDir,$destDir,$copyPerms = FALSE)
{
  // cleanup
  $sourceDir = rtrim($sourceDir,'/');
  $destDir   = rtrim($destDir,'/');
  // protect
  if (!is_dir($sourceDir)) return FALSE;
  // check destination dir
  if (!is_dir($destDir))
  {
    // create directory
    $permissions = $copyPerms ? fileperms($sourceDir) : 0777-umask();
    if (!@mkdir($destDir,$permissions,TRUE)) return FALSE;
  }
  // copy files and directories recursievely
  foreach (scandir($sourceDir) as $fileName)
  {
    // do not copy current or parent directory
    if (!in_array($fileName,array('.','..')))
    {
      $sourcePath = "$sourceDir/$fileName";
      $destPath   = "$destDir/$fileName";
      // is it a file?
      if (!is_dir($sourcePath))
      {
        // oopy file
        if (!@copy($sourcePath,$destPath)) return FALSE;
        if ($copyPerms) && !@chmod($destPath,fileperms($sourcePath))) return FALSE;
      }
      // copy directory, here we recurse
      elseif (!copyDir($sourcePath,$destPath,$copyPerms)) return FALSE;
    }
  }
  return TRUE; // success!
}

I kept your code virtually intact. However, you can find other solutions online:

https://stackoverflow.com/questions/5707806/recursive-copy-of-directory

https://stackoverflow.com/questions/2050859/copy-entire-contents-of-a-directory-to-another-using-php

and many more.


DAO Layer Code Review Please review my following code for DAO layer.

public List<Channel> getListOfChannels() throws DataAccessException {
        // will have the channel in form of List
        // to hold the channels list
        List<Channel> listChannels = null;
        Connection conn = null;
        Statement statement = null;
        ResultSet resultSet = null;

        try {
            // get the db connection from pool
                // this is DBCP lib on top doing this
            conn = ManageConnections.getConnection();
            statement = conn.createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE,
                    ResultSet.CONCUR_READ_ONLY);

            final String QUERY_STRING = "Select * from channel";
            resultSet = statement.executeQuery(QUERY_STRING);

            // is this good practice to put this ?
            if (isResultSetEmptyOrNull(resultSet)) {
                throw new DataAccessException(
                        "No more data of Channels found from db");
            }

            Channel channel = null;

            listChannels = new ArrayList<Channel>();

            while (resultSet.next()) {
                // get the object from the result set and

                listChannels.add(channel);
            }

            log.debug("getListOfChannels Got the list of channels "
                    + listChannels);

                } catch (SQLException ex) {
            throw new DataAccessException(SQL_EXCEPTION + ex, ex);
        } catch (DBConnectionException ex) {
            // re thrown, no logging
            throw new DataAccessException(ex);
        } catch (Exception ex) {
            // Generic exception thrown , now throw Custom Exceptionn
            throw new DataAccessException(GENERIC_EXCEPTION + ex, ex);
        } finally {
            try {
                if (conn != null) {
                    // this is DBCP lib on top doing this
                    ManageConnections.close(conn);
                }
                if (statement != null) {
                    statement.close();
                }
                if (resultSet != null) {
                    resultSet.close();
                }
            } catch (SQLException ex) {
                throw new DataAccessException(
                        "Exception while closing resource" + ex, ex);
            } catch (DBConnectionException ex) {
                throw new DataAccessException(
                        "Exception while closing resource" + ex, ex);
            }
        }

        return listChannels;

There are a couple of improvements that can be made here:

a) your QUERY_STRING is magic string. Move it to a different place,
because it make your code difficult to maintain
user linked list
instead of array list. It has O(1) on insertion in worst case,
whereas array list has O(n)
do not rethrow exceptions in finally
block. You will lose all details of your original exception
every time when you concatenate strings with "+" you make too many
operations with memory, because strings are immutable. use *.format
instead


A function to print a character number of times and in number of lines My task was:

Write a function that takes three arguments: a character and two integers. The
character is to be printed. The first integer specifies the number of times that the
character is to be printed on a line, and the second integer specifies the number
of lines that are to be printed. Write a program that makes use of this function.

And my code is: (i know i did not add different scenarios like if the first char is number etc, but please let me know if it looks ok)

#include <stdio.h>
void printing_char (char ch, int numberOfChars, int numberOfLines);

int main (void)
{
    char userChar;
    int lines,times;
    printf ("please enter a character, number of times in a line, and number of lines:\n");
    while ((scanf ("%c%d%d", &userChar, &times, &lines)) == 3)
    {
        printing_char (userChar, times, lines);
    }
    return 0;
}

void printing_char (char ch, int numberOfChars, int numberOfLines)
{
    int x;
    int y = 0;

    while (++y <= numberOfLines)
    {
            for (x = 0; x<numberOfChars; x++)
            {
                printf ("%c", ch);
            }
            printf ("\n");
    }
}

Only a couple of things:

int lines,times; - one declaration per line, please!
scanf("%c%d%d", &userChar, &times, &lines) - where does one number end and the next start?. You need to include some terminating characters, such as ,

printf("please enter a character, number of times in a line, and number of lines, separated by a comma:\n");
while ((scanf("%c,%d,%d", &userChar, &times, &lines)) == 3)

An expression such as while (++y <= numberOfLines) is confusing, what value does y have at each step? It'd be better to rewrite this as a for loop

for ( y = 0; y < numberOfLines; y++ )


Sorting an associative array alphabetically I am using a foreach loop to sort an associative array alphabetically. I would like to know if there is a more proper and/or efficient way of doing it.

The array:

Array
(
    [gr_c] => Array
        (
            [f] => 'value...'
            [a] => 'value...'
            [d] => 'value...'
            [m] => 'value...'
            [c] => 'value...'
            [t] => 'value...'
        )

    [gr_a] => Array
        (
            [h] => 'value...'
            [e] => 'value...'
            [m] => 'value...'
            [a] => 'value...'
            [o] => 'value...'
            [i] => 'value...'
            [c] => 'value...'
            [t] => 'value...'
            [b] => 'value...'
        )

    [gr_b] => Array
        (
            [h] => 'value...'
            [d] => 'value...'
        )
)

became:

Array
(
    [gr_c] => Array
        (
            [a] => 'value...'
            [c] => 'value...'
            [d] => 'value...'
            [f] => 'value...'
            [m] => 'value...'
            [t] => 'value...'
        )

    [gr_a] => Array
        (
            [a] => 'value...'
            [b] => 'value...'
            [c] => 'value...'
            [e] => 'value...'
            [h] => 'value...'
            [i] => 'value...'
            [m] => 'value...'
            [o] => 'value...'
            [t] => 'value...'
        )

    [gr_b] => Array
        (
            [d] => 'value...'
            [h] => 'value...'
        )
)

used snippet:

foreach ($array_name as $key => $value) {
    ksort($array_name[$key]);
}

That snippet of 3 lines you used,
is fine as it is, nothing really wrong with it.
It's proper, efficient, natural, easy to understand.

There is just one thing I'd pick on,
is that the $value variable in the foreach expression is not used.
Another way to achieve the same thing without unused variables is to use & to pass the loop variable by reference:

foreach ($array_name as &$arr) {
    ksort($arr);
}

This has the advantage that the loop index variable $key is now gone too,
we're working with the data that really matters, which is the $arr to sort.


Classifying connectivity status based on online and connected status of multiple nodes I need to determine the status of a parent object based on 2 variables of each of its children. I came up with a working solution, but this includes a nested "if-else if-else". Needless to say, it doesn't look very elegant.

I was wondering if there is a way to simplify this. I have muddled around with some map/reduce code, but did not get to anything that is more elegant than the code below.

const parent = {
  children: [{
      connected: true,
      online: true

    },
    {
      connected: true,
      online: true
    }
  ]
}

// all online & all connected  => connected
// all online & some connected => partially disconnected
// all online & none connected => disconnected
// some online => partially offline
// none online => offline

const onlineArr = parent.children.map(c => c.online);
const connectedArr = parent.children.map(c => c.connected);
let status;

if (!onlineArr.includes(true)) {
  status = 'Offline';
} else if (!onlineArr.includes(false)) {
  if (!connectedArr.includes(true)) {
    status = 'Disconnected';
  } else if (!connectedArr.includes(false)) {
    status = 'Connected';
  } else {
    status = 'Partially disconnected';
  }
} else {
  status = 'Partially offline';
}

console.log(status);

As a function
When you write code, even as an example, always write it as a function. A function is returnable, and thus can be written differently than non returnable flat execution.
Comment code mismatch

"Comments are just lies in waiting."
...by unknown guru.

Your comments do not match the code. The comments specify lowercase status, your code capitalists the status. Which is correct is anyone's guess. I will assume the comments are correct and that the status is formatted upon display. (It makes the solution simpler) Changed my mind and will assume the code has been tested and is correct.
Inefficiency === Inelegance
The nested statements are not inelegant (in a function it would not need any else statements), its the two Array.map and three Array.includes that are very inefficient for the task at hand, which to me is ugly inelegance.
Solution
It is the number of online, connected children that you need to know.
If the number of online children

is the same as the number of children then all are online.
is the less than as the number of children and not zero then some are online.
is zero then none are online

The same applies for connected children.
Thus count the two types and use the counts to return the status, as follows
function connectionStatus(clients) {
    const count = clients.length;
    var onC = 0, conC = 0;
    for (const {connected, online} of clients) {
        conC += connected;
        onC += online;
    }
    if (onC === count) {
        if (conC === count) { return "Connected" }
        return conC ? "Partially disconnected" : "Disconnected";
    }
    return onC ? "Partially offline" : "Offline";
}

connectionStatus(parent.children);


Data Explorer query that makes bar graphs I wrote a query for SEDE that I think is pretty cool. It uses the currently available graphing capabilities (scatter plots) to create a bar graph. I have used it as the subject for a self-answer on MSE.

This query finds potential "recommendation questions" based on the words in the title.

The graph looks like this when I ran the query for Super User:

Side note: they are planning to expand the graphing capabilities in the future. Right now they're very limited, so I had to work within the limitations of the system. I'm also aware that my bar graph looks a lot different from the "average" bar graph, but I'm ok with it.

My code right now isn't very long or complex because I'm just starting to teach myself SQL (mostly to make SEDE queries).

The main things I want to know are:

Can I refactor it so that the list of when like then statements occurs only once? (Preferably without complicating the code too much.)

Right now, the first time, it assigns the number for the X axis. The second time, it orders it so that all of the points for each bar are continuous.
Are there any optimizations I can make? So far, I've been using top # statements to keep things from taking too long.

I have prettied up my code for review, but most of the comments are from when I posted it to MSE. Because of the large number of "non-programmers" there (and people without SQL experience), I included a lot of details that are superfluous if you know the basics of SQL (ignore it if you want).

--This limits the results to the first 500
--It's sorting by the bar number,
-- so the query may get cut off like this one,
-- which has 4 bars instead of 6.
select top 500

--Here is the X axis.
--Each number is the bar that the result will be put in.
(case
    when lower(title) like '%open source%' then 0
    when lower(title) like '%recommend%'   then 1
    when lower(title) like '%free%'        then 2
    when lower(title) like '%good%'        then 2
    when lower(title) like '%best%'        then 2
    when lower(title) like '%better%'      then 2
    when lower(title) like '%any%'         then 2
    when lower(title) like '%what%should%' then 3
    when lower(title) like '%which%should%'then 3
    when lower(title) like '%is there%'    then 3
    when lower(title) like '%looking for%' then 4
    else 5 end
)

--This is the first 25 characters of the title
,left(title,25)

--Here is the Y axis. It sorts by the X axis,
-- and then just numbers upwards
,row_number() over (order by
  (case
    when lower(title) like '%open source%' then 0
    when lower(title) like '%recommend%'   then 1
    when lower(title) like '%free%'        then 2
    when lower(title) like '%good%'        then 2
    when lower(title) like '%best%'        then 2
    when lower(title) like '%better%'      then 2
    when lower(title) like '%any%'         then 2
    when lower(title) like '%what%should%' then 3
    when lower(title) like '%which%should%'then 3
    when lower(title) like '%is there%'    then 3
    when lower(title) like '%looking for%' then 4
    else 5 end
  )
) as Number

--This query searches posts
from posts

--This provides stricter criteria for the graph as a whole
where (title like '%library%'
    or title like '%tool%'
    or title like '%book%'
    or title like '%tutorial%')
and closeddate   is null
and deletiondate is null ;

I'll answer your main questions:

Can I refactor it so that the list of when like then statements occurs only once? (Preferably without complicating the code too much.)

Yes, but not at the expense of complicating the code. In SEDE we unfortunately don't have the permission needed to create user-defined functions.

I have introduced two concepts. One is a string with the comma separated words to select the posts your interested in. This string can then be STRING_SPLIT and each value gets transformed in the right hand side of the LIKE expression.

The other concept is a temporary table that holds the phrase and a bar value. That allows for a join on Posts.Title with a LIKE expression to add the bar value column to a title from the posts row.

By introducing common table expressions the functional steps to obtain the right sets can be broken up, making the final projection a breeze.

Are there any optimizations I can make? So far, I've been using top # statements to keep things from taking too long.

On Super User this doesn't run too long, the execution plan looks like this:

and I can't think of any optimizations within the limited options we have within SEDE. I did add an extra where clause when selecting from the Posts table to make sure only questions are queried, as they only have titles.

Without further ado here is my optimized version of the query

-- selection of titles, comma separated
declare @selection nvarchar(250) = 'library,tool,book,tutorial'

-- table to hold the phrases to go in each bar
create table #titles (phrase nvarchar(50), value int);
-- add the phrase and which bar
insert into #titles values ('open source',0)
insert into #titles values ('recommend',1)
insert into #titles values ('free',2)
insert into #titles values ('good',2)
insert into #titles values ('best',2)
insert into #titles values ('better',2)
insert into #titles values ('any',2)
insert into #titles values ('what%should',3)
insert into #titles values ('which%should', 3)
insert into #titles values ('is there',3)
insert into #titles values ('looking for',4)

-- the Common Table Expressions to make our life easier
;with
-- the questions to be considered from the posts table
questions as (
  select distinct
         id
       , title
  from posts
  inner join string_split(@selection,',') -- gives us a table
     -- do a case insensitive like with a proper collate
     on title like concat('%',value ,'%') collate SQL_Latin1_General_CP1_CI_AS
  where posttypeid = 1 -- only questions (this reduces the set drastically)
  and closeddate is null
  and deletiondate is null
),
-- for each qualifying row add the bars to with it
titlecategories as (
  select distinct id
       , title
       , coalesce(
           value,
           (select max(value) + 1 from #titles) -- title has no matching value
         ) value
  from questions q
  left outer join #titles t
    -- do a case insensitive like
    on title like concat('%', t.phrase ,'%') collate SQL_Latin1_General_CP1_CI_AS
),
-- make sure each post only appears once
categories as (
  select id
       , min(title) title
       , min(value) value -- lower is better is idea ....
  from titlecategories
  group by id
)

-- final projection, partition and order by over value
select top 1500
       value
     , left(title, 25) title
     , ROW_NUMBER() OVER (
        PARTITION BY(value)
        ORDER BY (value)
       ) AS Number
from categories
order by value
       , Number

I did notice that if I leave out the top statement, I get some glitches in the graph but I'm unable to pinpoint what is causing that. For now I write that off on the used library on the Stack Exchange Data Explorer.


Simple Java Neural Network I've written a toy neural network in Java. I ran it several million times with the same outputs with only the randomized weights changing from run to run. The average of all of the outputs is not 0.5, as I would have expected. The code is in this Github Repository.

The Layer class:

public class Layer extends AbstractLayer {

    private double[][] weights;

    public Layer(int neurons, int prevLayerNeurons) {
        super(neurons);
        weights = new double[neurons][prevLayerNeurons + 1];
        randomize(weights);
    }

    protected void randomize(double[][] x) {
        for (int j = 0; j < x.length; j++) {
            for (int i = 0; i < x[j].length; i++) {
                weights[j][i] = Math.random();
            }
        }
    }

    @Override
    public double[] compute(double inputs[]) {
        Preconditions.checkArgument(inputs.length == weights[0].length - 1, "incorrect number of inputs");
        double[] ret = new double[neurons];
        for (int i = 0; i < neurons; i++) {
            double acc = 0;
            for (int j = 0; j < inputs.length; j++) {
                acc += inputs[j] * weights[i][j];
            }
            acc -= weights[i][weights[i].length - 1];
            acc = sigmoid(acc);
            ret[i] = acc;
        }
        return ret;
    }

    private double sigmoid(double x) {
        return 1 / (1 + Math.exp(-x));
    }
}

And the Network class:

public class Network {

    private List<AbstractLayer> layers;

    public Network(int[] neuronCounts) {
        layers = new LinkedList<AbstractLayer>();
        layers.add(new InputLayer(neuronCounts[0]));
        for (int i = 1; i < neuronCounts.length; i++) {
            layers.add(new Layer(neuronCounts[i], neuronCounts[i - 1]));
        }
    }

    public boolean[] run(double[] inputs) {
        double[] tmps = inputs;
        for (int i = 0; i < layers.size(); i++) {
            tmps = layers.get(i).compute(tmps);
        }
        boolean[] rets = new boolean[tmps.length];
        for (int i = 0; i < rets.length; i++) {
            rets[i] = tmps[i] > 0.5;
        }
        return rets;
    }

    public AbstractLayer getLayer(int i) {
        return layers.get(i);
    }

}

These are the most relevant classes, but there are several others in the repository.

As it stands now, the class Bootstrap will run a 3-layer network with a fixed arrangement and set of inputs.

I'd appreciate any comments on the structure of the program, and on the correctness (or incorrectness) of the output.

Finding the average output
Let's figure out what your average output should be.  Your neural network has 3 inputs in the first layer, 2 nodes in the second layer, and one output.  Each weight is randomized to a value from 0..1, so call it 0.5 on average.
The inputs you use in the program are: 1, 0, 1.  On each layer, you also have a "bias" input of -1.  So with average weights of 0.5, the the input layer will do the following:
inputs = 1, 0, 1, -1
output = sigmoid(1*0.5 + 0*0.5 + 1*0.5 - 1*0.5) = sigmoid(0.5) = 0.62

The second layer has two nodes, each with average input 0.62.  It will do:
inputs = 0.62, 0.62, -1
output = sigmoid(0.62*0.5 + 0.62*0.5 - 1*0.5) = sigmoid(0.12) = 0.53

So your average output should be 0.53.  I modified your program to sum the outputs and found that this was close (actual was 0.528).  Now as for the percentage of time the output is above 0.5, that depends on the distribution of the output, and I don't know that it is easy to compute by hand.  But your program shows through experimentation that the answer is roughly 60% of the time.
Random weights
I think that your choice of random weights from the range 0..1 is the source of your confusion.  If you were to choose random weights in the range -1..1 like this:
weights[j][i] = Math.random()*2 - 1.0;

then your output would be 0.5 on average and the percentage of outputs greater than 0.5 would be 50% (I modified your program to verify this).  Perhaps that is what you were expecting.
As far as your neural net code goes, it appears to be correct as far as I can tell.


Loading either all shops or a specifically named shop from a DataTable public  delegate DataTable loadDataTable();

DataTable shops = (cmbShop.Text == "All Shops") ?
      new loadDataTable(() =>
       {
       Program.con.GET_Table_From_DataBase("sh", "select * from shops ");
       return Program.con.dst.Tables["sh"];
       }
      ).Invoke()
     :
       new loadDataTable(() =>
       {
        Program.con.GET_Table_From_DataBase("sh", "select * from shops where shopname='" +  cmbShop.Text + "' ");
        return Program.con.dst.Tables["sh"];
       }
  ).Invoke();

I am just setting the value of DataTable shops here.

I am learning about lambda expressions, so just for learning purposes, I want to know if this code can be shortened while using lambda expressions.

You don't need any lambda expressions at all.

You can write

if (cmbShop.Text == "All Shops")
    Program.con.GET_Table_From_DataBase("sh", "select * from shops ");
else
    Program.con.GET_Table_From_DataBase("sh", "select * from shops where shopname='" +  cmbShop.Text + "' ");

DataTable shops = Program.con.dst.Tables["sh"];


Backup Linux system to web First I create a .tar of the root directory with all the file permissions saved and excluding all irrelevant directories (are these the right folders to be excluding?).

sudo tar -cvpzf /HDD/systemBackup/backup.tar.gz --one-file-system --exclude=/HDD --exclude=/NAS --exclude=/proc exclude=/tmp --exclude=/mnt --exclude=/dev --exclude=/sys /

Then the .tar is split into smaller files of 100mb for easier upload efficiency.

split -b 100MB /HDD/systemBackup/backup.tar.gz "/HDD/systemBackup/$date-"

Then I upload the parts to the Internet.

I am looking to optimise ease of restoration and speed efficiency (ignoring (to some extent) the speed of the upload to the Internet).

Will this method also work on both a Raspberry Pi and an Ubuntu system?

Is this a good method of backing up my Linux system to the web?

When it comes to system backups, I think it's more important to know what you include than to know what you exclude.
So instead of specifying what you exclude,
I think it's better to think about what you need to include.
It might be a shorter list,
and less noisy without all the --exclude flags.

Your current method is wasteful: the backup takes twice the size it needs,
once for the .tar.gz, and once for the split version. You could pipe the output of the tar command to split to save some space, for example:

sudo tar -cvpz --one-file-system /usr /etc /home ... | split -b 100MB - "/HDD/systemBackup/$date-"

Most importantly, as @tarleb also said, make sure to test restoring from backup. An all too common mistake is to not test restoring, and one day when you really need it, your backups turn out to be useless.

  Will this method also work on both a Raspberry Pi and an Ubuntu system?

The list of directories you want to backup might be a little bit different.
If you have to inspect both systems and see.
Other than that, the same technique should work, yes.

  Is this a good method of backing up my Linux system to the web?

This is one way to do it.
It's a full backup.
Its advantage is that it's simple.
Its drawback is that it wastes a lot of space,
because much of the data in each backup for different $date values will be duplicated.
And transferring over the network will be relatively slow.

An alternative would be using incremental backup,
which would save disk space and network bandwidth,
at the expense of considerably more complex backup and restore procedure.

One more way would be to rsync the interesting directories,
and take create a timestamped archive at the other side.
That would save disk space locally, save network bandwidth,
and still be relatively simple.


Message queue for cross thread sync public class WmSync : IDisposable
{
    public WmSync()
    {
        var sync = new Control();
        sync.Handle.GetHashCode(); // ensure handle is created
        _sync = sync;
        Thread.MemoryBarrier();
    }

    public void Dispose()
    {
        _sync.Dispose();
    }

    public void BeginInvoke(Action action)
    {
        _sync.BeginInvoke(action);
    }

    public void Invoke(Action action)
    {
        _sync.Invoke(action);
    }

    public bool InvokeRequired
    {
        get { return _sync.InvokeRequired; }
    }

    private readonly Control _sync;
}

The idea is to create this object in the main thread and use it from background threads to execute pieces of code in the main thread.

For example:

// somewhere in the very beginning of the program, in the main thread
static WmSync SyncQueue = new WmSync();

// somewhere in a background thread
SyncQueue.Invoke(
    () =>
    {
        if (_settingsForm != null)
            _settingsForm.Show();
    });

This is just an artificial sample, but it should give you idea. Is this code thread safe?

Thread.MemoryBarrier()
What do you expect the call to Thread.MemoryBarrier() will help your code ?
What does Thread.MemoryBarrier do ? It prevents that the compiler and the hardware subtly transform a program’s memory operations.
So basically for your code it ensures that then following instructions will be executed in the order they are written.

var sync = new Control();
sync.Handle.GetHashCode(); // ensure handle is created
_sync = sync;

Thread safety
To answer "Is this code thread safe ?", you first need to set your definition of thread safety aka what does thread safety means for you. See also what-is-this-thing-you-call-thread-safe
General

the code is lacking a EndInvoke() which should be integrated.

you are misusing a Control to access the main (ui) thread of an application.

wouldn't a SynchronizationContext be a better fit here ?
See also: https://stackoverflow.com/a/20300351/2655508


Arduino code and PING))) ultrasonic rangefinder Context
I've started digging into Arduino and different type of sensors which is fun, but I'm worried that that my coding is too explicit. I've lots of experience for coding corporate programs, though it is not in C/C++. I don't consider myself a beginner, but I think something is wrong.

Blocks
I'd place my code in this section.  It is using PING))) sensor to detect how far an object is based on the travel time of an emitted ultra-sonic signal. This sketch is taken from the TinkerCad website which can be found here: tinkercad.com.
/*
  Ping))) Sensor

  This sketch reads a PING))) ultrasonic rangefinder and returns the distance to the closest object in range. To do this, it sends a pulse to the sensor to initiate a reading, then listens for a pulse to return.  The length of the returning pulse is proportional to the distance of the object from the sensor.

  The circuit:
   * +V connection of the PING))) attached to +5V
   * GND connection attached to ground
   * SIG connection attached to digital pin 7

  http://www.arduino.cc/en/Tutorial/Ping

  This example code is in the public domain.
*/

const int speedOfSoundInAirInMetersPerSecond = 343;
const float speedOfSoundInAirInCentimeterPerMicrosecond =
  speedOfSoundInAirInMetersPerSecond / 10000.0;
const float centimeterToInchRatio = 2.54;

int inches = 0;
int cm = 0;

long readTravelTimeInMicroseconds(int triggerPin, int echoPin)
{
  pinMode(triggerPin, OUTPUT);  // Clear the trigger
  digitalWrite(triggerPin, LOW);
  delayMicroseconds(2);

  digitalWrite(triggerPin, HIGH);
  delayMicroseconds(10);

  digitalWrite(triggerPin, LOW);
  pinMode(echoPin, INPUT);

  // Reads the echo pin, and returns the sound wave travel time in microseconds
  return pulseIn(echoPin, HIGH);
}

void setup()
{
  Serial.begin(9600);
}

void loop()
{
  // I had to divided that speed by 2 because
  // I'm only interested in the time which took
  // signal to reach an object. I don't need to know
  // how much distance we covered by the signal to the object and back.
  cm = (speedOfSoundInAirInCentimeterPerMicrosecond / 2)
    * readTravelTimeInMicroseconds(7, 7);

  inches = (cm / centimeterToInchRatio);
  Serial.print(inches);
  Serial.print("in, ");
  Serial.print(cm);
  Serial.println("cm");
  delay(100);
}

My thoughts

Am I not being too explicit? I've learnt over the years that it is always better to beexplicit. But, am I not going overboard here?

Are there styles and conventions, styles I should follow when working with Arduino or electronics of this type? My question concerns with the technical side of the code.

VeryLongIdentifiersInCamelCase do not make your code explicit. They just add noise.

The definition
const float speedOfSoundInAirInCentimeterPerMicrosecond =
  speedOfSoundInAirInMetersPerSecond / 10000.0;

is very hard to read. 10000 stands out as weird. After all, there are 1000000 microseconds in a second. It took me quite a time to realize that two orders of magnitude are hidden in meters-to-centimeters conversion.

By default, pulseIn timeouts in 1 second. In reality it means that there is no obstacle within 170 or so meters (aka 500 feet). I seriously doubt your hardware is capable detecting an echo at such distances. It feels safe to shorten the timeout for the sake of better responsiveness.

The production quality code must deal with the ambient noise, which will result in false positives. At least, test that the signal you detected is indeed an echo - that is, it is about as long as the ping. You should also special-case inPulse returning 0. Currently, you are returning 0 distance, whist in fact it means the distance is practically infinite.

Unless I am seeing things, either the code, or documentation is wrong. According to the spec

if value is HIGH, pulseIn() waits for the pin to go from LOW to HIGH, starts timing, then waits for the pin to go LOW and stops timing. Returns the length of the pulse in microseconds

which means it returns the duration of the echo, not the turnaround time you are after. Unfortunately, I don't have the hardware to play with.
Does your code give reasonable results?


Git commit logger for daily stand-up meeting At work, my team hosts a daily standup, in which we discuss what we're working on, what we did the day before and any potential blockers for future development.
I thought as a fun little side project that I would write a script in bash to go through each directory in my /dev/ folder and get all commits from the previous day.
We use Git for the most part, but I do have one or two repositories still under a different source control system. This code is to mitigate that problem 2>> /dev/null;.
Here is the code:
gitUsername="userNameHere"
echo "Here are yesterday's commits:"

for dir in ./*/ ;
    do (cd "$dir"; git log --pretty=format:'%Cred%h%Creset -%Creset %s %Cgreen(%cD) %C(bold blue)<%an>%Creset' --since yesterday --author $gitUsername} 2>> /dev/null;);
done;

echo "That's everything."

I have never written anything in bash before, so any suggestions would be much appreciated!
Also, just a side note in case anybody is interested, we don't read commits to measure workload, I am printing them more as a reminder of the things I was working on the day before.

A much much much much better way of doing this is:
find . -type f -exec git --no-pager log --pretty=format:'%Cred%h%Creset -%Creset %s %Cgreen(%cD) %C(bold blue)<%an>%Creset' --since yesterday {} --author $gitUsername \;
And a cleaner way to write long statements it to add '\' to break into new lines.
find . -type f \
-exec git --no-pager log --pretty=format: \
'%Cred%h%Creset -%Creset %s %Cgreen(%cD) %C(bold blue)<%an>%Creset' \
--since yesterday {} \
--author "${gitUsername}" \;

This way, 7 lines are written as 1 and to stdout.

--no-pager returns everything to your terminal stdout instead your editor.
find . says which directory to search in, so a for loop may be useful to only search desired directories
Also, explicitly defining the desired dirs will remove the need to redirect to stderr and when useful errors are displayed, will be meaningful to you.

https://linux.die.net/man/1/find
https://www.golinuxcloud.com/find-exec-multiple-commands-examples-unix/


Delete K nodes in a linked list having their data less than the next node's The question is from here.

  Given two numbers N and K where N is the number of nodes and K is the number of nodes to be deleted, delete K nodes whose data is less than their next node's data. If no such nodes exist, delete the last node.

But it is still giving me TLE. I tried all the optimisations that could occur to me. I know this can be done with stacks but I would like to know if it can't done in the allocated time with linked lists.

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

struct Node
{
    int value;
    struct Node *next;
};

void addnode(struct Node** head, struct Node** tail, int data)
{
    struct Node* temp = (struct Node*)malloc(sizeof(struct Node));
    temp->value = data;
    temp->next = nullptr;
    if( *head == nullptr)
    {
        *head = temp;
        *tail = *head;
        return;
    }
    (*tail)->next = temp;
    *tail = temp;
}

void deleteNode(struct Node** head, struct Node** prev, struct Node** temp)
{
    if( (*prev)->next == *head)
    {
        *head = (*prev)->next->next;
        return;
    }
    (*prev)->next = (*temp)->next;
}

void deletenodeatlast(struct Node** prev)
{
    (*prev)->next = (*prev)->next->next;
}

void display(struct Node* head)
{
    struct Node* p = head;
    while(p != nullptr)
    {
        printf("%d ", p->value);
        p = p->next;
    }
    printf("\n");
}

int main()
{
    int T;
    scanf("%d", &T);
    while(T--)
    {
        struct Node* head = nullptr;
        struct Node* tail = head;
        int N, K;
        scanf("%d %d", &N, &K);

        for(int i = 0; i < N; i++)
        {
            int data;
            scanf("%d", &data);
            addnode(&head, &tail, data);
        }
        for(int i = 0; i < K; i++)
        {
            bool isGreater = false;
            struct Node* p = head;
            struct Node* prev = (struct Node*)malloc(sizeof(struct Node));
            prev->value = 1000000;
            prev->next = p;

            while( p->next != nullptr)
            {
                if( p->value < p->next->value)
                {
                    deleteNode(&head, &prev, &p);
                    isGreater = true;
                }
                else
                {
                    prev = p;
                    p = p->next;
                }

                if( isGreater )
                {
                    break;
                }
            }
            if( !isGreater)
            {
                deletenodeatlast(&prev);
            }
        }
        display(head);
    }
    return 0;
}

Do you know Shlemiel The Painter?
He never moves his paint can, and so repeatedly walks all over the town.

What you need to do is take the algorithm for removing one friend, look at how it works when applied k times, and then cut out all the redundant Setup work.

Aside from that, you aren't using any C++ containers or algorithms, nor smart-pointers (std::unique_ptr), new or the like.

Don't assume nothing will go wrong. While it is unlikely with malloc() as long as you stay reasonable, scanf() parses user-input, and users will do everything possible, even if you think it's impossible.

There's more, but that should be a good start.


Render Large jQuery Mobile Listview I am trying to write an efficient loop for rendering a jQuery Mobile listview.  Is this the most efficient loop?  I append elements 100 at a time to try and allow the user to see progress without blocking the UI.  This currently still needs work.

I also use a blend of html strings and jQuery DOM objects.  I'm not sure about the performance cost.

What are your thoughts? Assume items could be an array of 600/700 items.

/**
 * Renders the passed in list items into a JQM listview.
 * @param items {Array{Objects}} - An array of key/value objects
 */
function renderSourceItems(items)
{
    var content = $page.find(".content");

    if (!items)
    {
        content.html('
No Data Found
');
    }
    else
    {
        var column,
            value,
            list,
            markup = "";

        column = _.keys(items[0])[0]; // The key is the database column

        content.empty();

        $('
', {
            "data-role" : "listview",
            "data-filter": "true",
            "data-theme" : "c"
        }).appendTo(content).listview();

        list = content.find('ul');

        for (var i = 0; i < items.length; i++)
        {
            value = items[i][column];
            markup += '
' + (value || "[No Value]") + '
';

            if (i % 100 === 0)
            {
                list.append(markup).listview("refresh");
                markup = "";
            }
        }

        // refresh triggers the page to adjust its sizes,
        // otherwise long lists can force the footer navbar to the bottom of the list intead of the screen bottom.
        list.append(markup).listview("refresh");
    }
}

//elements that don 't change should be declared in a scope that doesn't
 //get executed all the time to avoid redundant opearation
var content = $(".content", $page);

function renderSourceItems(items) {

  //JS normally pulls up variable declarations, even when declared
  //anywhere. To avoid readability issues, it's best you declare
  //them up here
  var column, list;

  //the "return early" or "blacklist" method returns if any anomalies
  //are found earlier in the code. That way, you don't use nested ifs
  if (!items) {
    content.html('<p> No Data Found </p>');
    return;
  }

  //down here, it's assumed that items does exist and has content

  column = _.keys(items[0])[0];
  content.empty();

  //depending on the browser, DOM strings vs jQuery DOM vary in
  //performance. Since it varies, I'd rather improve readability
  //by using one of them that's more readable, where in my case
  //it's jQuery DOM building
  list = $('<ul/>', {
    "data-role": "listview",
    "data-filter": "true",
    "data-theme": "c"
  });

  //our recursive function using setTimeout to avoid freezing UI
  (function looper(i) {

    var value, item;

    if (!(i % 100)) {
      value = items[i][column];

      //each item gets built using our readable method
      //here we make a list item and append to it a link with
      //the given data
      item = $('<li/>').appendTo(list);
      $('<a/>', {
        'href': '#apage ',
        'data - column ': column,
        'data - value ': value
      }).text(value || ' [No Value]').appendTo(item);

    }

    setTimeout(function () {
      if (++i < items.length) {
        //when not yet the last item, repeat
        looper(i);
      } else {
        //else we end

        //now you notice that we only append the list to the DOM down here
        //why not append it beforehand? The issue is DOM access - it's slow!
        //we take advantage of jQuery using DOM fragments, DOM elements that
        //are not in the DOM and just operate off-DOM. We only append
        //to the DOM after everything is completed, thus minimizing
        //DOM access from items.length times to just 1
        list.appendTo(content).listview("refresh");
      }
    }, 200);

    //start with index 0
  }(0));

}


Computer guesses the user's number After the feedback for my last script, I realized I should be using functions and trying to adhere to the style guide. I tried to do that here, though still very beginner.

The rounding in Python confused me a bit, and originally the program got stuck for some numbers when the range narrowed down to 1. I think it should work for all cases now (only tested 1-1000).

# Users chooses number in given range, comp guesses

HIGH_NO = 10000
LOW_NO = 1

def getno():
    # Get a number in range from user.
    x = ""
    while not x or x > HIGH_NO or x < LOW_NO:
        print("Enter a number between %s and %s" % (LOW_NO, HIGH_NO))
        x = input()
        try:
            x = int(x)
        except ValueError:
            x = getno()
    return(x)

def approxhalf(lowno, highno):
    # Find integer approx halfway point between two values.
    x = abs(round((lowno+highno)/2))
    return x

def guessno():
    # Computer finds number by halving
    no1 = LOW_NO
    no2 = HIGH_NO
    userno = getno()
    compguess = ""
    count = 0

    while compguess != userno:
        count += 1
        compguess = approxhalf(no1, no2)

        if abs(no2-no1) > 2 or compguess == userno:
            print('%s. I guess %s' % (count, compguess))
            if compguess > userno:
                no2 = compguess
            elif compguess < userno:
                no1 = compguess
            elif compguess == userno:
                print("Got it!")
            else:
                print("wtf dude")
        else:
            # For when the low and high no.s are consecutive
            if no1 == userno:
                print('%s. I guess %s' % (count, no1))
                print("Got it")
                break
            elif no2 == userno:
                print('%s. I guess %s' % (count+1, no2))
                print("Got it")
                break
            else:
                print("I.. I don't understand")

def main():
    while True:
        guessno()
        print('\n')

main()

General logic and flow

I recommend removing not x and instead just originally setting x = LOW_NO-1 for your while

x = LOW_NO - 1
while HIGH_NO < x or x < LOW_NO:

Next, I recommend you just keep the while loop instead of recursively calling getno if the user enters and invalid int. You could just use pass to tell the loop to skip onto the next loop. The old value of x will be preserved so you the condition is still true. Though I do recommend informing the user why their input was rejected.

try:
    x = int(x)
except ValueError:
    print ("That's not a valid number")

Lastly, just return x, no need for brackets.

In your approxhalf, there's no need for abs. Your LOW_NO and HIGH_NO values should prevent wildly off numbers, and if you ever want to use negative numbers there's no need to prevent it like this. I do recommend using int() here just so that it prints without decimal places, since the actual result is based on whole numbers. You also don't need to assign x just to return it. Return the value directly.

def approxhalf(lowno, highno):
    # Find integer approx halfway point between two values.
    return int(round((lowno+highno)/2))

The reason rounding has confused you is probably to do with how floats work. For example 0.1 is actually 0.1000000000000000055511151231257827021181583404541015625. This can affect rounding in cases where you think a number is x.5 but it's actually represented in memory as x.49942342341, leading to a number that rounds down instead of up.

In your checking system, I think you should rearrange the flow.

It's confusing to check if compguess is right and at the same time check if it's more than 2 off. You should do each distinct condition as it comes, my preference is to start with the correct answer and broaden out from there:

if compguess == userno:
    print('%s. I guess %s' % (count, compguess))
    print("Got it!")
    break

A note, you used while compguess != userno. But you break out of the loop for other conditions later down. I'd recommend instead using while True and then use break for the various points you actually want the loop to be over.

if abs(no2-no1) <= 2:
    # For when the low and high numbers are consecutive
    if no1 == userno:
        print('%s. I guess %s' % (count, no1))
        print("Got it")
    elif no2 == userno:
        print('%s. I guess %s' % (count, no2))
        print("Got it")
    else:
        print("ERROR: Neither %s nor %s == %s" % (no1, no2, userno))
    break

About that last else. If you're going to have a condition for a seemingly impossible situation, then you should at least output some relevant data to understand what went wrong. Also I put the break outside the if branches because it should break regardless of which condition is True. Note that now we don't need an if for the next part, we know it has to evaluate since no other condition (which would end the loop) was True.

if compguess > userno:
    no2 = compguess
elif compguess < userno:
    no1 = compguess

Style advice

Reading PEP0008 will tell you all about what I'm saying here and more. It's highly recommended reading.

You should name your functions using underscores, as that's the recommendation. So you'd have get_no(), guess_no() and approx_half(). As for variable names, CamelCase is better for compGuess and userNo.

You almost but not quite have docstrings. You have comments explaining what each function does but formatting as an actual docstring has a practical usage.

def approx_half(lowno, highno):
    """ Find integer approx halfway point between two values."""
    return int(round((lowno+highno)/2))

Enclosing the text in triple quotes when it's at the start of a function will make it a docstring, which means that people can access it in code or in the interpreter to investigate your description of it.

>>> help(approx_half)
Help on function approx_half in module __main__:

approxhalf(lowno, highno)
    Find integer approx halfway point between two values.


Conway's Game of Life in F# Since I'm learning F# along with functional programming, I managed to implement the rules for Conway's Game of Life. I'm not sure if I can improve some of its parts, though. For example, the neighbours function does an ugly range testing, but I can't think of anything simpler.

module Game

open System

type Cell =
    | Alive
    | Dead

let amount_neighbours (board: Cell[,]) (pos: int * int) =
    let range (n: int) (limit: int) =
        let min = if n < 1 then 0 else n - 1
        let max = if n > (limit - 2) then (limit - 1) else n + 1
        [min .. max]

    let x_range = range (fst pos) (Array2D.length1 board)
    let y_range = range (snd pos) (Array2D.length2 board)

    List.sum [for x in x_range ->
        List.sum [for y in y_range -> if board.[x, y] = Alive && (x, y) <> pos then 1 else 0]]

let lifecycle (board: Cell[,]) =
    Array2D.init (Array2D.length1 board) (Array2D.length2 board) (fun i j ->
        let neighbours = amount_neighbours board (i, j)
        match neighbours with
            | 2 -> board.[i, j]
            | 3 -> Alive
            | _ -> Dead)

let rec process_game (board: Cell[,]) (n: int) =
    match n with
        | x when x > 0 ->
            printfn "Iteration"
            printfn "%A" board
            process_game (lifecycle board) (n - 1)
        | _ -> 0

[<EntryPoint>]
let main args =
    let board = Array2D.init 5 5 (fun i j -> if i = 2 && j > 0 && j < 4 then Alive else Dead)
    ignore (process_game board 4)
    0

I thought it would make sense if you created a function alive, which would take care of bounds checking for you. And also simplify the the final expression by using a single sequence expression instead of two like you do.

But I'm not sure it's actually much better than your version:

let amount_neighbours (board: Cell[,]) (pos: int * int) =
    let alive board pos =
        let (x, y) = pos
        if x < 0 || x >= Array2D.length1 board ||
           y < 0 || y >= Array2D.length2 board then
            false
        else
            board.[x, y] = Alive

    let vicinity x = seq { x - 1 .. x + 1 }

    seq {
        for x in vicinity (fst pos) do
        for y in vicinity (snd pos) do
        if (x, y) <> pos && alive board (x, y) then
            yield true
    } |> Seq.length


Implementing JoinWithAnd method Objective

  Write a function JoinWithAnd in Java with the following signature:

String JoinWithAnd(String a, String b, String c, String d, String e)

  So that:

  If no input is provided, it returns an empty string
  If any one input is provided, it returns the string as it is
  If more than one inputs are provided, it combines all strings with AND in between and returns

  The input/output looks something like this:

 -----------------------------------------------------------
|Function Call                             |Returns         |
 -----------------------------------------------------------
|JoinWithAnd(null, null, null, null, null);|“”              |
|JoinWithAnd(“A”, null, null, null, null); |“A”             |
|JoinWithAnd(null, null, “C”, null, null); |“C”             |
|JoinWithAnd(“A”, “B”, null, null, null);  |“A AND B”       |
|JoinWithAnd(“A”, null, “C”, “D”, null);   |“A AND C AND D” |
 -----------------------------------------------------------

  Constraints

  DO NOT change the method signature
  DO NOT use arrays
  DO NOT use StringBuffer or any other type of optimized String variants
  DO NOT use Java Collection

The closest I could get is:

public class JoinWithAnd {

    private final String infix = " AND ";
    private final String regex = "( AND null)+";

    private String compute(String a, String b, String c, String d, String e) {
        return (a + infix + b + infix + c + infix + d + infix + e).replaceAll(regex, "");
    }

    public static void main(String[] args) {
        JoinWithAnd oj = new JoinWithAnd();
        System.out.println(oj.compute("P", null, "R", "S", null));
    }

}

But this doesn't seem to be sufficient since it uses replaceAll.

Could anyone please suggest in what other ways this problem could be solved keeping all the constraints in mind?

Regex
I think your String.replaceAll(String, String) is fine, as regular expressions don't fall afoul of any of the rules. :) Just watch out for @Bastien Aracil's comment, that you will be removing literal "null" values.
With that said...
Break the rules...?

DO NOT use arrays
DO NOT use StringBuffer or any other type of optimized String variants

These are going to be nearly impossible to conform, since a String is fundamentally a char array, and even a simple "a" + "b" uses an underlying StringBuilder for the concatenation. If one can partially 'hand-wave' the implicit StringBuilder away when using the + operator, I suppose one can also do a smaller 'hand-wave' of the implicit char array modification with String.concat(String):
private static String join(String first, String second) {
    if (first == null || first.isEmpty()) {
        return second == null ? "" : second;
    }
    return second == null ? first : first.concat(" AND ").concat(second);
}

What we do here is to codify our logic as such:

If the first string is either null or empty, we either return second or an empty one.
Afterwards, if the second string is null, we return first, i.e. no concatenation done, or actually perform the concatenation with " AND ".

We then just need to 'recursively' call this with the five arguments, and a null or "" as a 'starter' with a:
private static String joinWithAnd(String a, String b, String c, String d, String e) {
    return join(join(join(join(join(null, a), b), c), d), e);
}

Java 8
Going further, we can just rely on streams (not collections) and StringJoiner (technically not a String 'variant', whatever that means) to get the same output:
private static String joinWithAnd(String a, String b, String c, String d, String e) {
    return Stream.of(a, b, c, d, e)
                .filter(Objects::nonNull)
                .collect(Collectors.joining(" AND "));
}

And, if one is being extra pedantic with the question's language, we can replace Stream.of(T...), technically an array, with multiple Stream.concat(Stream, Stream) calls:
// return Stream.of(a, b, c, d, e)
return Stream.concat(Stream.concat(Stream.concat(Stream.concat(
                Stream.of(a), Stream.of(b)), Stream.of(c)), Stream.of(d)), Stream.of(e))


I need a better way to add css per each page on wordpress I just started to use WordPress to start building a website, but as I need to use the get_header() function to retrieve the <head> content and I have a very large project, I wanted to put a CSS file depending on the page that is being displayed. After a few tries I came up with a solution, but I think that I can do it in a better way and I need your help to optimize it.
Here is my PHP snippet that goes into the functions.php file:
function website_files_per_page() {
    $pagelist = get_pages();
    $index = sizeof($pagelist);
    while ($index --) {
        $pagename = $pagelist[$index]->post_name;
        if (is_page($pagename)){
            $styleName = "website_".$pagename."_styles";
            $styleDir = get_theme_file_uri("/style/".$pagename.".css");
            wp_enqueue_style($styleName, $styleDir);
            break;
        }
    }
}
add_action("wp_print_styles", "website_files_per_page");

Simpler approach
I haven’t tested this but based on this SO answer using global $post and $post->page_title to set $pagename should allow you to remove the loop.
function website_files_per_page() {
    global $post;
    $pagename = $post->post_title;
    if ($pagename) {
        return; //no page name
    }
    $styleName = "website_".$pagename."_styles";
    $styleDir = get_theme_file_uri("/style/".$pagename.".css");
    wp_enqueue_style($styleName, $styleDir);
}
add_action("wp_print_styles", "website_files_per_page");

That would allow the code to run in \$O(1)\$ time instead of \$O(n)\$.
Let PHP handle the iteration
If the suggestion to use $post doesn’t work, you may be able to change the while loop to a foreach. foreach provides an easy way to iterate over arrays:
foreach ($pagelist as $post) {
    $pagename = $post->post_name;
    if (is_page($pagename)) {
        ...

With this approach there is no need to initialize and update a counter (e.g. `$index) to dereference elements in the array.
If there was a need to access the index/counter variable the second syntax can be used for that:
foreach ($pagelist as $index => $post) {


Line and bitmap collision detection I am trying to implement an efficient collision detection between a line joining two points and a bitmap mask.

Example:

Currently I am using Bresenham's line drawing algorithm to draw each pixel of the line between the two points and comparing that pixel with the bitmap (If the pixel is black return true else continue drawing the line).

collision(x0,y0,x1,y1) {
  let dx = Math.abs(x1 - x0),
    dy = Math.abs(y1 - y0),
    sx = (x0 < x1) ? 1 : -1,
    sy = (y0 < y1) ? 1 : -1,
    err = dx - dy,
    e2

  // loop through line drawing
  while (!((x0 == x1) && (y0 == y1))) {
    e2 = err << 1;
    // check line point x,y against bitmap
    if (bitmap[x0][y0] == 1) {
      return 1;
    }
    if (e2 > -dy) {
      err -= dy;
      x0 += sx;
    }
    if (e2 < dx) {
      err += dx;
      y0 += sy;
    }
  }
  // If looped through whole line and not returned collision return false
  return 0
}

Is this a good/efficient approach? Or is there a set method or better approach for this problem?

Thanks in advance.

Performance review
Your code.
You have a few inefficiencies. When implementing the same function many times with many iterations within the function, reducing the number of steps will provide worth while performance gains.
Some suggestions.
Use === rather than == as it is a little quicker
Change the sign of dy, that means that the test if(e2 > -dy)  can be changed to if(e2 > dy) saving the need to negate dy each step.
Change the lines
 // Change
 dy = Math.abs(y1 - y0)
 // to
 dy = -Math.abs(y1 - y0)

 // Change
 err = dx - dy;
 // to
 err = dx + dy

 // Change
 if (e2 > -dy) {
   err -= dy;
   x0 += sx;
 }
 // to
 if (e2 > dy) {  // removes the - operation
   err += dy;
   x0 += sx;
 }

Remove the double array index and use a column reference for the bitmap
// just before the loop
var column = bitmap[x0];

// then in the loop you only need to index the row
if (column[y0] === 1) {
  return 1;
}
if (e2 > dy) {
  err += dy;
  x0 += sx;
  column = bitmap[x0]; // and update the column only when x0 changes.
}

Not a performance thing, just a style note. You have left out the ; after the return 0 and the locals declaration e2.
Better yet
You could also flatten the bitmap array and use an index rather than a coordinate. You change the y step to a modulo size. This reduces the number of tests and eliminates the need to index into a 2D array.
// using bitmap as a flat array with width as the pixel width
// Assuming coords are ints
function collision(x0, y0, x1, y1, width) {
  const dx = Math.abs(x1 - x0);
  const dy = -Math.abs(y1 - y0);
  const sx = x0 < x1 ? 1 : -1;
  const sy = y0 < y1 ? width : -width;
  const endIndex = x1 + y1 * width;
  var e2, err = dx + dy;
  var index = x0 + y0 * width;

  // loop through line drawing
  while (index !== endIndex) {
    if (bitmap[index] === 1) { return 1 }
    e2 = err << 1;
    if (e2 > dy) {
      err += dy;
      index += sx;
    }
    if (e2 < dx) {
      err += dx;
      index += sy;
    }
  }
  return 0;
}

Peformance
The function is out of context so performance improvements can not be suggested.
As a random line an improvement would be to step over a quad tree. But this would be a very complex bit of code and would depend on the content of the bitmap and how many pixels you are sampling. It would make major improvements when the ratio of "on" pixels is low and clumped, but poor as the ratio of "on" pixels approaches 50% and the distribution is non clumpy.
The are many other solutions but they all depend on what the line is looking for, how one line is related to the rest, and the distribution and content of the bitmap.


Generating a random MAC address I need the ability to generate random MAC addresses, so I wrote up a little function that does that:
>>> random_mac()
'7C:93:B7:AF:BA:AE'
>>> random_mac()
'D8:D8:A0:D4:A5:3F'
>>> random_mac(unicast=False, universal=True)
'55:47:C6:EE:C6:2B'
>>> random_mac(unicast=True, universal=False)
'FE:A1:4B:98:76:B6'

I decided to allow the user to pick if it's unicast/multicast and universally/locally administered; even though I'll only need unicast/universal. This caused me headaches though because I'm still not great at dealing with bits. The LSB of the first octet indicates uni/multicast, and the second LSB of the octet indicates universal/local, so these bits can't be random.
After playing around with a few failed ideas (generating all random bits, then "fixing" the two bits later), I finally decided to generate a random number between 0 and 63 inclusive, left shift it twice, than add the two bits on after. It works, but it's ugly and looks suboptimal.
It's not a lot of code, but I'd like a few things reviewed:

Is there a better approach? It feels hacky generating it as two pieces then adding them together. I tried explicitly setting the bits, but the code to decide between |, and & and ~ got messier than what I have now, so I went with this way.
The number constants are bugging me too. The numbers kind of sit on a border of self-explanatory and magic, so I decided to name them to be safe. LAST_SIX_BITS_VALUE feels off though.
Is treating a boolean value as a number during bitwise operation idiomatic? Is it clear as I have it now?
Attaching the first octet to the rest is suboptimal as well. Speed isn't a huge concern, but I'm curious if there's a cleaner way that I'm missing.

from random import randint, randrange

N_MAC_OCTETS = 6
OCTET_VALUE = 256
LAST_SIX_BITS_VALUE = 63

def random_mac(unicast: bool = True, universal: bool = True) -> str:
    least_two_bits = (not unicast) + ((not universal) << 1)
    first_octet = least_two_bits + (randint(0, LAST_SIX_BITS_VALUE) << 2)
    octets = [first_octet] + [randrange(OCTET_VALUE) for _ in range(N_MAC_OCTETS - 1)]
    return ":".join(f"{octet:02X}" for octet in octets)

Examples of the bits for the first octet for different inputs:
def display(mac):
    print(mac, f"{int(mac.split(':')[0], 16):08b}")

# Unicast, Universal
>>> display(random_mac(True, True))
04:27:DE:9A:1B:D7 00000100  # Ends with 0,0

# Unicast, Local
>>> display(random_mac(True, False))
72:FB:49:43:D5:F2 01110010  # 1,0

# Multicast, Universal
>>> display(random_mac(False, True))
7D:BF:03:4E:E5:2A 01111101  # 0,1

# Multicast, Local
>>> display(random_mac(False, False))
2F:73:52:12:8C:50 00101111  # 1,1

Negating an argument is somewhat counterintuitive. Consider passing them as multicast and local instead.

I would seriously consider defining
  UNIVERSAL = 0x01
  MULTICAST = 0x02

and pass them as a single argument, is in
      random_mac(UNIVERSAL | MULTICAST)

Using both randint and randrange feels odd. I would stick with randrange.

First octet needs a special treatment anyway. That said, consider
  def random_mac(special_bits = 0):
      octets = [randrange(OCTET_VALUE) for _ in range(N_MAC_OCTETS)]
      octets[0] = fix_octet(octet[0], special_bits)
      return ":".join(f"{octet:02X}" for octet in octets)

with
  def fix_octet(octet, special_bits):
      return (octet & ~0x03) | special_bits


MVC API controller processing POST request Do I need to restructure for readability?

 public HttpResponseMessage Post([FromBody]BoatProperties props)
    {
        // model validation
        if(ModelState.IsValid)
        {
            // check if guid is set, else set it; don't set when user is admin
            if(props.Guid == null && !User.IsInRole("Admin"))
            {
                props.Guid = Membership.GetUser().ProviderUserKey as Guid?;
            }

            Boat boat = BoatProvider.GetBoat(props.Guid);

            try
            {
                // can't find existing boat: create new boat
                if (boat == null)
                {
                    boat = new Boat(props);
                    BoatProvider.AddNew(boat);
                }
                // otherwise update existing boat
                else
                {
                    BoatProvider.Update(boat);
                }
            }
            catch(Exception ex)
            {
                // error while updating/adding model
                return Request.CreateErrorResponse(HttpStatusCode.InternalServerError, ex.Message);
            }
            // everything ok!
            return Request.CreateResponse(HttpStatusCode.OK, BoatProvider.GetBoat(props.Guid));
        }
        // validation failed
        return CreateResponse(HttpStatusCode.BadGateway);
    }

As you can see I'm exiting all those conditions with a return statement. If I restructure my code like this:

if(!ModelState.IsValid)
{
    return CreateResponse(HttpStatusCode.BadGateway);
}
if (props.Guid == null  && !User.IsInRole("Admin"))
{
    props.Guid = guid;
}

I will have less indentations and it may be more readable and all the steps seem more distanced from each other.

I'd certainly try to reduce the indentations, not in the least because in the case of if(ModelState.IsValid) it involves a 30-line block, and the else is just a single line. Also: if you're going to fail, do it fast.

But there are other issues to consider.

You expect an Exception to happen when you create or update a Boat? That seems odd.
I understand the GetBoat then AddNew vs Update logic, but I'd move that out of the Controller and into a Business layer. Keep your Controllers clean. Right now this is a fairly simply operation and already there are twenty or so lines. Just imagine the mess it'll become once you need to do more complicated operations.
I'm also a bit mystified by the logic WRT the update: you retrieve the boat and then you do BoatProvider.Update(boat); -- shouldn't you have done something with that boat first?
Most/all of your comments are superfluous. Comments shouldn't tell me what is happening (that's what your code your do), but why.


Creating SwiftReachability Class in Swift 2.2 I am novice at creating independent classes for user. I have seen many Reachability library for Swift, but I need to create my own class for that. Here is my code:

class SwiftReachability: NSObject {

let REACHABILITY_NOTIFIER_KEY = "reachability_notifier_key"
var backgroundQueue: NSOperationQueue?
var isCancelled: Bool = false

//MARK: - Checking Internet Connecion
func isConnectedToNetwork() -> Bool{

    var zeroAddress = sockaddr_in(sin_len: 0, sin_family: 0, sin_port: 0, sin_addr: in_addr(s_addr: 0), sin_zero: (0, 0, 0, 0, 0, 0, 0, 0))
    zeroAddress.sin_len = UInt8(sizeofValue(zeroAddress))
    zeroAddress.sin_family = sa_family_t(AF_INET)

    let defaultRouteReachability = withUnsafePointer(&zeroAddress) {

        SCNetworkReachabilityCreateWithAddress(nil, UnsafePointer($0))
    }

    var flags = SCNetworkReachabilityFlags(rawValue: 0)

    SCNetworkReachabilityGetFlags(defaultRouteReachability!, &flags)

    #if DEBUG
        print("Reachability flags = \(flags.rawValue) uint = \(UInt32(kSCNetworkFlagsReachable))")
    #endif

    let isReachable = (flags.rawValue & UInt32(kSCNetworkFlagsReachable)) != 0
    let needsConnection = (flags.rawValue & UInt32(kSCNetworkFlagsConnectionRequired)) != 0

    return (isReachable && !needsConnection) ? true : false
}

func startNotifier(){

    NSNotificationCenter.defaultCenter().addObserver(self, selector: #selector(SwiftReachability.networkConnectionObserver), name: REACHABILITY_NOTIFIER_KEY, object: nil)
    callingBackgroungThread()
}

func stopNotifier(){

    NSNotificationCenter.defaultCenter().removeObserver(self, name: REACHABILITY_NOTIFIER_KEY, object: nil)
    backgroundQueue?.cancelAllOperations()
    isCancelled = true
}

func callingBackgroungThread(){

    backgroundQueue = NSOperationQueue()

    let operation = NSBlockOperation {

        if self.isConnectedToNetwork(){

            NSNotificationCenter.defaultCenter().postNotificationName(self.REACHABILITY_NOTIFIER_KEY, object:nil)
        }
    }

    operation.completionBlock = {

        if !self.isCancelled{

            self.callingBackgroungThread()
        }
    }

    delay(5.0, closure: {

        if !self.isCancelled{

            self.backgroundQueue?.addOperation(operation)
        }
    })
}

func networkConnectionObserver(){

    print("Observer")
    showAlert("Network Identified")
}

//MARK: Dispatch Asyn Queue After
func delay(delay:Double, closure:()->()) {

    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, Int64(delay * Double(NSEC_PER_SEC))), dispatch_get_main_queue(), closure)
}

func showAlert(message: String){

    let alertController = UIAlertController(title: "Bing", message: message, preferredStyle: UIAlertControllerStyle.Alert)

    let okAction = UIAlertAction(title: "Ok", style: UIAlertActionStyle.Default, handler: { (UIAlertAction) in

        self.stopNotifier()
    })

    alertController.addAction(okAction)

    dispatch_async(dispatch_get_main_queue(), {
        let appDelegate = UIApplication.sharedApplication().delegate as! AppDelegate
        appDelegate.window?.rootViewController!.presentViewController(alertController, animated: true, completion: nil)
    })
}
}

Can anyone review it to make it better?

Improvements of your code

Let's start with your isConnectedToNetwork() method.

var zeroAddress = sockaddr_in(sin_len: 0, sin_family: 0, sin_port: 0, sin_addr: in_addr(s_addr: 0), sin_zero: (0, 0, 0, 0, 0, 0, 0, 0))

As of Swift 1.2(?), all imported C structs have a default initializer which sets all
members to zero:

var zeroAddress = sockaddr_in()

let defaultRouteReachability = withUnsafePointer(&zeroAddress) {
    SCNetworkReachabilityCreateWithAddress(nil, UnsafePointer($0))
 }

Here defaultRouteReachability is an optional which is later force-unwrapped.
Better check with guard and optional binding if the call succeeded:

guard let defaultRouteReachability = withUnsafePointer(&zeroAddress, {
    SCNetworkReachabilityCreateWithAddress(nil, UnsafePointer($0))
}) else {
    NSLog("Could not create reachability reference")
    return false
}

var flags = SCNetworkReachabilityFlags(rawValue: 0)

This can be shorted to

var flags = SCNetworkReachabilityFlags()
// or
var flags : SCNetworkReachabilityFlags = []

let isReachable = (flags.rawValue & UInt32(kSCNetworkFlagsReachable)) != 0
let needsConnection = (flags.rawValue & UInt32(kSCNetworkFlagsConnectionRequired)) != 0

SCNetworkReachabilityFlags is an OptionSetType and that has a set-like interface,
there is no need for rawValue and UInt32 conversion:

let isReachable = flags.contains(.Reachable)
let needsConnection = flags.contains(.ConnectionRequired)

Finally, there is no reason to make this an instance method because it does not use
any state, you could make it a type method instead. Then it would look like this:

class func isConnectedToNetwork() -> Bool{

    var zeroAddress = sockaddr_in()
    zeroAddress.sin_len = UInt8(sizeofValue(zeroAddress))
    zeroAddress.sin_family = sa_family_t(AF_INET)

    guard let defaultRouteReachability = withUnsafePointer(&zeroAddress, {
        SCNetworkReachabilityCreateWithAddress(nil, UnsafePointer($0))
    }) else {
        NSLog("Could not create reachability reference")
        return false
    }

    var flags : SCNetworkReachabilityFlags = []
    SCNetworkReachabilityGetFlags(defaultRouteReachability, &flags)

    let isReachable = flags.contains(.Reachable)
    let needsConnection = flags.contains(.ConnectionRequired)
    return isReachable && !needsConnection
}

Your notification mechanism is far too complicated in my opinion. Instead of
scheduling an NSOperation which reschedules itself with dispatch_after, you could
simply create a repeating NSTimer. Also you should check if the
startNotifier/stopNotifier calls are properly balanced.

Even better, you can register a notification callback which is automatically called
when the reachability status changes. I'll come back to that later.

The SwiftReachability class should not do any UI-related stuff like displaying
alerts. Use NSNotification or callback methods to make the class reusable.

A different design

Let's have a look at the "Supporting IPv6 DNS64/NAT64 Networks"
document from Apple:

  Connect Without Preflight

  The Reachability APIs (see SCNetworkReachability Reference) are intended for diagnostic
  purposes after identifying a connectivity issue. Many apps incorrectly use these APIs
  to proactively check for an Internet connection by calling the SCNetworkReachabilityCreateWithAddress
  method and passing it an IPv4 address of 0.0.0.0, which indicates that there is a
  router on the network. However, the presence of a router doesn’t guarantee that an
  Internet connection exists. In general, avoid preflighting network reachability.
  Just try to make a connection and gracefully handle failures. If you must check for
  network availability, avoid calling the SCNetworkReachabilityCreateWithAddress method.
  Call the SCNetworkReachabilityCreateWithName method and pass it a hostname instead.

Your method can only test for general Internet connectivity (reachability of 0.0.0.0),
but not for reachability of a given host (as recommended by Apple).

To add this feature, it makes sense to make the SCNetworkReachability reference
a (private) instance variable, and provide multiple init methods:

class SwiftReachability {

    private let reachability: SCNetworkReachability

    init?() {
        var zeroAddress = sockaddr_in()
        zeroAddress.sin_len = UInt8(sizeofValue(zeroAddress))
        zeroAddress.sin_family = sa_family_t(AF_INET)

        guard let reachability = withUnsafePointer(&zeroAddress, {
            SCNetworkReachabilityCreateWithAddress(nil, UnsafePointer($0))
        }) else {
            return nil
        }
        self.reachability = reachability
    }

    init?(hostname : String) {
        guard let reachability = SCNetworkReachabilityCreateWithName(nil, hostname) else {
            return nil
        }
        self.reachability = reachability
    }
}

More init methods (e.g. for SCNetworkReachabilityCreateWithAddressPair) can be added
if necessary. The initializers are failable.

Checking the connection status consists of two parts:

retrieve the SCNetworkReachabilityFlags,
evaluate the SCNetworkReachabilityFlags.

For reasons that become more apparent later, I suggest to implement these as separate
methods. Retrieving the flags is done as above, only as an instance method:

class SwiftReachability {
    // ...

    func flags() -> SCNetworkReachabilityFlags {
        var flags: SCNetworkReachabilityFlags = []
        SCNetworkReachabilityGetFlags(reachability, &flags)
        return flags
    }
}

Computing the (boolean) connection status from the flags can be done as an extension
method of SCNetworkReachabilityFlags:

extension SCNetworkReachabilityFlags {
    func connected() -> Bool {
        let isReachable = self.contains(.Reachable)
        let needsConnection = self.contains(.ConnectionRequired)
        return isReachable && !needsConnection
    }
}

The reachability status can now be determined like this:

if let reachability = SwiftReachability(hostname: "google.com") {
    let connected = reachability.flags().connected()
    print(connected)
} else {
    print("Could not determine reachability status")
}

Change notifications

Instead of polling the connection status in regular intervals, you can register
a callback function which is called automatically if the status changes.

class SwiftReachability {
    // ...

    private var notifierActive = false
    var callback : ((SwiftReachability, SCNetworkReachabilityFlags) -> Void)?

    func startNotifier() {
        if notifierActive { return }

        var context = SCNetworkReachabilityContext(version: 0, info: nil,
                                                   retain: nil, release: nil, copyDescription: nil)
        context.info = UnsafeMutablePointer(Unmanaged.passUnretained(self).toOpaque())

        SCNetworkReachabilitySetCallback(reachability, { (_, flags, info) in
            let mySelf = Unmanaged<SwiftReachability>.fromOpaque(COpaquePointer(info)).takeUnretainedValue()
            mySelf.callback?(mySelf, flags)
        }, &context)

        SCNetworkReachabilitySetDispatchQueue(reachability, dispatch_get_main_queue())
        notifierActive = true
    }

    func stopNotifier() {
        if !notifierActive { return }

        SCNetworkReachabilitySetCallback(reachability, nil, nil)
        SCNetworkReachabilitySetDispatchQueue(reachability, nil)
        notifierActive = false
    }

    deinit {
        stopNotifier()
    }
}

This is a bit tricky because the callback  is a pure C function. In Swift, this must
be a global function or a closure which does not capture any context. It is therefore
not possible to use self in the callback closure. Instead, the self instance
pointer must be to a Void pointer, passed to the callback, and converted back to
an instance pointer (mySelf), compare
How to cast self to UnsafeMutablePointer type in swift.

Here is a simple example how this could be used in view controller for regular
updates of the connection status:

class ViewController: UIViewController {

    var reachability : SwiftReachability?

    override func viewDidLoad() {
        super.viewDidLoad()

        reachability = SwiftReachability()
        if let reach = reachability {
            reach.callback = { (_, flags) in
                let connected = flags.connected()
                print("Connected:", connected)
                // Update display, show alert, whatever ....
            }
            reach.startNotifier()
        } else {
            print("Could not determine connection status")
            // Update display, show alert, whatever ....
        }
    }
}

Note that the view controller must hold a reference to the reachability object.
The connection flags are passed to the callback, so now we can use the
connected() extension method which we defined above.

Of course, one could also use the NSNotificationCenter to inform clients of
changes in the connection status instead of a callback method.

Remark: There is an excellent implementation of Reachability in Swift: https://github.com/ashleymills/Reachability.swift.
Similarities between that code and the code suggested here are not intentional but could not be avoided.


Pending method invocation for a game I need to do pending methods invocation in Java (Android) in my game project. A method represents actions in a scene.

Maybe you're familiar with Context.startActivity in Android, which is not suddenly starting the activity, but statements below it is still executed before starting the requested activity. I've found 3 ways, but I'm not sure which one should I choose by considering the performance and PermGen issues (actually I don't know whether Android has PermGen issue if there are so many classes). These methods will not be called very frequent, may be once in 5 seconds, but may be they will be very many (there are so many scenes).

Please suggest the best way of doing this, by considering memory usage (maybe PermGen too), performance, ease of coding, and bug freedom.

Using switch-case

I need to add each method call in switch-case.

public class MethodContainer {

    public void invoke(int index) {
        switch (index) {
        case 0:
            method0();
            break;
        .
        .
        .
        case 100:
            method100();
            break;
        }
    }

    private void method0() {
        ...
    }

    .
    .
    .

    private void method100() {
        ...
    }
}

Using for-loop and annotation/reflection

Like the above, but make coding easier (except in defining constants).

public class MethodContainer {

    private static final int METHOD_0 = 0;
    ...
    private static final int METHOD_100 = 100;

    public void invoke(int index) {
        for (Method m : MethodContainer.class.getMethods()) {
            MyAnnotation annotation = m.getAnnotation(MyAnnotation.class);
            if (annotation != null && annotation.value() == index) {
                try {
                    m.invoke(this);
                    break;
                } catch (...) {
                    ...
                }
            }
        }
    }

    @MyAnnotation(METHOD_0)
    private void method0() {
        ...
    }

    .
    .
    .

    @MyAnnotation(METHOD_100)
    private void method100() {
        ...
    }
}

Using inner classes

There's no need to declare constants and no reflection, but too many classes.

public class MethodContainer {

    public void invoke(Runnable method) {
        method.run();
    }

    private Runnable method0 = new Runnable() {

        public void run() {
            ...
        }
    };

    .
    .
    .

    private Runnable method100 = new Runnable() {

        public void run() {
            ...
        }
    };
}

Your first example (switch-case) is so poorly thought of in the Object Oriented community that there is a refactoring designed specifically to replace it with your third implementation: Replace Conditional With Polymorphism.

To be honest, I would have never thought of your second implementation. It isn't completely heinous, though I would probably use the constructor to create an array of method objects that you can index into directly - rather than run through a for loop every time you want to make a call.

Between a revised second implementation (array with annotation/reflection) and the third implementation (inner classes) I would personally be more fond of the "inner class" version. I don't think that memory or performance is going to be a factor at this level and I think the third, inner class, version is much more readable (at least in Java - in other languages, you could implement something like the second approach directly).


Roman Numeral to Decimal Conversion I'm mostly looking to get feedback on how well I've implemented the C++ language. But I would also like feedback on my algorithm. Did I make it more complex than it needs to be? Is there anything I could have done better?

Things my algorithm prevents:

IIII <- "too many multiples," better represented as IV
IIIV <- "cannot add the same digit to a subtraction," better represented as VI
IVI <- "addition cannot follow subtraction," better represented as V
IXX <- "cannot subtract from a multiple," better represented as XIX

Things it does not prevent (that I'm aware of):

XICV <- "bad format," better represented as CXIV
IXIX <- "repeated subtraction," better represented as XVIII
VV <- "multiple is value of existing digit," better represented as X

Note: This uses the rules of roman numerals I was taught, and I realize that there isn't really a an accepted standard of rules for representing numbers using roman numerals.

#include <string>
#include <stack>
#include <iostream>
#include <ctype.h> // required for tolower()

using namespace std;

int get_roman_value( char );
int roman_to_decimal( string& );

int main( int argc, char* argv[] )
{

    if( argc > 1 )
    {
        string roman(argv[1]);
        try {
            cout << roman_to_decimal(roman) << endl;
        } catch( const char* e ) {
            cerr << e << endl;
        }

    }

    return 0;
}

int get_roman_value( char digit )
{
    switch( tolower(digit) )
    {
        case 'i': return    1; break;
        case 'v': return    5; break;
        case 'x': return   10; break;
        case 'l': return   50; break;
        case 'c': return  100; break;
        case 'd': return  500; break;
        case 'm': return 1000; break;
        default: throw "bad digit";
    }
    return 0;
}

int roman_to_decimal( string& roman )
{

    int decimal = 0;    // the final result
    int multiple = 0;   // keeps track of multiples, eg. In XXV, X would be a multiple
    int current = 0;    // the digit currently being processed
    int last_add = 0;   // the last digit that wasn't a multiple, or subtraction operation
    int last_sub = 0;
    stack<int> digits;  // queue of the digits
    string::iterator i; // iterator to loop through the roman string

    for( i = roman.begin(); i < roman.end(); i++ )
        digits.push( get_roman_value(*i) );

    do
    {

        // get the current digit
        current=digits.top(), digits.pop();

        // if there are too many multiples, then throw up
        if( multiple > 2 )
            throw "too many multiples";

        // check for a substraction operation
        if( !digits.empty() && current > digits.top() )
        {
            if( multiple > 0 )
                throw "cannot subtract from a multiple";
            if( last_add == digits.top() )
                throw "addition cannot follow subtraction";
            decimal+=(current-digits.top()), last_sub=digits.top(), digits.pop();
        }
        // check for multiples
        else if( !digits.empty() && current == digits.top() )
            decimal += current, multiple++;
        // regular addition operation
        else
            if( last_sub == current )
                throw "cannot add the same digit to a subtraction";
            else
                decimal+=current, multiple=0, last_add=current;

    } while( !digits.empty() );

    return decimal;

}

Well, first of all you should probably declare variables closer to usage. Not only this will compress your code a bit, it will also save you the trouble of assigning random things to them. For example int current = 0 declaration can be omitted.

You normally want to use != operator when dealing with iterators, so that you can switch containers, implementations, etc; and you'd generally want to increase the iterator without making an extra copy of same. So for example the following

for( i = roman.begin(); i < roman.end(); i++ )
    digits.push( get_roman_value(*i) );

is better written as

for( i = begin(roman); i != end(roman); ++i )
    digits.push( get_roman_value(*i) );

also, there are some obscure reasons to write begin(container) and end(container) instead of invoking member functions, but most would say it's a matter of taste.

Depending on how serious your are, you shouldn't throw strings, instead throw something that derives from exception, which is a part of STL which lives in "<exception>" header.

Then there's some redundancy in your switch, for example

case 'i': return    1; break;

the break statement is not necessary, and most programmers are very familiar with case -> return pattern. Ultimately it's a matter of taste, I guess.

EDIT Answer to comment

I would prefer to remove the declaration of int current = 0, and instead use:

// get the current digit
int current = digits.top();
digits.pop();

Although I personally like your use of comma operator, most people emphatically do not; and encourage to separate statements on separate lines.

I also noticed you are a bit inconsistent with spaces, it's advisable to always have same rules everywhere. For example there are places where you surround the assignment operator with spaces, and then there are cases when you don't.


Spell-checking application in C++ This is my current coursework for university where I am to write a spell checking application and compile using C++11.

I have a dictionary file that is read into the program automatically and stored into a binary search tree in order. The user provides a text file that they would like to check which is split into tokens and stored in a string vector.

The program then loops through the string vector and checks if that word exists in the BST. If the word is not in the BST, it will print the word to the console as incorrectly spelled.

#include <iostream>
#include <sstream>
#include <fstream>
#include <vector>
#include <cstring>
#include "bst.h"

using namespace std;

string read_file(string &filename)
{
    ifstream file(filename);
    // If there is a file
    if (file)
    {
        string data;
        // Find total number of bytes of file
        file.seekg(0, file.end);
        data.resize(file.tellg());
        file.seekg(0, file.beg);
        // Read text into data string
        file.read(&data[0], data.size());
        // Close the file
        file.close();
        // Return the string
        return data;
    }
    else return string("");
}

void split_words(vector<string> &words, string &data)
{
    char *token, *delim = " .,?!:;/\"\'\n\t";
    // Set token to first word
    token = strtok(&data[0], delim);
    // Split rest of words
    while (token != nullptr)
    {
        // Convert each word from dictionary to lower case
        for (int i = 0; i < strlen(token); ++i)
        {
            char word = tolower(token[i]);
            token[i] = word;
        }
        // Push word to end of vector
        words.push_back(token);
        // Get the next word
        token = strtok(nullptr, delim);
    }
    // Free the memory
    token = nullptr;
    delim = nullptr;
    delete token;
    delete delim;
}

int main(int argc, char **argv)
{
    string file_to_check, file_data, word_dictionary = "dictionary.txt";
    int spell_count = 0;

    BinarySearchTree *tree = new BinarySearchTree();
    vector<string> words;

    // Loop through arguments
    for (int i = 0; i < argc; ++i)
    {
        // Set file name if provided as argument
        if (string(argv[i]) == "-i" && argv[i+1] != nullptr)
            file_to_check = argv[i+1];
    }

    // If there was no file name as argument, prompt user
    if (file_to_check.empty())
    {
        cout << "File name: ";
        getline(cin, file_to_check);
        cout << endl;
    }

    // If file name is not empty, run spell checking methods
    if (!file_to_check.empty())
    {
        // Read words from dictionary.txt into file_data string
        file_data = read_file(word_dictionary);
        // Split the words and store into vector
        split_words(words, file_data);
        // Insert words into Binary Search Tree
        for (int i = 0; i < words.size(); ++i)
            stringstream(words[i]) >> *tree;

        // Store the data read from specified file
        file_data = read_file(file_to_check);
        // Split sentences and store each word in words vector
        split_words(words, file_data);
        cout << endl;

        // Loop through words vector and check if it exists in dictionary
        for (int i = 0; i < words.size(); ++i)
        {
            // Print out non-occurring words
            if (!tree->exists(words[i]))
            {
                spell_count++;
                cout << words[i] << endl;
            }
        }
        cout << endl;

        // Print the total number of spelling mistakes
        cout << spell_count << " spelling mistakes" << endl;
    }
    else
    {
        // If still no file specified, print message and exit
        cout << "No file specified!" << endl;
        return 0;
    }

    // Free the memory
    delete tree;

    return 0;
}

using namespace std; Never do this
Prefer using \n over std::endl as the latter flushes the buffer which is rarely needed and can be detrimental to performance.
const your arguments whenever possible. More about const correctness
* and & are considered to be part of the type in C++ so prefer writing int& foo instead of int &foo. Likewise for *.
Never omit optional braces as doing so will lead to bugs eventually.
Reaching the end of main will automatically return 0 so you can remove the trailing return 0
Don't initialize more than one variable per line.
I like that you use the prefix operator. Your naming is pretty good too so why are you doing this?

// Close the file
file.close();

or this

// Loop through arguments
for (int i = 0; i < argc; ++i)

Your comments are completely superfluous and can all be removed.
What is this?

// Free the memory
token = nullptr;
delim = nullptr;
delete token;
delete delim;

delete is only used after a matching new. It feels like you misunderstood something in regard to memory managment. Read up on this again.
new
delete
memory managment

BinarySearchTree *tree = new BinarySearchTree();

I don't know how this is implemented but a line like that looks like something from Java. Do you really need to put this on the heap?

Personally I think the word splitting could be done rather elegantly in regex. However many people are quite opposed to using regex. I've also read that in the past regex performance in C++ was rather poor but it might be worth measuring to see if it's within acceptable limits for your use-case.

Regex example:

std::regex rx{R"(([^ .,?!:;/\"\'\n\t]+))"};
auto data_begin = std::sregex_iterator(data.begin(), data.end(), rx);
auto data_end = std::sregex_iterator();
for (std::sregex_iterator it = data_begin; it != data_end; ++it) {
    // do something with words here
}