DAL Returns a collection of business objects in MVP In Model View Presenter (MVP) pattern, it is said that our DAL always should returns business models.

If I want to get a list of business objects from my DAL, is it acceptable to do it as follows or better to get a DataTable from DAL and then in the applications BLL, converts it to a List?

public List<Employee>  GetNewEmployees()
{
    string selectStatement = "SELECT Employee.Emp_ID, Employee.Initials + ' ' + Employee.Surname AS Name,...";

    using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
    {
        using (SqlCommand sqlCommand = new SqlCommand(selectStatement, sqlConnection))
        {
            sqlConnection.Open();
            using (SqlDataReader dataReader = sqlCommand.ExecuteReader())
            {
                List<Employee> list = new List<Employee>();
                while (dataReader.Read())
                {
                    list.Add (
                    new EpfEtfMaster {
                        EmployeeID = (int) dataReader ["emp_id"],
                        EmployeeName = (string) dataReader ["Name"],
                        AppointmentDate = (DateTime) dataReader["appointment_date"],
                    });
                }
                return list;
            }
        }
    }
}

You don't want to pull the entire Employee table unless necessary.

public IEnumerable<Employee>  GetNewEmployees()
{
    string selectStatement = "SELECT Employee.Emp_ID, Employee.Initials + ' ' + Employee.Surname AS Name,...";

    using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
    {
        using (SqlCommand sqlCommand = new SqlCommand(selectStatement, sqlConnection))
        {
            sqlConnection.Open();
            using (SqlDataReader dataReader = sqlCommand.ExecuteReader())
            {
                while (dataReader.Read())
                {
                    yield return new EpfEtfMaster(){
                        EmployeeID = (int) dataReader ["emp_id"],
                        EmployeeName = (string) dataReader ["Name"],
                        AppointmentDate = (DateTime) dataReader["appointment_date"],
                    };
                }
            }
        }
    }
}

By switching to IEnumerable and yield return, when you call GetNewEmployees().First(), only the first row is read.

The names like sqlCommand, sqlConnection and dataReader can be shortened to command, connection and reader without affecting the reader.
Try to not repeat yourself with SomeThing someThing = new SomeThing(...);. The first class name can be replaced by using var.

Result :

using (var connection = new SqlConnection(db.GetConnectionString))

new SqlConnection(db.GetConnectionString)

Use verb for method and noun for field/properties. Here GetX suggest it is a method but is used like a property, you can rename it to ConnectionString.


Parsing specifically nested XML Tags I've written a function that works however I'm sure there is a better way. I need to parse specific tags from an xml document (Microsoft Word docx document.xml).

Here is the general structure of the xml in question.

//A ton of crap
...
<w:tbl>
  <w:tr>
    <w:tc>
      <w:p>
        <w:r>
          <w:t>Data_I_want</w:t>
        </w:r>
      </w:p>
    </w:tc>
  </w:tr>
  <w:tr>
    <w:tc>
      <w:p>
        <w:r>
          <w:t>Data_I_want</w:t>
        </w:r>
      </w:p>
    </w:tc>
  </w:tr>
</w:tbl>
...
// A ton more crap
//Same structure repeats and I need to grab that n number of times where n is unknown.
// Also the order of the data must be preserved within each parent tbl tag.

Here is an excerpt of my code:

def recurse_search_tables(self, xml_data):
    """
    Recursively traverse the xml child nodes..
    """
    tag_base = r'{http://schemas.openxmlformats.org/wordprocessingml/2006/main}'
    for child in xml_data:
        if child.tag.replace(tag_base,'') == 'tbl':
                for c in child:
                    if c.tag.replace(tag_base,'') == 'tr':
                        for tr in c:
                            if tr.tag.replace(tag_base,'') == 'tc':
                                for tc in tr:
                                    if tc.tag.replace(tag_base,'') == 'p':
                                        for p in tc:
                                            if p.tag.replace(tag_base,'') == 'r':
                                                for r in p:
                                                    if r.tag.replace(tag_base,'') == 't':
                                                        try:
                                                            self.decide_to_print(r.text.encode('UTF-8'))
                                                        except:
                                                            pass
                                                        finally:
                                                            self.recurse_search_tables(child)
                                                    else:
                                                        self.recurse_search_tables(child)
                                            else:
                                                self.recurse_search_tables(child)
                                    else:
                                        self.recurse_search_tables(child)
                            else:
                                self.recurse_search_tables(child)
                    else:
                        self.recurse_search_tables(child)
        else:
            self.recurse_search_tables(child)

Calling that mess via:

tree = ET.parse('document.xml')
root = tree.getroot()
self.recurse_search_tables(root)

Now like i said, this code works however it isn't the fastest (but is is suitable). How can I improve this?

Thanks to Ferada for the hint. Here is what I was able to replace that monster with. Execution time went from more than 590 seconds to less than 2 seconds.

Note: all of the string replacements were another performance pain point, using a single string is much quicker here.

def recurse_search_tables(self, root):
    # Get individual tables and their children nodes.
    for table in root.iter(r'{http://schemas.openxmlformats.org/wordprocessingml/2006/main}tbl'):
        for t in table.iter(r'{http://schemas.openxmlformats.org/wordprocessingml/2006/main}t'):
            self.decide_to_print(t.text.encode('UTF-8'))


Valid memory address in Python I am looking for suggestions of improving this function to check whether a memory address is appropriate or not.

def isweird(addr, cpu_name):
    """Check if a memory address is weird
    Args:
        addr (str): a memory address
        cpu_name (str): cpu name
    Returns:
        bool: True if the address is weird
    """
    if not isinstance(addr, six.string_types):
        raise Exception('The memory address must be a string.')

    if addr == '0x0':
        return True

    addr = addr.lower()

    # Strip leading zeroes
    addr = addr[2:].lstrip('0')

    if utils.is64(cpu_name):
        if len(addr) <= 8:
            val = int(addr, 16)
            if val <= (1 << 16):  # val <= 0xffff (ie: first 64k)
                return True
        elif addr.startswith('ffffffff'):
            addr = addr[8:]  # 8 == len('ffffffff')
            val = int(addr, 16)
            if val >= ((1 << 32) - (1 << 16)):  # val >= 0xffffffffffff0000 (ie: last 64k)
                return True
    else:
        val = int(addr, 16)
        if val <= 1 << 16:  # val <= 0xffff (ie: first 64k)
            return True
        if val >= ((1 << 32) - (1 << 16)):  # val >= 0xffff0000 (ie: last 64k)
            return True
    return False

In this code:

if len(addr) <= 8:
    val = int(addr, 16)
    if val <= (1 << 16):  # val <= 0xffff (ie: first 64k)
        return True
elif addr.startswith('ffffffff'):
    addr = addr[8:]  # 8 == len('ffffffff')
    val = int(addr, 16)
    if val >= ((1 << 32) - (1 << 16)):  # val >= 0xffffffffffff0000 (ie: last 64k)
        return True

You can replace the inner if statements with return statements:

if len(addr) <= 8:
    val = int(addr, 16)
    return val <= (1 << 16)  # val <= 0xffff (ie: first 64k)
elif addr.startswith('ffffffff'):
    addr = addr[8:]  # 8 == len('ffffffff')
    val = int(addr, 16)
    return val >= ((1 << 32) - (1 << 16))  # val >= 0xffffffffffff0000 (ie: last 64k)

This is possible because if these conditions are false,
the execution will reach the return False at the end of the function.
The reason to rewrite this way is so that the reader doesn't have to follow the execution path until the end of the function,
he can know on these lines that no further statements will be executed,
the return value is already decided at these points.

Some of the comments are incorrect and misleading:

if val <= (1 << 16):  # val <= 0xffff (ie: first 64k)

if val >= ((1 << 32) - (1 << 16)):  # val >= 0xffffffffffff0000 (ie: last 64k)

1 << 16 is 0x10000, not 0xffff.

(1 << 32) - (1 << 16) is 0xffff0000, not 0xffffffffffff0000.

Following the naming conventions of Python,
the function isweird should be named is_weird instead.


What should I do to improve this snippet Though it works I wonder if there might be a better way to do this. I would like to reuse this loading and establishing of fields of another class(es) (the static TileMapper class mentioned at the end) in a C#/.net/XNA environment.

I noticed that when moving through the OpenFileDialog to find files it felt sluggish.
 Is that because I had to set [STAThreadAttribute] just to get it to open?

private void loadMapToolStripMenuItem_Click(object sender, EventArgs e)
    {
        OpenFileDialog openFile = new OpenFileDialog();
        openFile.Title = "Select a File";
        openFile.InitialDirectory = Application.StartupPath + @"\Content\Map\";
        openFile.Filter = "MAP Files (.MAP)|*.MAP";
        openFile.FilterIndex = 1;
        openFile.Multiselect = false;

        if (openFile.ShowDialog() != DialogResult.Cancel)
        {
            try
            {
                // Example file - _W25_H25_SET_GL_test.Map
                TileMapper.LoadMap(new FileStream(openFile.FileName, FileMode.Open));
                // remove directory information
                string[] _mapname = openFile.FileName.Split('\\');
                string tempmap = _mapname.Last();
                // remove .map
                string[] removemap = tempmap.Split('.');
                MapName = removemap.First();
                //print file name
                txtMapName.Text = MapName;
                //get info stored in file name
                string[] nameParts = MapName.Split('_');
                string[] widthPart = nameParts[1].Split('W');
                string[] heightPart = nameParts[2].Split('H');

                TileMapper.MapWidth = int.Parse(widthPart[1]);
                TileMapper.MapHeight = int.Parse(heightPart[1]);
            }
            catch
            {
                System.Diagnostics.Debug.Print("Unable to load map file");
            }
        }
        else return;
     }

I noticed that when moving through the OpenFileDialog to find files it
  felt sluggish. Is that because I had to set [STAThreadAttribute] just
  to get it to open?

That is a StackOverflow or GameDev.StackExchange kind of question.

Refactoring the code like this could be a start:

private void loadMapToolStripMenuItem_Click(object sender, EventArgs e)
{
    var openFile = new OpenFileDialog
                       {
                           Title = "Select a File",
                           InitialDirectory = Application.StartupPath + @"\Content\Map\",
                           Filter = "MAP Files (.MAP)|*.MAP",
                           FilterIndex = 1,
                           Multiselect = false
                       };

    if (openFile.ShowDialog() != DialogResult.Cancel)
    {
        try
        {
            string otherMapName;

            LoadMap(TileMapper, openFile.FileName, out otherMapName);

            txtMapName.Text = otherMapName;
        }
        catch
        {
            //TODO: display an error message box?
            System.Diagnostics.Debug.Print("Unable to load map file");
        }
    }
}

//TODO: why not move this method inside TileMapper?
private void LoadMap(TileMapper tileMapper, string fileName, out string mapName)
{
    mapName = null;

    // Example file - _W25_H25_SET_GL_test.Map
    tileMapper.LoadMap(new FileStream(fileName, FileMode.Open));

    mapName = Path.GetFileNameWithoutExtension(fileName);

    //get info stored in file name
    string[] nameParts = mapName.Split('_');
    string[] widthPart = nameParts[1].Split('W');
    string[] heightPart = nameParts[2].Split('H');

    tileMapper.MapWidth = int.Parse(widthPart[1]);
    tileMapper.MapHeight = int.Parse(heightPart[1]);
}


Php class for url generator I wrote a class to generate URL as I need. (Using http or https and switching off the rewrite rules)
That is my first OOP code. It's the right approach, any suggestion?

inc/class.urls.inc.php

    class Url {

    public function __construct() {

        $this->rewrite = SITE_REWRITE;
        $this->domain = SITE_URL;
    }

    public function home() {
        return $this->domain;
    }

    public function page($page) {
        if ($this->rewrite == 1) {
            return $this->domain . '/' . $page . '.html'; // https://www.example.com/contact.html
        } else {
            return $this->domain . '/' . $page . '.php'; // https://www.example.com/cantact.php
        }
    }

    public function manufacter($slug) {
        if ($this->rewrite == 1) {
            return $this->domain . '/' . $slug; // https://www.example.com/apple
        } else {
            return $this->domain . '/manufacter.php?s=' . $slug; // https://www.example.com/manufacter.php?s=apple
        }
    }

    public function product($slug, $manufacter) {
        if ($this->rewrite == 1) {
            return $this->domain . '/' . $manufacter . '/' . $slug; // https://www.example.com/apple/iPhone
        } else {
            return $this->domain . '/product.php?s=' . $slug; // https://www.example.com/product.php?s=iPhone
        }
    }

    public function category($slug) {
        if ($this->rewrite == 1) {
            return $this->domain . '/' . 'categories/' . $slug; // https://www.example.com/categories/phone
        } else {
            return $this->domain . '/category.php?s=' . $slug; // https://www.example.com/category.php?s=phone
        }
    }

}

Page

<?php

DEFINE('SITE_NAME','Site Name');
DEFINE('SITE_DOMAINS','www.exemple.com');
DEFINE('SITE_HTTP','https://');
DEFINE('SITE_URL',SITE_HTTP.SITE_DOMAINS);
DEFINE('SITE_REWRITE',1);

include_once 'inc/class.urls.inc.php';

$url = new Url();

echo 'Home: '.$url->home() ."
";
echo 'Page: '.$url->page('contacts') ."
";
echo 'Category: '.$url->category('ipa') ."
";
echo 'Product: '.$url->product('punk-ipa','brew-dog') ."
";
echo 'Manufacter: '.$url->manufacter('brew-dog') ."
";
?>

Result

Home: https://www.exemple.com
Page: https://www.exemple.com/contacts.html
Category: https://www.exemple.com/categories/ipa
Product: https://www.exemple.com/brew-dog/punk-ipa
Manufacter: https://www.exemple.com/brew-dog

You are on the right track, but there is some hidden complexity and repetition:

if ($this->rewrite == 1) {
    // ...
} else {
    // ...
}

The if statement above is peppered all throughout your code. You actually have a need one base class Url and two child classes that specialized in the different routing strategies. First, you'll want the Url class to be abstract, and specify all the methods that return URLs to be abstract as well. The one thing both routing strategies share in common is the domain name, which should be a private field with a protected getter to enforce encapsulation.

Lastly, you see a static method called create which chooses which concrete instance to return based on environmental variables.

abstract class Url {
    private $domain;

    public function __construct($domain) {
        $this->domain = $domain;
    }

    public abstract function home();
    public abstract function page($page);
    public abstract function manufacter($slug);
    public abstract function product($slug, $manufacter);
    public abstract function category($slug);

    protected function getDomain() {
        return $this->domain;
    }

    public static function create(): Url {
        if (SITE_REWRITE) {
            return new SearchEngineFriendlyUrl(SITE_URL);
        }

        return new PhpFileUrl(SITE_URL);
    }
}

Now you need one concrete class for each routing strategy:

class PhpFileUrl extends Url {
    public function home() {
        // ...
    }

    public function page($page) {
        // ...
    }

    public function manufacter($slug) {
        // ...
    }

    public function product($slug, $manufacter) {
        // ...
    }

    public function category($slug) {
        // ...
    }
}

class SearchEngineFriendlyUrl extends Url {
    public function home() {
        // ...
    }

    public function page($page) {
        // ...
    }

    public function manufacter($slug) {
        // ...
    }

    public function product($slug, $manufacter) {
        // ...
    }

    public function category($slug) {
        // ...
    }
}

Once you have this, you no longer need IF statements anywhere:

class PhpFileUrl extends Url {
    public function page($page) {
        return $this->getDomain() . '/' . $page . '.php';
    }
}

class SearchEngineFriendlyUrl extends Url {
    public function page($page) {
        return $this->getDomain() . '/' . $page . '.html';
    }
}

Each class becomes very clean and easy to understand. Furthermore, if your SEO friendly URLs change, you need only change the one class without affecting the other, which reduces your testing effort.

Remember that every branch in your code (think if statement and switchs) is another path to failure, and therefore another path you must test... in each method the branch exists.

Now, using this is just as easy as before, but gives you a more maintainable solution:

$url = Url::create();

echo $url->page('about');


Map array of objects and apply condition I'm mapping through an array of objects and applying conditions on each object by adding a new value according to the condition. I use map in ramda. Can I do this better?

events = map(el => {
  if (!['ts', 'sxi', 'ht', 'hd'].includes(el.value)) {
    el['extras'] = {type: 'cx', label: 'Es', name: 'es', options: ['cf']}
  }
  return el;
}, events);

If you are not returning a new set of values from map just modifying the element, then you should just use each


Deleting a user from a database I was hoping if anyone could tell me if I'm doing this correctly, I know there are other questions like this but everyone codes different and I just feel that I'm not doing this right. I'm trying to make a modular OOP site so all the files will be called through the one index file which will then route the user to the correct page. All the code below works correctly (I have tested it) but I was curious about a few things.

Is this the correct way of using OOP and classes (mainly the DB
connection)?
Why (if it is correct) when I use Netbeans PHP that it doesn't pick
up the functions? See images below.

db.php (DB connection file)

$host = "localhost";
$username = "root";
$password = "";
$database = "als";

$errors = new errors();

try {
    $db = new PDO("mysql:host=$host;dbname=$database", $username, $password);
} catch(PDOException $ex) {
    trigger_error($ex->getMessage());
    $errors->addLog($ex->getPrevious(), true);
    exit;
}

User.php (class)

class user {
    private $db = null;

    public function __construct() {
        global $db;
        $this->db = $db;
    }

    public function deleteUser($ID) {
        if(isset($ID) && !empty($ID) && is_int($ID) == TRUE) {
            $deleteUser = $this->db->prepare("DELETE FROM `users` WHERE userid=?");
            $deleteUser->bindParam(1, $ID, PDO::PARAM_INT);
            $deleteUser->execute();
        } else {
            return FALSE;
        }
    }
}

index.php (main file)

include 'config/autoloader.php';
include 'config/db.php';

$user = new user();

$user->deleteUser(1);

Why (if it is correct) when I use Netbeans PHP that it doesn't pick up the functions? See images below.

As you don't declare types in PHP, Netbeans has no way of knowing that $db is a PDO object. But you can tell Netbeans this:

/* @var $db PDO  */
private $db = null;

For further ways to Type-hint for PHP in Netbeans check out this post I wrote a while ago.

  Is this the correct way of using OOP and classes (mainly the DB connection)?

I would pass the PDO object to the user class in the constructor instead of using global.

It looks cleaner, and it would make writing unit tests a lot easier as well (you could pass a connection to a test database without changing the code or mock the PDO class). It also has the neat side-effect that you are able to define the type and thus enable auto complete.

Misc

classes should start with an upper-case letter (User).
is_int($ID) == TRUE can be written as is_int($ID).
I find it a bit odd that you return false for wrong parameters, but not for an unsuccessful delete in deleteUser.
checking empty has the effect of not deleting the user with id 0. Is this on purpose? If yes, I would make this explicit ($id != 0), if no, just remove it, it doesn't do anything else that is_int wouldn't cover.

So I would probably rewrite it like this:

class User { // upper-case U
    private $db = null;

    public function __construct(PDO $db) { // defining a type allows for auto-complete and makes testing easier
        $this->db = $db;
    }

    public function deleteUser($ID) {
        if(!isset($ID) || !is_int($ID)) { // no need to check empty. if needed, add $ID != 0
            return FALSE; // wrong parameters, return early
        }
        $deleteUser = $this->db->prepare("DELETE FROM `users` WHERE userid=?");
        $deleteUser->bindParam(1, $ID, PDO::PARAM_INT);
        return $deleteUser->execute(); // returns true on success, otherwise false

    }
}


Threadpool implementation in C++11 I am implementing a very primitive threadpool with C++11 using mutex and conditional variable. The class ThreadPool is the main thread holder and will spawn a number of threads upon construction, and join all threads on destruction. The main thread adds task of type function<void()> into the queue and then wakes up any waiting threads through condition variable.

As a new C++11 learner, is there anything I can improve for the following code (or fix bugs)? For example, making the task function more generic and implementing the lock-free mechanism (best practice?).

#include <vector>
#include <queue>
#include <memory>
#include <thread>
#include <mutex>
#include <condition_variable>
#include <future>
#include <functional>
#include <iostream>
#include <stdexcept>
#include <unistd.h>

// Class: ThreadPool
class ThreadPool {

  // Class: Worker
  class Worker {

    public:

    // Constructor.
    explicit Worker(ThreadPool* master) : _master{master} {};

    // Functor.
    void operator ()() {

      ::std::function<void()> task;

      while(true) {
        { // Acquire lock.
          ::std::unique_lock<::std::mutex> lock(_master->_mutex);

          _master->_condition.wait(
            lock,
            [&] () { return !_master->_tasks.empty() || _master->_terminate.load(); }
          );

          if(_master->_terminate.load()) return;

          task = _master->_tasks.front();
          _master->_tasks.pop_front();
        } // Release lock.

        task();
      }
    }

    private:

    ThreadPool* _master;
  };

  public:

    ThreadPool(size_t);
    template<class F>
    void enqueue(F f);
    ~ThreadPool();

    ThreadPool(const ThreadPool&) = delete;
    ThreadPool& operator = (const ThreadPool&) = delete;

  private:

    friend class Worker;

    ::std::vector< ::std::thread > _workers;
    ::std::deque< ::std::function<void()> > _tasks;
    ::std::mutex _mutex;
    ::std::condition_variable _condition;
    ::std::atomic_bool _terminate {false};
};

// Constructor
ThreadPool::ThreadPool(size_t threads) {
  for(size_t i = 0;i<threads;++i) {
    _workers.push_back(::std::thread(Worker(this)));
  }
}

// Destructor.
ThreadPool::~ThreadPool() {
  _terminate.store(true);
  _condition.notify_all();
  for(auto& t : _workers) t.join();
}

// Procedure: enqueue
template<class F>
void ThreadPool::enqueue(F f) {

  { // Acquire lock
    ::std::unique_lock<::std::mutex> guard(_mutex);
    _tasks.push_back(::std::function<void()>(f));
  } // Release lock

  // wake up one thread
  _condition.notify_one();
}

void foo() {
  printf("hello by %p\n", ::std::this_thread::get_id());
  sleep(1);
}

int main() {

  ThreadPool tp(4);

  while(1) {

    printf("main %p: enque\n", ::std::this_thread::get_id());
    tp.enqueue(foo);
    sleep(1);
  }

  return 0;
}

I don't see why this should have a hypothetical name:

void foo() {
    printf("hello by %p\n", ::std::this_thread::get_id());
    sleep(1);
}

If this is meant as a test function, then name it as such.  As obvious as the functionality may be, there's still need for such names in an otherwise serious program.

Also, you can use std::this_thread::sleep_for, which is as of C++11:

std::this_thread::sleep_for(std::chrono::seconds(1));

If you also have C++14, use a literal instead:

std::this_thread::sleep_for(1s);

(You'll also need <chrono> for all of this.)


Push all zeros to tail of int array My script pushes zeros to end of an int array. I'd like to be reviewed on efficiency, style, and obviously if there is a bug I would like to know.

def nonzero(a):
  ''' given an array of ints, push all zeros to the end '''
  zeros = [0 for i in range(a.count(0))]
  x = [ i for i in a if i != 0]
  x.extend(zeros)
  return(x)

The function does not preserve object identity. Zeros of other types get replaced with an integer:

>>> nonzero([0, 0.0, 0j])
[0, 0, 0]

If the input only ever contains integers, then this isn't a problem, but it's worth remembering that Python has several different types of number.
The docstring is not quite right. "push all the zeros to the end" implies that the input array is modified, but that's not true: a new array is created and returned.
There's a neat trick for implementing this:

def nonzero(a):
    """Return a copy of the iterable a with all zeros at the end."""
    return sorted(a, key=lambda x: x == 0)

This works because False compares less than True, and because Python's sort is guaranteed to be stable: it never changes the relative order of elements with the same sort key.


Performing a special multiplication on two square matrices I have a function called mult2 that takes in two square NumPy matrices and returns a matrix that is the result of a special multiplication:

p=P(1,344)
PI,PIT=[],[]
for i in xrange(344):
    PI.append(p**i)
    PIT.append((p**i).transpose())

def mult2(a,b):
    C=np.sum((a*b)**2,axis=1)
    CE1=np.matrix([[0.0] for _ in xrange(len(a))])
    API,PITB=[],[]
    AB=a*b
    for i in xrange(len(a)):
        API.append(np.diag(np.diag(a*PI[i])))
        PITB.append(np.diag(np.diag(PIT[i]*b)))
        CE1+=np.sum(API[i]*PITB[i]*AB,axis=1)
    CE2=np.matrix([[0.0] for _ in xrange(len(a))])
    BPI,PITA=[],[]
    for i in xrange(len(a)):
        BPI.append(np.diag(np.diag(b*PI[i])))
        PITA.append(np.diag(np.diag(PIT[i]*a)))
        CE2+=np.sum(a*BPI[i]*PITA[i]*b,axis=1)
    CE3=np.matrix([[0.0] for _ in xrange(len(a))])
    for i in xrange(len(a)):
        CE3+=np.sum(AB*API[i]*PITB[i],axis=1)
    CE1E2=np.matrix([[0.0] for _ in xrange(len(a))])
    for i in xrange(len(a)):
        CE1E2+=np.sum(API[i]*PITB[i]*API[i]*PIT[i]*b,axis=1)
    CE2E3=np.matrix([[0.0] for _ in xrange(len(a))])
    for i in xrange(len(a)):
        CE2E3+=np.sum(a*BPI[i]*PITA[i]*BPI[i]*PIT[i],axis=1)
    CE1E3=np.matrix([[0.0] for _ in xrange(len(a))])
    for i in xrange(len(a)):
            for j in xrange(len(a)):
                CE1E3+=np.sum(np.matrix(API[i]*PITB[i]*API[j]*PITB[j]),axis=1)
    CE1E2E3=np.matrix([[0.0] for _ in xrange(len(a))])
    for i in xrange(len(a)):
        CE1E2E3+=np.sum(np.matrix((API[i]*PITB[i])**2),axis=1)
    return C-(CE1+CE2+CE3-CE1E2-CE1E3-CE2E3+CE1E2E3)

p is another matrix of the same dimensions (n by n) as a and b specially a circulant matrix.

My trouble is that these matrices are 344 by 344 and I am looking to run this function lots of times.

Storing the matrices that are used later in the function in the lists actually sped up the computation significantly.

I know this is a very intensive computation so it should take a while to run. That being said, can anyone spot any improvements I can make?

From what I can tell, you could combine all of the for loops into only 2 distinct loops:

A loop to setup the matrices used for later calculations
A loop to do the rest of the calculations

Instead of performing list comprehension to create an array to pass to each numpy matrix, you could simply multiply a base array by the len(a) to create said array:

base_list = [[0.0]]*len(a)  # Creates [[0.0], [0.0], [0.0], ... ]

This way you do not temporarily store each element in the xrange object without needing to.

Use more descriptive variable names. What is a, b? Using more descriptive variable names helps improve comprehension and readability.

Here is my version of mult2():

def mult2(a, b):

    AB = a*b
    API, BPI, PITA, PITB = [],[],[],[]

    # Base list used to seed each np.matrix()
    base_list= [[0.0]]*len(a)

    C = np.sum((a*b)**2, axis=1)
    CE1 = np.matrix(base_list)
    CE2 = np.matrix(base_list)
    CE3 = np.matrix(base_list)
    CE1E2 = np.matrix(base_list)
    CE2E3 = np.matrix(base_list)
    CE1E3 = np.matrix(base_list)
    CE1E2E3 = np.matrix(base_list)

    # Create arrays used in later matrix calculations
    for index in xrange(len(a)):
        API.append(np.diag(np.diag(a*PI[index])))
        PITB.append(np.diag(np.diag(PIT[index]*b)))
        BPI.append(np.diag(np.diag(b*PI[index])))
        PITA.append(np.diag(np.diag(PIT[index]*a)))

    # Do matrix calculations
    for index in xrange(len(a)):
        CE1 += np.sum(API[index]*PITB[index]*AB, axis=1)
        CE2 += np.sum(a*BPI[index]*PITA[index]*b, axis=1)
        CE3 += np.sum(AB*API[index]*PITB[index], axis=1)
        CE1E2 += np.sum(API[index]*PITB[index]*API[index]*PIT[index]*b, axis=1)
        CE2E3 += np.sum(a*BPI[index]*PITA[index]*BPI[index]*PIT[index], axis=1)
        CE1E2E3 += np.sum(np.matrix((API[index]*PITB[index])**2), axis=1)

        for pos in xrange(len(a)):
            CE1E3 += np.sum(
              np.matrix(API[index]*PITB[index]*API[pos]*PITB[pos]), axis=1)

    return C - (CE1 + CE2 + CE3 - CE1E2 - CE1E3 - CE2E3 + CE1E2E3)

Remember, good code is readable code: group like statements together.

Also, PEP8 is the Python standard style convention. It provides a very nice style guide for code and helps code look as Pythonic as possible.


Getting prime numbers between a range I have the following code to get the prime numbers between a range of numbers. Is this the best and fastest method?

int[] numberCollection = Enumerable.Range(startNum,Count).ToArray<int>();
string primeNumbers = string.Join(",", from number in numberCollection
                                       where (IsPrime(number)==true)
                                       select number);

Where :

startNum is the starting number
Count is the number of numbers in the collection
IsPrime is a method defined as follows:

  private static bool IsPrime(int number)
  {
      for (int i = 2; i < number/2; i++)
          if (number % i == 0) return false;
      return true;
  }

If you are only executing this once, it should work fine as is.  If you are going to execute it more than once per century, you should use the Sieve of Eratosthenes, as suggested by Somnath Rakshit.

Using the algorithm as described on wikipedia:

static class MathExtensions
{
    static public List<int> GetPrimes(int start, int end)
    {
        List<int> res = new List<int>();

        // 1. Create a list of consecutive integers from 2 through n: (2, 3, 4, ..., n).
        int[] numbers = Enumerable.Range(0, end).ToArray();

        // 2. Initially, let p equal 2, the first prime number.
        int idx = 2;
        do
        {
            // If the number is in range, add it to the result
            if (numbers[idx] >= start)
            {
                res.Add(numbers[idx]);
            }

            // 3. Starting from p, enumerate its multiples by counting to n in increments of p, and mark them in the list (these will be 2p, 3p, 4p, ... ; the p itself should not be marked).
            for (var i = idx; i < end; i += idx)
            {
                numbers[i] = 0;
            }

            // 4. Find the first number greater than p in the list that is not marked. If there was no such number, stop. Otherwise, let p now equal this new number (which is the next prime), and repeat from step 3.
            while (idx < end && numbers[idx] == 0)
            {
                idx++;
            }
        }
        while (idx < end);

        return res;
    }
}


Writing Custom functions to grab data from JSON I'm working on a project where i am using VueJS as front-end framework.

I've so much data in JSON format to work with for that i've written a helper function to retrieve data from JSON. much like we query SQL for table rows.

here is what i wrote

function getWhere(field, value) {
   var result = [];

    $vm.services.forEach(function(object, key){
       if(object[field] == value) {
            result.push(object)
        }
    })

   return result.length == 1 ? result[0] : result;
 }

2 Questions

Is it okay to grab data from JS objects like this i mean iterating the whole object to get a single field?
Is there a built-in solution in JS or VueJS for this kind of tasks, if Yes should i use those solutions or the function i posted here is okay to use.

result[0] : result

This is a bad idea. This will keep users of your functions guessing and unnecessarily put in guard logic to check if it's a thing, or an array of things before proceeding. If you expect many results, then always return an array. If you expect just one result, return just one (regardless of how many match), and null if none.

  iterating the whole object to get a single field

Array searches are O(n) at worst. Tough luck if you're item is on the tip of the list. While we can't do anything in the complexity side of things, your code could be simplified. Seeing $vm.services appears to be an array, you can just use array.filter.

// Will always return an array, regardless of match count
function getWhere(field, value) {
  return $vm.services.filter(function(object){
    return object[field] === value;
  });
});

Alternatively, if you just really want one item among all matches, then array.find is your guy. It takes the same form as filter, but stops going through the array when it hits a match and just returns that one item, without wrapping it in an array.

// Will always return 1 item from the array, and undefined if none
function getWhere(field, value) {
  return $vm.services.find(function(object){
    return object[field] === value;
  });
});

  Is there a built-in solution in JS or VueJS for this kind of tasks, if Yes should i use those solutions or the function i posted here is okay to use.

Yes, filter and find are just a few of many. See array methods.


Improving Custom Comparator.compare() method I have the class SeguimientoSeccion with this relevant attributes

public class SeguimientoSeccion implements Serializable {
    private Integer id;

    private Transportista transportista;
    private String fijacion;
    private Integer month;
    private Integer year;
}

I created a custom comparator to sort by

year - Integer
month - Integer
fijacion - String
Transportista.nombre - String

SeguimientoSeccionComparator.java (class I want to improve)

public class SeguimientoSeccionComparator implements Comparator<SeguimientoSeccion>
{
    public int compare(SeguimientoSeccion o1, SeguimientoSeccion o2)
    {
        int result = o1.getYear().compareTo(o2.getYear());

        if (result == 0) {
            result = o1.getMonth().compareTo(o2.getMonth());
        }

        if (result != 0) return result;

        if (o1.getFijacion() == null && o2.getFijacion() == null)
            result = 0;
        else if (null == o1.getFijacion())
            result = 1;
        else if (null == o2.getFijacion())
            result = -1;
        else
            result = o1.getFijacion().compareTo(o2.getFijacion());

        if (result != 0) return result;

        if (o1.getFijacion() == null && o2.getFijacion() == null)
            result = 0;
        else if (null == o1.getTransportista())
            result = 1;
        else if (null == o2.getTransportista())
            result = -1;
        else
            result = o1.getTransportista().getNombre().compareTo(o2.getTransportista().getNombre());

        return result;
    }
}

EXTRA INFO:

equals and hashCode autogenerated by Eclipse IDE:

@Override
public int hashCode() {
    final int prime = 31;
    int result = 1;
    result = prime * result
            + ((fijacion == null) ? 0 : fijacion.hashCode());
    result = prime * result + ((id == null) ? 0 : id.hashCode());
    result = prime * result + ((month == null) ? 0 : month.hashCode());
    result = prime * result
            + ((transportista == null) ? 0 : transportista.hashCode());
    result = prime * result + ((year == null) ? 0 : year.hashCode());
    return result;
}
@Override
public boolean equals(Object obj) {
    if (this == obj)
        return true;
    if (obj == null)
        return false;
    if (getClass() != obj.getClass())
        return false;
    SeguimientoSeccion other = (SeguimientoSeccion) obj;
    if (fijacion == null) {
        if (other.fijacion != null)
            return false;
    } else if (!fijacion.equals(other.fijacion))
        return false;
    if (id == null) {
        if (other.id != null)
            return false;
    } else if (!id.equals(other.id))
        return false;
    if (month == null) {
        if (other.month != null)
            return false;
    } else if (!month.equals(other.month))
        return false;
    if (transportista == null) {
        if (other.transportista != null)
            return false;
    } else if (!transportista.equals(other.transportista))
        return false;
    if (year == null) {
        if (other.year != null)
            return false;
    } else if (!year.equals(other.year))
        return false;
    return true;
}

You should consider using the native language/library support for work like this.

Let me prime that, by adding this private function to your clas:

private Integer getTransNombre() {
    return transportista == null ? null : transportista.getNombre();
}

That gets the number, if the transportista is not null. Now, using that function, we can create a reusable comparator instance using the tricks that Java 8 introduces. Consider these chained comparators:

private static final Comparator<SeguimientoSeccion> COMP = Comparator.comparing(SeguimientoSeccion::getYear)
        .thenComparing(SeguimientoSeccion::getMonth)
        .thenComparing(SeguimientoSeccion::getFijacion, Comparator.nullsLast(Comparator.naturalOrder()))
        .thenComparing(SeguimientoSeccion::getTransNombre, Comparator.nullsLast(Comparator.naturalOrder()));

How does that work? It gets details about the instances, if they compare as "equals" then the next test is run. You can sort nulls to the end, and you can go from there.

If you still need your comparator class, it could be:

public class SeguimientoSeccionComparator implements Comparator<SeguimientoSeccion> {
    public int compare(SeguimientoSeccion o1, SeguimientoSeccion o2) {
        return COMP.compare(o1, o2);
    }
}


Efficient text to binary conversion in Java I wrote a Java FX application that translates binary to and from String and numbers in Java. Whenever the program is translating large amounts of text to binary it takes a very long time, and leaves the application unresponsive. Could a more efficient algorithm be used to speed up the process?

    // Turns binary into text.
private void binaryToText() throws BinaryFormatException
{
    if (input.toString().length() % 8 != 0)
        throw new BinaryFormatException("Binary must be a multiple of 8");

    String input = this.input.toString();
    String _byte = "", textResult = "";

    for (int i = 1 ; i <= input.length() ; i++) {
        _byte += input.charAt(i-1);
        if (i % 8 == 0) {
            int charCode = Integer.parseInt(_byte, 2);
            textResult += (char) charCode;
            _byte = "";
        }
    }

    this.output = textResult;
}

// Turns text into binary.
private void textToBinary()
{
    String binResult = "";
    final String fourBitPrepend = "0000";
    final String twoBitPrepend = "00";
    final String bitPrepend = "0";
    byte[] charCodes = input.toString().getBytes();

    for(byte ch : charCodes) {
        String binary = Integer.toBinaryString(ch);
        switch(binary.length()) {
            case 4 : binResult += fourBitPrepend + binary; break;
            case 6 : binResult += twoBitPrepend + binary; break;
            case 7 : binResult += bitPrepend + binary; break;
            default : binResult += binary;
        }
    }

    this.output = binResult;
}

Sure, you may speed up your code execution. But the core problem remains: Inresponsive UI executing a long running task.

If you expect the user wait for completion a long running task you should consider UI supported synchronized mechanisms to adress it.

Asynchronous execution is mentioned here:

http://docs.oracle.com/javafx/2/threads/jfxpub-threads.htm

The idea is: Execute your code in a separate thread and synchronize results by publishing them through the Java FX thread.


implementation of a stack in C Below is my implementation of a stack in C. Is it correct? And/or is there anything that I should fix to make it better in any way?

stack.h

#ifndef STACK_H
#define STACK_H

#define EMPTY_STACK -1
typedef struct stack
{
  char ch;
  struct stack* prev;
} Stack;

extern Stack* init_stack(void);
extern char pop(Stack*);
extern void push(Stack*, char);

#endif

stack.c

#include "stack.h"
#include "stdlib.h"

Stack* init_stack()
{
  Stack* ret = (struct stack*)malloc(sizeof(struct stack));
  ret->prev = NULL;
  return ret;
}

char pop (Stack* stck)
{
  char ret;
  if (stck->prev)
  {
    ret = stck->ch;
    Stack* prv = stck->prev;
    stck->prev = prv->prev;
    stck->ch = prv->ch;
    free(prv);
    return ret;
  }
  else
    return EMPTY_STACK;
}

void push (Stack* stck, char ch)
{
  Stack* new_nd = (struct stack*)malloc(sizeof(struct stack));

  new_nd->prev = stck->prev;
  new_nd->ch = stck->ch;

  stck->prev = new_nd;
  stck->ch = ch;
}

Thanks!

I would advise you not implementing stacks with a linked list because for each char of info you consume 2 * sizeof(void*) because of memory alignment in your struct.

Because of this you won't be able to handle a big amount of data.

What happens if you push EMPTY_STACK ? Your pop will return a false EMPTY_STACK when going through the if. Isn't there a better way to handle this ?


Inserting a record into a MySQL table using PDO Already many days I'm documenting about the SQL Injection. I was wondering if the code I wrote is vulnerable in some of its parts.

<?php
if($_POST['nameone'] AND $_POST['nametwo'])
{
   $conn = new PDO("mysql:host=localhost;dbname=dbname;charset=utf8", 'dbuser', 'dbpass');
   mysql_query('SET NAMES utf8');
   $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
   $conn->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
   $stmt = $conn->prepare("INSERT INTO `test_PDO`(`nameone`, `nametwo`) VALUES (:nomeone,:nometwo)");
   $nameone = $_POST['nameone'];
   $nametwo = $_POST['nametwo'];
   $stmt->bindParam(':nomeone', $nameone);
   $stmt->bindParam(':nometwo', $nametwo);
   $stmt->execute();
   echo "New records created successfully $nameone -- $nametwo";
   $conn = null;
}
?>

<form method="POST">
  Name one: <input type="text" name="nameone">

  Name two: <input type="text" name="nametwo">

  <input type="submit">
</form>

It would be great if somebody could review and confirm that this code is indeed not vulnerable to SQL injection. Or if it is vulnerable, kindly point out where, and how to make it better.

No, it's not vulnerable to SQL Injection, as you use prepared statements, and you use them correctly.

If you don't have any CSRF protection it is however vulnerable to XSS because of the echo (and of course CSRF), but I'm assuming that that's more of a debug statements.

Misc

You don't really need the variables nameone and nametwo. One-time variables are only useful if you need to give them some name to increase readability.
You shouldn't use mysql_*. So if you want to set the character set you should use PDO (you can also pass it as optional MYSQL_ATTR_INIT_COMMAND parameter when initializing the connection, see eg here).
try to be consistent with your names (nome vs name).
use camelCase to make your names easier to read (nameone -> nameOne).


LeetCode: Insert delete getrandom o1 C# https://leetcode.com/problems/insert-delete-getrandom-o1/
please comment on performance

Implement the RandomizedSet class:
bool insert(int val) Inserts an item val into the set if not present.
Returns true if the item was not present, false otherwise. bool
remove(int val) Removes an item val from the set if present. Returns
true if the item was present, false otherwise. int getRandom() Returns
a random element from the current set of elements (it's guaranteed
that at least one element exists when this method is called). Each
element must have the same probability of being returned. Follow up:
Could you implement the functions of the class with each function
works in average O(1) time?
Example 1:
Input ["RandomizedSet", "insert", "remove", "insert", "getRandom",
"remove", "insert", "getRandom"] [[], [1], [2], [2], [], [1], [2], []]
Output [null, true, false, true, 2, true, false, 2]
Explanation RandomizedSet randomizedSet = new RandomizedSet();
randomizedSet.insert(1); // Inserts 1 to the set. Returns true as 1
was inserted successfully. randomizedSet.remove(2); // Returns false
as 2 does not exist in the set. randomizedSet.insert(2); // Inserts 2
to the set, returns true. Set now contains [1,2].
randomizedSet.getRandom(); // getRandom() should return either 1 or 2
randomly. randomizedSet.remove(1); // Removes 1 from the set, returns
true. Set now contains [2]. randomizedSet.insert(2); // 2 was already
in the set, so return false. randomizedSet.getRandom(); // Since 2 is
the only number in the set, getRandom() will always return 2.
Constraints:
\$-2^{31} <= val <= 2^{31} - 1\$. At most \$10^5\$ calls will be made to insert, remove, and getRandom. There will be at least one element in the data
structure when getRandom is called.

public class RandomizedSet {

     private HashSet<int> _set;
        /** Initialize your data structure here. */
        public RandomizedSet()
        {
            _set = new HashSet<int>();
        }

        /** Inserts a value to the set. Returns true if the set did not already contain the specified element. */
        public bool Insert(int val)
        {
            if (_set.Contains(val))
            {
                return false;
            }
            _set.Add(val);
            return true;
        }

        /** Removes a value from the set. Returns true if the set contained the specified element. */
        public bool Remove(int val)
        {
            if (_set.Contains(val))
            {
                _set.Remove(val);
                return true;
            }
            return false;
        }

        /** Get a random element from the set. */
        public int GetRandom()
        {
            Random rand = new Random();
            int key = rand.Next(_set.Count);
            return _set.ElementAt(key);
        }
}

/**
 * Your RandomizedSet object will be instantiated and called as such:
 * RandomizedSet obj = new RandomizedSet();
 * bool param_1 = obj.Insert(val);
 * bool param_2 = obj.Remove(val);
 * int param_3 = obj.GetRandom();
 */

The HashSet<int> won't be changed hence make it readonly.
Instead of calling Contains() prior to the call to Add(), if this evaluates to false, can be simplified to just return _set.Add(val); because the Add() method returns false if the value is allready in the HashSet. Reference
Instead of calling Contains() prior to calling Remove() can be simplified as well to just return _set.Remove(val); because Remove() will return false if the item isn't in the HashSet. Reference
Calling GetRandom() in repeatedly short order  may result in the same element because the Seed of a created Random in .NET framework is based on the current timestamp. It is better to create a class-level Random to be used.

Summing up leads to
public class RandomizedSet {

    private readonly HashSet<int> _set;
    /** Initialize your data structure here. */
    public RandomizedSet()
    {
        _set = new HashSet<int>();
    }

    /** Inserts a value to the set. Returns true if the set did not already contain the specified element. */
    public bool Insert(int val)
    {
        return _set.Add(val);
    }

    /** Removes a value from the set. Returns true if the set contained the specified element. */
    public bool Remove(int val)
    {
        return _set.Remove(val);
    }

    private readonly Random rand = new Random();
    /** Get a random element from the set. */
    public int GetRandom()
    {
        int key = rand.Next(_set.Count);
        return _set.ElementAt(key);
    }
}


Best practice to create XML messages with DOMDocument class in PHP I want to create XML messages for webservice communication. These messages should be created from a pool of reusable elements. Therefore I have created different classes. A "factory" class, that only returns a message class. An element class, that consists of the reusable parts and the message classes that are the blueprints for the desired XML messages.

My code delivers the expected result, but I'm looking for a best practice. Especially a way to get rid of rewriting the same save() and __construct method in every of the message classes.

Update:

  I have improved my example and moved the root property and the
  save() method from the message classes to the Elements class. I
  also changed the name of the method from save to getMessage as
  there is a DOMDocument method with the same name. Due to these
  changes I have little less redudant code. I still don't know if it is
  good practice, particularly to use the "grandparent" methods in the
  message classes.

// class to create webservice messages
class Messages{

    private function __construct(){}

    public static function get($type) {
        //some error handling if class not exists
        return new $type;
    }
}

// message no.1
class Message_1 extends Elements{

    public function __construct() {
        parent::__construct();
        $this->root = $this->createElement("message1");
    }

    public function add_anotherElement(){
        $this->root->appendChild($this->add_anotherElementBlock("foo", "bar"));
    }

    public function add_element(){
        $this->root->appendChild($this->add_someElementBlock("foo", "bar"));
    }
}

// message no.2
class Message_2 extends Elements {

    public function __construct() {
        parent::__construct();
        $this->root = $this->createElement("message2");
    }

    public function add_elements(){
        $this->root->appendChild($this->add_anotherElementBlock("foo", "bar"));
        $this->root->appendChild($this->add_someElementBlock("foo", "bar"));
    }

}

// message no.3
class Message_3 extends Elements {

    public function __construct() {
        parent::__construct();
        $this->root = $this->createElement("message3");
    }

    public function add_element(){
        // unique Element
        $this->root->appendChild($this->createElement("foo", "bar"));
    }

}

// reusable elements
class Elements extends DOMDocument{

    protected $root;

    protected function add_someElementBlock($foo, $bar) {
        $node = $this->createElement("root");

        $attr = $this->createAttribute("id");
        $attr->value = $foo;
        $node->appendChild($attr);

        $subnode = $this->createElement("sub",$bar);

        $node->appendChild($subnode);

        return $node;
    }

    protected function add_anotherElementBlock($foo, $bar) {
        $node = $this->createElement("anotherRoot");

        $subnode = $this->createElement("anotherSubNode",$bar);
        $attr = $this->createAttribute("anotherAttribute");
        $attr->value = $foo;
        $subnode->appendChild($attr);

        $node->appendChild($subnode);

        return $node;
    }

    public function getMessage(){
        return $this->saveXML($this->root);
    }
}

$message1 = Messages::get('Message_1');
$message1->add_element();
$message1->add_anotherElement();

$message2 = Messages::get('Message_2');
$message2->add_elements();

$message3 = Messages::get('Message_3');
$message3->add_element();

//********************************************
echo "
";
print_r(htmlentities($message1->getMessage()));
echo "
";
echo "
";
echo "
";
print_r(htmlentities($message2->getMessage()));
echo "
";
echo "
";
echo "
";
print_r(htmlentities($message3->getMessage()));
echo "
";

Your message classes currently have two responsibilities: representing an application logic message and transforming itself into a representation. If you want to add other ways of serialization you have to modify the message classes itself.

The message classes only should represent the message while some other facility should be responsible for serializing your messages into xml. This keeps the interface of your message classes clean (not inheriting all those methods from DOMDocument) and allows to easily add other serialization formats.

This particular situation wants a strategy pattern: a dedicated MessageXmlSerializer takes a message, creates the surrogate DOMDocument and itself delegates the serialization of the message further to specialized classes for each message:

<?php

class MessageXmlSerializer {

    private $messageSerializers;

    public function serialize(Message $message) {
        $node = $this->createDocumentContainer();

        foreach ($this->messageSerializers as $serializer) {
            if ($serializer->canHandle($message)) {
                $node->appendChild($serializer->serialize($message));
            }
        }
    }

    protected function createDocumentContainer() {
        $node = new DOMDocument();
        // Do your intialization here
        return $node;
    }
}

The dedicated serializers are rather simple. Of course they all share a common interface:

interface MessageSerializer {
    public function canHandle(Message $message);

    public function serialize(Message $message);
}

And finally the serializer specialized on serializing Message_1

class MessageOneSerializer implements MessageSerializer {
    public function canHandle(Message $message) {
        return $message instanceof Message_1;
    }

    public function serialize(Message $message) {
        // create your node here and return it
    }
}

So far the basic concept. Now new message types can easily be added and serializiation the messages itself are separated. Of course, as for any pattern, this concept can (should) be extended and adjusted to your needs.


Binary guess game in C++ This is a program in C++ that guesses a number given by the user. I have to do homework and there's my attempt to solve an exercise from the programming book. If you know what can be done better and how to make more beautiful my code, let know me.
#include <iostream>

int main() {
    std::cout << "Enter number from range 0 to 100: ";
    int num;
    std::cin >> num;
    int guess = 50;
    bool lower = false;
    bool upper = false;
    int min = 0;
    int max = 100;
    while (guess != num) {
        if (num < guess) {
            std::cout << "Is this number lower than " << guess << "? (0 - false, 1 - true)\n";
            std::cin >> lower;
            if (lower) {
                upper = false;
                max = guess;
            }
        } else if (num > guess) {
            std::cout << "Is this number greater than " << guess << "? (0 - false, 1 - true)\n";
            std::cin >> upper;
            if (upper) {
                lower = false;
                min = guess;
            }
        } else {
            std::cout << "Let's guess, this number is equal to 50, isn't it?\n";
        }
        guess = (min+max)/2;
    }
    std::cout << "You entered a number " << guess << ", did I guess?\n";
    return 0;
}

Overview:
You need to concentrate on writing self-documenting code. This basically means using self-documenting variable names and writing functions that do specific obvious self-documented tasks.
int main(int argc, char* argv[])
{
     // Handle command line arguments.
     // Set appropriate variables.

     playGuessNumberGame();  // Pass parameters you extracted from
                             // command line or environment.
}

void playGame()
{
     int  hiddenNumber = getNumberFromPlayer();
     bool finished     = false;

     while(!finished) {
         int guess = getNumberFromPlayer();  // Or calculate guess ?
         printHint(hiddenNumber, guess);

         finished = guess == hiddenNumber;
     }
}

Another thing I would think about is storing state in an object that is updated. In this case you have the guess that is updated based on whether it was lower or higher. I would wrap this logic into an object that understands how to use this data:
class Guesser
{
     int   min;
     int   max;
     int   currentGuess;
     public:
         Guesser(): min(0), max(100), currentGuess(50) {}
         int getBestGuess() const {return currentGuess;}
         void updateGuess(bool toLow) {
             if (toLow) {
                 int newGuess = currentGuess + (max - currentGuess)/2;
                 min = currentGuess;
                 currentGuess = newGuess;
             }
             else {
                 int newGuess = min + (currentGuess - min)/2;
                 max = currentGuess;
                 currentGuess = newGuess;
             }

         }
};

This way you isolate the variables that are associated with guessing, so they are not accidentally modified by external code.
Issues
There seems to be an issue with your tests. You are both testing the actual value then asking for user input to confirm what you actually know.
    if (num < guess) {
        // Why are you asking this question.
        // You already know that the num is less than guess.
        // Are you trying to automate the processes or trying to
        // get user input to direct the action?
        std::cout << "Is this number lower than " << guess << "? (0 - false, 1 - true)\n";
        std::cin >> lower;

        // We know it is lower.
        // but you only update things if the user confirms.
        // if you get it wrong does this upset the program?
        if (lower) {
            upper = false;
            max = guess;
        }

Code Review:
You don't check if the input is valid:
    int num;
    std::cin >> num;

When accepting user input, always validate that you got something valid. Otherwise you will end up with the application locking up or going into an infinite loop. In this case the stream simply sets itself as bad when invalid data is input, which can be detected with if ()
   if (std::cin >> num) {
        // Read worked we have a number in the variable.
   }

Note: This will validate things like 54XX as good, since the first part is a valid number.  You may want to check the whole line to make sure it is just a number:
bool getValidNumberFromUser(int& num)
{
   std::string userInput;
   std::getline(std::cin, userInput);

   std::stringstream userInputStream(userInput);

   if (userInputStream >> num) {
        // Succesfully read a number.
        // but there should be no other trash on the line.
        // so let us validate that we got just a number.
        char garbage;
        if (userInputStream >> garbage) {
            // we got some garbage so there was an error on
            // user input. Lets return false from this function.
            return false;
        }
        // If we got here not garbage.
        return true;
   }
   // If we get here the user input did not even start with a number
   // So let us fail.
   return false;
}

bool isNumberOutOfRange(int number)
{
    return number < 0 || number > 100;
}

int getNumberFromPlayer()
{
    std::cout << "Enter number from 0 -> 100\n";

    int playerNumber;
    while(!getValidNumberFromUser(playerNumber) || isNumberOutOfRange(playerNumber)) {
        std::cout << "You entered an invalid number. Try Again\n";
    }
    return playerNumber;
}


LeetCode: Roman to Integer in Java I tried solve this easy leetcode challenge in java. Challenge link

Here is my solution

class Solution {
  public int romanToInt(String s) {
    Map<String, Integer> data
        = new HashMap<String, Integer>() {
        {
            put("I", 1);
            put("V", 5);
            put("X", 10);
            put("L", 50);
            put("C", 100);
            put("D", 500);
            put("M", 1000);
            put("IV", 4);
            put("IX", 9);
            put("XL", 40);
            put("XC", 90);
            put("CD", 400);
            put("CM", 900);
        }
    };
    String[] edge = {"IV", "IX", "XL", "XC", "CD", "CM"};
      int sum = 0;
      for (String val : edge) {
          if(s.isBlank()) {
              break;
          } else if(s.contains(val)) {
             sum += data.get(val);
             int index = s.indexOf(val);
             s = s.substring(0, index) + s.substring(index + 2);
          }
      }
      s = s.trim();
      for (char c: s.toCharArray()) {
          sum += data.get("" + c);
      }
      return sum;
      }
}

I am learning to compute the complexity:
Here is my interpretation
Looks like it is O(6) constant ~ O(1) * O(len(string)) ~ O(n) => O(n)
Correct me if it is not O(n)

Any suggestions on my approach to problem and code where I can reduce time and space complexity.

Your variable names are bad, s, data, all of these could be better, input, romanNumerals and so on.

Your formatting is a little bit off, consider using an autoformatter.

Also your method will spectacularly fail when handed invalid input.

put("I", 1);

Be aware of Autoboxing.

for (String val : edge) {
    if(s.isBlank()) {

Have the if outside the loop, so that you don't even need to start the loop just to bail out immediately.

sum += data.get("" + c);

I'm absolutely not a friend of such casts. Most of the time, they mask what you actually wanted and depend on implementation details, especially with numbers.

Otherwise it seems to work. What you could do is split the initial Map into two, romanNumerals and compoundRomanNumerals, that would simplify the overall logic and would remove the duplication you have with the edge array.

If you can help it, try to avoid string operations, they will always consume a lot of memory, for example:

String c = a + b;

Will result in:

Allocate c with enough memory to hold a and b
Copy a into c
Copy b into c

So if a and/or b is large, and I mean large, your memory usage will spike.

What you could do instead, is to copy the String into a StringBuilder, as that one will internally work on an array and will not create immutable instances with every change.

And you should handle error cases, null as input, Hello World! as input, such stuff. Would be a great exercise for writing unittests.


Checking if a directory is empty when having over a million files inside I have the following function to confirm if a directory contains files:

/**
 * Check if the directory is empty
 *
 * Function to ascertain if the specified directory contains files.
 * Comparing to two, because parent and current are present inside
 * the returned array. Only 3+ denotes files on the target dir.
 *
 * @param str $dir              Directory to scan.
 * @param bol $count            Whether the return value should be the count result
 *
 * @return bool|int Integer with count, TRUE or FALSE for check
 */
public function directory_with_contents($dir, $count = false) {

    try {
        if (!is_readable($dir)) return NULL;

        if ($count) {
            return count(scandir($dir));
        } else {
            return (count(scandir($dir)) == 2) ? false : true;
        }
    }
    catch(PDOException $e) {
        throw new userman_Exception("
ups!

". $e->getMessage());
    }

}

The functions works well, but it is being used with directories that contain over one million files inside. The scandir() takes his time to return the results, and in turn this function with a large amount of files becomes slow.

Is any optimization that can be done as to have this function performing any faster when dealing with directories that have a large number of files?

Why is there a catch for a PDOException?  Also, why are you masking it with another exception?  That doesn't make much sense.  How is the final catch block supposed to know what it's actually catching?

Anyway, you need to use a lower-level method than scandir.  You could use a DirectoryIterator instance, but I would probably just go with opendir.

For example:

 function is_directory_empty($path)
 {
      $dir = opendir($path);
      while (($f = readdir($dir)) !== false) {
           if ($f !== '.' && $f !== '..') {
                return true;
           }
      }
      return false;
  }

The important part here is that the loop bails upon finding the first file.  Additionally, . and .. will usually be the first two entries, so it should only have to run 3 times at maximum (though that is implementation specific, I believe, since the dots are not required to be first -- if the dots aren't first, it would run at most twice for an empty directory and once for a non-empty one).

It's worth noting that this function has terrible error handling.  In particular, there's no way to know if the directory is empty or the opendir call failed.  You could throw an exception:

if ($dir === false) {
     //I chose UnexpectedValueException because it's what DirectoryIterator throws
     throw new UnexpectedValueException("Unable to open directory: {$path}");
}

Another way would be to take a more procedural, C-esque approach:

define('DIR_EMPTY', 0);
define('DIR_NOT_EMPTY', 1);
define('DIR_ERROR_OPEN', 2);

function is_directory_empty($path)
{
    $dir = opendir($path);
    if ($dir === false) {
         return DIR_ERROR_OPEN;
    }
    while (($f = readdir($dir)) !== false) {
         if ($f !== '.' && $f !== '..') {
              return DIR_EMPTY;
         }
    }
    return DIR_NOT_EMPTY;
}

$dir_empty = is_directory_empty("/some/path");
if ($dir_empty == DIR_EMPTY) {
    echo 'Empty!';
} else if ($dir_empty == DIR_NOT_EMPTY) {
    echo 'Not empty!';
} else {
    //(Should arguably be an else-if to check the last condition)
    echo 'Error opening path';
}

I would consider having your file-counting functionality, and your is-empty functionality be two separate functions.  There's a definite relation possible between them (is_empty(x) === !file_count(x)), but they're really different functionalities.

I'm not much a fan of a boolean flag changing the functionality of a function.  It's an anti-pattern in my opinion (and quite a few other people's opinion, though I'm too lazy to find links at the moment).

Instead, consider:

function count_directory_files($path)
{
    ...
}

function is_directory_empty($path)
{
    ...
}

The names clearly express the functionality.  There's no need for a magical flag.

directory_with_contents is a bit badly named to begin with, but if I had to guess what it does, I would guess that it returns true if a directory is empty, and false otherwise.  The counting-functionality is hidden.

(On the bad naming note: what does directory_with_contents mean?  Consider if you hadn't named it.  Could you look at that and know, with certainty, what it does?  I would imagine not.  In my opinion, functions that return booleans should typically be named as a binary predicate: is_<object>_<property>, can_<object>_<property>, etc.  [I'm actually a fan of camelCase, like, isDirectoryEmpty, but that's a superfluous opinion to this post :p.])


Traversal of a tree in a top-view order I was trying to solve this problem.

  Problem Statement

  You are given a pointer to the root of a binary tree. Print the top
  view of the binary tree.  You only have to complete the function.

For example :

     3
   /   \
  5     2
 / \   / \
1   4 6   7
 \       /
  9     8
Top View : 1 -> 5 -> 3 -> 2 -> 7

I figured that the answer could be obtained by keeping on going till the leftmost node recursively and then traverse the tree till the rightmost node from the root.

Although I have managed to solve this problem I feel that my solution is just a little hack and not the best possible solution. The function I had written is

/*
struct node
{
    int data;
    node* left;
    node* right;
};

*/
void top_view(node * root)
{
   static node* temp = root;
   if(root == NULL)
    {
    return;
   }
   top_view(root->left);
   cout<<root->data<<" ";
   if(root == temp)
      {
      root = root->right;//Don't want to print the root element twice
      while(root != NULL)
         {
         cout<<root->data<<" ";
         root = root->right;
      }
   }
}

You can define two helper functions: one traverses the leftmost branch using post-order and another one traverses the rightmost branch using pre-order. See what I mean:

static void top_view_post_order(node* root)
{
    if (!root) return;

    top_view_post_order(root->left);
    cout << root->data << " ";
}

static void top_view_pre_order(node* root)
{
    if (!root) return;

    cout << root->data << " ";
    top_view_pre_order(root->right);
}

void top_view(node * root)
{
    top_view_post_order(root);
    top_view_pre_order(root->right);
}

The above passed the hackerrank's test.

Edit: What comes to static fields in functions, most books suggest that their presence is an indicator of a design flaw; don't do them - pass some structure to the function as "state information". Also, if you were to write multithreaded code, there is a great potential for data races as well.


GeeksforGeeks: Rotate matrix by 90 degrees Please review the code readability, I would like to have an easier to follow code.
Thanks

using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace ArrayQuestions
{
    /// <summary>
    /// https://www.geeksforgeeks.org/inplace-rotate-square-matrix-by-90-degrees/
    /// </summary>
    [TestClass]
    public class RotateMatrix90
    {
        [TestMethod]
        public void Rotate3x3Test()
        {
            int[,] mat =
            {
                {1, 2, 3},
                {4, 5, 6},
                {7, 8, 9}
            };
            int[,] expected =
            {
                {3, 6, 9},
                {2, 5, 8},
                {1, 4, 7}
            };
            rotateMatrix(mat);
            for (int r = 0; r < mat.GetLength(0); r++)
            {
                for (int c = 0; c < mat.GetLength(1); c++)
                {
                    Assert.AreEqual(expected[r, c], mat[r, c]);
                }
            }

        }

        void rotateMatrix(int[,] mat)
        {
            int size = mat.GetLength(0);
            for (int x = 0; x < size / 2; x++)
            {
                for (int y = x; y < size - x - 1; y++)
                {
                    int temp = mat[x, y];             // save 1
                    mat[x, y] = mat[y, size - x - 1]; // move 3 into 1
                    mat[y, size - x - 1] = mat[size - x - 1, size - y - 1]; // move 9 into 3
                    mat[size - x - 1, size - y - 1] = mat[size - y - 1, x]; // move 7 into 9
                    mat[size - y - 1, x] = temp;
                }
            }
        }
    }
}

A simple improvement for readability could be to eliminate the - 1 in the indices by replacing size with max = mat.GetLength(0) - 1;:

void rotateMatrix(int[,] mat)
{
  int max = mat.GetLength(0) - 1;
  for (int x = 0; x < (max + 1) / 2; x++)
  {
    for (int y = x; y < max - x; y++)
    {
      int temp = mat[x, y];             // save 1
      mat[x, y] = mat[y, max - x]; // move 3 into 1
      mat[y, max - x] = mat[max - x, max - y]; // move 9 into 3
      mat[max - x, max - y] = mat[max - y, x]; // move 7 into 9
      mat[max - y, x] = temp;
    }
  }
}

You could also make a pair of vars for max - x and max - y as :

void rotateMatrixReview(int[,] mat)
{
  int max = mat.GetLength(0) - 1;
  for (int x = 0; x < (max + 1) / 2; x++)
  {
    for (int y = x; y < max - x; y++)
    {
      int xmax = max - x;
      int ymax = max - y;
      int temp = mat[x, y];             // save 1
      mat[x, y] = mat[y, xmax]; // move 3 into 1
      mat[y, max - x] = mat[xmax, ymax]; // move 9 into 3
      mat[xmax, ymax] = mat[ymax, x]; // move 7 into 9
      mat[max - y, x] = temp;
    }
  }
}

Just for the fun: another approach could be:

void Swap(int[,] mx, int r1, int c1, int r2, int c2)
{
  int tmp = mx[r1, c1];
  mx[r1, c1] = mx[r2, c2];
  mx[r2, c2] = tmp;
}

void rotateMatrix(int[,] mat)
{
  int m = mat.GetLength(0) - 1;

  // l = level in the matrix from outer = 0 to inner
  for (int l = 0; l < (m + 1) / 2; l++)
  {
    // o = offset along rows and cols
    for (int o = 0; o < m - 2 * l; o++)
    {
      Swap(mat, l, l + o, l + o, m - l);
      Swap(mat, m - l - o, l, m - l, m - l - o);
      Swap(mat, l + o, m - l, m - l - o, l);
    }
  }
}


Programming challenge “Greatest Odd Divisor” I am trying to solve a programming problem on a coding platform. When I submit the code on the coding platform, it throws a "Time Limit Exceeded" error. Can someone check my solution and help optimize it?

A food delivery company X gets a lot of orders every day. It charges some
commission from the restaurants on these orders. More formally, if an order
value is K, X charges a commission which is the greatest odd divisor of K. You
can assume that an order value will always be an integer. Given an order value
N, and let C(N) be the greatest odd-divisor of N, output the value of C(1) +
C(2) + C(3) + … + C(N).
INPUT : Input will be an integer, N, the order value. 1 <= N <= 10^9
OUTPUT : Single integer which is the answer

import java.util.*;
public class Solution {
    static double GOD(double num)
    {
        if(num%2!=0)
        {
            return num;
        }
        else
        {
            for (double i = num / 2; i > 0 ;i--)
            {
                if (num % i == 0 && i % 2 != 0)
                {
                    return i;
                }
            }
            return 0;
        }
     }
public static void main(String args[] ) throws Exception {

    Scanner sc = new Scanner(System.in);
    double num = sc.nextDouble();
    double sum = 0;
    for(double i = 1; i<=num; i++)
        {
        sum = sum + GOD(i);
    }
    System.out.println((int)sum);
}
}

You should not be using doubles to perform integer arithmetic. Stick to int or long.

This is a brute-force search that follows the instructions very literally.  That is the wrong approach. Similar to Project Euler Problem 3, the fast way to find the largest factor is by finding the smallest factors. Just divide by 2 as many times as possible.

Example:

$$360 = 2^3 \cdot 45$$

The largest odd divisor is therefore 45.


Classes representing items in an RPG game I wrote a little program in C# that contains classes representing Items in RPG game. I wanted to have access to all inherited classes parameters from list contains parent Item class instances, so this is my code:

using System;
using System.Collections.Generic;

public interface SpecialObject
{
    int GetIntParameter
    {
        get;
    }
}

public interface ElementalsObject
{
    int[] GetElementalsParameters
    {
        get;
    }
}

public class Item
{
    private string _name;
    private int _cost;

    public string name
    {
        get { return _name; }
    }

    public int cost
    {
        get { return _cost; }
    }

    public Item(string name, int cost)
    {
        _name = name;
        _cost = cost;
    }
}

public class Weapon : Item, SpecialObject, ElementalsObject
{
    private int dmgs;
    private int[] elementalsDmgs;

    public int GetIntParameter
    {
        get { return dmgs; }
    }

    public int[] GetElementalsParameters
    {
        get { return elementalsDmgs; }
    }

    public Weapon(string name, int cost, int dmgs, int[] elementalsDmgs = null) : base(name, cost)
    {
        if (elementalsDmgs != null)
            this.elementalsDmgs = elementalsDmgs;
        else
            this.elementalsDmgs = new int[] { 0, 0, 0, 0 };

        this.dmgs = dmgs;
    }
}

public class Armor : Item, SpecialObject, ElementalsObject
{
    private int armorPts;
    private int[] elementalsArmorPts;

    public int GetIntParameter
    {
        get { return armorPts; }
    }

    public int[] GetElementalsParameters
    {
        get { return elementalsArmorPts; }
    }

    public Armor(string name, int cost, int armorPts, int[] elementalsArmorPts = null) : base(name, cost)
    {
        if (elementalsArmorPts != null)
            this.elementalsArmorPts = elementalsArmorPts;
        else
            this.elementalsArmorPts = new int[] { 0, 0, 0, 0 };
        this.armorPts = armorPts;
    }
}

public class Food : Item, SpecialObject
{
    private int _hungryRestorePts;

    public int GetIntParameter
    {
        get { return _hungryRestorePts; }
    }

    public Food(string name, int cost, int hungryRestorePts) : base(name, cost)
    {
        _hungryRestorePts = hungryRestorePts;
    }
}

class Equipment
{
    public static void Test()
    {
        List<Item> items = new List<Item>();
        items.AddRange(new Item[] { new Item("Stone", 1),
                                    new Armor("Steel Helmet", 80, 10),
                                    new Weapon("Great Sword", 120, 80),
                                    new Armor("Storm Cuirass", 1000, 120, new int[4] {0, 0, 0, 60}),
                                    new Weapon("Fire Spear", 1400, 60, new int[4] {0, 80, 0, 0}),
                                    new Food("Apple", 5, 10) });

        string[] el = new string[4] { "Water", "Fire", "Earth", "Wind" };

        foreach(Item i in items)
        {
            Console.WriteLine("Name: " + i.name + ", Cost: " + i.cost);
            if(i is SpecialObject)
            {
                //SpecialObject so = i as SpecialObject;
                SpecialObject so = (SpecialObject)i;
                Console.WriteLine(" Damages/ArmorPts/HungryRestorePts: " + so.GetIntParameter);
            }

            if(i is ElementalsObject)
            {
                ElementalsObject eo = i as ElementalsObject;
                Console.WriteLine(" Elementals parameters: ");

                for (int e = 0; e < el.Length; e++)
                {
                    Console.WriteLine("  " + el[e] + ": " + eo.GetElementalsParameters[e] + ", ");
                }
            }
            Console.WriteLine();
        }
    }
}

What do you think about it? Is it good code? If no then how I can write it better? :)

public interface SpecialObject
{
    int GetIntParameter
    {
        get;
    }
}

public interface ElementalsObject
{
    int[] GetElementalsParameters
    {
        get;
    }
}

What does "object" mean? If it refers to a domain concept, then it should have a more specific name (any "thing" can be an "object"); if it's a technical term, then it should be removed (all classes are objects, no need to say so explicitly).
GetIntParameter and GetElementalsParameters are pretty poor names. What does the parameter represent? Name the property after that, not after how it may be supplied to the implementing class.

public class Item
{
    private string _name;
    private int _cost;

    public string name
    {
        get { return _name; }
    }

    public int cost
    {
        get { return _cost; }
    }

    public Item(string name, int cost)
    {
        _name = name;
        _cost = cost;
    }
}

This is ok although it doesn't really do much (or anything), just a class holding two properties. You could make the code shorter by writing it like this instead. And remember public properties should be PascalCase.

public class Item
{
    public string Name { get; private set; } // private set only required if not using C# 6.0
    public int Cost { get; private set; }

    public Item(string name, int cost)
    {
        Name = name;
        Cost = cost;
    }
}

So now we see a class implementing your interfaces.

public class Weapon : Item, SpecialObject, ElementalsObject
{
    private int dmgs;
    private int[] elementalsDmgs;

    public int GetIntParameter
    {
        get { return dmgs; }
    }

    public int[] GetElementalsParameters
    {
        get { return elementalsDmgs; }
    }

    public Weapon(string name, int cost, int dmgs, int[] elementalsDmgs = null) : base(name, cost)
    {
        if (elementalsDmgs != null)
            this.elementalsDmgs = elementalsDmgs;
        else
            this.elementalsDmgs = new int[] { 0, 0, 0, 0 };

        this.dmgs = dmgs;
    }
}

First of all the constructor can be shortened by assigning elementalsDmgs like so: this.elementalsDmgs = elementalsDmgs ?? new int[] { 0, 0, 0, 0 };.

Now you see how the property names on your interfaces are unhelpful. What does GetIntParameter mean or return on a Weapon, Armor, or Food object, aside from "an int"? Who knows. What is the meaning of the values that GetElementalsParameters returns on Weapon and Armor? Well I know it returns something to do with "elementals" (whatever that is - maybe that word makes sense in your domain), but who knows the meaning of the parameters?

The much bigger problem is that GetIntParameter actually has no meaning; your classes have int parameters, so you created an interface to represent it so you can get the value from all classes implementing that interface. But on Food and Armor it contains a number of "points", and on Weapon it refers to "damages". (Assuming I'm interpreting "dmgs" correctly - which is another issue, don't skip the vowels! "dmgs" only makes sense to you, everyone else has to guess what it might mean.) The fact that they're always int does not make them in any way related, and is not a shared behaviour or functionality.

class Equipment
{
    public static void Test()
    {
        ...
        string[] el = new string[4] { "Water", "Fire", "Earth", "Wind" };
        ...
        Console.WriteLine(" Elementals parameters: ");
        ...
        Console.WriteLine("  " + el[e] + ": " + eo.GetElementalsParameters[e] + ", ");

Ah, so the elementals parameters are "Water", "Fire", "Earth", "Wind". So the ElementalsObject should have properties with those names, each of which can be assigned an int, rather than a property with an int array. I mean, who can guarantee that the values of the int array were even supplied in the correct order?

if(i is SpecialObject)
      SpecialObject so = (SpecialObject)i;

if(i is ElementalsObject)
      ElementalsObject eo = i as ElementalsObject;

It's not clear why you do this in two different ways just a few lines apart, but neither is the best way to do it as both involve casting twice. What you should do is this, as it only casts once:

 var o = i as SpecialObject;
 if (o != null)
     // cast is valid.

I don't really want to take the time to rewrite the code myself as this doesn't feel like "real" code, more like an experiment. But some suggestions are:

Rename SpecialObject to something clearer; rename GetIntParameter to something to do with points.
Rename the ElementalsObject interface to something clearer; remove current property and replace with properties of actual names (Fire, Water, etc).
Consider whether the above should even be interfaces. To me this calls for a base class for these different types of "items", so you don't have to implement the interface properties on each type of item.
Use a factory for item creation. Creating an item like so new Armor("Storm Cuirass", 1000, 120, new int[4] {0, 0, 0, 60}) is no good as it's too time consuming and error prone.
You shouldn't have to say if (i is SpecialObject) and if (i is ElementalsObject). Use polymorphism to call a method on the objects which returns the correct result per object type. (In this case, you could simply override ToString() in the classes, but again this requires a proper base class structure.)


Replacing words from a sentence I am extremely new at scheme and I am doing this problem from here:

  Write a procedure switch that takes a sentence as its argument and
  returns a sentence in which every instance of the words I or me is
  replaced by you, while every instance of  you is replaced by me except
  at the beginning of the sentence, where it’s replaced by I.  (Don’t
  worry about capitalization of letters.) Example:

    (switch ’(You told me that I should wake you up))  (i told you that you should wake me up)

I used additional functions from here.

(define (switchnext n)
(cond ((empty? n) '())
    ((or (equal? (first n) 'you) (equal? (first n) 'You)) (se 'me (switchnext (bf n))))
    ((or (equal? (first n) 'i) (equal? (first n) 'I) (equal? (first n) 'me)) (se 'you (switchnext (bf n))))
(else (se (first n) (switchnext (bf n))))))

(define (switch n)
(cond ((empty? n) '())
    ((or (equal? (first n) 'you) (equal? (first n) 'You)) (se 'i (switchnext (bf n))))
(else (se (first n) (switchnext (bf n))))))

I absolutely hate this code; I feel it is too repetitive. I think part of it is because I probably have no way of knowing when is beginning of a sentence. How can I improve this code?

It'd be easier to separate this into two parts. First, we can do all the word transformations using map and a single word switcher:

(define (switch-word word)
  (cond ((equal? word 'I) 'you)
        ((equal? word 'me) 'you)
        ((equal? word 'Me) 'you)
        ((equal? word 'you) 'me)
        ((equal? word 'You) 'me)
        (else word)))

(define (switch sentence)
    (map switch-word sentence))'

That handles all the word swapping in one go, without having to write the iteration code yourself.

Now all we need to do is change the first word from me to I, if appropriate:

(define (me-to-I word)
    (if (equal? word 'me) 'I word))

(define (switch sentence)
    (let ((switched (map switch-word sentence)))
        (cons (me-to-I (car switched)) (cdr switched))))


Compact a comma delimited number list into ranges I'm looking for a clever way to do the following operation:

Take a list of numbers:

1, 2, 3, 4, 5, 12, 13, 14, 19

and compact it into a string like so:

1-5, 12-14, 19

With the following rule: only compress into a range (i.e. use a dash) when the count of numbers in the range is 3 or more.

I.e.: 1, 2, 4, 5 would result in: 1, 2, 4, 5 and NOT: 1-2, 4-5

This is my implementation, but I feel like it could be improved upon greatly:

 public string CompactNumberRanges(IEnumerable<int> numbers,
                                   int requiredRangeCount)
 {
     if (requiredRangeCount <= 1)
         throw new ArgumentOutOfRangeException("requiredRangeCount");

     int[] sorted = numbers.OrderBy(e => e).ToArray();

     StringBuilder b = new StringBuilder();

     for (int i = 0; i < sorted.Length; i++)
     {
         int cv = sorted[i];
         int count = 0;

         for (int j = cv; ; j++)
         {
             if (Array.IndexOf(sorted, j) == -1)
                 break;
             else
                 count++;
         }

         if (count == 0)
             throw new InvalidOperationException();
         else if (count < requiredRangeCount)
             b.Append(", ").Append(cv);
         else if (count >= requiredRangeCount)
         {
             b.Append(", ").AppendFormat("{0}-{1}", cv, sorted[i + count - 1]);

             i += count - 1;
         }
     }

     return b.ToString().Trim(',', ' '); ;
 }

As an example:

List<int> numbers = new List<int>
    (
        new int[] { 1, 2, 3, 4, 5, 12, 13, 14, 19 }
    );

Console.WriteLine(CompactNumberRanges(numbers, 3));
//  output: 1-5, 12-14, 19

I have this working fine, I'm really just looking to find better ways to do the same operation because I think it's a fun one.

You could calculate a value to group on, i.e. number - index. All consecutive numbers have the same value:

index  number  number - index
-----  ------  --------------
  0       1          1
  1       2          1
  2       3          1
  3       4          1
  4       5          1
  5      12          7
  6      13          7
  7      14          7
  8      19         11

Then you just check how many items there is in each group to determine if it should be a range.

Example:

List<int> numbers = new List<int> { 1, 2, 3, 4, 5, 12, 13, 14, 19 };

List<string> items = numbers
  .Select((n,i) => new { number = n, group = n - i })
  .GroupBy(n => n.group)
  .Select(g =>
    g.Count() >= 3 ?
      g.First().number + "-" + g.Last().number
    :
      String.Join(", ", g.Select(x => x.number))
  )
  .ToList();

Console.WriteLine(String.Join(", ", items));

Output:

1-5, 12-14, 19


How To Subtract CBOT Grain Prices I had to calculate the difference between Chicago Board of Trade grain futures prices using VB.net and came up with the following (admittedly clunky) solution.  It's a little tricky because CBOT prices, as posted, mix octal and decimal numbers.  Anyway, here is my code for you to enjoy and tear apart:

   Public Function GrainSubtraction(ByVal numberOne As Double, ByVal numberTwo As Double) As Double

    ' This is a somewhat convoluted function to subtract CBOT grain prices ... but it seems to work
    ' It assumes the numbers have no decimals - that is, if the price of corn is $4.75 3/4 per bushel, this number is represented as 4756 where the last digit (6) represents 6/8

    ' Basically, we perform octal subtraction on the smallest unit digit and regular decimal subtraction on the remaining digits
    ' I use arrays to do this so I can isolate each digit - there is probably a more efficient way to do this, though - I'm not a quant

    Dim tempOne As Double = 0.0
    Dim tempTwo As Double = 0.0

    Dim inverted As Boolean = False 'Flags whether the second number is larger than the first
    Dim answer As Double = 0.0 ' This will be our ultimate response
    Dim multiplier As Double = 1.0

    ' Here, we compare the two numbers and invert them if the second number is largest then the first

    If numberOne = numberTwo Then Return 0.0

    If numberOne < numberTwo Then
        tempOne = numberTwo
        tempTwo = numberOne
        inverted = True
    Else
        tempOne = numberOne
        tempTwo = numberTwo
    End If

    Dim lengthOne = CStr(tempOne).Length ' How many digits in number one?
    Dim lengthTwo = CStr(tempTwo).Length

    Dim I As Integer = 0
    Dim errorFlag As Boolean = False 'Let's us know whether we hit an error with this number

    Dim numberLength As Integer
    If lengthOne > lengthTwo Then numberLength = lengthOne Else numberLength = lengthTwo

    ' Create our array
    Dim numberOneArray(numberLength) As Integer
    Dim numberTwoArray(numberLength) As Integer
    Dim answerArray(numberLength) As Integer ' This will hold our answer as an array

    ' Set Array Values To Zero
    For I = 0 To numberLength - 1
        numberOneArray(I) = 0
        numberTwoArray(I) = 0
    Next

    For I = 0 To lengthOne - 1
        numberOneArray(I) = tempOne Mod 10
        tempOne = Int(tempOne / 10)
    Next

    For I = 0 To lengthTwo - 1
        numberTwoArray(I) = tempTwo Mod 10
        tempTwo = Int(tempTwo / 10)
    Next I

    ' This is where we do the actual analysis

    For I = 0 To numberLength - 1

        If I = 0 Then ' We're doing Octal subtraction here

            If numberOneArray(I) >= numberTwoArray(I) Then
                answerArray(I) = numberOneArray(I) - numberTwoArray(I)
            Else
                If I = numberLength Then
                    errorFlag = True
                Else
                    numberOneArray(I) = numberOneArray(I) + 8
                    numberOneArray(I + 1) = numberOneArray(I + 1) - 1
                    answerArray(I) = numberOneArray(I) - numberTwoArray(I)
                End If
            End If

        Else ' We're doing Decimal subtraction here

            If numberOneArray(I) >= numberTwoArray(I) Then
                answerArray(I) = numberOneArray(I) - numberTwoArray(I)
            Else
                If I = numberLength Then
                    errorFlag = True
                Else
                    numberOneArray(I) = numberOneArray(I) + 10
                    numberOneArray(I + 1) = numberOneArray(I + 1) - 1
                    answerArray(I) = numberOneArray(I) - numberTwoArray(I)
                End If
            End If

        End If

    Next

    multiplier = 1

    If errorFlag = True Then Return 0.0

    For I = 0 To numberLength - 1
        answer = answer + (answerArray(I) * multiplier)
        multiplier = multiplier * 10
    Next

    If inverted = True Then answer = -answer
    Return answer

End Function

The following will shorten your code considerably:

Public Function GrainSubtraction(ByVal numberOne As Double, ByVal numberTwo As Double) As Double
    Dim Dec1 As Integer = CInt(Int(numberOne / 10.0))
    Dim Oct1 As Integer = CInt(numberOne - (Dec1 * 10))
    Dim Dec2 As Integer = CInt(Int(numberTwo / 10))
    Dim Oct2 As Integer = CInt(numberTwo - (Dec2 * 10))
    Dim ResultDec, ResultOct As Integer
    If Oct1 < Oct2 Then
        Dec1 -= 1
        ResultOct = CInt(7 - (Oct2 - Oct1))
    Else
        ResultOct = CInt(Oct1 - Oct2)
    End If
    ResultDec = CInt(Dec1 - Dec2)
    Return CDbl((Math.Abs(ResultDec * 10) + ResultOct) * Math.Sign(ResultDec))
End Function

This does simple octal subtraction on the last digits of the numbers supplied and decimal subtraction on the rest.  I left the parameters and the return type as doubles but it would make more sense to have them as integers, since that's what they basically are.