Quickest way of finding the largest integer n such that n! has length of m bits Factstone Benchmark

  Amtel has announced that it will release a 128-bit computer chip by
  2010, a 256-bit computer by 2020, and so on, continuing its strategy
  of doubling the word-size every ten years. (Amtel released a 64-bit
  computer in 2000, a 32-bit computer in 1990, a 16-bit computer in
  1980, an 8-bit computer in 1970, and a 4-bit computer, its first, in
  1960.)

  Amtel will use a new benchmark – the Factstone – to advertise the
  vastly improved capacity of its new chips. The Factstone rating is
  defined to be the largest integer \$n\$ such that \$n!\$ can be represented as an unsigned integer in a computer word.

  Given a year \$1960\le y\le 2160\$, what will be the Factstone rating of Amtel's most recently released
  chip?

  Input

  There are several test cases. For each test case, there is one line of
  input containing \$y\$. A line containing \$0\$ follows the last test case. >

  Output

  For each test case, output a line giving the Factstone rating.

  Sample Input 1

1960
1981
0

  Sample Output 1

3
8

import math
n = int(input())
while n != 0:
    n = (n - 1960) / 10 + 2
    m = pow(2,n)
    i = 1
    while m > 0:
        m -= math.log(i,2)
        i += 1
    print(i - 2)
    n = int(input())

Can someone please help me time optimize this code? I'm doing it for a site that checks my code but I always fail at the time check.

import math
n = int(input())
while n != 0:
    n = (n - 1960) / 10 + 2

What does n represent now? n as a name for the value which is called y in the task description is already unnecessarily obscure, but aliasing it really confuses things.

In addition, note that in Python / does floating point division. If the output for 1969 should be the same as the output for 1960 (and I'm pretty sure the spec requires that) then there's a bug here.

    m = pow(2,n)

What does m represent? If you want people to review your code, use descriptive names!

    while m > 0:
        m -= math.log(i,2)
        i += 1

How many times does this loop execute for the largest possible input? How can you avoid a linear loop? (Hint: a certain Stirling analysed \$\log n!\$ ...)

(Or, arguably cheating, there are only 21 decades in view, so you could hard-code a lookup.)


Username verification class Everything in this code is completely working, but I still feel that this code needs to be refactored. any suggestions?

<?php
    class Db_CheckUsername{
        protected $_conn;
        protected $_username;
        protected $_minimumChars = 8;
        protected $_errors = array();

        public function __construct($username, $conn){
            $this->_username = $username;
            $this->_conn= $conn;
        }

        public function check(){

            $sql = "SELECT * FROM accounts WHERE username = '{$this->_username}'";

            $result = $this->_conn->query($sql);
            $numRows = $result->num_rows;

            if(preg_match('/\s/',$this->_username)){
                $this->_errors[] = "Spaces are not allowed";
            }

            if($numRows > 0){
                $this->_errors[] = "Username Taken";
            }

            if(strlen($this->_username) < $this->_minimumChars){
                $this->_errors[] = "Username Should Contain, atleast {$this->_minimumChars} chars";
            }

            return $this->_errors ? false : true;
        }

        public static function isUsernameCorrect($username,$conn){

        }

        public function getErrors(){
            return $this->_errors;
        }
    }

I meant to write one or two brief points when I started writing this, but erm, ended up a bit more :).  Anyway, some of the stuff is quite briefly explained, so if anything needs more elaboration/defense, let me know and I'll edit in more information.

SQL Injection

Your query that checks for the username is open to SQL injection.  You should use prepared statements.

Don't pass in the username

If you pass the username to check() instead of to the constructor, the object can be reusable.

public function check($username) { ... }

Allow configuration

You might as well allow $_minimumChars to be configured.  There's a lot of ways to go about this, but as long as it's only one configuration item, the easiest way would be an argument in the constructor:

public function __construct(mysqli $conn, $minimumChars = 8)
{
    $this->_conn = $conn;
    $this->_minimumChars = $minimumChars;
}

Be strict about types when possible (and reasonable)

If the signature is:

public function __construct(mysqli $conn, $minimumChars = 8)

And the object is created accidentally with something that is not an instance of mysqli, the error will be caught much sooner than if the strict check is not there.  Not particularly required, just there's not a very strong reason against it.

Break it up into more classes -- Part 1 - Validators

The benefit of this is very arguable with a small code base, however, later down the road, it will likely be nice.  Consider breaking each validation into its own class:

interface Validator {
    public function isValid($input);
    public function getErrors();
}

//Because I'm lazy :p
class Validator_AbstractValidator {

    private $_errors = array();

    abstract public function isValid($input);

    public function getErrors()
    {
        return $this->_errors;
    }

    protected function _addError($message) {
        $this->_errors[] = $message;
    }

}

class Validator_NoSpaces extends Validator_AbstractValidator {
    public function isValid($input) {
        //You would probably want to add a check that $input can be cast to a string
        if (preg_match('/\s/', $input)){
            $this->_addError("Spaces are not allowed");
            return false;
        }
        return true;
    }
}

class Validator_MinimumLength extends Validator_AbstractValidator {

    protected $_minLength;

    public function __construct($minLength)
    {
        if (!is_int($minLength) && !ctype_digit($minLength)) {
            throw new Validator_Exception("minLength must be an integer");
        }
        $this->_minLength = (int) $minLength;
    }

    public function isValid($input)
    {
        //Once again, should probably have some kind of validation that $input
        //can be cast to a string
        if (strlen($input) > $this->_minLength) {
            return true;
        } else {
            $this->_addError("Input must be at least {$this_minLength}");
            return false;
        }
    }

}

The advantage of this is that it allows you to abstract away the idea of validations, and it allows you to centralize the management of validation rules.  For example, suppose that your username and email address fields both had the requirement that they not contain spaces.

Now, what if you want to change the logic from the regular expression, to a more strict, strpos($input, ' ') === false?  If the validations are all abstracted away, you only have to change this code in one place.  If you've repeated the low level validations inside of your Db_CheckUsername, you have to change it in two places.  (This is one of the major aspects of DRY by the way.)

You may have noticed that the error messages are hard coded.  This would likely not be the desired behavior.  What you could do there is store the errors in some kind of array indexed by error type, and then instead of _addError adding a raw message, it would add an error type (which would map to an error).  (Note: this approach is 100% stolen from the Zend Framework validators :))

Break it up into more classes -- Part 2 - Element

This obviously leaves your code in a rather awkward spot, though the obvious solution of replacing the validations in the check method with the validators is essentially the solution.

What I would do is abstract away the idea of form element.  You could take the idea as far as a you want (rendering, data manipulation, so on), but I'll just focus on validation:

//We want the element to have an isValid method and a getErrors method()
//(you could make a separate interface for this, but really an element is
//just a collection of Validator instances)
interface Form_ElementInterface extends Validator { }

class Form_Element_AbstractElement extends Validator_AbstractValidator implements Form_ElementInterface
{

    protected $_validators = array();

    public function addValidator(Validator $validator)
    {
        $this->_validators[] = $validator;
    }

    public function getValidators()
    {
        return $this->_validators;
    }

    protected function _addErrors(array $errors)
    {
        foreach ($errors as $error) {
            $this->_addError($error);
        }
    }

    public function hasErrors()
    {
        return (count($this->getErrors()) !== 0);
    }

    public function isValid($input)
    {
        foreach ($this->_validators as $validator) {
            if (!$validator->isValid($input)) {
                $this->_addErrors($validator->getErrors());
            }
        }
        return !$this->hasErrors();
    }

}

class Form_Element_Username extends Form_Element_AbstractElement
{
    public function __construct($minLength = 8)
    {
        $this->addValidator(new Validator_NoSpaces());
        $this->addValidator(new Validator_MinimumLength($minLength));
        //...
    }
}

Some of the names have gotten a little bleh, but hopefully the concept is still there.  (I can never decide what to name interfaces/abstract classes...)

I don't particularly like the design of Form_Element_Username there.  I would probably just use the elements from the outside instead of creating a complicated graph of objects and dependencies (a mysqli would have to be passed through there for example).

$username = new Form_Element();
$username->addValidator(new Validator_NoSpaces());
$username->addValidator(Validator_UserNotExists($db));

Or if you decided to take the flexible error message route, it may look like:

$username = new Form_Element();
$username->addValidator(new Validator_NoSpaces(array(
    'messages' => array(
        Validator_NoSpaces::CONTAINS_SPACE => "Usernames cannot contain spaces"
    ),
)));
$username->addValidator(Validator_UserNotExists(array(
    'db' => $db,
    'messages' => array(
        Validator_UserNotExists::USERNAME_EXISTS => "The username provided already exists",
    ),
)));

Note that passing in an array of arguments instead of proper separate parameters is often bad practice.  (In fact, it's definitely bad practice here.)  It has the advantage though of allowing you to easily store data in config files and then construct validators based on that.

Break it up into more classes -- Part 3 - Form

(Not going to write code on this as the concept should be clear by now.)

Just as an Element is a collection of validations, a form is, in a lot of ways, just a collection of elements.  You could extend the idea of elements and validators to entire forms which would provide centralized management of form logic, and much cleaner use than the 10-20 lines it would take to make each element in the way presented above.

class Form_UserRegistration extends Form_AbstractForm
{
    //In the constructor or some kind of setup method (called by the abstract constructor),
    //you would want to a username element, a password element, and so on
}

Then the usage:

$errors = null;
$regForm = new Form_UserRegistration();

if ($_SERVER['REQUEST_METHOD'] === "POST") {
    if ($regForm->isValid($_POST)) {
        //Create the user
    } else {
        //Render these under the form or something
        $errors = $regForm->getErrors();
    }
}

Design note

It's a bit of an odd design to mutate the errors array behind the scenes while only returning a bool from isValid.  In particular, it may be a bit cleaner design to directly return the array of errors.  I didn't go with this approach because it complicates use of the objects, and the framework I'm most familiar with does it with two separate methods, so it's just what I'm used to.

Basically, the isValid method could look like:

For completeness:

public function validate($input) {
    $errors = array();

    if (...) {
        $errors[] = "...";
    }

    return $errors;

}

//Usage:
$errors = $obj->validate($input);
if (count($errors)) {
    //errors
} else {
    //success
}

(Or you could return true in place of the empty array)

ZF Credit

I should mention that Validator and Element concepts are highly based on the Zend Framework Zend_Validate_Interface and Zend_Element_Abstract classes.


Unit testing Spark transformation on DataFrame Looking for suggestions on how to unit test a Spark transformation with ScalaTest.  The test class generates a DataFrame from static data and passes it to a transformation, then makes assertion on the passing static data generated in the test class.  (The transform creates a second column b defined as col("a").plus(5).)

I got this to work but wonder if there's a better way to create the DataFrame?   asJava feels awkward, as does defining the row data and schema separately.

class TransformTest extends FlatSpec with Matchers with SharedSparkContext {
  "Transformer" should "add column to dataframe" in {
    val sqlContext = new SQLContext(sc)
    val rows = Seq[Row](
      Row(1),
      Row(2),
      Row(3)
     ).asJava
   val schema = StructType(Seq(StructField("a", IntegerType)))

   val df = sqlContext.createDataFrame(rows, schema)
   val df2 = new Transform().addCol(df)
   assert(df2.count() > 0)
   assert(df2.agg(sum("a")).first.getLong(0) == 6)
   assert(df2.agg(sum("b")).first.getLong(0) == 21)
  }
}

Indeed, asJava looks awkward and completely unnecessary here. You can approach this problem in a few different ways.

pass RDD[Row] instead of Seq[Row] to avoid asJava:

sqlContext.createDataFrame(sc.parallelize(Seq(Row(1), Row(2), Row(3))), schema

use product types instead of Row to avoid both asJava and passing schema:

sqlContext.createDataFrame(Seq(1, 2,  3).map(Tuple1(_))).toDF("a")

or

case class Record(x: Int)

sqlContext.createDataFrame(Seq(1, 2,  3).map(Record(_)))

use implicit conversions to avoid asJava and passing schema and get concise syntax (supported for Seq[Int] only in Spark 2.0+):

import spark.implicits._

Seq(1, 2, 3).toDF("a")

In general you should prefer product types (tuples, case classes) over Row objects. You get schema for free as well as some static typing.


Find cycles in Graph using Union Find Algorithm on Adjacency List My code detects if a cycle is present in a graph using Union find algorithm with Weighted tree analysis and path compression.

It works and I have included test cases.

# check cycles in graph using union find
# weighted tree balancing added

from collections import defaultdict
class graph:

  def __init__(self):

    self.graph = defaultdict(list)
    self.parents = {}
    self.size = {}

  def addnode(self, key1, key2, edge=0):
    if key2 not in self.graph:
      self.graph[key2] = []

    self.graph[key1].append(key2)

  def findcycle(self):

    for key in self.graph:
      self.parents[key] = -1
      self.size[key] = 1

    for vertex in self.graph:
      for neighbour in self.graph[vertex]:
        x_par = self.getparent(vertex)
        y_par = self.getparent(neighbour)
        if x_par == y_par:
          return True
        self.union(x_par, y_par)

  def getparent(self, vertex):

    while self.parents[vertex] != -1:
      vertex = self.parents[vertex]

    return vertex

  def union(self, vertex, neighbour):

    x_par = self.getparent(vertex)
    y_par = self.getparent(neighbour)
    if self.size[x_par] <= self.size[y_par]:
      self.parents[x_par] = y_par
      self.size[y_par] += self.size[x_par]
    else:
      self.parents[y_par] = x_par
      self.size[x_par] += self.size[y_par]

g = graph()
g.addnode(0, 1)
g.addnode(2, 3)
g.addnode(4,2)
g.addnode(4,1)

if g.findcycle():
    print ("Graph contains cycle")
else :
    print ("Graph does not contain cycle ")

addnode(self, key1, key2, edge=0)

The parameter edge is not used.  Either remove it, or use it, but keeping it with a default value and not using it is obfuscation.

self.graph is a defaultdict(list), so testing if key2 is not present, and setting that key’s value to an empty list if it isn’t present, which is the raison d’être for the defaultdict(list), isn’t using that class properly.  You want the key to exist, but don’t want to add any values to it.  Simply use:

_ = self.graph[key2]

findcycle(self)

This function returns True or None.  It would be much easier to write documentation for the function if it returned only one “kind” of value.  Add return False to the end of the function.

Test cases

Put your test cases inside a __name__ == '__main__' guard.  Then you can import this as a module into another file without the tests running, but running this file by itself will still execute the tests.

if __name__ == '__main__':
    g = graph()
    g.addnode(0, 1)
    # ...

PEP8

Follow the PEP8 standard guidelines.  Put a space after all commas, don’t put a space before the parentheses in function calls.  Use an automatic checker (pylint, pyflakes, ...) to ensure you don’t violate these guidelines.


Random string generation I'm using this C# function to generate random coupons for a system. How can I improve it?

public static string GenerateCoupon(int length)
{
    string result = string.Empty;
    Random random = new Random((int)DateTime.Now.Ticks);
    List<string> characters = new List<string>() { };
    for (int i = 48; i < 58; i++)
    {
        characters.Add(((char)i).ToString());
    }
    for (int i = 65; i < 91; i++)
    {
        characters.Add(((char)i).ToString());
    }
    for (int i = 97; i < 123; i++)
    {
        characters.Add(((char)i).ToString());
    }
    for (int i = 0; i < length; i++)
    {
        result += characters[random.Next(0, characters.Count)];
        Thread.Sleep(1);
    }
    return result;
}

Business requirements are:

The length of coupons are not fixed and static
Coupons can contain A-Z, a-z and 0-9 (case sensitive alphanumeric)
Coupons should be unique (which means that we store them as key in a table in database, and for each coupon, we check its uniqueness)

Let's look at some of the code:
Random random = new Random((int)DateTime.Now.Ticks);

You don't need to create a seed for the Random constructor from the clock, the parameterless constructor does that:
Random random = new Random();

List<string> characters = new List<string>() { };

You don't need the initialiser brackets when you don't put any items in the list at that point:
List<string> characters = new List<string>();

result += characters[random.Next(0, characters.Count)];

Using += to concatenate strings is bad practice. Strings are not appended at the end, as strings are immutable. Code like x += y; actually end up as x = String.Concat(x, y). You should rather use a StringBuilder to build the string.

Thread.Sleep(1);

Why on earth are you sleeping in the middle of the loop?

Instead of creating a list of strings, just use a string literal to pick characters from:
public static string GenerateCoupon(int length) {
  Random random = new Random();
  string characters = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
  StringBuilder result = new StringBuilder(length);
  for (int i = 0; i < length; i++) {
    result.Append(characters[random.Next(characters.Length)]);
  }
  return result.ToString();
}

Consider if all those characters should be included, or wheter you should omit similar looking characters like o, O and 0. Having the characters in a string literal makes it easy to do that.
Edit:
If you are going to call the method more than once, you should send in the random generator as a parameter:
public static string GenerateCoupon(int length, Random random) {
  string characters = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
  StringBuilder result = new StringBuilder(length);
  for (int i = 0; i < length; i++) {
    result.Append(characters[random.Next(characters.Length)]);
  }
  return result.ToString();
}

Usage:
Random rnd = new Random();
string[] coupon = new string[10];
for (int i = 0; i < coupon.Length; i++) {
  coupon[i] = GenerateCoupon(10, rnd);
}
Console.WriteLine(String.Join(Environment.NewLine,coupon));

Example output:
LHUSer9dPZ
btK0S01yLb
hruw4IXINJ
hwMdRDRujt
cr4TDezvcZ
b8tVETNXNL
JrG6sfXgZF
Y7FRypnRiQ
JbfnhY3qOx
quWNakbybY


Project Euler 46: Finding a counterexample to a Goldbach conjecture I'm working on problem 46 from project euler:

  It was proposed by Christian Goldbach that every odd composite number
  can be written as the sum of a prime and twice a square.

9 = 7 + 2×1^2
15 = 7 + 2×2^2
21 = 3 + 2×3^2
25 = 7 + 2×3^2
27 = 19 + 2×2^2
33 = 31 + 2×1^2

  It turns out that the conjecture was false. What is the smallest odd
  composite that cannot be written as the sum of a prime and twice a
  square?

My code is so ridiculously inefficient and it takes minutes before an answer pops up. (Boundary numbers are just rough estimates)

numbers = [(2*x**2) for x in list(range(1, 1000))]
normal = list(range(1, 10000000))

#produces primes under n
def primes(n):
    sieve = [True] * n
    for i in range(3, int(n**0.5) + 1, 2):
        if sieve[i]:
            sieve[i * i:: 2 * i] = [False] * int((n - i * i - 1)/(2 * i) + 1)
    return [2] + [i for i in range(3, n, 2) if sieve[i]]

primes = primes(1000)
final = []

#add the two lists in every way possible
added = [x + y for x in numbers for y in primes]

#if it does not appear in a normal number line send it to final
for x in added:
    if x not in normal:
        final.append(x)

print(min(final))

I also lack the knowledge to use any mathematical tricks or algorithms. Where can I start learning code efficiency/performance and simple algorithms to use in my code?

Optimization

It seems that you have tried to optimize your code using the sieve of Eratosthenes; however, the sieve of Eratosthenes is only useful if there is a given upper bound in the problem statement.

An optimal solution can be achieved for this problem via brute-force in the following manner (pseudocode):

n = ... # the smallest odd composite number

loop forever:
    solution_found = False
    for all possible x:
        possible_prime = n - 2 * x ** 2
        if is_prime(possible_prime):
            solution_found = True
            break
    if not solution_found:
        print("Answer: " + n)
        break
    n = n + ... # what to increment by?

Note that the above is merely pseudocode. For the line for all possible x, it is possible to bound x. Also (and most importantly), the is_prime() method needs to be cleverly thought out, possibly caching values.

Note: I just tested my implementation based on the above pseudocode and got the answer instantaneously. Too bad Goldbach didn't have a computer!


CakePHP 2.x Custom PagesController & Routing I want to direct all requests that haven't got a valid controller action to my custom page controller.

I.e. I want /pages/new & /users/login etc to work correctly, but a request for /foo or /foo/bar should be directed to /pages/display/$path.

The following code works, but I'm pretty certain I'm not doing something the right way(tm).

// In app/Config/routes.php
Router::connect('/', array('controller' => 'pages', 'action' => 'displayHome'));

$controllerList = App::objects('controller');
foreach ($controllerList as $controller) {
    $contstr = substr($controller, 0, strlen($controller)-10);
    $contstr = Inflector::tableize($contstr);
    if($contstr != 'app') {
        Router::connect('/'. $contstr, array('controller'=>$contstr));
        Router::connect('/'. $contstr .'/:action', array('controller'=>$contstr));
        Router::connect('/'. $contstr .'/:action/*', array('controller'=>$contstr));

        // FIXME: Check if admin routing is set
        Router::connect('/admin/'. $contstr, array('controller'=>$contstr, 'admin'=>true));
        Router::connect('/admin/'. $contstr .'/:action', array('controller'=>$contstr, 'admin'=>true));
        Router::connect('/admin/'. $contstr .'/:action/*', array('controller'=>$contstr, 'admin'=>true));
    }
}

Router::connect('/**', array('controller'=>'pages', 'action'=>'display'));

Would someone please be able to review this and tell me if this is okay/horribly wrong?

Use a pattern
The general principle is fine, though it won't handle plugin routes (which I assume isn't a problem)
However instead of a loop (and therefore defining 6 routes per controller) a parameter for the controller name can be used which will be more concise and likely overall more performant. The code could be rewritten like this:
// Define a home route
Router::connect('/', array('controller' => 'pages', 'action' => 'displayHome'));

// Generate a regex-like controller name pattern (e.g. "posts|comments|users")
$controllerList = App::objects('controller');
$controllerList = array_map(function($c) {
    return Inflector::underscore(substr($c, 0, strlen($c)-10);
}, $controllerList));
$controllerList = array_filter($controllerList, function ($c) {
    return $c !== 'app'
});
$controller = implode('|', $controllerList);

// Define normal routes
$params = array('controller' => $controller);
Router::connect('/:controller', $params);
Router::connect('/:controller/:action', $params);
Router::connect('/:controller/:action/*', $params);

// Define admin routes
$params += array('admin' => true, 'prefix' => 'admin');
Router::connect('/admin/:controller', $params);
Router::connect('/admin/:controller/:action', $params);
Router::connect('/admin/:controller/:action/*', $params);

// Define catchall
Router::connect('/**', array('controller'=>'pages', 'action'=>'display'));

In this way there are 8 routes in total, no matter how many controllers there are in the application.


Validating user input in C# coming from XAML controls I have page where I am inserting some data into Entry fields. For each Entry I have written in XAML different controls with parameters. One of them is Formated parameter where I am giving a string which I am checking on C# logic code. The problem which I figure out that some one could insert wrong string or string but for example with lower case. How can I improve this code for any wrong input scenarios ?

This is my xaml code:

<ContentPage.Content>

    <StackLayout Padding="7,7,7,7" VerticalOptions="FillAndExpand" HorizontalOptions="FillAndExpand" Spacing="0">

      <cv:LabelEntryView x:Name="Name" LabelText="some text 4" ColorBg="White" MaxLength="20" Placeholder="some text 4" Formated="NameValidation" Keyboard="Text" >
      </cv:LabelEntryView>

      <cv:LabelEntryView x:Name="CardNr" LabelText="some text 3" ColorBg="White" MaxLength="20" Placeholder="some text 3" Formated="CardNrValidation" Keyboard="Numeric"  >
      </cv:LabelEntryView>

      <cv:LabelEntryView x:Name="ExpDate" LabelText="some text 2" ColorBg="White" MaxLength="5" Placeholder="some text 2" Formated="ExpDate" Keyboard="Numeric" >
      </cv:LabelEntryView>

      <cv:LabelEntryView x:Name="SecurNr" LabelText="some text" ColorBg="White" MaxLength="3" Placeholder="some text" Keyboard="Numeric" >
      </cv:LabelEntryView>

    </StackLayout>

  </ContentPage.Content>

and here is my C# code with if statement checking string:

private void Entry_TextChanged(object sender, TextChangedEventArgs e)
        {
            if (Formated == "NameValidation")
            {
                Entry ent = (Entry)sender;
                ToUpper(ent);
            }
            else if (Formated == "CardNrValidation")
            {
                Entry ent = (Entry)sender;
                CardNumberValidation(ent);
            }
            else if (Formated == "ExpDate")
            {
                Entry ent = (Entry)sender;
                ExpDate(ent,e);
            }
}

Maybe I have to assign type for each if check? If yes, how can I do it?
Thank you for your reviews.

This is not how it should be done in WPF especially that you are not validating the text but just changing it upper case - you validate only the numeric input.

The right way would be to use the Binding Validation

This method would allow you to write specialized validators without ifs or switches.


Stack implementation with StackInterface The code that I wrote is as follows:

public  class Astack implements StackInterface {
private int sz;
private Object[] a;
private int topPosition = -1;
public Astack(int sz){
   a = new Object[sz];
   this.sz = sz;
}
public int size(){
return topPosition+1;
}
public boolean isEmpty(){
  if(topPosition == -1){
  return true;
}
  return false;
}
public void push(Object element) throws StackFullException{
  if(topPosition+1 == sz){
    throw new StackFullException("Stack Full.");
  }
  a[++topPosition] = element;
}
public Object top() throws StackEmptyException{
  if(topPosition == -1){
    throw new StackEmptyException("Stack already Empty");
  }
return a[topPosition];
}
public Object pop() throws StackEmptyException{
  if(topPosition == -1){
    throw new StackEmptyException("Stack already Empty");
  }
  return a[topPosition--];
}
}

The StackInterface that I wrote:

public interface StackInterface{
    public int size();
    public boolean isEmpty();
    public Object top() throws StackEmptyException;
    public void push(Object element) throws StackFullException;
    public Object pop() throws StackEmptyException;
}

There isn't really much to optimize here, but there are definitely improvements you can make regarding usability and coding style.

use generics instead of Object, otherwise the stack is difficult to use in practice.
in practice, you would probably want a stack to increase its size automatically instead of throwing an exception.
your indentation is not consistent, making your code difficult to read.
you need more vertical whitespace, for example before a new method
your variable names could use improvement. What's a sz? What's an a? What's an Astack?


Calculating linux based octal file permission I'm applying for a mid level devloper position.

In order to  progress to 2nd interview, you need to pass a technical test.
One of the questions was to write a function to work out linux based octal file permissions.

How would you guys improve it, and why?

using System;
using System.Collections;
using System.Linq;

public class ReadWriteExecute
{
    public static int CalculatePermissionSum(string permString)
    {
        int octalPerm = 0;

        foreach (char permission in permString.ToArray())
        {
            switch (permission)
            {
                case 'r':
                    octalPerm += 4;
                    break;
                case 'w':
                    octalPerm += 2;
                    break;
                case 'x':
                    octalPerm += 1;
                    break;
                case '-':
                    octalPerm += 0;
                    break;
            }
        }
        return octalPerm;
    }

    public static string SymbolicToOctal(string permString)
    {
        string octalPerm = string.Empty;
        for(int x=0; x<=6;x+=3)
        {

            octalPerm += CalculatePermissionSum(new string(permString.Skip(x).Take(3).ToArray())).ToString();
        }
        return octalPerm;
    }

    public static void Main(string[] args)
    {
        // Should write 752
        Console.WriteLine(ReadWriteExecute.SymbolicToOctal("rwx-x--r-"));
    }
}

My thoughts for improvement:

You don't need to run ToArray() on your string to loop through it. Plus, it seems like CalculatePermissionSum() is expecting a single permission string like "rwx" or "r-x", so in a fixed-length situation like this (where the flags are also in a predefined order), don't bother with a loop.

int octalPerm = 0;
octalPerm += (permString[0] == 'r') ? 4 : 0;
octalPerm += (permString[1] == 'w') ? 2 : 0;
octalPerm += (permString[2] == 'x') ? 1 : 0;
return octalPerm;

SymbolicToOctal doesn't account for permission representations where the first character is a non-permission flag, like a directory "drwx-x--r-"
There's no error-checking in case an unexpected value comes through. If I ran SymbolicToOctal("Hello World"), I should really get an exception of some kind.
I disagree with the previous comment about using regular expressions to validate the input. Validation is good, but regular expressions would add a (relatively) huge amount of overhead for such simple validation. Code that works on file permissions isn't likely to be run a couple times here and there - it's more likely to be used as part of something that might run a LOT, so every inefficiency will add up. If you validate, just check the length and the expected individual characters. It might be more code compared to a regex, but it'll run a LOT faster.
If I was an interviewer, I would give a LOT of brownie points to the interviewee who threw in some unit-testing code.


Encrypt and backup folder to email daily, when online I have a backup script which should backup a folder and send it to email. This should be done once a day. As this is on my laptop which is not online 24/7 I need to check that I am online and can send email. For this script I have an entry in crontab running every 2 hours.

Because the folder is a really important to me and I am quite new to BASH, I would like to ask you if there are any weak points in my code:

#!/usr/bin/env bash

cd ~/projects

DATE=`date +%Y%m%d`
PASS="mysecretpasswordiwontwritehere"
# lastbackup file stores date of last backup in YYYYMMDD format
LASTBACKUP=$(< ./personalwiki/lastbackup)

# Check if backup already exists for current date
# and exit if yes.

if [[ "$LASTBACKUP" == "$DATE" ]]; then
  echo "Do nothing, already backed up."
  exit 0
fi

# Check if we are online
# and exit if not.

ping -c 1 www.google.com

if [[ $? -ne 0 ]]; then
  echo "Offline, cannot do anything."
  exit 0
fi

echo "Online and not equal, need to start backup"

# Pack an encrypt folder
tar cz personalwiki | openssl enc -aes-256-cbc -e -pass pass:$PASS > $DATE.enc

# Send email
MTO="mybackupemail@gmail.com"
MSUB="PW backup as on $DATE"
MATT="$DATE.enc"
EMAIL="backuprobot@mymachine.com" mutt -s "$MSUB" -a $MATT -- $MTO < /dev/null

# Write date of backup to file
echo -n $DATE > ./personalwiki/lastbackup

I would make the following changes, in order to make it more robust, and simpler:

Do not use openssl and symmetric encryption.  Use gpg and public key encryption.  The advantage is that you do not have to store any passwords, anywhere (keep the secret key away from the backup script).  Public key encryption is great for backups (you only need to access your secret key when you have to restore a backup, which happens rarely).
Do not mess around with timestamp files.  Instead, keep the last archive around, and inspect the datestamp of the archive (you can use find <path> -ctime 1 to see if the archive is older than 24 hours).  If your last backup is too old, make a new one.  Keeping one old backup around is another safety line for you, even though it's only stored locally.
Do not use a cron job.  Instead, invoke your script from a DHCP exit hook, which is run whenever your computer acquires an IP address.  You can then check if you have Internet access, and mail your backup straight away.  This also works if you're only online for a short while (and your cron job might miss that window of opportunity).
Check exit codes of everything and make sure your script alerts you somehow if things went wrong.  You might need to find a way to interface with your desktop environment in order to do that (notify-send works in Ubuntu).


Rust code that reads two columns of data from .txt file I'm writing a code to read data from text file in Rust. The target files have two columns and non-fixed length rows, for example,

example.txt

1  1
2  4
3  9
4  16

To achieve this, I wrote following code.

use std::fs::read_to_string;

pub fn open_file(filename: &str) -> (Vec<f32>, Vec<f32>){
    let data = read_to_string(filename);
    let xy = match data {
        Ok(content) => content,
        Err(error) => {panic!("Could not open or find file: {}", error);}
    };

    let xy_pairs: Vec<&str> = xy.trim().split("\n").collect();
    let mut x: Vec<f32> = Vec::new();
    let mut y: Vec<f32> = Vec::new();

    for pair in xy_pairs {
        let p: Vec<&str> = pair.trim().split(" ").collect();

        x.push(p[0].parse().unwrap());
        y.push(p[1].parse().unwrap());
    }

    (x, y)
}

I'm a beginner of Rust, so I don't know that I follow the best practice in Rust and this is the most efficient code.

I want some advice about this function.

I honestly think you should be breaking up the treatment of the file into two parts, as you are already (implicitly) doing so in your function. One part to read the file into a line iterator, another to actually parse.

The "reading into an iterator" is a simple combination of std::fs::File::read_line; I'll instead focus on the other part:

pub fn read_xy_pairs(i: impl Iterator<Item = String>) -> (Vec<f32>, Vec<f32>) {
    i
        .map(|string| -> Vec<f32> {
            string.split(" ")
                .take(2)
                .map(|element| element.parse::<f32>())
                .filter(|element| element.is_ok())
                .map(|element| element.unwrap())
                .collect()
        })
        .filter(|item| item.len() == 2)
        .map(|mut item| (item.swap_remove(0), item.swap_remove(0)))
        .unzip()
}

The processing is clearly laid out, in order:

I convert each element of the iterator, in turn, into an iterator of &str via String::split()
I then truncate this iterator to only take the first two elements lazily
Each element of this iterator gets parsed() and I filter any element that isn't okay, unwrapping those that are
This iterator gets collected into a Vec<f32>
From there, I filter out any element that does not have exactly 2 components, and convert those that do into a tuple2 of (f32, f32)
I then unzip this iterator of (f32, f32) into a (Vec<f32>, Vec<f32>) to fit your type signature


Function which returns the smallest / largest numbers in a nested array structure Given is an array which contains sub-arrays. The elements in this sub-arrays are numbers.

The task is to implement a JavaScript-function which returns an array with the smallest / largest numbers in the single arrays.

I have figured out this solution:

// Returns the smallest / largest
//   numbers contained in a
//   set of arrays.

// Parameter ---------------------
// 1.: Array - Containing other arrays.
// 2.: Function - Either Math.max or
//  Math.min

// Return -------------------------
// Array - Containing numbers in case
//  of success.
//  Containing 'undefined' in case
//  of elements which aren't arrays.
//  Containing NaN in case of elements
//  which aren't numbers.

function getExtrema(structure, funct) {

  var ret = structure.map(function(value) {
    if (Array.isArray(value)) {
      return funct.apply(null, value);
    }
  });

  return ret;
}

Complete code (with examples) on CodePen: http://codepen.io/mizech/pen/bEBMaW?editors=101

Are there better ways to solve the described task?

I shoudn't be able to think to this elegant and concise solution by myself! Notably because till now I didn't pay great attention to some of the acute features of JS.

So it was for me the opportunity to examine them deeper. From this I notice a possible little improvement, by slightly reducing the code, like this:

function getExtrema(structure, funct) {

  var ret = structure.map(function(value) {
    if (Array.isArray(value)) {
      return funct(...value);
    }
  });

  return ret;
}

Here is the working modified example: http://codepen.io/anon/pen/obYMmQ?editors=101.


Compare and find duplicates in 2 corresponding columns in 2 sheets I want to compare (500) and find duplicate daily records within 2 sheets, and copy the unmatched row to another sheet, copy the match from another to 3rd sheet, and delete the matched records from original sheet.

I have 3 worksheets (results, Master List, Follow Ups) " results" update daily with 500 records, and added to "master list", duplicate row added to "follow ups"

All have similar columns heading A to O.

I want to compare Column B (unique) and column A of worksheet "results" to " Master List".

The flow would be:

  Match a first cell value in column B of "results" to Column B cell values of " Master List"

  If match found - compare column A of "results" to Column A cell values of " Master List"

  If match found

  Copy the row of match from "Master List" for Column A to O to next available row of "Follow Ups"
  Mark the match row in "results" to be deleted in the end when search loop finished

  Else if match not found

  check next value in column B of " result" until last record

  When whole search ends, delete marked records for match found in "results" and copy all the left out records to Next available table row in "Master List".

I am kind of stuck and don't want to run in long loop, looking for expert help with shortest and fastest possible code. Here is some code already written and working, but not working well.

If possible optional approach (can both column value jointly compared with another sheet):

Set sht1 = xlwb.Worksheets("results")
    Set sht4 = xlwb.Worksheets("Master List")
    Set sht5 = xlwb.Worksheets("Follow Ups")

    For i = 2 To sht1.Range("A1").SpecialCells(xlCellTypeLastCell).Row
            For j = 2 To sht4.Range("A1").SpecialCells(xlCellTypeLastCell).Row
                    If sht1.Cells(i, 2) = sht4.Cells(j, 2) And sht1.Cells(i, 1) = sht4.Cells(j, 1) Then

                    'sht4.Rows(j).Copy
                   ' sht5.Activate

                    'sht5.Cells(1, sht5.Range("A1").SpecialCells(xlCellTypeLastCell).Row).Select

                    sht4.Rows(j).Copy _
                    Destination:=sht5.Cells(sht5.Range("A1").SpecialCells(xlCellTypeLastCell).Row + 1, 1)

                        'sht1.Rows(i).Delete

                        'i = i - 1
                     End If

            Next
    Next

sht1.Range("A2:O" & sht1.Range("A1").SpecialCells(xlCellTypeLastCell).Row).Copy Destination:=sht4.Cells(sht4.Range("A1").SpecialCells(xlCellTypeLastCell).Row, 1)

First things first, you have to figure out what your code is doing. You need to break your code up into little steps. Make sure each step makes sense and is done well. Then, you can start combining them in useful ways because it's clear what your code is doing and how.

Only then can you start really improving performance. Trying to do so before you have a clear idea of what your code is doing and how is a bad idea.

#1: Take your workbook/sheets and give them proper, descriptive names

Dim targetBook As Workbook
Set targetBook = '/ whatever xlwb is

With targetBook

    Dim resultsSheet As Worksheet
    Set resultsSheet = targetBook.Sheets("results")

    Dim masterSheet As Worksheet
    Set masterSheet = targetBook.Sheets("Master List")

    Dim followUpSheet As Worksheet
    Set followUpSheet = targetBook.Sheets("Follow Ups")

End With

#2: Find your end rows and put them in properly named variables

Dim resultsFinalRow As Long
With resultsSheet
    resultsFinalRow = .Cells(.Rows.Count, 1).End(xlUp).Row
End With

Dim masterFinalRow As Long
With masterSheet
    masterFinalRow = .Cells(.Rows.Count, 1).End(xlUp).Row
End With

Dim followUpFinalRow As Long
With followUpSheet
    followUpFinalRow = .Cells(.Rows.Count, 1).End(xlUp).Row
End With

Dim isMatch As Boolean
Dim resultsRow As Long
Dim masterRow As Long
For resultsRow = 2 To resultsFinalRow
    For masterRow = 2 To masterFinalRow

    ...

#3 Lay the framework for your loop

Dim isMatch As Boolean
Dim resultsRow As Long
Dim masterRow As Long
For resultsRow = 2 To resultsFinalRow
    For masterRow = 2 To masterFinalRow

        isMatch = (resultsSheet.Cells(resultsRow, 2) = masterSheet.Cells(masterRow, 2)) _
                And (resultsSheet.Cells(resultsRow, 1) = masterSheet.Cells(masterRow, 1))

        If isMatch Then

            '/ Do Stuff

        End If

    Next masterRow
Next resultsRow

#4: Flesh out your loop logic

Rather than remembering which rows to delete at the end, just delete them as you go. Keeps things nice and clean.

Dim copyRange As Range
Dim isMatch As Boolean
Dim matchFound As Boolean
Dim resultsRow As Long
Dim masterRow As Long
For resultsRow = 2 To resultsFinalRow

    matchFound = False

    For masterRow = 2 To masterFinalRow

        isMatch = (resultsSheet.Cells(resultsRow, 2) = masterSheet.Cells(masterRow, 2)) _
                And (resultsSheet.Cells(resultsRow, 1) = masterSheet.Cells(masterRow, 1))

        If isMatch Then

            matchFound = True

            With masterSheet
                Set copyRange = .Range(.Cells(masterRow, 1), .Cells(masterRow, 15))
            End With

            copyRange.Copy Destination:=followUpSheet.Cells(followUpRow, 1)
            followUpRow = followUpRow + 1

        End If

    Next masterRow

    If matchFound Then
        resultsSheet.Rows(resultsRow).Delete
        resultsRow = resultsRow - 1
    End If

Next resultsRow

#5: Clean Up

With resultsSheet

    resultsFinalRow = .Cells(.Rows.Count, 1).End(xlUp).Row '/ find new final row

    Set copyRange = .Range(.Cells(1, 1), .Cells(resultsFinalRow, 15))
    copyRange.Copy Destination:=masterSheet.Cells(masterFinalRow + 1, 1)

End With

Now we have a sub that's actually pretty clear and understandable:

Option Explicit

Public Sub CRquestion()

    Dim targetBook As Workbook
    Set targetBook = "" '/ whatever xlwb is

    With targetBook

        Dim resultsSheet As Worksheet
        Set resultsSheet = targetBook.Sheets("results")

        Dim masterSheet As Worksheet
        Set masterSheet = targetBook.Sheets("Master List")

        Dim followUpSheet As Worksheet
        Set followUpSheet = targetBook.Sheets("Follow Ups")

    End With

    Dim resultsFinalRow As Long
    With resultsSheet
        resultsFinalRow = .Cells(.Rows.Count, 1).End(xlUp).Row
    End With

    Dim masterFinalRow As Long
    With masterSheet
        masterFinalRow = .Cells(.Rows.Count, 1).End(xlUp).Row
    End With

    Dim followUpFinalRow As Long
    With followUpSheet
        followUpFinalRow = .Cells(.Rows.Count, 1).End(xlUp).Row
    End With

    Dim followUpRow As Long
    followUpRow = followUpFinalRow + 1 '/ next empty follow up row

    Dim copyRange As Range
    Dim isMatch As Boolean
    Dim matchFound As Boolean
    Dim resultsRow As Long
    Dim masterRow As Long
    For resultsRow = 2 To resultsFinalRow

        matchFound = False

        For masterRow = 2 To masterFinalRow

            isMatch = (resultsSheet.Cells(resultsRow, 2) = masterSheet.Cells(masterRow, 2)) _
                    And (resultsSheet.Cells(resultsRow, 1) = masterSheet.Cells(masterRow, 1))

            If isMatch Then

                matchFound = True

                With masterSheet
                    Set copyRange = .Range(.Cells(masterRow, 1), .Cells(masterRow, 15))
                End With

                copyRange.Copy Destination:=followUpSheet.Cells(followUpRow, 1)
                followUpRow = followUpRow + 1

            End If

        Next masterRow

        If matchFound Then
            resultsSheet.Rows(resultsRow).Delete
            resultsRow = resultsRow - 1
        End If

    Next resultsRow

    With resultsSheet

        resultsFinalRow = .Cells(.Rows.Count, 1).End(xlUp).Row

        Set copyRange = .Range(.Cells(1, 1), .Cells(resultsFinalRow, 15))
        copyRange.Copy Destination:=masterSheet.Cells(masterFinalRow + 1, 1)

    End With

End Sub

Performance Improvements

Standard VBA Tune Ups

The lowest-hanging VBA performance fruit are ScreenUpdating, EnableEvents and Calculation.

Application.ScreenUpdating = False
Application.EnableEvents= False
Application.Calculation= XlManual

...

Code

...

Application.ScreenUpdating = True
Application.EnableEvents= True
Application.Calculation= XlAutomatic

Every time you access the worksheet, events trigger, formulas recalculate and
Turning those options off will make your code inordinately faster. Just make sure they get reset back to normal at the end.

Delete all in one go

To iteratively build a list of rows to delete, then delete them all at once, the best way is to use Range.Union(). So you create a range then, whenever you find a row to delete, add that row to your range. At the end, take your compound range and call Range.EntireRow.Delete once to do the whole thing in one operation.

This would modify the code like so:

        Next masterRow

        If matchFound Then
            If deleteRange Is Nothing Then

                '/ for the first time we set the range
                Set deleteRange = resultsSheet.Cells(resultsRow, 1)

            Else

                '/ add the current row to our range
                Set deleteRange = Union(deleteRange, resultsSheet.Cells(resultsRow, 1))

            End If
        End If

    Next resultsRow

    If Not deleteRange Is Nothing Then deleteRange.EntireRow.Delete


Rails view to list users in a zebra-striped table <% i = 0 %>
<% @patients.each do |patient| %>
 <tr class="<%= i % 2 == 0 ? 'Even' : 'Odd' %>">
    <%= link_to patient.id, patient %>
    <%= patient.user.username %>
    <%= patient.user.first_name %>
    <%= patient.user.last_name %>
    <%= patient.user.email %>
    <%= patient.user.active %>
    <%= patient.user.disabled %>

<%= link_to 'Detail', patient %>

<% i += 1 %>

I don't like that I have to define an i variable, is there a better way?

Rails has a built in method, cycle, which will cycle between two more more options for you, so you don't have to manage the "Even"/"Odd" selection.

<% @patients.each do |patient| %>
  <%= content_tag :tr, :class => cycle('Even', 'Odd') do %>
    <%= content_tag :td, link_to(patient.id, patient) %>
    <% [:username, :first_name, :last_name, :email, :active, :disabled].each do |property| %>
      <%= content_tag :td, patient.user.send(property) %>
    <% end %>

        <%= content_tag :li, link_to('Detail', patient) %>

  <% end %>
<% end %>

If you happen to use Haml, it looks quite nice. :)

- @patients.each do |patient|
  %tr{:class => cycle('Even', 'Odd')}
    %td= link_to(patient.id, patient)
    - [:username, :first_name, :last_name, :email, :active, :disabled].each do |property|
      %td= patient.user.send(property)
    %td
      %ul.Horizlist
        %li= link_to('Detail', patient)


Returning information about the result of a file processing, without using output parameters As part of a C# class CsvConverter that converts various file types to CSV, I am designing a private class CsvConversionResult. My current design choice is to return a null value of CsvConversionResult if processing is successful.

What can I add to or remove from my design to use "best practices"? Returning primitive data types is out of the question since I want to return more than one thing.

public static class CsvConverter
{
    public class CsvConverterError
    {
        public string Message { get; set; }
        public int LineNumber { get; set; }
        public string FileName { get; set; }

        public CsvConverterError(string message, int lineNumber, string fileName)
        {
            Message = message;
            LineNumber = lineNumber;
            FileName = fileName;
        }
    }

    // returns null if successful
    public static CsvConverterError WriteCsvRowsToFile(char delimiter,
                                                 string inputFilePath,
                                                 string outputFilePath,
                                                 string qualifier = "")
    {
        CsvConverterError error = null;

        /* Some code here that sets the value of error accordingly */

        return error;
    }

As one of the commenters pointed out, adding a boolean flag to your result is one of the better ways to go about returning a successful result, especially when compared to returning null. In fact, I have a wrapper around a search result object where I do exactly this in the code for my current project.

  "null" makes me think of "something wrong".

This is exactly why null should not be used for this situation. At worst you'll have a programmer later who is totally confused about what is going on in your code, as most people will not return null from a function, especially for successful completion of that code.

This is the code I use:

public class SearchMemberEligibilityWrapper
{
    public List<RegularMember> ResultList { get; set; }
    public int ResultCount { get; set; }
    public bool ResultSuccessful { get; set; }
    public SearchType ResultSource { get; set; }
    public string ResultError { get; set; }

    public SearchMemberEligibilityWrapper()
    {
        this.ResultList = new List<RegularMember>();
        this.ResultCount = 0;
        this.ResultSuccessful = false;
        this.ResultSource = SearchType.Default;
        this.ResultError = string.Empty;
    }

    public enum SearchType
    {
        Default,
        QNXT,
        Database,
        Historical
    }
}

Then the code that uses the SearchMemberEligibilityWrapper checks for the value of ResultSuccessful to decide what to do after the search attempt. You have three states that the search result can exist in:

Search unsuccessful (error)
Search successful, 0 results
Search successful, >0 results

If you're concerned about having multiple errors per file, then using this pattern you can even have a List<string> property for your errors, and list your errors individually, rather than simply having one error for everything.


Reading a file without blocking the main thread Background Info: Running Python 3.6 on Windows 10

I have a text file parsed like this:

Ann Marie,Smith,ams@cia.com

There can over 100+ names in the text file.

Goal: When the program starts up, read in the text file using a thread, and load the data into a combobox. The main thread can continue loading other GUI components and isn't blocked by the method reading the text file.

import wx
import concurrent.futures

class MyDialog(wx.Frame):

    def __init__(self, parent, title):
        wx.Frame.__init__(self, parent, title=title, size=(250, 270))

        self.panel = wx.Panel(self, size=(250, 270))
        self.combo = wx.ComboBox(self.panel, -1, pos=(60, 100))
        self.start_read_thread()

        # code to load other GUI components

        self.Centre()
        self.Show(True)

    def read_employees(self,read_file):
        emp_list = []
        with open(read_file) as f_obj:
            for line in f_obj:
                emp_list.append(line)
        wx.CallAfter(self.combo.Append, emp_list)

    def start_read_thread(self):
        filename = 'employees.txt'
        with concurrent.futures.ThreadPoolExecutor(max_workers=2) as executor:
            executor.submit(self.read_employees, filename)

app = wx.App(False)
frame = MyDialog(None, "Sample editor")
app.MainLoop()

Not much to remark on. Beside some lack of documentation about which thread each function is expected to run in.

You can also do incremental append as you are reading to let the user see you are still doing something in case the file is really massive or slow to read:

def read_employees(self,read_file):
    emp_list = []
    with open(read_file) as f_obj:
        for line in f_obj:
            emp_list.append(line)
            if len(emp_list) > 5 #adjust as needed
                wx.CallAfter(self.combo.Append, emp_list)
                emp_list = []
    wx.CallAfter(self.combo.Append, emp_list)


Partitioning input strings by even and odd indexes I have been trying to learn javascript for a year now, and have just embarked on HackerRank's 30 days of code. After a couple of days I finally solved this problem. Here is the task (if you click the link, press x to the signup page):

  Task

  Given a string S of length N that is indexed from 0  to N-1, print its even-indexed and odd-indexed characters as  space-separated strings on a single line (see the Sample below for more detail).

  Sample Input

  2
     Hacker
     Rank

  Sample Output

  Hce akr
     Rn ak

Here is my solution. Any performance optimizations I could make here?:

function processData(input) {
    //Enter your code here

    //split the string into an array
    let words = input.split("\n").slice(1),
    odd = "",
    even = "",
    newStr = "";
    //turn each string inside the array into its own array
    let wordsArr = words.map((val) => {
        return val.split("");
    })
    //outer loop for the outer array
    for (let i = 0, len = wordsArr.length; i < len; i++){
          //inner loop for the inner arrays
        for (let j = 0, len2 = wordsArr[i].length; j < len2; j++){
         //if the index is even or odd, add to the respective strings
             if (j % 2 === 0){
                even += wordsArr[i][j]
            } else if (j % 2 !== 0) {
                odd += wordsArr[i][j];
            }

            //if the end of the array is detected, print the string
            //and set odd/even strings to be blank for the next iteration
            if (j == len2 - 1){
                newStr = `${even} ${odd}`;
                even = "";
                odd = "";
                console.log(newStr);
            }
    }
}

}

One possible solution that achieves the described result is:

`${s.replace(/./g, (s, pos) => [s, ''][pos & 1])} ${s.replace(/./g, (s, pos) => ['', s][pos & 1])}`

...But in order to answer to your question:

// 0
let wordsArr = words.map((val) => {
    return val.split("");
})

// can be rewritten as
let wordsArr = words.map(val => val.split(''))

// 1
for (let i = 0, len = wordsArr.length; i < len; i++){

// personally I use
i += 1
// following to Douglas Crockford
// this is what actually happens under the hood

// 2
if (j % 2 === 0){
   even += wordsArr[i][j]
} else if (j % 2 !== 0) {
   odd += wordsArr[i][j];
}

// can be symplified to
[even, odd][j & 1] += wordsArr[i][j];

Happy coding!


Brute-force search for solution to an unsolved mathematical inequality From this Wikipedia article, the following unsolved problem is presented as a
result of Waring's problem:

It has been proven that a finite number of k exist, and so far none are known. The follow code is meant to brute-force search for a solution:

#include <boost/multiprecision/cpp_dec_float.hpp>
//#include <cmath>
#include <iostream>

typedef boost::multiprecision::number<boost::multiprecision::cpp_dec_float<1000>> arbFloat;

enum returnID {success = 0, precisionExceeded = 1};

int main(){
    arbFloat k;

    for(k = 6; pow(3, k) - (pow(2, k) * floor(pow(1.5, k))) <= pow(2, k) - floor(pow(1.5, k)) - 2; ++k);

    if(pow(3, k) - (pow(2, k) * floor(pow(1.5, k))) <= pow(2, k) - floor(pow(1.5, k)) - 2){
        std::cout << "Solution at k = " << k << ".\n";
        return returnID::success;
    } else {
        std::cout << "Error: Precision exceeded at k = " << k << ".\n";
        return returnID::precisionExceeded;
    }
}

I am using the Boost multiprecision library cpp_dec_float, which works for large integers and decimals. The data type needs to be able to work with integers, because the solution for k needs to be a positive integer. The data type needs to work with decimals because (3/2)^k (equivalently 1.5^k) returns a decimal.

In the code I have commented out the cmath library, because it appears to be included in boost/multiprecision/cpp_dec_float.

In the for-loop, I use <= to be equivalent to the pseudocode !>.

After the for-loop, I re-check if it is a solution because currently if the precision is exceeded, it breaks the for-loop. The if-else checks if it is has either exceeded precision or is an actual solution.

The precision in this example code is 1000 (line 5), but this is changed manually as I search for larger and larger numbers that may be a solution.

I am trying to squeeze every ounce of speed out of this code, so any optimizations that make it quicker will be of great help!

I am running this code on Ubuntu 16.04, and I compile it using g++ with the -Ofast flag for the highest possible optimization during compilation.

Thanks!

A first step in optimizing would be to avoid computing the same values multiple times.

Secondly, I would drop the floating point arithmetic altogether. You have integer bases with integer exponents, so use integers. \$(\frac{3}{2})^k\$ = \$\frac{3^k}{2^k}\$ and applying floor also results in an integer.

I'm not familiar with the problem at hand, but the mention that none are known leads me to think that the values might be quite higher than 1000, otherwise a solution would have been trivial to find.


Calculator for the price of a team of ninjas and samurai var addToTeam = document.getElementsByClassName('addToTeam'),
    parentElement = document.getElementsByClassName('playerTypeBlock'),
    playerName = document.getElementsByClassName('heading'),
    totalVal = document.getElementById('totalvalue'),
    tableBody = document.querySelector('.totalPlayers');

var updatePlayers = function (evt) {

    evt.preventDefault();

    var amount = totalVal.innerHTML,
        productId = this.parentElement.getAttribute('data-player-type-id'),
        productPrice = this.parentElement.getAttribute('data-player-value'),
        playerName = this.parentNode.querySelector('.heading').innerHTML,
        playerId = $(this).parent().data('playerTypeId'),
        updateTotal = parseFloat(amount) + parseFloat(productPrice);

    var quantity = parseInt($(this).data('click'), 10) || 0;
    quantity++;
    $(this).data('click', quantity);

    totalVal.innerHTML = Math.round(updateTotal * 100) / 100;

    if (quantity > 1) {
            $('table .totalPlayers').find('#player-' + playerId + ' .quantity').text(quantity);

    }
    else {
        $('table .totalPlayers').append('
' + playerName + '	' + quantity + '
');
    }

};

for (var i = 0; i < addToTeam.length; i++) {
    addToTeam[i].addEventListener('click', updatePlayers);
}

Ninja

$10
 Add player

Samurai

$20.25
 Add player

            Players
            Quantity

 Total: $
 0.00

It looks like any additional functionality I want to add would have to go through updatePlayers function like for instance if I wanted to remove a player that removePlayer function would need to be inside the updatePlayers function too and If I had a hundred players on the page I would be attaching event listeners to each of them.

Although this works fine, I want to know if there are any OOP techniques I can use to make my code more scalable and extensible?

SEE DEMO

The main problem is to mix javascript objects with DOM elements. It's conceptual wrong to do this. However this has some advantages, first of all the execution speed.

The alternative, indeed, is to calculate everything with pure javascript and, when something change, rewrite all the DOM elements. This solution, when the data are very large and the elements change too frequently, could be not optimal.

Said this, according with your approach, I try to separate as much possible the DOM with the Javascript with some class/method bridge between them.

First rule: works always with data-* when search values on DOM. Because the css class could be change during a refactory by other developers (after several years maybe). It's also possibile that what you want see don't match what you want manipulate. For example: the quantity of Ninjas could be "1000" and you would write "1k". So it's better to store this value in a data-attribute.

Premise: This is only a point to start.

Here we go.

For link a DOM element with a Javascript Object I've added a data attribute data-object

and of course here

and here

Now we have the object Player

function Player(id, name, value) {
    this.id = id;
    this.name = name;
    this.value = value;
}

We need a Factory of players starting from DOM. Because, if something change on your HTML you should modify only this method:

Player.getPlayerFromDOM = function (domElement) {
    var $dom = $(domElement);
    var playerType = $dom.attr("data-player-type-id");
    var player = {
        id    : +$dom.attr("data-player-type-id"),
        value : +$dom.attr("data-player-value"),
        name  : $dom.find('.heading').text()
    }
    return new Player(player.id, player.name, player.value);
}

Another thing to do is to write a function that fetch the schema of players (how much cost a ninja? How much a samurai?) I think that the better solution is to inject a json with this values, but I don't know if you want/can. So fetching it by DOM.

Player.getSchema= function() {
    var players = {};
    $('[data-object="player"]').each(function() {
        var player =  Player.getPlayerFromDOM(this);
        players[player.id] = player;
    });
    return players;
}

For players we have all.
There is now the player list:

function PlayersList($table) {
    var $tbody = $table.find("totalPlayers");
    // be sure if there is the totalPlayers class
    if($tbody.length == 0) {
        $tbody = $table.append($('
').addClass("totalPlayers"));
    }
    this.$playersContainer = $tbody;
    this.$table = $table;
}

PlayersList.prototype.addRow = function(cells) {
    var $tr = $('
');
    for(var i = 0, clen = cells.length; i < clen; i++) {
        var $td = $('');
        if(cells[i].hasOwnProperty('className')) {
            $td.addClass(cells[i].className);
        }
        if(cells[i].hasOwnProperty('data')) {
            for(var dataAttr in cells[i].data) {
                $td.attr("data-"+dataAttr, cells[i].data[dataAttr]);
            }
        }
        $tr.append($td.html(cells[i].value))
    }
    this.$playersContainer.append($tr);
    return $tr;
}

It's for a simple table managment. Method AddRow add a row with some class or some data-attribute. It could be a simple method of a superclass more generic for table management.
So, when a player is added, the table must be updated with new values.

PlayersList.prototype.addPlayer = function(player) {
    var $table = this.$table;
    var $row = $table.find("tr[data-player-type-id='"+player.id+"']");
    if($row.length === 0) {
        $row = this.addRow(
            [
                {value: player.name},
                {value: 1, className: 'quantity', data:{quantity:1}}
            ]
        ).attr("data-player-type-id", player.id);
    }else {
        var newValue = +$row.find("[data-quantity]").attr("data-quantity")+1;
        $row.find("[data-quantity]").attr("data-quantity", newValue).text(newValue);
    }
    this.$table.trigger('ninja.table-updated')
}

Nothing of special. Just note the trigger of custom event ninja.table-updated that notice the listeners that something is changed (in this case, we will add a listener for total calculation).

For totalcalculation, let's create a method with compute the total with a given schema (maybe the schema could change, or could depends by other things)

PlayersList.prototype.calcTotal = function(schema) {
    var total = 0;
    this.$table.find('tbody').find("tr").each(function() {
        var playerId = $(this).attr("data-player-type-id")
        total += schema[playerId].value*$(this).find("[data-quantity]").text();
    });
    return total;
}

So we have all that we need:

var playersSchema = Player.getSchema();
var $teams = $('.teams');
var playerTable = new PlayersList($teams);

$('body').on('click', '.addToTeam', function() {
    var player = Player.getPlayerFromDOM($(this).parents('[data-player-type-id]'));
    playerTable.addPlayer(player);
});

$teams.on('ninja.table-updated', function() {
    $('#totalvalue').text(playerTable.calcTotal(playersSchema));
});

function Player(id, name, value) {
 this.id = id;
 this.name = name;
 this.value = value;
}

Player.getPlayerFromDOM = function (domElement) {

 var $dom = $(domElement);
 var playerType = $dom.attr("data-player-type-id");
 var player = {
  id    : +$dom.attr("data-player-type-id"),
  value : +$dom.attr("data-player-value"),
  name  : $dom.find('.heading').text()
 }
 return new Player(player.id, player.name, player.value);
}

Player.getSchema= function() {
 var players = {};
 $('[data-object="player"]').each(function() {

  var player =  Player.getPlayerFromDOM(this);
  players[player.id] = player;
 });
 return players;
}
function PlayersList($table) {
 var $tbody = $table.find("totalPlayers");
 // be sure if there is the totalPlayers class
 if($tbody.length == 0) {
  $tbody = $table.append($('
').addClass("totalPlayers"));
 }
 this.$playersContainer = $tbody;
 this.$table = $table;
}

PlayersList.prototype.addPlayer = function(player) {
 var $table = this.$table;
 var $row = $table.find("tr[data-player-type-id='"+player.id+"']");
 if($row.length === 0) {
  $row = this.addRow(
   [
    {value: player.name},
    {value: 1, className: 'quantity', data:{quantity:1}}
   ]
  ).attr("data-player-type-id", player.id);
 }else {
  var newValue = +$row.find("[data-quantity]").attr("data-quantity")+1;
  $row.find("[data-quantity]").attr("data-quantity", newValue).text(newValue);
 }
 this.$table.trigger('ninja.table-updated')
}

PlayersList.prototype.addRow = function(cells) {
 var $tr = $('
');
 for(var i = 0, clen = cells.length; i < clen; i++) {
  var $td = $('');
  if(cells[i].hasOwnProperty('className')) {
   $td.addClass(cells[i].className);
  }
  if(cells[i].hasOwnProperty('data')) {
   for(var dataAttr in cells[i].data) {
    $td.attr("data-"+dataAttr, cells[i].data[dataAttr]);
   }
  }
  $tr.append($td.html(cells[i].value))
 }
 this.$playersContainer.append($tr);
 return $tr;
}

PlayersList.prototype.calcTotal = function(schema) {
 var total = 0;
 this.$table.find('tbody').find("tr").each(function() {
  var playerId = $(this).attr("data-player-type-id")
  total += schema[playerId].value*$(this).find("[data-quantity]").text();
 });
 return total;
}
var playersSchema = Player.getSchema();
var $teams = $('.teams');
var playerTable = new PlayersList($teams);

$('body').on('click', '.addToTeam', function() {
 var player = Player.getPlayerFromDOM($(this).parents('[data-player-type-id]'));
 playerTable.addPlayer(player);
});

$teams.on('ninja.table-updated', function() {
 $('#totalvalue').text(playerTable.calcTotal(playersSchema));
});

Ninja

$10
 Add player

Samurai

$20.25
 Add player

            Players
            Quantity

 Total: $
 0.00


JavaScript pixel to check track user The code is just to check user session.

'use strict';

import Cookie from 'js-cookie';
import  _ from '../helpers';

module.exports = (event) => {
  const spCookieName = 'sp';
  const suCookieName = 'su';
  try {
    const e = event || {};

    let sp = Cookie.get(spCookieName);
    let isNewDevice = 1;
    if (sp) {
      Cookie.set(spCookieName, sp);
      isNewDevice = 0;
    } else {
      sp = _.uuid();
      Cookie.set(spCookieName, sp);
      isNewDevice = 1;
    }

    let su = Cookie.get(suCookieName);
    let isUserNewIn30Days = 0;
    if (!su) {
      Cookie.set(suCookieName, _.uuid(), { expires: 30 });
      isUserNewIn30Days = 1;
    }

    let pID = Cookie.get('pID'); // page session ID
    if (!pID) {
      pID = _.uuid();
      Cookie.set('pID', pID);
    }

    return {
      uID: sp,
      uNw: isNewDevice,
      uUq: isUserNewIn30Days,
      sID: Cookie.get('sID'),
      pID: pID
    };
  } catch (e) {
    return {};
  }
};

In some JavaScript engines (if I remember correctly, V8) cannot optimize code inside a try-catch. So while it may be negligible in this case, it's a practice to avoid. The workaround to it is to move out the code from the try catch to a function and call that function from the try-catch instead.

Not all your cookie names appear to be in variables. Move them into variables for consistency as well as to avoid hardcoding the cookie names everywhere.

Lastly, ES6 modules are by default run in strict mode. So no need to define 'use strict';.

The code can be simplified into the following. If you just separate the IO from the data manipulation, you can easily see patterns in the code which you can simplify.

import Cookie from 'js-cookie';
import  _ from '../helpers';

module.exports = (event) => {
  const uidCookieName = 'sp';
  const pidCookieName = 'pID';
  const sidCookieName = 'sID';
  const suCookieName  = 'su';

  const sID = Cookie.get(sidCookieName);
  const uID = Cookie.get(uidCookieName) || _.uuid();
  const pID = Cookie.get(pidCookieName) || _.uuid();
  const su  = Cookie.get(suCookieName)  || _.uuid();

  const uNw = !!Cookie.get(uidCookieName);
  const uUq = su ? 0 : 1;

  Cookie.set(uidCookieName, uID);
  Cookie.set(pidCookieName, pID);
  Cookie.set(suCookieName, su, { expires: 30 });

  return { uID, sID, pID, uNw, uUq };
}


Check SMART status on MacOS and pop up alert So I don't know much about bash scripting.  I am just a beginner.  I made this script which checks the SMART status of my root volume and if it is failing then it immediately throws a pop up alert if my drive is failing.
Here's the code:

A=$( diskutil info disk0 | grep SMART )

if [ "$A" != "   SMART Status:             Verified" ]
then

osascript -e 'tell application "Finder" to activate' -e 'tell
application "Finder" to display dialog "Your Drive is failing,
Please backup all your important files now" buttons
{"OK"} with icon stop'
fi

I know this is so simple. Do you have any suggestions to improve it?

Some suggestions:

A direct string comparison test may be flaky especially if the string contains multiple space characters. I can think of three alternatives:

Use regex matching: this can be achieved by the =~ operator inside [[ ]]: if [[ ! "$A" =~ Verified ]].
Use awk:

A=$(diskutil info disk0 | awk '/SMART Status:/{print $NF}')
if [ "$A" != 'Verified' ]
# ...

Check exit status instead of output:

if sudo smartctl -H -q silent /dev/disk0
# ...

Use more descriptive names: smart_status_text is more self-evident than A.
Properly format your code: indent the commands inside the if block and don't leave a blank line after then.


Advice on approach to organizing my business logic & data access? I've been researching various patterns for structuring my business logic & data access, particular in the context of C# and the Entity Framework. I've come up with a basic idea of how I think I'd like to do it, based on several SO answers:

https://stackoverflow.com/questions/1169188/ef-objectcontext-service-and-repositry-managing-context-lifetime
https://stackoverflow.com/questions/678922/using-transactions-with-business-processes-and-the-repository-pattern

I envision a UnitOfWork that encapsulates an IObjectContext (abstract so I can swap out contexts for testing), and exposes a repository of IQueryable<TEntity>.

interface IUnitOfWork
{
    private IObjectContext _context;

    public void SetupContext(); // Create object context here

    IQueryable<TEntity> Repository<TEntity>()
            where TEntity : class, IEntityWithKey;
}

I intend to write service classes that contain my business logic, and follow this structure:

class MyService : IService
{
    public DoWork()
    {
        // Same as nesting one using within the other
        using( IUnitOfWork unitOfWork = unitOfWorkFactory.Create() )
        using( TransactionScope tScope = new TransactionScope() )
        {
            var repository = unitOfWork.Repository<TEntity>();

            /* Do some work with repository */

            unitOfWork.Commit(); // Save changes
            tScope.Complete(); // End transaction
        }
    }
}

Am I going crazy with design patterns? Is there a better way to do this?

By reviewing this I have two general comments to your code:

Transaction

Is there any reason to use explicit transaction scope? When you call SaveChanges on the context it already uses transaction. It either uses ambient transaction or create a new one database transaction. So unless you are using multiple transactional resources (multiple database connections, MSMQ, etc.) or calling SaveChanges multiple times you don't need to create transaction explicitly. It is point of unit of work to save changes as one atomic unit. Also one side note: Default transaction used in SaveChanges uses default (or current) isolation level configured on database connection. Default for SQL Server is ReadCommitted. Default isolation level for TransactionScope is Serializable.

Testing

You mentioned that you want to make this abstraction to be able to replace context for testing. If you mean unit testing you can stop and go back to redesign your interfaces. You cannot unit test your business layer code if it uses IQueryable (Linq-to-entities) exposed by your data access layer. There is currently no provider which can simulate linq-to-entities behaviour for your unit tests. Once you use same in memory collection to mock IQueryable you will use linq-to-objects in your unit tests instead. That means you will test different code.

The only correct way is either:

Not expose IQueryable from your DAL. Keep all linq-to-entities queries as internal implementation of your repositories (= you must materialize result in repositories by for example calling .ToList).
Use integration tests against real test database instead of using unit tests with mocked data access layer

I wrote several answers about this problematic on Stack Overflow:

Unit testing DbContext
Using the repository pattern to support multiple providers

Check also linked answers in those questions.


LeetCode 126: Word Ladder II I'm posting my code for a LeetCode problem copied here. If you would like to review, please do so. Thank you for your time!
Problem

Given two words (begin and end), and a dictionary's word list,
find all shortest transformation sequence(s) from begin to end,
such that:
Only one letter can be changed at a time.
Each transformed word must exist in the word list. Note that begin
is not a transformed word. Note:

Return an empty list if there is no such transformation sequence.
All words have the same length.
All words contain only lowercase alphabetic characters.
You may assume no duplicates in the word list.
You may assume begin and end are non-empty and are not the same.

Inputs
"hit"
"cog"
["hot","dot","dog","lot","log","cog"]
"hit"
"cog"
["hot","dot","dog","lot","log"]

Outputs
[["hit","hot","dot","dog","cog"],["hit","hot","lot","log","cog"]]
[]

Code
#include <vector>
#include <string>
#include <unordered_set>
#include <unordered_map>
#include <algorithm>

struct Solution {
    const inline std::vector<std::vector<std::string>> findLadders(
                const std::string begin,
                const std::string end,
                const std::vector<std::string> &words
    ) {
        std::unordered_set<std::string> dict_words(words.begin(), words.end());

        if (dict_words.find(end) == dict_words.end()) {
            return {};
        }

        graph graph;
        std::vector<std::vector<std::string>> paths;
        std::vector<std::string> path = {begin};

        if (make_graph(graph, begin, end, dict_words)) {
            find_paths(graph, begin, end, path, paths);
        }

        return paths;
    }

private:

    typedef unordered_map<std::string, std::vector<std::string>> graph;

    const inline bool make_graph(
        graph &graph,
        const std::string begin,
        const std::string end,
        std::unordered_set<std::string> &dict_words
    ) {
        std::unordered_set<std::string> candidate_words;
        candidate_words.insert(begin);

        while (!candidate_words.empty()) {
            if (candidate_words.find(end) != candidate_words.end()) {
                return true;
            }

            for (std::string word : candidate_words) {
                dict_words.erase(word);
            }

            std::unordered_set<std::string> curr_word;

            for (std::string word : candidate_words) {
                std::string parent = word;

                for (int chari = 0; chari < word.size(); chari++) {
                    char character = word[chari];

                    for (int letter = 0; letter < 26; letter++) {
                        word[chari] = 'a' + letter;

                        if (dict_words.find(word) != dict_words.end()) {
                            curr_word.insert(word);
                            graph[parent].push_back(word);
                        }
                    }

                    word[chari] = character;
                }
            }

            std::swap(candidate_words, curr_word);
        }

        return false;
    }

    static const inline void find_paths(
        graph &graph,
        const std::string begin,
        const std::string end,
        std::vector<std::string> &path,
        std::vector<std::vector<std::string>> &paths
    ) {
        if (begin == end) {
            paths.push_back(path);

        } else {
            for (std::string child : graph[begin]) {
                path.push_back(child);
                find_paths(graph, child, end, path, paths);
                path.pop_back();
            }
        }
    }
};

References

Problem

Discuss

Solution

Direct character iteration
                for (int letter = 0; letter < 26; letter++) {
                    word[chari] = 'a' + letter;

does not need to use integers. Instead,
for (char letter = 'a'; letter <= 'z'; letter++) {
    word[chari] = letter;


Java Cash Register adding methods Just started learning Java three days ago and am currently doing a problem in my textbook where I do some basic cash register functionality. So far my code can
factor in taxes, let the user get change, and tell them the amount the actual tax is.

public class Register {
    private double purchase;
    private double payment;
    private double tax;
    public Register()
    {
    }
    public Register(double tax)
    {
        this.tax = tax;
    }

    public void recordPurchase(double amount)
    {
        purchase = purchase + amount;
    }
    public void enterPayment(double amount)
    {
        payment = amount;
    }

    public void recordTaxablePurchase()
    {
        purchase = purchase  + (purchase *(tax/100));
    }
    public double getTotalTax()
    {
        return (purchase*0.10);
    }
    public void getSalesTotal()
    {

    }
    public double giveChange()
    {
        double change = payment - purchase;
        purchase = 0;
        payment = 0;
        return change;
    }

}

Running it

public class RegisterTest {

    public static void main(String [] args)
    {
        Register pay = new Register(10);
        pay.recordPurchase(100);
        pay.recordTaxablePurchase();
        pay.enterPayment(200);

        double change = pay.giveChange();
        System.out.println(change);
    }
}

Now the next part is to do:

  After closing time, the store manager would like to know how much business was transacted during the day. Modify the CashRegister class to enable this functionality. Supply methods getSalesTotal and getSalesCount to get the total amount of all sales and the number of sales. Supply a method reset that resets any counters and totals so that the next day’s sales start from zero.

My thoughts: let's add two instance variables which should always be private:

private double totalPurchase;
private int count;

Change recordPurchase to:

public void recordPurchase(double amount)
    {
        purchase = purchase + amount;
        totalPurchase = totalPurchase + amount;
        count = count+1;
    }

Also do the same for tax.

Then simply do:

public double giveSalesTotal()
    {
        return totalPurchase;
    }

    public int getSalesCount()
    {
        return count;
    }

Does this seem like an ok way of doing it? It works, but seems really bad to create another new variable like totalPurchase, instead of using the original purchase.

consistent indentation style

You have the opening bracket of your class blocks on the same line:

public class Register {
    private double purchase;

public class RegisterTest {

but other brackets that open blocks are on the next line.
No matter what style you choose, be consistent. I suggest you put the brackets on the next line all the time as this is what the majority of your code looks like already.

initialisation of variables

In your constructor, only one variable is initialised according to a parameter.

public Register(double tax)
{
    this.tax = tax;
}

Sure, numeric types have default values, but what are they?
It's best to assign values for variables to make things clear.

public Register(double tax)
{
    this.tax = tax;
    purchase = 0;
    payment = 0;
}

that other constructor

Your empty default constructor should be calling your other constructor.
This way you can set the default value for the parameter, make sure everything is initialised and reuse the existing code:

public Register()
{
    this(0.0);
}

public Register(double tax)
{
    this.tax = tax;
    purchase = 0;
    payment = 0;
}

combine operators

Do this

a += b;

instead of that

a = a + b;

calculate the factor for the tax only once

You do this calculation (tax/100) over and over again, although it has a constant result. Do it only once in the constructor to calculate the right factor and later just multiply by that factor:

public Register(double tax)
{
    this.tax = 1 + tax/100;

welcome to Panama

As I tried to point out in a comment, doing the tax calculation is totally optional in your code. If you forget to call the method, it simply doesn't happen.

Your Register is easily manipulated to save taxes. Financial systems are often complicated and people hate to deal with them. Now if your class tries to be helpful with doing financial calculations, it should ensure that the calculations are done right. But as you expose many public methods that have to be called in a certain order, somebody using your class could easily screw that up (either to their advantage or disadvantage). In a real world application, you'd have to run checks on all values if they are within acceptable boundaries.

So far this review is mostly suggesting things that could be considered good practices, but with that last paragraph, it's clear that a line has to be crossed and more fundamental changes are necessary that are likely incompatible with the book you are reading and how it intends to develop more complex concepts proceed at your own ris... uh oooh here it coo...

....oooomes. Whoa, what a bumpy ride! Everybody still on board? Hey, where's the... hmmm looks like that Register class went missing. Whoops! But no worries I'm sure we can come up with a new one.

My expectation from a cash register is to do one main operation on it: I tell it the price to pay, the amount of money given and it will tell me how much change should be given. That would be a method then that I call pay which has the following signature:

double pay (double price, double payment)

I can do this all over again with different values for the next customer in the queue. The values change whenever I interact with the register. The only thing that stays constant is the tax. This means that only tax should be a member of the class, because it persists throughout multiple method calls. The rest of the information comes in as parameter values.

Here's how that looks:

public class Register
{
    private final double tax;

    public Register()
    {
        this(0.0);
    }

    public Register(double tax)
    {
        this.tax = 1 + tax/100;
    }

    public double pay(double price, double payment)
    {
        return payment - price * tax;
    }

    public static void main (String[] args)
    {
        Register register = new Register(10);

        double change = register.pay(100, 200);
        System.out.println(change);
    }
}

I got lazy and added main to Register. I find myself doing that a lot to create a small executable example program for a specific class contained in that class. In some sense, this is not just self documenting code, but executable documentation. Of course having a separate class is the better choice for more sophisticated testing (unit tests etc.).

tax calculation

  tell them the amount the actual tax is

I guess you are referring to that method:

public double getTotalTax()
{
    return (purchase*0.10);
}

The problem with that is that it performs a calculation with a fixed tax factor. If the number passed to the Register is a different tax, this number would be wrong.

To add this method to the new class, it needs to receive the price as a parameter, because purchase is not a member of the class any more.
An advantage is that the tax can be calculated, without making a purchase.

public double calculateTax(double price)
{
    return price * (tax - 1);
}

As tax is a factor > 1, 1 has to be subtracted. Wait!, now this calculation has to be done over and over again. Isn't that bad? Yes, but I suspect that calculating the tax happens a lot less often. If need be, this factor could also be stored in a variable of the class.

purchase counting

I would not add up all purchases in the register itself. In programming, if you have many of something, there's often a way to "hold" many things. In general these are called data structures and come in a variety of flavours from simple arrays (that String[] args is one such example) to generic collections.

If somebody wants to purchase many items, they come with such an array for example. Both in real life and online stores, people shop around filling a shopping basket and throw that at the cashier when they are finished. (some roll it, but throwing is most common)

Now imagine your shopping basket is held by the cashier all the time and whenever you want to add or remove something, you have to go to that guy to do it for you. Give him a break, fill your basket yourself.

Here's how a method taking many prices could look like:

public double pay(double price, double payment)
{
    return payment - price * tax;
}

public double pay(double[] prices, double payment)
{
    double total = 0;

    for (double price : prices)
        total += price;

    return pay(total, payment);
}

You can do this with more bling bling in Java 8, but the more important thing to take away is calling the other method to do the calculation there, to keep it in one place and not duplicate it.

I'd say that's it, but guess who turns up with even more stuff when you think "that's it"? Yes, it's

the god d*** store manager

Yeah, it's your third day here and he's already complaining. One problematic thing is that his requests are not very good:

  Modify the CashRegister class

I wouldn't do that. The Register class is working fine. Never touch a running system. There are two better approaches to this:

Create a subclass CountingRegister extends Register, do the counting in the overridden pay methods.
Create a decorator/wrapper for the Register class that does the counting in its pay methods before delegating them to it's decoratee/wrappee1.

You haven't heard about inheritance, composition or patterns yet, but this is how I would do it.

Full example code:

public class Register
{
    private final double tax;

    public Register()
    {
        this(0.0);
    }

    public Register(double tax)
    {
        this.tax = 1 + tax/100;
    }

    public double pay(double price, double payment)
    {
        return payment - price * tax;
    }

    public double pay(double[] prices, double payment)
    {
        double total = 0;

        for (double price : prices)
            total += price;

        return pay(total, payment);
    }

    public double calculateTax(double price)
    {
        return price * (tax - 1);
    }

    public static void main (String[] args)
    {
        Register register = new Register(10);
        double change;

        change = register.pay(100, 200);
        System.out.println(change);

        double[] prices = new double[]{25, 4, 39};
        change = register.pay(prices, 200);
        System.out.println(change);

        System.out.println("tax for 100 " + register.calculateTax(100));
    }
}

1 the thing that's decorated/wrapped, like employer - employee.


coffeescript and angular directive I feel this is deeply inelegant, but I really want the option of classical inheritance in the future. How could this be written better. FYI animation here is from GSAP if you are curious.

class Modal

    angular.element(document).ready =>
        @modalElem   = document.getElementById 'modal'
        @overlayElem = document.getElementById 'overlay'

    @isOpen = false

    @getWindowDim : ->
        return dim =
            height : window.innerWidth or document.documentElement.clientWidth
            width  : window.innerHeight or document.documentElement.clientHeight

    @openModal : (attrs) ->

        unless @isOpen

            @isOpen = true

            width   = attrs.width
            height  = attrs.height
            content = attrs.content

            wDim    = @getWindowDim()

            @modalElem.style.width      = 0
            @modalElem.style.height     = 0

            @overlayElem.style.display  = 'block'

            TweenLite.to @modalElem, 0.75,
                width       : width
                height      : height
                marginTop   : (height/-2)
                marginLeft  : (width/-2)
                opacity     : 1
                ease        : Back.easeOut
        @

    @closeModal : ->

        if @isOpen

            @isOpen = false

            TweenLite.to @modalElem, 0.33,
                width       : 0
                height      : 0
                marginTop   : 0
                marginLeft  : 0
                opacity     : 0
                ease        : Back.easeOut
                onComplete  : =>
                    @overlayElem.style.display = 'none'
        @

modal   = angular.module 'modal'

modal.directive 'openmodal', ->
    return (scope, element, attrs) ->
        element.bind 'click', ->
            Modal.openModal attrs

modal.directive 'closemodal', ->
    return (scope, element, attrs) ->
        element.bind 'click', ->
            Modal.closeModal()

Honestly, I'd recommend against creating a class. You may want classical inheritance in the future, but right now it's not necessary. Premature optimization rarely benefits anyone. Besides, if you do want classical inheritance later, it's pretty trivial to convert the code.

I'd just write it as simply as possible for now, and see what I'll actually need later:

do -> # wrap in an IIFE
  # simple local variables
  isOpen      = false
  modalElem   = null
  overlayElem = null
  modal       = angular.module 'modal'

  angular.element(document).ready ->
    modalElem   = document.getElementById 'modal'
    overlayElem = document.getElementById 'overlay'

  open = (attrs) ->
    return if isOpen # return early
    isOpen = true

    options =
      opacity:    1
      ease:       Back.easeOut
      height:     attrs.height
      width:      attrs.width
      marginTop:  attrs.height / -2
      marginLeft: attrs.width / -2

    modalElem.style.width  = 0
    modalElem.style.height = 0
    overlayElem.style.display = 'block'
    TweenLite.to modalElem, 0.75, options

  close = ->
    return unless isOpen # return early
    TweenLite.to modalElem, 0.33,
      width:      0
      height:     0
      marginTop:  0
      marginLeft: 0
      opacity:    0
      ease:       Back.easeOut
      onComplete: ->
        overlayElem.style.display = 'none'
        isOpen = false # moved this here, so it flips when the modal's fully closed

  modal.directive 'openmodal', ->
    (scope, element, attrs) ->
      element.bind 'click', -> open attrs

  modal.directive 'closemodal', ->
    (scope, element, attrs) ->
      element.bind 'click', -> close()

If you're compiling this as a single file with CoffeeScript's default IIFE wrapper, you can skip the do -> and indentation; it'll be added automatically.

If you don't want to take the approach above, here are some tips for your current code:

Using explicit return is only necessary if you want something other than the last evaluated expression to be returned. In you open/close functions, it'd be beneficial to simply return early (like I do in the code above) instead of having a big if branch.

Also, a neat trick:

getWindowDim : ->
  return dim =
    height : window.innerWidth or document.documentElement.clientWidth
    width  : window.innerHeight or document.documentElement.clientHeight

can be written as

getWindowDim : ->
  height : window.innerWidth or document.documentElement.clientWidth
  width  : window.innerHeight or document.documentElement.clientHeight

The return is implicit, there's no need for a variable assignment, and CoffeeScript will automatically make an object.


Converting xy coordinates to theta without discontinutities This one is, or rather, should be, fairly simple. I have a list of tuples of XY positions, and am trying to pull out angles relative to the origin from it. However, unlike atan2 / friends, I want it to be continuous, in other words not wrapping. I know (and am assuming) that the differences between successive XY coordinates are small, and that there is a minimum radius. (In other words, it should pick the multiple of \$2\pi\$ that makes the difference between the previous angle and the current angle the smallest.)

Currently, this is the relevant snippet:

thetas = [math.atan2(coords[0][1], coords[0][0])]
prev = thetas[0]
for index in range(1, len(coords)):
    coord = coords[index]
    theta = math.atan2(coord[1], coord[0]) + (prev // (2*math.pi)) * (2 * math.pi)
    while theta - prev > math.pi:
        theta -= 2*math.pi
    while prev - theta > math.pi:
        theta += 2*math.pi
    assert abs(theta - prev) < math.pi/2
    thetas.append(theta)
    prev = theta

However, it's slow, and I doubt it's Pythonic. It's also doing a lot of "magic", for lack of a better word. Any suggestions for improvement?

Line profiler results here.

for index in range() is almost always wrong. Consider

prev = coords[0]
for coord in coords[1:]:
    theta = ...
    prev = coord

Instead of computing theta directly, compute delta - this way you may actually take an advantage of small differences:

prev = (1, 0)    # Initial vector is along the X axis
theta = 0.0
for coord in coords[]:
    delta = angle_between(coord, prev)
    theta += delta
    prev = coord

Computing delta is simply an application of a cross-product formula:

def angle_between(curr, prev):
    return asin(cross_product(curr, prev) / (norm(curr) / norm(prev))