Simple checkboxes with React I was given the following exercise as part of a job interview process. I submitted my code and received the feedback that my solution is "close" but not "correct". My code executes just fine and displays the desired results. Can somebody please tell me what would have made my code more "correct"? or - did I misunderstand the requirements? (I realize that the Constructor is no longer needed but Codepen was complaining without it).

Requirements:

Using data variable implement a small Form using checkboxes

[x] Sonia
[ ] Maria
[ ] John
[x] Michael

On every form change on any checkbox print its current state, which should look like data.

const data = {
  sonia: true,
  maria: false,
  john: false,
  michael: true
};

My solution:

class App extends React.Component{

    constructor() {
        super();

        this.state = {
            data: {
                Sonia: false,
                Maria: false,
                John: false,
                Michael: false
            }
        };

        this.handleClick = this.handleClick.bind(this);
    }

    handleClick(event) {

        let data = this.state.data;

        const target = event.target;
        const value = target.type === 'checkbox' ? target.checked : target.value;
        const name = target.name;

        data = this.state.data;

        for (const i in data) {
            if (i === name) {
                data[i] = value
            }
        }
        this.setState( {data:data});
    }

    render() {
        return (

                <form>
                  {Object.keys(this.state.data).map((key, index) =>

                            <input
                                name={key}
                                id={index}
                                type="checkbox"
                                checked={this.state.data[key].value}
                                onChange={(event) => this.handleClick(event)} />

                                    {key}

                    )}
                </form>

Current State:

                        {Object.keys(this.state.data).map((key, index) =>

{ `${key}:  ${this.state.data[key]}`}

                        )}

        );
    }
}
ReactDOM.render(<App />, document.getElementById('app'));

I think everything Sam Onela said is great. I'm gonna add a few more things.

Don't modify your state directly

In general, it's considered bad practice to directly modify your state object. According to the react docs:

  Never mutate this.state directly, as calling setState() afterwards may replace the mutation you made. Treat this.state as if it were immutable.

Instead you should either

Pass setState an object with only what you want to change (I chose this)
Clone your state, modify the clone, then pass it to setState

Stop using index like that

Using index in your loops introduces another variable you don't need, and actually makes things slightly less correct.

Don't use index for key prop

The key prop should be a unique key to identify that element.
The most unique things we have about each of these checkboxes is the name that goes with them.
If we rearranged this list, we would want the same names to stay checked, not the same indexes in the list.

Don't use index as an id

At least not directly.
You want your ids to be unique on the page, and using the ids 1, 2, 3, etc. seems like not a good way to be unique. It would be better to use something like ${checkbox}-${key} so we get "checkbox-Sonia", "checkbox-Maria", etc.

It also used to be the case that ids needed to start with a letter and not a number, but I believe this is no longer the case.

You don't need an extra data layer in your state

It just adds needless comlexity. Just put the checkbox values right in your state, and only move them when you need to do more complex things with your state.

this.state = {
  Sonia: false,
  Maria: false,
  John: false,
  Michael: false
};

handleClick can be way simpler

One line in fact:

handleClick(key) {
    this.setState({ [key]: !this.state[key] });
}

Heck, it's so small that I would probably even put it inline, but different people might disagree on that.

Revised Version

Here's what my revised version of the code would look like:

class App extends React.Component {
  constructor(props) {
    super(props);
    // I guess you don't have props, so it doesn't really matter,
    // but you need to pass them to `super()` if you want them
    // to be available on `this.props`

    // Doesn't need the extra `data` layer
    this.state = {
      Sonia: false,
      Maria: false,
      John: false,
      Michael: false
    };
  }

  // handleClick was inlined

  render() {
    return (

        <form>
          {Object.keys(this.state.data).map((key) => (

              <input
                name={key}
                id={`checkbox-${key}`}
                type="checkbox"
                checked={this.state.data[key]}
                onChange={() => this.setState({ [key]: !this.state[key] })}
              />

                {/* Don't need a `key` prop here */}
                <label htmlFor={`checkbox-${key}`}>
                  {key}

          ))}
        </form>

Current State:

            {Object.keys(this.state).map(key => (

{`${key}: ${this.state.data[key]}`}

            ))}

    );
  }
}

Note that this does still use an arrow function and not a bound method.
I think that this lets to code be much more readable, and the possible performance loss wouldn't be realized until you have components that implement PureComponent deeper in the call chain receiving this function (currently it goes directly to an <input>.


Observer pattern implementation in C# In order to implement the Observer pattern in C#, one of the ways to go (at least, the one I chose) is to make classes that implement the IObservable<T> for the observable objects and the IObserver<T> for the observers.

In a project of mine I created a base class from which every observable inherits:

public class Observable<T> : IObservable<T>
{
    private SubscriptionManager<T> _subscriptionManager;

    public Observable()
    {
        _subscriptionManager = new SubscriptionManager<T>(
            new List<IObserver<T>>());
    }

    public IDisposable Subscribe(IObserver<T> observer)
    {
        _subscriptionManager.Subscribe(observer);

        return _subscriptionManager;
    }

    public void Notify(T obj)
    {
        _subscriptionManager.Notify(obj);
    }
}

and an IDisposable class that manages the subscriptions to the observable object:

public class SubscriptionManager<T> : IDisposable
{
    private ICollection<IObserver<T>> _observers;
    private IObserver<T> _observer;

    public SubscriptionManager(ICollection<IObserver<T>> observers)
    {
        if (observers == null)
        {
            throw new ArgumentNullException("observers");
        }

        _observers = observers;
    }

    public void Subscribe(IObserver<T> observer)
    {
        _observers.Add(observer);
        _observer = observer;
    }

    public void Notify(T obj)
    {
        foreach (var observer in _observers)
        {
            observer.OnNext(obj);
        }
    }

    public void Dispose()
    {
        _observers.Remove(_observer);
    }
}

Do you see anything that can be done more efficiently, or in a better way? Do you see anything else that may be wrong?

Here's a problem:

var o = new Observable<Unit>();
var a = o.Subscribe(_ => Console.WriteLine("A"));
var b = o.Subscribe(_ => Console.WriteLine("B"));
a.Dispose();
o.Notify(Unit.Default);

This prints

A

Disposing of a removed the wrong observer.

I'd recommend reading Why shouldn't I implement IObservable<T>?

  The reason you shouldn't implement IObservable<T> is the same reason
  you don't usually implement IEnumerable<T>, is that somebody has most
  likely already built the thing you want.


Securely delete a file I am trying to write a script to securely delete a file. What I am intending to do is to pad the file with /dev/zero once, and then with /dev/urandom three times and then delete the file.

For now the user only accepts one file at a time and the functionality is limited, if I am on the right track then I'll add more options later for recursive, verbose mode off/on, etc.  Is this a good way of doing the thing I want to do? Both from performance aspect and the security.

#! /bin/bash

if [ "$#" -eq 0 ]; then
    echo "ERR: Arguement is not supplied." 1>&2
    exit 1
elif [ "$#" -gt 1 ]; then
    echo "Too many arguments have been provided." 1>&2
    exit 1
fi

if [ ! -f "$1" ]; then
    echo "File $1 is not found" 1>&2
    exit 2
fi

filesize=$(stat -c "%s" "$1")
if [ "$filesize" -eq 0 ]; then
    filesize=1
fi

echo "Padding file with zeros"
dd if=/dev/zero of=$1 bs=$filesize count=1

echo "Padding file with randoms, first time"
dd if=/dev/urandom of=$1 bs=$filesize count=1

echo "Padding fxile with randoms, second time"
dd if=/dev/urandom of=$1 bs=$filesize count=1

echo "Padding file with randoms, third time"
dd if=/dev/urandom of=$1 bs=$filesize count=1

echo "Deleting the file"
rm -f -- $1

echo "File is securely deleted, or at least we think so."

Concept Review
There are a number of purpose built tools to do this job already. The one that comes to mind first is 'shred'. shred is installed by default on all Redhat and Ubuntu systems (in coreutils package on both systems). It is also installed on my RaspberryPi, so, it is ubiquitous.
Shred does similar things to what your script does, but it will be faster, and more secure, presumably. Running:
shred -u file

will repeatedly overwrite, and then delete the file.
Note that shred itself contains in the documentation:
   CAUTION: Note that shred relies on a very important assumption: that the file
            system overwrites data in place.  This is the traditional way to do
            things, but many modern file system  designs  do  not
            satisfy this assumption.  The following are examples of file systems
            on which shred is not effective, or is not guaranteed to be effective
            in all file system modes:

   * log-structured or journaled file systems, such as those supplied with AIX and
     Solaris (and JFS, ReiserFS, XFS, Ext3, etc.)

   * file systems that write redundant data and carry on even if some writes fail,
     such as RAID-based file systems

   * file systems that make snapshots, such as Network Appliance's NFS server

   * file systems that cache in temporary locations, such as NFS version 3 clients

   * compressed file systems

These same cautions apply to your code.
There is really only one way to manage content that you need to securely erase later, and that is to encrypt the data before you store it, or to store it in an encrypted filesystem.
Deleting the file then is as simple as 'forgetting' the key, or corrupting critical/small parts of it.
The bottom line is that since almost all Linux implementations now use ext3 or some other Journaled file system, that your secure erase is not likely going to be secure enough.
Code Review
Assuming the process would actually work in your setup....
As for your actual script, the code is neat enough, and the command-line arguments are handled OK. There are still a number of problems though:
you have no error-handling for any of your dd commands. if all the dd commands fail (perhaps someone is running in a chroot jail and /dev/zero and /dev/urandom are not accessible), but the rm works, it will seem like the process succeeded, but you have not actually changed any bytes on disk... which is bad.
A user who runs your command as:
securedelete "My Document"

would expect "My Document" file to be overwritten, but you have not quoted the $1 in any of the use-cases, so the dd and rm commands will fail with things like:
dd if=/dev/zero of=My Document bs=$filesize count=1

Actually, that's a good example of a bad failure. All the dd commands will fail, doing nothing, and then, at the end, you will have:
rm -f -- My Document

which will force-delete two files, one called My the other called Document.


Looking For a More Efficient/Elegant Way To Write a MySQL Query I'm querying a MySQL database and displaying the info in order of timestamp (DESC). Essentially, I'm grabbing the last record in the database, displaying it's date and year as the header of it's section and then going backwards from there. I've put together these if and else if statements to display the appropriate month name according to the timestamp. It works fine but I know I could probably do this with an array or some sort of other loop. I'm still learning this type of coding so I was wondering if someone could help me find a way to make this code more efficient. Thanks!

$sql = "SELECT * FROM user ORDER BY timestamp DESC LIMIT 1";
$result = $mysqli->query($sql);
$row_lastone = mysqli_fetch_array($result);
$timestamp_lastone = date_parse($row_lastone['timestamp']);
$month_lastone = $timestamp_lastone['month'];
$year_lastone = $timestamp_lastone['year'];
if ($month_lastone==1){
$lastone_month = "January";
}
else if ($month_lastone==2){
$lastone_month = "February";
}
else if ($month_lastone==3){
$lastone_month = "March";
}
else if ($month_lastone==4){
$lastone_month = "April";
}
else if ($month_lastone==5){
$lastone_month = "May";
}
else if ($month_lastone==6){
$lastone_month = "June";
}
else if ($month_lastone==7){
$lastone_month = "July";
}
else if ($month_lastone==8){
$lastone_month = "August";
}
else if ($month_lastone==9){
$lastone_month = "September";
}
else if ($month_lastone==10){
$lastone_month = "October";
}
else if ($month_lastone==11){
$lastone_month = "November";
}
else if ($month_lastone==12){
$lastone_month = "December";
}
echo "<h1>".$lastone_month." ".$year_lastone."</h1>";

Thanks for the help!

Try something like this:

$sql = "SELECT * FROM user ORDER BY timestamp DESC LIMIT 1";
$result = $mysqli->query($sql);
$row_lastone = mysqli_fetch_array($result);

$date = date("M Y", strtotime($row_lastone['timestamp']));

echo "<h1>".$date."</h1>";


Return whether the cards can be rearranged I solved this question on LeetCode.com:

Alice has a hand of cards, given as an array of integers.  Now she wants to rearrange the cards into groups so that each group is size W, and consists of W consecutive cards.  Return true if and only if she can.  For e.g., for hand=[1,2,3,6,2,3,4,7,8] and W=3, the answer should be true.

as:
class Solution {
public:
    bool isNStraightHand(vector<int>& nums, int k) {
        if(nums.size()%k!=0) return false;

        map<int, int> _m;
        for(int num: nums) _m[num]++;

        while(_m.size()) {
            auto it=_m.begin();
            int count=1;
            int prev=it->first;
            while(count<=k) {
                it->second--;
                if(count>1 && it->first-prev!=1) return false;
                else prev=it->first;
                count++;
                if(it->second==0) {
                    auto backupIt=it;
                    _m.erase(backupIt);     //am I causing UB here?
                }
                it++;
            }
        }

        return true;
    }
};

This works, but it doesn't look like a sturdy solution. I am curious to know if I am causing Undefined Behavior (UB) when erasing the element above.  Earlier, I just had _m.erase(it);, but that wasn't good either.  I think so, since the official website says:

References and iterators to the erased elements are invalidated.

so, when I do a it++ in the following line, isn't that invalid? That part in particular can probably be improved.

The answer is "yes" it is UB. To fix it just rewrite the part as:
if(it->second==0)
{
    it = _m.erase(it);
}
else
{
     it++;
}


Perl mutiple children creation My program will fork, launching $n children. When a child finishes, the parent will launch a new one, if we have more to launch.

#!/usr/bin/perl

use strict;
use warnings;
use POSIX ":sys_wait_h";

#function, that every child will run
sub func
# {{{
{
      my $i = shift;
      my $delay = shift;

      print "$i started\n";
      sleep($delay);
      print "$i finished\n";
}
# }}}

#max number of concurrent children
my $max_n = 2;

#number of seconds each child will sleep
my @delays = (8, 5, 5);
my $n_elements = scalar(@delays);

#number of currently running children
my $n;
if ($n_elements > $max_n) {
      $n = $max_n;
}
else {
      $n = $n_elements;
}

#initial children creation
my $i;
for ($i = 0; $i < $n; $i++) {
      my $pid = fork();
      if ($pid == 0) {
            #child runs the function
            func($i, $delays[$i]);
            exit(0);
      }
}

#any children still running?
while ($n > 0) {
      #wait until one of them dies
      my $dead_child_pid = wait();
      print "$dead_child_pid done\n";

      #do we need to create new children?
      if ($i < $n_elements) {
            #create a child
            my $pid = fork();
            if ($pid == 0) {
                  func($i, $delays[$i]);
                  exit(0);
            }
            else {
                  #move pointer to the next element of @delays
                  ++$i;
            }
      }
      else {
            #decrease number of running children
            $n--;
      }
}

print "parent finished\n";

When I run it, it prints:

1 started
0 started

Here I check the processes:

bash-4.2$ ps -A | grep perl
 3433 pts/2    00:00:00 perl
 3434 pts/2    00:00:00 perl
 3435 pts/2    00:00:00 perl

So, it created 3 processes: 3433 - parent, 3434 and 3435 - children. Then:

1 finished
3435 done
2 started

Now again:

bash-4.2$ ps -A | grep perl
 3433 pts/2    00:00:00 perl
 3434 pts/2    00:00:00 perl
 3438 pts/2    00:00:00 perl

It launched the 3-rd child with pid 3438 after the death of the 2-nd.

0 finished
3434 done
2 finished
3438 done
parent finished

So, everything seems to be correct with the exception of the reversed launching order of children in the 1-st loop.

Maybe, it's possible to unite children creation from the both loops into a function. As their code has similar parts:

  my $pid = fork();
  if ($pid == 0) {
        #child runs the function
        func($i, $delays[$i]);
        exit(0);
  }

Do you see any more drawbacks in my code?

I think this can be improved by using queues as the main construct to convey what is going on.

First, you have a pending queue:

my @pending = (8, 8, 5);

Second, you have an empty queue with the currently active workers:

my @workers;

And this queue has a size limit:

my $max_workers = 2;

This can all be run by a single while loop:

while(@workers < $max_workers) {
    ... add one more worker ...
    ... remove all workers that are done ...
    ... break out of loop is @pending and @workers are both empty
    ... wait for a worker to exit (or sleep) ...
}

When this loop exits, you're all done.


RESTful API for Todos using Hapi My goal is to create a simple restful api that will be accessed by an AngularJS front end. Even though it's fairly simple, I'd like understand how to make it more reliable, secure, and best-practices compliant. The whole api is contained in a single file, I'm not overly concerned about organization at the moment (unless it can help with the items I've mentioned above). I'm using mongoose, hapi, and joi for validation.

server.js

'use strict';

var Hapi = require('hapi');
var Good = require('good');
var Mongoose = require('mongoose');
var Joi = require('joi');

Mongoose.connect('mongodb://xxxx:xxxx@proximus.modulusmongo.net:xxxx/xxxx');

var todoSchema = new Mongoose.Schema({
  title: { type: String, required: true },
  isCompleted: { type: Boolean, required: true },
  createdAt: { type: Date, required: true },
  updatedAt: { type: Date, required: true }
});

var Todo = Mongoose.model('Todo', todoSchema, 'Todos');

var server = new Hapi.Server(3000);

server.route({
  method: 'GET',
  path: '/api/v1/todos',
  handler: function(request, reply) {

    Todo.find(function(err, todos) {
      if (err) {
        reply(err);
        return;
      }

      reply(todos);
    });
  }
});

server.route({
  method: 'GET',
  path: '/api/v1/todos/{id}',
  handler: function(request, reply) {

    Todo.find({ _id: request.params.id }, function(err, todo) {
      if (err) {
        reply(err);
        return;
      }

      reply(todo);
    });

  },
  config: {
    validate: {
      params: {
        id: Joi.string().required()
      }
    }
  }
});

server.route({
  method: 'PUT',
  path: '/api/v1/todos/{id}',
  handler: function(request, reply) {

    Todo.findOne({ _id: request.params.id }, function(err, todo) {
      if (err) {
        reply(err);
        return;
      }

      todo.title = request.payload.title;
      todo.isCompleted = request.payload.isCompleted;
      todo.updatedAt = new Date();
      todo.save();

      reply(todo);
    });
  },
  config: {
    validate: {
      params: {
        id: Joi.string().required()
      },
      payload: {
        _id: Joi.string(),
        __v: Joi.number(),
        title: Joi.string(),
        isCompleted: Joi.boolean(),
        createdAt: Joi.date(),
        updatedAt: Joi.date()
      }
    }
  }
});

server.route({
  method: 'POST',
  path: '/api/v1/todos',
  handler: function(request, reply) {

    var newTodo = new Todo({
      title: request.payload.title,
      isCompleted: request.payload.isCompleted,
      createdAt: new Date(),
      updatedAt: new Date()
    });

    newTodo.save(function(err, todo) {
      if (err) {
        reply(err);
        return;
      }

      reply(todo);
    });
  },
  config: {
    validate: {
      payload: {
        title: Joi.string(),
        isCompleted: Joi.boolean(),
      }
    }
  }
});

server.route({
  method: 'DELETE',
  path: '/api/v1/todos/{id}',
  handler: function(request, reply) {

    Todo.findByIdAndRemove(request.params.id, function(err, todo) {
      if (err) {
        reply(err);
        return;
      }

      reply(todo);
    });
  },
  config: {
    validate: {
      params: {
        id: Joi.string().required()
      }
    }
  }
});

server.pack.register(Good, function(err) {
    if (err) {
      throw err;
    }

    server.start(function() {
      server.log('info', 'Server running at: ' + server.info.uri);
    });
});

I really like your code.

Use of 'use strict';
No comments, but the code is written so that they are not needed anywhere
Good naming, good size of functions, things are in their logical place

The only thing is that I would not put uid/pwd in the source code, but use environment variables:

Mongoose.connect('mongodb://xxxx:xxxx@proximus.modulusmongo.net:xxxx/xxxx');


Code to find value into a table by 2 criteria (x,y) My code finds the value that corresponds to two values in a table. It works fine, but I'd like to know what you might do differently.

Table sample:

Input:

=findval(3200,100)

Output:

4,6

Code:

Function findval(x As String, y As String)

Dim LastRow As Long
Dim LastCol As Integer
Dim x_rgn As Range
Dim y_rgn As Range
    With ActiveSheet
        LastRow = .Cells(.Rows.Count, "A").End(xlUp).Row
    End With
    With ActiveSheet
        LastCol = .Cells(1, .Columns.Count).End(xlToLeft).Column
    End With

Set x_rgn = Range(Cells(1, 1), Cells(1, LastCol))
Set y_rgn = Range(Cells(1, 1), Cells(LastRow, 1))

With x_rgn
    Set val_x = .Find(What:=x, _
                        After:=.Cells(.Cells.Count), _
                        LookIn:=xlFormulas, _
                        LookAt:=xlWhole, _
                        SearchOrder:=xlByRows, _
                        SearchDirection:=xlNext, _
                        MatchCase:=False)
    val_x = val_x.Address
    val_x = Range(val_x).Column
End With
With y_rgn
    Set val_y = .Find(What:=y, _
                        After:=.Cells(.Cells.Count), _
                        LookIn:=xlFormulas, _
                        LookAt:=xlWhole, _
                        SearchOrder:=xlByRows, _
                        SearchDirection:=xlNext, _
                        MatchCase:=False)
    val_y = val_y.Address
    val_y = Range(val_y).Row
End With
findval = Cells(val_y, val_x).Value
End Function

First step:

Write down your assumptions

These would be mine;

This data table has a header row in row 1 and a header column in column A.
The values in the headers could be any legal text.
The values in the headers are unique

What's the fastest way to find an exact match in a list?

My answer: A dictionary (or any other Linked-List object)

Dim rowList as Scripting.Dictionary, columnList as Scripting.Dictionary
    Set rowList = New Scripting.Dictionary
    Set columnList = New Scripting.Dictionary

    finalrow = Cells(Rows.Count, 1).End(xlUp).Row
    finalColumn = Cells(1, Columns.Count).End(xlToLeft).Column

Dim cellText As String

    For i = 1 To finalrow
        cellText = Cells(i, 1).Text
        rowList.Add cellText, i
    Next i

    For i = 1 To finalColumn
        cellText = Cells(1, i).Text
        columnList.Add cellText, i
    Next i

Now we have 2 dictionaries, where each row/column (i) is referenced by its cell value.

rowList.item("0") will return row "5", "100" returns row "6" and so on. This will work with any size of table and any possible values for the headers, so long as there are no repeated values.

Now you just take your 2 input values, access the associated items, and either they exist and you have your co-ordinates, or they don't exist, and you can tell your user.

For added speed, read your row/column into an array before iteratively adding them to your list.

Bonus: Aside from the initial time to create the lists, searches are effectively instant for any size of list and any number of searches.


Controller to list users I think I put too much code in my controller that was supposed to go in the model.

This is a part of my controller, I'm not gonna paste everything since there is a lot of code.

  public function ajaxUsers() {
    if($_GET["action"] == "listUsers") {

        if(!isset($_POST["search"])) {
            $this->_data['Records'] = $this->_model->getUsers();
            $this->_data['Result'] = "OK";
            $this->_data['TotalRecordCount'] = $this->_model->countUsers();
        }
        else {
            foreach($_POST['fields'][0] as $key => $post) {
                if ($post != "" && $key != "reg_date") {
                    $searchTerms = explode(' ', $post);
                    foreach ($searchTerms as $term) {
                        $term = trim($term);
                        if (!empty($term)) {
                            $like[] = $key." LIKE '%".trim($term, '\'')."%'";
                        }
                    }

                }
                else if ($post != "" && $key == "reg_date") {
                    foreach ($post[0] as $key2 => $date) {
                        $datetofrom = strtotime($date);
                        $datetofrom = date('Y-m-d', $datetofrom);
                        if ($date != "" && $key2 == "datefrom") {
                            $like[] = "DATE_FORMAT(".$key.", '%Y-%m-%d') >= '".$datetofrom."'";

                        }
                        if ($date != "" && $key2 == "dateto") {
                            $like[] = "DATE_FORMAT(".$key.", '%Y-%m-%d') <= '".$datetofrom."'";
                        }
                    }
                }
            }

            ($like) ? $where_clause = "WHERE ". implode(' AND ', $like) : $where_clause = "";
            $this->_data['Records'] = $this->_model->filterUsers($where_clause);
            $this->_data['Result'] = "OK";
            $this->_data['TotalRecordCount'] = $this->_model->countfilterUsers($where_clause);
        }
        echo json_encode ($this->_data);
    }
}

And my model is mostly database queries:

  public function getUsers() {
    $data = $this->_db->select("SELECT * FROM ".PREFIX."users" . $this->_sort);
    return $data;
}

public function countUsers() {
    $data = $this->_db->select("SELECT count(*) as id FROM ".PREFIX."users");
    return $data[0]->id;
}

public function filterUsers($like_clause) {
    $data = $this->_db->select("SELECT * FROM ".PREFIX."users " .$like_clause. $this->_sort);
    return $data;
}
public function countFilterUsers($like_clause) {
    $data = $this->_db->select("SELECT count(*) as id FROM ".PREFIX."users ".$like_clause);
    return $data[0]->id;
}

Should I moved the foreach loops in the model?

Should I moved the foreach loops in the model?

Yes, definitely.

Currently, you have SQL in the model and the controller, which isn't a good idea. It makes it hard to make out what the actual query will be, and splitting up one functionality (database lookup in this case) in multiple classes is not a good idea in general. SQL should be part of the user model (or DAO), so create a method called searchBy($keywordsArray) there and use that.

Security

You are completely open to SQL injection, as you put $_POST values directly into your query as column name and like clause. When putting variable content into queries, you should always use prepared statements to defend against SQL injection if possible.

When it's not possible, for example with variable column names, then you should prepare a whitelist of your columns, and check if the user input is in it.


Code to construct a complete graph I have tried to design a class for a complete graph.

Needless to say, disadvantages of my current solution are very visible:

dependent inputs have led to verification headaches.

In this code we rely on 2 inputs set of nodes and edge map. This creates the need to add a special verification function called addEdgeVerification to ensure both these
data structures are in sync.how to avoid this complexity of inter-parameter dependency at the same not burdening the client with headache of providing a complex data structure as input to our code ?
forces illegal states when some functions are invoked out of sequence.

This code returns illegal state if getAdj is called before complete graph has been constructed. How can this illegal state issues be avoided, without the need to force user to provide a complete graph ? Note the name of code is CompleteGraphConstructor. This means users will make use of our code to construct a complete graph. We dont users to take any effort to construct graph themselves. We want them to provide tiny inputs such as edges and nodes, and we should take the pain to join pieces.

But alternatives are also not free from handicaps:

dumping everything in constructor would make it a god constructor

A constructor can help us reduce/eliminate illegal state. But it would result in god constructors.  It does not look like a good option, either.

I am looking for recommendations on code architecture.

public class CompleteGraphConstructor<T> {

    private final Map<T, HashMap<T, Double>> graph;
    private Set<T> nodes;

    public CompleteGraphConstructor() {
        graph = new HashMap<T, HashMap<T, Double>>();
    }

    /**
    * Add the nodes needed to be part of a graph.
    *
    * @param nodes     the set nodes to add in the graph.
    * @return           true if nodes list is set, and false if it is not.
    */
    public boolean addNodes (Set<T> nodes) {
        // prevent overriting the nodes.
        if (this.nodes != null) {
            /**
            * Should exception check be performed inside or outside?
            */
            if (nodes == null) throw new NullPointerException("The node cannot be null.");
            if (nodes.size() == 0) throw new NullPointerException("The size of node cannot be zero.");
            this.nodes = nodes;
            return true;
        }
        return false;
    }

    /**
    * Add all edges of the graph.
    *
    * @param nodeId  if nodeId is not null.
    * @param allEdges   all the edges of input node
    */
    public void addEdges (T nodeId, Map<T, Double> allEdges) {

        addEdgeVerification(nodeId, allEdges);

        graph.put(nodeId, (HashMap<T, Double>) allEdges);
    }

    private void addEdgeVerification(T nodeId, Map<T, Double> allEdges) {
        if (!nodes.contains(nodeId)) throw new NoSuchElementException("The source node: " + nodeId + " does not exist.");

        // making sure that edges include nodes provided in the node Set.
        for (T node : allEdges.keySet()) {
            if (!nodes.contains(node)) throw new NoSuchElementException("The target node: " + nodeId + "  not exist.");
        }

        // make sure that all the nodes in the edge set are included in allEdges
        for (T node : nodes) {
            if (node != nodeId) {
                if (!allEdges.containsKey(nodeId)) throw new IllegalArgumentException("The input map does not contain all edges. ");
                }
            }
        }

    public Map<T, Double> getAdj (T nodeId) {
        if (!nodes.contains(nodeId)) throw new NoSuchElementException("The node " + nodeId + " does not exist.");
        if (!graph.containsKey(nodeId)) throw new IllegalStateException("The graph is not populated with " + nodeId);

        return graph.get(nodeId);
    }
}

I think you expect addNodes to be called exactly once, and expect it to be called before any other method.

Therefore, remove the addNodes method, and move the Set<T> nodes parameter to the constructor.

The next thing you want to do is define all edges. To do that you expect the user to call addEdges repeatedly (one for each node).

A simpler way to do that might be to pass in a list of all edges. If you do the following then you only need to call the method once (because the List can contain all edges for all nodes):

public class Edge<T>
{
    T node1;
    T node2;
    Double d;
}

public void addEdges(Iterable<Edge> allEdges) {
    for (Edge edge : allEdges) {
        addEdge(edge.node1, edge.node2, edge.value);
        addEdge(edge.node2, edge.node1, edge.value);
    }
}

private void addEdge(T node1, T node2, Double value) {
    if (!graph.containsKey(node1))
        graph.put(node1, new HashMap<T, Double>());
    HashMap<T, Double> map = graph.get(node1);
    if (map.containsKey(node2))
        throw new DuplicateEdgeException("Duplicate " + node1 + " to " + node2);
    map.put(node2, value);
}

Your Set<T> nodes is only used for assertions. I'm not sure how useful that is.

Also I don't understand your comment about a "god constructor". A constructor initializes the object using input data: that is something to love.

In summary, perhaps a class like the following. This constructs an arbitrary graph, using only a collection of edges as its input; it supports an isComplete method to test whether the resulting graph is complete (which, you could call as an assertion in the constructor):

class Graph
{
    public class Edge<T>
    {
        T node1;
        T node2;
        Double d;
    }

    private final Map<T, HashMap<T, Double>> graph;

    public Graph(Iterable<Edge> allEdges) {
        graph = new HashMap<T, HashMap<T, Double>>();
        for (Edge edge : allEdges) {
            addEdge(edge.node1, edge.node2, edge.value);
            addEdge(edge.node2, edge.node1, edge.value);
        }
    }

    private void addEdge(T node1, T node2, Double value) {
        if (!graph.containsKey(node1))
            graph.put(node1, new HashMap<T, Double>());
        HashMap<T, Double> map = graph.get(node1);
        if (map.containsKey(node2))
            throw new DuplicateEdgeException("Duplicate " + node1 + " to " + node2);
        map.put(node2, value);
    }

    public bool isComplete()
    {
        int count = graph.size();
        for (HashMap<T, Double> map : graph.values())
            if (count != (map.size() + 1))
                return false;
        return true;
    }

    public Map<T, Double> getAdj (T nodeId) {
        if (!graph.containsKey(nodeId)) throw new IllegalStateException("The graph is not populated with " + nodeId);
        return graph.get(nodeId);
    }
}


Select value based on condition on dataframe I have some data in data frame and would like to return a value based on specific conditions.  It is highly time consuming.

I tried three methods:

Method 1:
Without dataframe, this is the simple logic I have and it is super fast.

@numba.vectorize(['float64(float64, float64)'])
def Method1(a,b):
    x=0.0
    y=0.0
    z=0.0

    if (a <= 0.002):
        x=0.5
        y=2500
        z=20000
    elif (a <= 0.003):
        x=0.3
        y=2500
        z=15000
    elif (a <= 0.005):
        x=0.2
        y=1000
        z=10000
    else:
        return 0.0

    return min(max(x*b,y),z)

%timeit Method1(0.001,200000)

Method 2 - Input the condition data as a dataframe and run the function

dict = {'amin':[0.000,0.002,0.003],
       'amax':[0.002,0.003,0.005],
       'dfx':[0.5,0.3,0.2],
       'dfy':[2500,2500,1000],
       'dfz':[20000,15000,10000]}
df=pd.DataFrame(dict)

@numba.vectorize(['float64(float64, float64)'])
def Method2(a,b):
    x=0.0
    y=0.0
    z=0.0

    x=df[(a<=df.amax) & (a>=df.amin)]['dfx'].values
    y=df[(a<=df.amax) & (a>=df.amin)]['dfz'].values
    z=df[(a<=df.amax) & (a>=df.amin)]['dfy'].values

    if (len(x)==0) or (len(y)==0) or (len(z)==0):
        return 0.0
    else:
        return min(max(x[0]*b,y[0]),z[0])
%timeit Method2(0.001,200000)

Method 3 - looped the rows of the df

def Method3(a,b):
    for index,row in df.iterrows():
        if (mPD >= row['amin']) & (mPD <= row['amax']):
            return min(max(row['dfx']*b,row['dfy']),row['dfz'])
    return 0.0
%timeit Method3(0.001,200000)

Method 1 gets finished in 1.2 micro seconds

Method 2 takes 2.47 milli seconds (1000 times slower than the Method 1)

Method 3 takes ~80 micro seconds

Please help me how to improve the performance of Method 2 / 3.
Also, please let me know why Method 3 is faster?

P.S. I plan to use Numba so cannot use lambda in the functions.

There is some overhead to numpy, and even more overhead to pandas. You won't be able to attain the performance of Method1 using pandas.

I'll comment on the methods one at a time:

Method1

There is no need to initialize x, y, and z.
You don't deal with the case where a is negative
For me, Method1 is twice as fast when I leave off the @numba decorator.

Method2

Don't name a dictionary dict! This is the name of the class. I've renamed your dict as param_dict below.
Again, there is no need to initialize x, y, and z.
You're running the two inequalities three times each.

Better would be to set valid_rows = (a<=df.amax) & (a>=df.amin) or similar.

def Method2a(a,b):
    valid_rows = (a <= df.amax) & (a >= df.amin)
    if not any(valid_rows):
        return 0.0
    x=df[valid_rows]['dfx'].values
    y=df[valid_rows]['dfz'].values
    z=df[valid_rows]['dfy'].values
    return min(max(x[0]*b,y[0]),z[0])

With a setup where Method2 takes 2.25 ms, this takes 1.35 ms.

Accessing a dataframe at a boolean array is slower than at an index.

It's much better to find the first True index of valid_rows first.

def Method2b(a,b):
    valid_rows = (a <= df.amax) & (a >= df.amin)
    if not any(valid_rows):
        return 0.0
    idx = np.where(valid_rows)[0][0]
    x=df['dfx'].iat[idx]
    y=df['dfz'].iat[idx]
    z=df['dfy'].iat[idx]
    return min(max(x*b,y),z)

This takes 531 µs

Drilling down into this, just the first line takes 472 µs (try it with the method truncated after the first line, not returning anything)! That's where we can improve.

We don't really need two sets of comparisons. There's enough information in df.amin together with the final value of df.amax:

def Method2c(a,b):
    idx = np.searchsorted(df.amin.values, a) - 1
    # special case if a == df.amin.iat[0]
    if idx < 0:
        if a == df.amin.iat[0]:
            idx = 0
        else:
            return 0.0
    # special case if a is bigger than all values in df.amin
    elif idx == df.shape[0] - 1:
        if a > df.amax.iat[idx]:
            return 0.0
    x=df['dfx'].iat[idx]
    y=df['dfz'].iat[idx]
    z=df['dfy'].iat[idx]
    return min(max(x*b,y),z)

This takes 38.6 µs. Note the idea is closer to your approach in Method1.

This is about as well as we can hope to do with pandas, as the bottle neck is now actually the three accesses:

    x=df['dfx'].iat[idx]
    y=df['dfz'].iat[idx]
    z=df['dfy'].iat[idx]

which actually takes 24.6 µs by itself!

Edit: Actually, using .values instead of .iat saves a good amount here, cutting the whole run time down to 19.3 µs for:

def Method2c_values(a,b):
    idx = np.searchsorted(df['amin'].values, a) - 1
    # special case if a == df['amin'].values[0]
    if idx < 0:
        if a == df['amin'].values[0]:
            idx = 0
        else:
            return 0.0
    # special case if a is bigger than all values in df.amin
    elif idx == df.shape[0] - 1:
        if a > df['amax'].values[idx]:
            return 0.0
    x=df['dfx'].values[idx]
    y=df['dfz'].values[idx]
    z=df['dfy'].values[idx]
    return min(max(x*b,y),z)

If we go back to the dictionary (which I'm calling param_dict) we can speed it up quite a bit:

def Method2d(a,b):
    idx = np.searchsorted(param_dict['amin'], a) - 1
    # special case if a == df.amin.iat[0]
    if idx < 0:
        if a == param_dict['amin'][0]:
            idx = 0
        else:
            return 0.0
    # special case if a is bigger than all values in df.amin
    elif idx == len(param_dict['amin']) - 1:
        if a > param_dict['amax'][idx]:
            return 0.0
    x=param_dict['dfx'][idx]
    y=param_dict['dfz'][idx]
    z=param_dict['dfy'][idx]
    return min(max(x*b,y),z)

This takes 6.91 µs. Now the bottleneck is back to the first line, which is taking 5.66 µs by itself.

We can rewrite this to do np.searchsorted ourselves, with the extra logic for the special cases worked in:

def Method2e(a,b):
    idx = 0
    for value in param_dict['amin']:
        if a < value:
            # if idx is 0, we're out of bounds
            if not idx:
                return 0.0
            break
        elif a == value:
            # if idx is 0, we need to adjust by 1
            if not idx:
                idx = 1
            break
        idx += 1
    else:
        # a is larger than every element of param_dict['amin']
        if a > param_dict['amax'][-1]:
            return 0.0
    idx -= 1
    x=param_dict['dfx'][idx]
    y=param_dict['dfz'][idx]
    z=param_dict['dfy'][idx]
    return min(max(x*b,y),z)

This is 823 ns. We could tweak mildly to put the idx == 0 part out of the main loop and other such things, but I'll leave it as is.

Method3

This method is fine, except that

def Method3(a,b):
    for index,row in df.iterrows():
        pass

takes 112 µs for me.


Improving iterative manipulation on XML elements In this reply, there's an example of how to remove a node from a XML structure.

XElement doc = XElement.Load("test.xml");
doc.XPathSelectElement("//beep").Remove();

Suppose now that we wish to remove not a single node (specifying it by the string value) but a set of nodes (specifying them by an array of strings).

One way to resolve it is to loop through the array and execute the removal for each string element. However, I'd like to know if someone can suggest a neater approach.

IEnumerable<String> beeps = new[] { "//beep" };
XElement doc = XElement.Load("test.xml");
foreach(String beep in beeps)
  doc.XPathSelectElement(beep).Remove();

Let us say you have a node like this which is having two nodes , node1 and node 2, you need to specify xpath using "|" seperated . here is an example .

XPathSelectElement should be replaced with XPathSelectElements.

<?xml version="1.0" encoding="utf-8" ?>
<customer>
  <node1>
  </node1>
  <node2>
  </node2>
</customer>

//all you need sepcify is a "|" to work it out.
doc.XPathSelectElements("//node1 | //node2").Remove();


Use local storage event I use the following code which is working OK to register and emit event for local storage on the browser. It will be great if you can give me some ideas on how to improve it for production use since I'm fairly new to JS.

"use strict";
    function lsHelper() {
        let status = "available";
        let timeout = 5000;

        //Register a function by its name, itself (callback) and the needed time span for the check cycle
        this.RegisterLocalStorageFunction = function(name, callback) {

            //Set local storage items for function and its result
            resetFunction(name);

            //Setting the event
            window.addEventListener('storage', function(e) {
                //Getting the params or eventually just the status

                //Call the function if params are set and reset all
                if (e.key === name && e.newValue != status) {
                    let result = callback(e.newValue);
                    //Set result
                    localStorage.setItem(name + "Result", result);
                    //Reset call
                    localStorage.setItem(name, status);
                }
            });
        }

        this.CallLocalStorageFunction = function(name, params, callback) {
            //Check if the function is already called
            if (localStorage.getItem(name) == placeholder
                && localStorage.getItem(name + "Result") == status) {

                //Call remote function
                localStorage.setItem(name, params);

                //Define timeout and event for future usage
                let timeout = {};
                let storageEvent = {};

                //Check in interval for result
                storageEvent = function(e) {
                    //Check wheter the result is set
                    if (e.key == name + "Result" && e.newValue != status) {
                        //Call callback with results
                        callback(e.newValue);

                        //Reset result in local storage
                        localStorage.setItem(name + "Result", status);

                        //Dismiss listener and timeout
                        clearTimeout(timeout);
                        window.removeEventListener('storage', storageEvent, false);
                    }
                }

                //Add listener to event
                window.addEventListener('storage', storageEvent);

                //Start timout to cancel listener if no result is set, hence no remote function was called
                timeout = setTimeout(function() {
                    //Dismiss interval
                    clearTimeout(timeout);
                    //Clean up
                    window.removeEventListener('storage', storageEvent, false);
                }, timeout);
            }
        }

        //Reset local storage values
        function resetFunction(name) {
            localStorage.setItem(name, status);
            localStorage.setItem(name + "Result", status);
        }
    };

first of all remove your global use strict.

Why? Global use strict declarations can lead to big fails in your software.

From Mozilla website:

  This syntax has a trap that has already bitten a major site: it isn't possible to blindly concatenate non-conflicting scripts. Consider concatenating a strict mode script with a non-strict mode script: the entire concatenation looks strict! The inverse is also true: non-strict plus strict looks non-strict. Concatenation of strict mode scripts with each other is fine, and concatenation of non-strict mode scripts is fine. Only concatenating strict and non-strict scripts is problematic.

So what you can do is to use the module pattern:

var lsHelper = (function() {

    "use strict";
    let status = "available";
    let timeout = 5000;

    return {
       //Register a function by its name, itself (callback) and the needed time span for the check cycle
        RegisterLocalStorageFunction: function(name, callback) {

            //Set local storage items for function and its result
            resetFunction(name);

            //Setting the event
            window.addEventListener('storage', function(e) {
                //Getting the params or eventually just the status

                //Call the function if params are set and reset all
                if (e.key === name && e.newValue != status) {
                    let result = callback(e.newValue);
                    //Set result
                    localStorage.setItem(name + "Result", result);
                    //Reset call
                    localStorage.setItem(name, status);
                }
           });
       },
        CallLocalStorageFunction: function(name, params, callback) {
            //Check if the function is already called
            if (localStorage.getItem(name) == placeholder &&
                localStorage.getItem(name + "Result") == status) {

                //Call remote function
                localStorage.setItem(name, params);

                //Define timeout and event for future usage
                let timeout = {};
                let storageEvent = {};

                //Check in interval for result
                storageEvent = function(e) {
                    //Check wheter the result is set
                    if (e.key == name + "Result" && e.newValue != status) {
                        //Call callback with results
                        callback(e.newValue);

                        //Reset result in local storage
                        localStorage.setItem(name + "Result", status);

                        //Dismiss listener and timeout
                        clearTimeout(timeout);
                        window.removeEventListener('storage', storageEvent, false);
                    }
                }

                //Add listener to event
                window.addEventListener('storage', storageEvent);

                //Start timout to cancel listener if no result is set, hence no remote function was called
                timeout = setTimeout(function() {
                    //Dismiss interval
                    clearTimeout(timeout);
                    //Clean up
                    window.removeEventListener('storage', storageEvent, false);
                }, timeout);
            }
        },

        //Reset local storage values
        resetFunction: function(name) {
            localStorage.setItem(name, status);
            localStorage.setItem(name + "Result", status);
        }
    };
})();

Second. The name lsHelper does not tell what the function does. I think that localStorageHelper is a better name.


User model with user roles in Codeigniter PHP/MySQL I am building an app that currently allows the users to have 3 different roles:

Consumer
Merchant
Admin

I have three tables:

user (Name, email, password etc with a status=0/1)
user_role (The list of roles available)
user_user_role (The table joining users to user roles in a many to many relationship. Also a status column indicating if the role is disabled for the user.)

But only two models:

User_model
User_role_model

In my User_model:

/**
 * @param $user_data
 * @param array $roles
 * @return bool|object
 */
function insert( $user_data, $roles = [] ){
    // Hash password
    if ( isset( $user_data['password'] ) ) {
        $user_data['password'] = password_hash($user_data['password'], PASSWORD_DEFAULT);
    }
    if ( $user_id = $this->db->insert( $this->table_name, $user_data ) ) {

        // Add user roles
        if (count($roles)) {
            foreach ($roles as $role_id) {
                $this->add_role($user_id, $role_id);
            }
        }

        return $user_id;

    } else {
        return FALSE;
    }
}

/**
 * Add role to user
 * @param $user_id
 * @param $role_id
 */
function add_role( $user_id, $role_id ){
    $this->db->insert(self::User_user_role_table_name, [
        'user_id' => $user_id,
        'user_role_id' => $role_id,
        'status' => self::Status_active,
        'created_on' => NULL,
        'modified_on' => NULL
    ]);
}

My questions:

Is my database schema ideal? (Each role has access to a specific area with each area's controller extended from a core controller. i.e. Dashboard extends Admin_controller - which extends from a global controller. I don't foresee a need to have complex permissions in a typical RBAC.)
Should I have 1 model instead of 2?
Should I have the roles parameter in my insert function?

I think the schema is fine with one possible exception. I don't see any value in the status column in the user_user_role table. The same effect is achieved by removing the row connecting the user and a given role. If the connections isn't there then that role is 'disabled' for that user.

Unless you intend to add functionality that adds, removes or otherwise manages roles then one model is all you need.

Roles as a param to insert makes sense but perhaps a default value (beyond an empty array) should be provided. Otherwise, you could end up with users that don't have permission to do anything. Or, keep the definition $roles = [] and then if(empty($roles)), throw an exception.

I think you have a problem in the line

if ( $user_id = $this->db->insert( $this->table_name, $user_data ) ) {

$this->db->insert() returns either true or false. To get the id for the newly inserted records you will need to use $this->db->insert_id(); perhaps like this.

if ($this->db->insert( $this->table_name, $user_data ) ) {
    $user_id = $this->db->insert_id();
    ...

Also, in the interest of elegant code instead of

return $user_id;
} else {
    return FALSE;
}

use

} //end of if ($this->db->insert(...

return isset($user_id) ? $user_id : FALSE;


Using hash_file for image recognition My team is writing software to be able to grade a basic skills in word/excel/powerpoint test. We are trying to figure out the best way to verify that they included a correct image in their word doc. We know how to get the image file that they use through the xml of the word file. The problem is when we want to verify that the image is the one we expect to be there. Right now we are using the hash_file function in php:

//we know this is the correct hash for the image
$img_hash_correct = 'ab9df995315e2c5cbd0d5cf2210410afa44958c3ca55147497c7a6761340e058';

$hashed_file_submitted = hash_file("sha256", 'hashtest/word/media/image2.jpg');

if ($hashed_file_submitted == $img_hash_correct) {
   echo 'same image!   ';
} else {
   echo 'these are not the same   ';
}

This code has worked after doing some testing. Is this a reliable way to verify that a submitted image is what we are looking for, or is there a more reliable way that you know of?

Without knowing much about your use case, the code looks straightforward. It does exactly what you specify and nothing more.

A few points:

The constant image hash can be defined as global(?)
Use === for comparison.


Returning a database connection I've written this MySQL database class for my Java application and I'd just like some feedback on it before moving onto making my next class.

The idea here is that I have a simple properties file which stores the hostname, username, password, and database name. I then connect to the database (if possible) and return the Connection.

Although I am not really sure what should happen if the properties file doesn't exist, or the MySQL connection fails, should I return Connection which would be null, throw a new exception, or re-throw the existing exception?

package app;

import java.io.IOException;
import java.io.InputStream;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.SQLException;
import java.util.Properties;

public class Database {

    private String hostname;
    private String database;
    private String username;
    private String password;
    private Connection conn;

    public Connection getConnection() {

        if(conn == null) {

            try {

                System.out.println("Getting properties file...");
                Properties prop = new Properties();
                InputStream input = Database.class.getResourceAsStream("database.properties");
                prop.load(input);
                this.hostname = prop.getProperty("hostname");
                this.database = prop.getProperty("database");
                this.username = prop.getProperty("username");
                this.password = prop.getProperty("password");
                input.close();

            } catch (IOException ex) {

                // Error Reading Properties File
                ex.printStackTrace();

            } finally {

                if (this.database != null) {

                    try {
                        System.out.println("Connecting to database...");
                        Class.forName("com.mysql.jdbc.Driver");
                        conn = DriverManager.getConnection("jdbc:mysql://" + this.hostname + "/" + this.database + "?user=" + this.username + "&password=" + this.password);

                    } catch (SQLException ex) {

                        // Error Connecting to Database
                        ex.printStackTrace();

                    } catch (ClassNotFoundException ex) {

                        // Error Connecting to Database
                        ex.printStackTrace();

                    }

                }

            }

        }

        return conn;

    }

}

The original solution from the question, as well as the one suggested by @oopexpert seem too complex and over-engineered.

I guess that the author of the question is trying to implement just a simple wrapper around DriverManager.getConnection(), which reads connection parameters from an expected location.

Some remarks about the original design choices:

getConnection() should return a new Connectioneach time. The huge drawback of the original solution is that the returned instance represents a sort of singleton. What happens if this instance is closed somewhere by the caller?
I don't suggest to use connection pooling here, e.g. Hikari - let this topic just remain out of scope of this review.
There is not enough reason to use stateful objects for the functionality: we need just to provide some additional expected values to the factory method and can remain in static context.
The exceptions should be re-thrown, except the I/O that might occur when the expected properties cannot be loaded: this is critical and blocking for the functionality and means a completely unwanted state of the application.
Since JDBC4, there is no need to call Class.forName to load the driver. THe driver only needs to be on the classpath.

How it could look like after refactoring:

public class Database {

  private static final String PROPS_FILE_NAME = "database.properties";
  // optionally extract prop keys in constants

  public static Connection getConnection() throws SQLException {
    Properties properties = readProperties();
    String url = String.format("jdbc:mysql://%1$s/%2$s",
                               properties.getProperty("hostname"),
                               properties.get("database"));
    return DriverManager.getConnection(url,
                                       properties.getProperty("username"),
                                       properties.getProperty("password"));
  }

  private static Properties readProperties() {
    try (InputStream input = Database.class.getResourceAsStream(PROPS_FILE_NAME)) {
      Properties props = new Properties();
      props.load(input);
      return props;
    } catch (IOException ex) {
      throw new IllegalStateException(ex);
    }
  }

}


List of search results using Selenium I have written the following code for looping through WebElements in Selenium.

Can someone please provide me with feedback on how to improve my code?

   public static void checkAbsentSearchResult(String title, String Testgoal) throws IOException{
         try{
                List<WebElement> allSearchResults = driver
                              .findElements(By
                                           .xpath("//*[contains(@class, 'teaser-list')]/li/article/div[2]/h3"));
                //Check if xpath contains any WebElements, if not check if no results message is displayed
                if(allSearchResults == null || allSearchResults.size() < 1 ){
                       String element = driver.findElement(By.xpath("//div[@class='view-empty']")).getText();
                       if(element.contains("no results found")){
                              WriteToXSL.WriteTo(Testgoal, "OK", "");
                              WriteToPDF.addRow(Testgoal, "OK", "");
                       }else{
                              WriteToXSL.WriteTo(Testgoal, "NOK", element.toString());
                              WriteToPDF.addRow(Testgoal, "NOK", "");
                       }
                }else{
                       boolean resultsFound = false;
                       for (WebElement Element : allSearchResults) {
                              if (Element.getText().contains(title)) {
                                    WriteToXSL.WriteTo(Testgoal, "NOK", "");
                                    WriteToPDF.addRow(Testgoal, "NOK", "");
                                    resultsFound = true;
                                    break;
                              }// close if
                       }// close for loop
                       if (resultsFound == false) {
                              WriteToXSL.WriteTo(Testgoal, "OK", "");
                              WriteToPDF.addRow(Testgoal, "OK", "");
                       }
                }
         }catch(Exception e){
                WriteToXSL.WriteTo(Testgoal, "NOK", e.toString());
                WriteToPDF.addRow(Testgoal, "NOK", "");
         }
   }

Method names in Java are usually camelCase as well as variable names. So,

WriteToXSL.WriteTo should be WriteToXSL.writeTo,
WebElement Element should be WebElement element,
Testgoal should be testgoal.

WriteToXSL and WriteToPDF aren't the best class names. The Java Language Specification, Java SE 7 Edition, 6.1 Declarations says the following:

  Class and Interface Type Names

  Names of class types should be descriptive nouns or noun phrases, not overly long, in mixed
  case with the first letter of each word capitalized.

I would call them XslWriter and PdfWriter. (See also #3 here.)

Also related: Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions
element is a String here:

WriteToXSL.WriteTo(Testgoal, "NOK", element.toString());

Calling toString() on a String is not too useful, you could remove that. Actually, I'd rename the variable to emptyViewText to describe its exact content.
As far as I see driver.findElements() never returns null, so you could remove the null check here:

List<WebElement> allSearchResults = driver
              .findElements(By
                           .xpath("//*[contains(@class, 'teaser-list')]/li/article/div[2]/h3"));
//Check if xpath contains any WebElements, if not check if no results message is displayed
if(allSearchResults == null || allSearchResults.size() < 1 ){

The allSearchResults.size() < 1 condition could be changed to allSearchResults.isEmpty().
Comments on the closing curly braces are unnecessary and rather disturbing. Modern IDEs could show blocks.

    }// close if
}// close for loop

[“// …” comments at end of code block after } - good or bad?][4]
You could avoid the flag here with a return statement:

boolean resultsFound = false;
for (WebElement Element: allSearchResults) {
    if (Element.getText().contains(title)) {
        WriteToXSL.WriteTo(Testgoal, "NOK", "");
        WriteToPDF.addRow(Testgoal, "NOK", "");
        resultsFound = true;
        break;
    }
}
if (resultsFound == false) {
    WriteToXSL.WriteTo(Testgoal, "OK", "");
    WriteToPDF.addRow(Testgoal, "OK", "");
}

Consider this:

for (WebElement Element: allSearchResults) {
    if (Element.getText().contains(title)) {
        WriteToXSL.WriteTo(Testgoal, "NOK", "");
        WriteToPDF.addRow(Testgoal, "NOK", "");
        return;
    }
}
WriteToXSL.WriteTo(Testgoal, "OK", "");
WriteToPDF.addRow(Testgoal, "OK", "");

Actually, I would restructure the code to remove the duplicated XSL/PDF writing calls. I have changed them with two exceptions:

public static void checkAbsentSearchResult(String title, String testGoal) throws IOException {
    try {
        doCheckAbsentSearchResult(title, testGoal);
        WriteToXSL.WriteTo(testGoal, "OK", "");
        WriteToPDF.addRow(testGoal, "OK", "");
    } catch (Exception e) {
        WriteToXSL.WriteTo(testGoal, "NOK", e.toString());
        WriteToPDF.addRow(testGoal, "NOK", "");
    }
}

public static void doCheckAbsentSearchResult(String title, String testGoal) throws IOException {
    final List<WebElement> allSearchResults = driver.findElements(By
            .xpath("//*[contains(@class, 'teaser-list')]/li/article/div[2]/h3"));
    // Check if xpath contains any WebElements, if not check if no
    // results message is displayed
    if (allSearchResults.isEmpty()) {
        final String emptyViewText = driver.findElement(By.xpath("//div[@class='view-empty']")).getText();
        if (!emptyViewText.contains("no results found")) {
            throw new RuntimeException(emptyViewText);
        }
    } else {
        for (final WebElement searchResultElement: allSearchResults) {
            if (searchResultElement.getText().contains(title)) {
                throw new RuntimeException("");
            }
        }
    }
}

Note that it changes the output a little bit when there is an error. I guess it could be fine for a report.


Given an integer array of size n, find any three numbers Return any three descending numbers that appear sequentially in the input such that A[i] > A[j] > A[k] and i < j < k. Since problem description contains 'any', I could not unit test using JUnit.

I'm looking for request code review, optimizations and best practices.

final class DescendingTriplets {

    private final int high;
    private final int mid;
    private final int low;

    DescendingTriplets(int high, int mid, int low) {
        this.high = high;
        this.mid = mid;
        this.low = low;
    }

    public int getHigh() {
        return high;
    }

    public int getMid() {
        return mid;
    }

    public int getLow() {
        return low;
    }

    @Override
    public String toString() {
        return "high: " + high + " mid: " + mid + " low: "  + low;
    }
}

public final class ThreeDescending {

    private ThreeDescending() {}

    /**
     * This class is used to save high and mid values.
     */
    private static class State {
        int mid;
        int high;

        State(int low, int high) {
            this.mid = low;
            this.high = high;
        }
    }

    /**
     * Returns any three descending numbers in an array such that they appear one after other in the original sequence.
     * If such numbers dont exist then null is returned
     *
     * @param a         the input array
     * @return          object with three descending numbers in squence
     */
    public static DescendingTriplets getDescendingTriplets(int[] a) {
        if (a.length < 3) {
            throw new IllegalArgumentException("The arrays size should atleast be three");
        }

        /*
         * The current state of 'high' and 'mid' values to be used to compare 'min' against.
         */
        State stateCurrent = null;
        /*
         * The state of new 'high' and new 'mid' which we obtain as we travel the array.
         * Once both values(high and mid) are populated, we replace the stateCurrent with the stateInProgress.
         */
        State stateInProgress = null;

        int k = 0;
        // fetch the completedstate,
        for (int i = 0; i < a.length - 1; i++) {
            if (a[i] > a[i + 1]) {
                stateCurrent = new State(a[i + 1], a[i]);
                k = i + 1;
                break;
            }
        }

        for (int j = k + 1; j < a.length; j++) {
            // if smallest
            if (a[j] <= stateCurrent.mid) {
                return new DescendingTriplets(stateCurrent.high, stateCurrent.mid, a[j]);
            }

            // if greater than mid but less than high
            if (a[j] <= stateCurrent.high) {
                if (stateInProgress != null) {
                    stateCurrent = stateInProgress;
                }
                stateCurrent.mid = a[j];
            } else {
                // if greater than high
                if (stateInProgress != null) {
                    if (j > stateInProgress.high) {
                        stateInProgress.high = a[j];
                    } else {
                        stateInProgress.mid = a[j];
                        stateCurrent = stateInProgress;
                    }
                } else {
                    stateInProgress = new State(Integer.MIN_VALUE, a[j]);
                }
            }
        }

        return null;
    }

    public static void main(String[] args) {
        int[] a1 = {5, 14, 3, 2, 1};
        System.out.println(getDescendingTriplets(a1));

        int[] a2 = {4,7,5,1,3,8,9,6,2};
        System.out.println(getDescendingTriplets(a2));
    }
}

Return any three descending numbers that appear sequentially in the input such that A[i] > A[j] > A[k] and i < j < k.
  Since problem description contains 'any', I could not unit test using junits.

I think you could create inputs for tests where

there is only one of this sequence and check that,
there are only a couple of possible sequences and check that the returned one whether exists in the possible return values list or not,
there isn't any valid sequence.


Implementation of atoi() I implemented the atoi() function! Here is my code:

int my_atoi(char* pointer)
{
    int result = 0;
    char* pointer1;
    multiplier = 1;
    char sign = 1;

    if(*pointer == '-')
    sign =- 1;

    pointer1 = pointer;

    while(*pointer != '\0')
    {
        if(*pointer >= '0' && *pointer <= '9')
            multiplier = multiplier * 10;

        pointer = pointer + 1;
    }

    pointer = pointer1;

    while(*pointer != '\0')
    {
        if(*pointer >= '0' && *pointer <= '9')
        {
            result = result + ( (*pointer%48)  * multiplier);
            multiplier = multiplier / 10;
        }

        pointer = pointer+1;
    }

    return (result * sign) / 10;
}

I wonder if there is any way that I can improve my function. I know there is a problem with my function. What if the user wants to convert from char* to int this string: "232-19". What should I do then? Any advice would be really helpful!

Things you could improve

Variables/Initialization

Where do you declare multiplier?  I assume that since it is not declared within the method, it is declared as a global variable.  Try to avoid global variables.

The problem with global variables is that since every function has access to these, it becomes increasingly hard to figure out which functions actually read and write these variables.

To understand how the application works, you pretty much have to take into account every function which modifies the global state. That can be done, but as the application grows it will get harder to the point of being virtually impossible (or at least a complete waste of time).

If you don't rely on global variables, you can pass state around between different functions as needed. That way you stand a much better chance of understanding what each function does, as you don't need to take the global state into account.

So instead of using global variables, initialize the variables in main(), and pass them as arguments to functions if necessary.  In this case, I don't see the need for multiplier to be used outside of the function at all, so simply keep it declared within the function.
sign should be an int, and not a char.

Algorithm

Right now you are implementing a complicated and hard to follow method of converting a character into a number.  The easy way is to have isdigit() do the hard work for you.  This also will help you implement the DRY principle.

while(*pointer != '\0')
{
    if(*pointer >= '0' && *pointer <= '9')
        multiplier = multiplier * 10;

    pointer = pointer + 1;
}

pointer = pointer1;

while(*pointer != '\0')
{
    if(*pointer >= '0' && *pointer <= '9')
    {
        result = result + ( (*pointer%48)  * multiplier);
        multiplier = multiplier / 10;
    }

    pointer = pointer+1;
}

See how you have two loops doing almost identical things?  Here is how I simplified all of that by using isdigit().

while (isdigit(*c))
{
    value *= 10;
    value += (int) (*c - '0');
    c++;
}

You loop through the characters in the string as long as they are digits. For each one, add to the counter you're keeping - the value to add is the integer value of the character. This is done by subtracting the ASCII value of '0' from the ascii value of the digit in question.
Note that this code doesn't handle overflow. If you pass in "89384798719061231" (which won't fit in an int), the result is undefined.  The fix is simple enough, just use a long long int to mitigate against that.  We'll still have issues for extremely long numbers, but fixing that so that the function works as intended is a bit more complicated.

Documentation

Where did all of your comments go?  A newer developer would simply gawk at some of your code.

result = result + ( (*pointer%48)  * multiplier);

Comments can really go a long way into helping other understand your code.  Don't go overboard with them though, you will have to balance how much of them to put into your program.

Syntax/Styling

This looks like a typo.

if(*pointer == '-')
    sign =- 1;

Add a space for clarity.

if(*pointer == '-') sign = -1;

You should not be modifying your char* you accept as a parameter into the function. Therefore, declare the parameter as constant.

int my_atoi(const char* pointer)

Use more shorthand operators.

pointer++;         // same as pointer = pointer+1;
multiplier /= 10;  // same as multiplier = multiplier / 10;
multiplier *= 10;  // same as multiplier = multiplier * 10;

Final Code

#include <stdio.h>
#include <assert.h>
#include <ctype.h>

long long int my_atoi(const char *c)
{
    long long int value = 0;
    int sign = 1;
    if( *c == '+' || *c == '-' )
    {
        if( *c == '-' ) sign = -1;
        c++;
    }
    while (isdigit(*c))
    {
        value *= 10;
        value += (int) (*c-'0');
        c++;
    }
    return (value * sign);
}

int main(void)
{
    assert(5 == my_atoi("5"));
    assert(-2 == my_atoi("-2"));
    assert(-1098273980709871235 == my_atoi("-1098273980709871235"));
    puts("All good."); // I reach this statement on my system
}


List the movies I haven't seen I am new to programming and have been taking the time to learn C# by reading, doing some classes online for Unity, and using Youtube. I think you learn more from doing your own projects so I decided to write a program that asked the user to enter a movie they have not seen and then store the movie titles into a list and print out the movies they listed. I had problems getting it to take multiple movies but finally figured a way to do it but I believe there is a better way to write the program. So just asking general advice to improve upon what I have already written. Thank you.
 using System;
 using System.Collections.Generic;

 namespace MovieListProjectWithSwitch
 {
  internal class Program
  {
    private static void Main(string[] args)
    {
        Console.WriteLine("Enter a movie you want to see but have not seen it");
        var movieNotSeen = new List<string>();
        movieNotSeen.Add(Console.ReadLine());

        Console.WriteLine("Do you want to enter another movie? Y/N?");

        const string answerYes = "y";
        const string answerNo = "n";
        string userInput;

        while (true)
        {
            userInput = Console.ReadLine().ToLower();

            switch (userInput)
            {
                case answerYes:
                    {
                        Console.WriteLine("Enter your next movie you wish to add to the list");
                        movieNotSeen.Add(Console.ReadLine());
                        Console.WriteLine("Now the movies on your list are:");
                        Console.WriteLine();
                        foreach (var movie in movieNotSeen)
                            Console.WriteLine(movie);
                        Console.WriteLine();

                        Console.WriteLine("Enter another movie? Y/N?");
                        var userInput2 = Console.ReadLine().ToLower();
                        if (userInput2 == "y")
                        {
                            goto case answerYes;
                        }
                        else
                        {
                            goto case answerNo;
                        }
                    }

                case answerNo:
                    {
                        Console.WriteLine("Your movies in your list are:\n");
                        foreach (var movies in movieNotSeen)
                            Console.WriteLine(movies);
                        return;
                    }
            }
        }
    }
  }
 }

Welcome to codereview. Here are my suggestions:
Functional decomposition and Naming
The trickiest part of programming is to decouple your complex problem into smaller easier (sub)problems. It is easier to tackle small and focused problems than one large and vague. So, my first suggestion would be to try to find small and independent pieces of functionalities.
In your case here is one way to decompose:

Ask for a movie and store it
Ask for continuation
Print out the already gathered movies

You can directly map these to functions:
void EnterANewMovieAndStoreIt()
bool WishToContinue()
void PrintOutMovies()

Then you can compose them like this:

Ask for a movie and store it then
Print out the already gathered movies until
the consumer Answers with yes for continuation
when (s)he Answers with no then Print out already gathered movies

With that in hand your Main function will look like this:
static void Main(string[] args)
{
    do
    {
       EnterANewMovieAndStoreIt();

       PrintOutMovies();

    } while (WishToContinue());

    PrintOutMovies();
}

Store and Retrieve functionalities
Because we have separated storage and retrieval functionalities that's why they can be really small and concise functions:
private static void EnterANewMovieAndStoreIt()
{
    Console.WriteLine();
    Console.WriteLine("Please enter a movie you want to see but have not seen it yet.");
    var movieName = Console.ReadLine();
    ToBeWatchedMovies.Add(movieName);
}

private static void PrintOutMovies()
{
    Console.WriteLine();
    Console.WriteLine("Your haven't seen but to be watched movies:");
    foreach (var movie in ToBeWatchedMovies)
        Console.WriteLine(movie);
}

As you can see they rely on a shared resource called ToBeWatchedMovies. It is defined on a class level like this: static readonly List<string> ToBeWatchedMovies = new List<string>();
Error handling
Your current implementation does not handle that case when the user enters something different then Y or N. It is a good practice to assume that a userInput can be anything and handle that accordingly. In this example we can easily ask them nicely to try to provide the answer again.
So, the WishToContinue would look like this:
private static bool WishToContinue()
{
    while(true)
    {
        Console.WriteLine();
        Console.WriteLine("Do you want to enter another movie? Y/N?");
        var userInput = Console.ReadLine();

        if (string.Equals(userInput, AnswerYes, StringComparison.OrdinalIgnoreCase))
            return true;

        if (string.Equals(userInput, AnswerNo, StringComparison.OrdinalIgnoreCase))
            return false;

        Console.WriteLine($"Please provide either '{AnswerYes}' or '{AnswerNo}' ");
    }
}

Obviously this code could be further optimized but my main point here is that you should try to handle the wrong inputs as well.

Let's put everything together:
class Program
{
    static readonly List<string> ToBeWatchedMovies = new List<string>();
    const string AnswerYes = "y", AnswerNo = "n";
    static void Main(string[] args)
    {
        do
        {
           EnterANewMovieAndStoreIt();

           PrintOutMovies();

        } while (WishToContinue());

        PrintOutMovies();
    }

    private static void EnterANewMovieAndStoreIt()
    {
        Console.WriteLine();
        Console.WriteLine("Please enter a movie you want to see but have not seen it yet.");
        var movieName = Console.ReadLine();
        ToBeWatchedMovies.Add(movieName);
    }

    private static void PrintOutMovies()
    {
        Console.WriteLine();
        Console.WriteLine("Your movies in your list are:");
        foreach (var movie in ToBeWatchedMovies)
            Console.WriteLine(movie);
    }

    private static bool WishToContinue()
    {
        while(true)
        {
            Console.WriteLine();
            Console.WriteLine("Do you want to enter another movie? Y/N?");
            var userInput = Console.ReadLine();

            if (string.Equals(userInput, AnswerYes, StringComparison.OrdinalIgnoreCase))
                return true;

            if (string.Equals(userInput, AnswerNo, StringComparison.OrdinalIgnoreCase))
                return false;

            Console.WriteLine($"Please provide either '{AnswerYes}' or '{AnswerNo}' ");
        }
    }
}

I hope this helped you a bit.


Project Euler Problem 530: sum of sum of greatest common divisors Here is a description of Project Euler problem 530:

  Every divisor \$d\$ of a number \$n\$ has a complementary divisor \$n/d\$.

  Let \$f(n)\$ be the sum of the greatest common divisor of \$d\$ and \$n/d\$ over all positive divisors \$d\$ of \$n\$, that is \$\displaystyle f(n)=\sum_{d/n}\gcd\left(d, \frac{n}{d}\right)\$.

  Let \$F\$ be the summatory function of \$f\$, that is \$\displaystyle F(k) = \sum_{n=1}^{k}f(n)\$.

  You are given that \$F(10)=32\$ and \$F(1000)=12776\$.

  Find \$F(10^{15})\$.

My solution for Euler Problem 530 is as follows:

import java.math.BigInteger;

public class problem530 {

public static void main(String[] args) {

    double start = System.nanoTime();

    BigInteger result = BigInteger.ZERO;
    BigInteger one = BigInteger.ONE;

    for (long i = 1; i <= 1000; i++){
            if ( i == 1) {
                result = result.add(one);
            }
            if (PrimeTest.testLong(i) == true) {
                result = result.add(one).add(one);
            }
            else if (i != 1 && PrimeTest.testLong(i) == false) {
                result = result.add(one);
                for(long j = 1; j*2 <= i; j++) {
                    if (i%j == 0) {
                    long sub = i / j;
                    result = result.add(BigInteger.valueOf(GCD(sub, j)));
                }
            }
        }
    }

    System.out.println(result);

    double duration = (System.nanoTime() - start)/1000000000;

    System.out.println("Your code took " + duration + " seconds to execute.");

}
}

I have PrimeTest and GCD written in libraries. The code works (it returns the correct answer for the test cases). It takes unimaginably long to get the answer for \$F(10^{15})\$ though (essentially, I won't get the answer). I'm looking for ways to optimise it.

At this link, @alan wrote something there about a sieve (presumably in C++?) which I don't quite understand. Does anyone have any ideas for optimisation?

I perform GCD operations using the Eulerian algorithm.
Prime Testing is performed for:

n = 1; n*n <= (number being tested); n++

To give an idea of how long this code takes to execute:

It ran F(10)=32 in 0.008 seconds.
It ran F(1000)=12766 in 0.019 seconds.
It ran F(100,000)=2907546 in 29.724 seconds.

This was the optimisation suggested in another post on codereview.

It seems to me that the alternative summation suggested there is wrong! F(10) would give you 22, not 32. I'm not sure how the alternative summation was derived.

Special Cases and Branching

If you look at your prime checking, your code is:

if (prime(i) == true) {
    // ...
}
else if (prime(i) == false) {
    // ...
}

This ends up with you checking composite numbers for primality twice! Once in each branch! The second part could have just been else. Additionally, explicitly checking for == true is an anti-pattern. Prefer:

if (prime(i)) {
    // ...
} else {
    // ...
}

That said, why is being prime a special case? Your non-prime logic would've worked just fine on primes too! And for 1! So just drop all the branches completely and simply iterate.

Need a better algorithm

10^15 is simply too big to brute force the answer. Even dropping all the unnecessary extra work you're doing is likely insufficient. You'll have to try to come up with some optimal substructure this problem has so that you can use dynamic programming.


Terrain height map generator I wrote a Perl script that implements the algorithm described in this
article to generate terrain height maps.  The idea is to raise a random hill \$i\$ of size \$r_i\$ centered at point \$(x_i, y_i)\$, such that the height at point \$(x, y)\$ is

$$ z = r_i^2 - ((x - x_1)^2 + (y - y_1)^2) $$

The resulting terrain is the sum of many such hills, with some normalization and flattening.

The output of this program is a bunch of inserts into the database and a png. The only problem with it is that it has really bad performance.

#!/usr/bin/perl
# mapgen.pl
# Henry J Schmale
# August 16, 2015
# Generates a height map

use strict;
use warnings;
use DBI;
use Image::Magick;
use constant {
    MAX_X   =>  2000,
    MAX_Y   =>  2000,
    MAX_R   =>  1075,
    MIN_R   =>  100,
    NUM_HILLS => 250
};

my @heightmap;

makemap();

sub makemap {
    #init heightmap to 0
    for(my $i = 0 ; $i < MAX_Y; $i++){
        my @arr;
        for(my $j = 0; $j < MAX_X; $j++){
            $arr[$j] = 0;
        }
        $heightmap[$i] = \@arr;
    }

    # Start adding hills
    for(my $i = 0; $i < NUM_HILLS; $i++){
        makehill(int(rand(MAX_X)), int(rand(MAX_Y)), int(rand(MAX_R) + MIN_R));
    }
    # normallize
    my $maxvalue = 0;
    for(my $x = 0; $x < MAX_X; $x++){
        for(my $y = 0; $y < MAX_Y; $y++){
            if($heightmap[$x][$y] > $maxvalue){
                $maxvalue = $heightmap[$x][$y];
            }
        }
    }
    for(my $x = 0; $x < MAX_X; $x++){
        for(my $y = 0; $y < MAX_Y; $y++){
            $heightmap[$x][$y] /= $maxvalue;
        }
    }
    # Flatten
    for(my $x = 0; $x < MAX_X; $x++){
        for(my $y = 0; $y < MAX_Y; $y++){
            $heightmap[$x][$y] *= $heightmap[$x][$y];
            $heightmap[$x][$y] *= $heightmap[$x][$y];
            $heightmap[$x][$y] *= $heightmap[$x][$y];
        }
    }
    # Dump to file
    dumpToImg();
    dumpToDb();
}

sub makehill {
    my ($x1, $y1, $r) = @_;
    my $z;
    for(my $x = $x1 - $r; $x < ($x1 + $r); $x++){
        for(my $y = $y1 - $r; $y < ($y1 + $r); $y++){
            my $dx = $x - $x1;
            my $dy = $y - $y1;
            $z = ($r * $r) - (($dx * $dx) + ($dy * $dy));
            if($z > 0 and $x > 0 and $y > 0){
                $heightmap[$x][$y] += $z;
            }
        }
    }
}

sub dumpToDb {
    my $dbh = DBI->connect("dbi:SQLite:dbname=server.sqlite",'','')
            or die DBI::errstr;
    my $sth = $dbh->prepare("INSERT INTO map_heights(lat_pt, long_pt, z_pt)".
                            "VALUES(?, ?, ?)");
    for(my $x = 0; $x < MAX_X; $x++){
        for(my $y = 0; $y < MAX_Y; $y++){
            $sth->execute($x, $y, $heightmap[$x][$y]);
        }
    }
    $dbh->disconnect();
}

sub dumpToImg {
    my $im = Image::Magick->new;
    my $junk = MAX_X.'x'.MAX_Y;
    print $junk."\n";
    $im->Set(size => $junk);
    $im->ReadImage('canvas:white');
    warn "$im" if "$im";
    for(my $x = 0; $x < MAX_X; $x++){
        for(my $y = 0; $y < MAX_Y; $y++){
            my $z = $heightmap[$x][$y];
            my @pixel = ($z, $z, $z);
            $im->SetPixel(x=>$x, y=>$y, color=>\@pixel);
        }
    }
    my $x = $im->Write('heightmap.png');
    warn "$x" if "$x";
}

I see two things right off the bat that may help:

Calculate $maxvalue inside makehill() so you don't have to calculate it after
Do the normalization and flattening in 1 pass. Also, you can do the flattening in a single function call. You're basically raising each value to the 8th power, so use the power function. I don't know perl real well, but it's usually called pow() and takes a value and an exponent.

The other thing that looks problematic is setting each individual pixel through a function call. Is there any way to pass a buffer of memory to ImageMagick? If so, it would probably faster to do that.


Calculating positions along the edges of a rectangle I had the following three Tcl procedures, which calculate positions along three edges of a rectangle:

#---- Numbered pins at the top ---------------------------------------
# Given a list of triplets (number, name, index), place the pins for the
# signals "name<index>" along the top edge, positioned vertically at 'y'
# and horizontally at 'number' times a constant space left of 'xRight'
# (plus an arbitrary offset of 2).
proc setNumberedTopPins {xRight y pins} {
    set padWidth 95
    foreach {number name index} $pins {
        set xOffset [expr $number * $padWidth + 2]
        setPin $name [expr $xRight-$xOffset] $y Top $index
    }
}
#---- Numbered pins at the right -------------------------------------
# The same for the right edge.
proc setNumberedRightPins {x yTop pins} {
    set padWidth 95
    foreach {number name index} $pins {
        set yOffset [expr max(0, $number * $padWidth - 2.5)]
        setPin $name $x [expr $yTop-$yOffset] Right $index
    }
}
#---- Numbered pins at the bottom ------------------------------------
# The same for the bottom edge.
proc setNumberedBottomPins {xRight y pins} {
    set padWidth 95
    foreach {number name index} $pins {
        set xOffset [expr $number * $padWidth + 2]
        setPin $name [expr $xRight-$xOffset] $y Bottom $index
    }
}

Since the code for each of the procedures is almost identical, I decided to write a single generalized procedure and three specializations of it.

I had some difficulties with generalizing the fact that the offset is sometimes applied to the second (x) and sometimes to the third (y) argument of setPin. I wanted to make sure that this decision is only taken once, outside the foreach loop.

Here is the solution I came up with:

#---- Numbered pins at the top ---------------------------------------
proc setNumberedTopPins {xRight y pins} {
    setNumberedPins Top 2 $xRight $y $pins
}
#---- Numbered pins at the right -------------------------------------
proc setNumberedRightPins {x yTop pins} {
    setNumberedPins Right -2.5 $x $yTop $pins
}
#---- Numbered pins at the bottom ------------------------------------
proc setNumberedBottomPins {xRight y pins} {
    setNumberedPins Bottom 2 $xRight $y $pins
}
#---- Generic numbered pins ------------------------------------------
proc setNumberedPins {side offsetPin0 x y pins} {
    set padWidth 95
    switch $side {
        Top - Bottom {set coords {list [expr $x-$offset] $y}}
        Right {set coords {list $x [expr $y-$offset]}}
    }
    foreach {number name index} $pins {
        set offset [expr max(0, $number * $padWidth + $offsetPin0)]
        setPin $name {*}[eval $coords] $side $index
    }
}

I'm not an experienced Tcl programmer and I wonder if there is a better way to solve this. Particularly, there is still some amount of code duplication left (almost identical definition of auxiliary variable coords inside switch) and I would like to eliminate that (while keeping the code reasonably readable).

I would suggest that since only 2 of the procs are close enough to identical, just "combine" those:

set padWidth 95

proc setNumberedRightPins {x yTop pins} {
    foreach {number name index} $pins {
        set yOffset [expr {max(0, $number * $::padWidth - 2.5)}]
        setPin $name $x [expr {$yTop-$yOffset}] Right $index
    }
}

proc setNumberedTopPins {args} {
    setNumberedTopOrBottomPins {*}$args Top
}

proc setNumberedBottomPins {args} {
    setNumberedTopOrBottomPins {*}$args Bottom
}

proc setNumberedTopOrBottomPins {xRight y pins side} {
    foreach {number name index} $pins {
        set xOffset [expr {$number * $::padWidth + 2}]
        setPin $name [expr {$xRight-$xOffset}] $y $side $index
    }
}

Notes:

always brace your expressions, gain a big performance boost
constants can live in the global namespace, and you access them with a fully qualified variable name


Create an instance (view) given its dependency (view model) I have a set of views and view models in my project. There is a one-to-one mapping of views and view models, so each view requires a particular view model. I'd like to instantiate a series of views given a collection of view models.

Here's what I've come up with:

public class OverlayBundle : DrawingVisual
{
    private static readonly Dictionary<Type, Func<IOverlayViewModel, DrawingVisual>> _overlayCreators =
        new Dictionary<Type, Func<IOverlayViewModel, DrawingVisual>>
        {
            { typeof(ImageOverlayViewModel), x => new ImageOverlay((ImageOverlayViewModel)x) },
            { typeof(ValuesOverlayViewModel), x => new ValuesOverlay((ValuesOverlayViewModel)x) },
            { typeof(NoGoAreasOverlayViewModel), x => new NoGoAreasOverlay((NoGoAreasOverlayViewModel)x) },
        };

    private readonly OverlayBundleViewModel _viewModel;

    private IEnumerable<Visual> _overlays;

    public OverlayBundle(OverlayBundleViewModel viewModel)
    {
        _viewModel = viewModel;
        GenerateChildren();
    }

    private void GenerateChildren()
    {
        _overlays = _viewModel.GetAllOverlayViewModels().Select(GetOverlay);
        foreach (var overlay in _overlays)
            Children.Add(overlay);
    }

    private Visual GetOverlay(IOverlayViewModel viewModel)
    {
        return _overlayCreators[viewModel.GetType()](viewModel);
    }
}

This works, but I feel like there must be a better way. Any feedback would be appreciated.

A ViewModel should not be convertible to a View, it should be bound to it. The View should have a ViewModel property. You should not have child Views for child ViewModels. Instead, you should have one view that knows how to access the children of your top ViewModel (and there is only one of those).

The code seems way overcomplicated. Unless a Visual is a UI element like a page or control, it is probably a superfluous layer. MVVM where Views are POCO objects is usually a bad idea--it can't present to the screen, which is the most important job of a View. If I am understanding your code correctly, you could probably accomplish your goals by removing your Visual layer and converting your Bundle into a top-level ViewModel, directly binding it to your UI/View. Your child overlays should be child ViewModels of your existing ViewModels, of which there shold only be one per View, and the View knows how to access the children property of the parent ViewModel.