Angularjs Module Registration Structure I'm using meteor and angular. Since I can't add modules after the angular bootstrap, I made a workaround and somewhat not satisfied with the way it was coded.

How can I improve this?:

(function() {
    var controllers = ngMeteor.newControllers,
        filters     = ngMeteor.newFilters,
        directives  = ngMeteor.newDirectives,
        services    = ngMeteor.newServices

    ngMeteor.config(function($controllerProvider) {
        for (var prop in controllers) {
            if (controllers.hasOwnProperty(prop)) {
                $controllerProvider.register(prop, controllers[prop]);
            }
        }
    });

    ngMeteor.config(function($filterProvider) {
        for (var prop in controllers) {
            if (filters.hasOwnProperty(prop)) {
                $filterProvider.register(prop, filters[prop]);
            }
        }
    });

    for (var prop in directives) {
        if (directives.hasOwnProperty(prop)) {
            ngMeteor.directive(prop, directives[prop]);
        }
    }

    for (var prop in services) {
        if (services.hasOwnProperty(prop)) {
            ngMeteor.services(prop, services[prop]);
        }
    }
})();

The variables above will just have the same structure. For example:

/* Controllers */

ngMeteor.newControllers = {

    MainCtrl: function($scope) {
                // ...
    },

    SubCtrl: function($scope) {
                // ...
    }

};

Lets say you defined a module like so after the bootstrap:

myModule = angular.module('myModule',[]);

You can inject this module into ngMeteor after the bootstrap like so:

ngMeteor.requires.push('myModule');

You could easily group your controllers, directives, filters and services into separate modules and then add them to your app level module using this method. However, grouping your controllers, directives, filters and services into separate modules and then injecting it into your app level module is only one of many possible ways to organise your AngularJS app. If you're using AngularJS with MeteorJS, it would be easier to simply attach your controllers, directives, filters and services to the ngMeteor module because you have the ability to scatter these in whatever combination of javascript files as you like without having to enumerate them in your app level module.

For example, each controller could be in a separate javascript file and they could all be located in a folder called controller. Each controller javascript file would look something like this:

ngMeteor.controller('someControllerName',[$scope,function(){...}]);

Alternatively, you could put all controllers, directives, filters and services specific to a particular route into a javascript file under that route's folder. For example, lets say you have a route called "post", you could have a folder called post, and in your post folder you could have post.html, post.css and post.js, and the post.js would look something like this:

ngMeteor
.controller('someControllerName',[$scope,function(){...}])
.directive('someDirectiveName',[$scope,function(){...}])
.filter('someFilterName',[$scope,function(){...}])
.service('someServerName',[$scope,function(){...}]);

At the end of the day though, its personal preference and ngMeteor is flexible enough to allow you to structure your app in multiple ways.


Select non-task workorders, but include the costs of the tasks IBM's Maximo Asset Management platform has a WORKORDER table (265 columns).
Details:
The WORKORDER table contains two different kinds of rows:

WO records (istask = 0)
Task records (istask = 1).

The table has columns that store the actual costs (populated for both non-task WOs and for tasks):

actlabcost (actual labor cost)
actmatcost (actual material cost)
actservcost (actual services cost)
acttoolcost (actual tool costs)

View:
I've written a query/view that selects non-task WOs.
For those non-task workorders, the view rolls up the costs from the related tasks and summarizes them in these columns:

actlabcost_incltask
actmatcost_incltask
actservcost_incltask
acttoolcost_incltask
acttotalcost_incltask

I plan to use the view for multiple reports. So I've included all 265 columns in the view via select * (although, Oracle will convert the select * to actual column names when the view is created).
--WO_INCL_TASK_ACT_VW (non-task WOs, includes task actuals)
select
    t.task_actlabcost,
    t.task_actmatcost,
    t.task_actservcost,
    t.task_acttoolcost,
    t.task_acttotalcost,
    nt.actlabcost   + t.task_actlabcost     as actlabcost_incltask,
    nt.actmatcost   + t.task_actmatcost     as actmatcost_incltask,
    nt.actservcost  + t.task_actservcost    as actservcost_incltask,
    nt.acttoolcost  + t.task_acttoolcost    as acttoolcost_incltask,
    t.task_acttotalcost + nt.actlabcost + nt.actmatcost + nt.actservcost + nt.acttoolcost as acttotalcost_incltask,
    nt.*
from
    workorder nt --non-task WOs
left join
    (
    select
        parent,
        sum(actlabcost)  as task_actlabcost,
        sum(actmatcost)  as task_actmatcost,
        sum(actservcost) as task_actservcost,
        sum(acttoolcost) as task_acttoolcost,
        sum(actlabcost) + sum(actmatcost) + sum(actservcost) + sum(acttoolcost) as task_acttotalcost
    from
        workorder
    group by
        parent,
        istask
    having
        istask = 1
    ) t                --tasks
    on nt.wonum = t.parent
where
    nt.istask = 0

Question:
The view works just fine. However, it's fairly lengthy for what it does.
Can it be improved?

I know you say that

I plan to use the view for multiple reports. So I've included all 265 columns in the view via select *

but given the truly absurd column count in that table, I would consider a select-splat to be a very last resort. Are you able to narrow this at all?
Generally your query seems sane. Since you say that the following columns are non-nullable,
sum(actlabcost) + sum(actmatcost) + sum(actservcost) + sum(acttoolcost)

should be equivalent to
sum(actlabcost + actmatcost + actservcost + acttoolcost)


Life and death model in python The basic idea of the code is that agents come into the world, they age and then die. I want to keep track of who is dead and who is alive. Eventually, they will do more things but I think there should be a lot of things I am not doing best practice on. I assume I'm doing something wrong with "Global" as well.

# Classes

class Agent(object):
    def __init__(self, identification, productivity, wealth, preferences, age, health, alive, sex):
        self.identification = identification
        self.productivity = productivity
        self.wealth = wealth
        self.preferences = preferences
        self.age = age
        self.health = health
        self.alive = alive
        self.sex = sex
    def description(self):
        print("My id is", self.identification)
        print("My age is", self.age)

def Create_agents():
    global id
    alive.append(Agent(id,100,20,"blue",30,0,True,True ))
    id += 1

# functions
def Initialize():
    for x in range(3):
        Create_agents()

def Death():
    temp = alive.copy()
    for agent in temp:
        if agent.age>30:
            agent.alive = False
            print("Death of", agent.identification)
            dead.append(agent)
            alive.remove(agent)

def Time_skip():
    print("%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%SKIPPING TIME%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%")
    global year
    year += 1
    print("The new year is", year)
    for agent in alive:
        agent.age +=1
    Create_agents()
    Death()

# Values
alive = []
dead = []
id = 0
year = 0
Initialize()

# Testing
for agent in alive:
    agent.description()

Time_skip()

for agent in alive:
    agent.description()

The output is:

My id is 0
My age is 30
My id is 1
My age is 30
My id is 2
My age is 30
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%SKIPPING TIME%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
The new year is 1
Death of 0
Death of 1
Death of 2
My id is 3
My age is 30

Python has a standardized style guide called PEP 8. There is also Google's Python style guide which AFAIK is very similar to PEP 8.

Any code that goes largely against PEP 8 normally doesn't look Pythonic.

Python has a naming scheme to allow developers to know the variables type by it's name only.

Classes are in CamelCase,
Functions, methods and variables are in snake_case, and
Constants are in UPPER_SNAKE_CASE.

There should be a single space after commas.
Lines should be limited to 79 characters.
There should be one space between all methods, and two between top level classes and functions.

Overall your code adheres to these standards pretty well. Good job.
class Agent(object) is a relic from Python 2. In Python 2 classes didn't inherit from object by default. These are now called old style classes. However in Python 3 everything inherits from object by default - making all classes in Python 3 new style classes.
Your code would benefit from another class.

I'm reluctant to suggest this as the code has an over-reliance on global variables. In the future you should try to solve the problem without globals and then see if it can be made cleaner by using a class.

And so we can make a Simulation class. I would include all the global functions you have in it except initialize.
I would use an if __name__ == "__main__": guard to prevent your code from running if it's been imported accidentally.

This gets the following. There's some more changes that can be made, but the base of your code is good.

class Agent:
    def __init__(self, identification, productivity, wealth, preferences, age, health, alive, sex):
        self.identification = identification
        self.productivity = productivity
        self.wealth = wealth
        self.preferences = preferences
        self.age = age
        self.health = health
        self.alive = alive
        self.sex = sex

    def description(self):
        print("My id is", self.identification)
        print("My age is", self.age)

class Simulation:
    def __init__(self):
        self.alive = []
        self.dead = []
        self._id = 0
        self.year = 0

    def create_agent(self):
        self.alive.append(Agent(self._id, 100, 20, "blue", 30, 0, True, True))
        self._id += 1

    def deaths(self):
        for agent in self.alive.copy():
            if agent.age > 30:
                agent.alive = False
                print("Death of", agent.identification)
                self.dead.append(agent)
                self.alive.remove(agent)

    def time_skip(self):
        print("%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%SKIPPING TIME%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%")
        self.year += 1
        print("The new year is", self.year)
        for agent in self.alive:
            agent.age += 1
        self.create_agent()
        self.deaths()

if __name__ == '__main__':
    simulation = Simulation()
    for _ in range(3):
        simulation.create_agent()

    # Testing
    for agent in simulation.alive:
        agent.description()

    simulation.time_skip()

    for agent in simulation.alive:
        agent.description()


Aliasing utility class method in extended class Consider this setup:

class UtilityClass
{
    public function formatNumber($input) {
        return number_format($input, 0, '', ' ');
    }

    public function trimText($input) {
        return substr(trim($input), 0, 100);
    }

    // bunch of other methods
}

class SessionClass
{
    public $utils;
    private $permissions = array();

    function __construct() {
        $this->utils = new UtilityClass();
        $this->setPermissions();
    }

    public function getPermissions($type) {
        return $this->permissions[$type];
    }

    public function getSettings() {
        // do stuff
    }

    private function setPermissions() {
        // do stuff to set $this->permissions
    }

    // bunch of other methods
}

class BaseClass
{
    public $session;

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

    // bunch of methods
}

class ExtendedClass extends BaseClass
{
    // bunch of methods
}

$session = new SessionClass();
$extClass = new ExtendedClass($session);

When I want to use the formatNumber method of UtilityClass in my ExtendedClass, I can do it from within ExtendedClass like so:

$this->session->utils->formatNumber('some input');

...because the $session object, containing the $utils object, was injected into the $extClass object. However, if I use the formatNumber method from UtilityClass very often in ExtendedClass, it means quite a lot of (long) calls like written above. I could shorten it by using 'traits', changing:

trait Utilities
{
    function formatNumber($input) {
        // do something and return
    }
}

...and:

class ExtendedClass extends BaseClass
{
    use Utilities;

   // bunch of methods
}

I could then do the same call like this:

$this->formatNumber('some input');

I could put use Utilities in BaseClass so all extended classes could use it. But I would have to put an extra use Utilities in SessionClass, because that needs it as well. So then I end up with a $session object and an $extClass object which both contain the methods from the Utilities trait, and one is injected into the other. That seems wrong.

Another solution is 'aliasing' the formatNumber method from the UtilityClass class. To that end, I could change BaseClass:

class BaseClass
{
    function __construct($session) {
        $this->session = $session;
    }

    function get($input) {
        return $this->session->utils->formatNumber($input);
    }

    // more methods
}

This also seems not quite right. Is there a better solution? What would give the best performance/least redundancy?

Could PHP 5.6 offer the best solution? Using namespaces: Aliasing/Importing

Using use you can now 'alias' a function like this:

// aliasing a function (PHP 5.6+)
use function My\Full\functionName as func;

I'm not a fan of the UtilityClass. Here's some examples:

It breaks the single responsibility principle as it is not focused on solving a specific problem.
UtilityClass:trimText() conveys too little context about what it does, and the result of calling the method is unexpected. UtilityClass::trimText() would better suit the problem with signature like UtilityClass:shortenTextTo($text, $length);
UtilityClass::formatNumber() is not specific enough to explain clearly what it does. How does it format the number?

UtilityClass:trimText() seems like a good fit for a static helper. The problem you are trying to solve looks visual, and you should try to convey that. Look at the following:

class Text {
    public static function shortenTo($text, $length) {
        return substr(trim($text), 0, $length);
    }
}

// Html

My awesome blogpost!

<?= Text::shortenTo($text, 100); ?>

Now you have a utility for shortening a text to the wanted length.

Your session class also deals with the wrong problem. A session class would represent a Session (i.e a value object) or handle actual session management, but it wouldn't set permissions. An Authenticator class could use a SessionManager to assign specific permissions TO a session, but Sessions should have no understanding of what permissions are.

Hope that gives you something to work on. Your code is not horrible, but it seems to not focus on solving the right problems in the right places. The point of object-orientation is to represent code in realistic, understandable models, and your code does not do that at the moment. Focus on what a Session represents. What actions should you be able to do with a Session object?


.hasScroll function, checking if a scrollbar is visible in an element I have constructed a function that checks whether an element has a scrollbar or not, and can also check individual axes.

I don't know if this works in all major browsers and am unsure about the performance.

Is there any way to improve upon the code, did I miss anything?
How would a javascript equivalent look like?

Note: I first asked this question on Stack Overflow, but someone suggested I should post it here instead.

Code | Example

$.fn.hasScroll = function(axis){
    var sX = this.css("overflow-x"),
        sY = this.css("overflow-y");

    if(typeof axis == "undefined"){
        //Check both x and y declarations
        if(
            (sX == "hidden" && sY == "hidden") ||
            (sX == "visible" && sY == "visible")
        ){
            return false;
        }

        if(sX == "scroll" || sY == "scroll"){
            return true;
        }
    }else{
        //Check individual axis declarations
        switch(axis){
            case "x":
                if(sX == "hidden" || sX == "visible") return false;
                if(sX == "scroll") return true;
            break;
            case "y":
                if(sY == "hidden" || sY == "visible") return false;
                if(sY == "scroll") return true;
            break;
        }
    }

    //Compare client and scroll dimensions to see if a scrollbar is needed
    var bVertical = this[0].clientHeight < this[0].scrollHeight,
        bHorizontal = this[0].clientWidth < this[0].scrollWidth;

    return bVertical || bHorizontal;
};

=== > == (Not that it really matters in this case, but it's still better.)

Also, IIRC clientHeight etc are not cross-browser, this is why jQuery provides .position(), .offset(), .scrollTop() and .height() (whichever one best suits your needs).


Differences in word counts in two files How can I change my two functions to be more readable functions (i.e. in steps), because I am using pycharm.

import collections

def compute_word_importance(fpath1,fpath2):

    mylist1 = []
    mylist2 = []
    with open(fpath1, 'rt', encoding='UTF-8') as f:
        for line1 in f:
            for word in line1.split(" "):
                if word.strip('\n'):
                    mylist1.append(word.strip())
    with open(fpath2, 'rt', encoding='UTF-8') as x:
        for line2 in x:
            for word in line2.split(" "):
                if word.strip('\n'):
                    mylist2.append(word.strip())

    if mylist1==0 and mylist2==0:
        return None

    c=collections.Counter(mylist1)
    c.subtract(mylist2)
    return c

Change lines like this:

    for word in line1.split(" "):

To lines like:

    for word in (w.strip() for w in line1.split(" ")):

UPDATE:

Here are two ways you could tighten up your function:

1:

    def compute_word_importance(fpath1,fpath2):

        count = collections.Counter()
        with open(fpath1, 'rt', encoding='UTF-8') as fp:
            for line in f:
                count.update([w.strip() for word in line.split()])
        with open(fpath2, 'rt', encoding='UTF-8') as fp:
            for line in fp:
                count.subtract([w.strip() for word in line.split()])

        return len(counter) and counter or none

2:

    def compute_word_importance(fpath1,fpath2):

        def get_words(path):
            words = []
            with open(path, 'rt', encoding='UTF-8') as fp:
                for line1 in f:
                    words.extend([w.strip() for word in line.split()])

        counter = collections.Counter(get_words(fpath1))
        counter.subtract(get_words(fpath2))

        return len(counter) and counter or none


Possible memory-leak on a self-removable event handler A friend of mine asked me a question recently. He was needed to subscribe to an event of some object (window button click) and unsubscribe from it on a first call of a handler. Also he noticed that object is dynamically created and his handler is a delegate.

After some thinking I came up with a solution using closures of an object and a delegate itself:

class Program
{
    class A
    {
        public event EventHandler OnRaise;

        public void RaiseEvent(string text)
        {
            Console.WriteLine("a said: " + text);
            if (OnRaise != null)
                OnRaise(this, null);
        }
    }

    static void Main(string[] args)
    {
        var a = new A();

        EventHandler handler = null;
        handler = (x, y) =>
            {
                Console.WriteLine("handler's working");
                a.OnRaise -= handler;
            };

        a.OnRaise += handler;

        a.RaiseEvent("blah blah ");
        a.RaiseEvent("beep beep ");

        handler = null;
        a = null;

        GC.Collect();

        Console.ReadKey();
    }
}

It does all requested behavior but I'm not sure if it's good and safe from memory-leaks. It would be great if anyone can review the code and help with advice or offer a better way to solve the problem.

The code that you have will not result in holding onto a reference to any variables that the anonymous handler closes over once that handler is fired and the handler variable leaves scope or is set to something else (i.e. null), even if the A instance is kept alive.

So in short, it's fine.

Note that this is only ever a problem in the first place if the anonymous even handler closes over a variable that is a) expensive to keep alive, i.e. a very large collection of data and b) has a much shorter lifetime than the object that has the event being subscribed to.  If the object holding the event goes out of scope before, or very soon after, whatever is being closed over, then the object is not held in memory any longer than if the event handler is unsubscribed.  That specific case isn't actually all that common.


Beginner implementation of Merge Sort This is my first time implementing a merge sort (just getting started with algorithms & data structures).

Are there any obvious things I have done wrong?

I am also worried that I have overused recursion, as both the merge & the sort function are recursive.

function sort(array) {
  var len = array.length;
  var middle = Math.floor(len*0.5);
  var left = array.slice(0,middle);
  var right = array.slice(middle, len);

  if (len == 1) {
    return array;
  } else {

  }

  return merge(sort(left), sort(right));
}

function merge(left, right) {
  var a = left.length;
  var b = right.length;

  if (a > 0 && b > 0) {
    if (left[0] > right[0]) {
      return [].concat(left[0], merge(left.slice(1,a), right));
    } else {
      return [].concat(right[0], merge(right.slice(1,b), left));
    }
  } else if (a == 0) {
    return right;
  } else of (b == 0)
    return left;

}

var array = [];
for(var i = 0; i < 20; i++) {
   array.push(Math.round(Math.random() * 100));
}
console.log(array);
console.log(sort(array));

From the feedback in this question, I have improved my solution, which can be found here.

Using recursion in sort() is fine: n recursive calls to sort() will let you handle an array of length 2n, so you don't have to worry about too much recursion.  However, it would be better if you wrote the recursion in the standard pattern: base case first, then the recursive case otherwise.  The base case should include empty arrays.

Array.slice(start) gives you a slice from start to the end of the array.

function sort(array) {
    if (array.length <= 1) {
        return array;
    }
    var middle = Math.floor(array.length / 2);
    var left = array.slice(0, middle);
    var right = array.slice(middle);
    return merge(sort(left), sort(right));
}

On the other hand, merge() should not be written recursively.  To obtain a merged array of length n, you'll have to make n recursive calls to merge(), and that could easily result in a stack overflow for a long list.  Furthermore, each slice involves copying nearly the entire array, so the algorithm is grossly inefficient.  You would be much better off writing merge() using a while or for loop.

One of the advantages of mergesort over other sorting algorithms is that it can be a stable sort — that is, if the input contains two elements, neither of which is larger than the other, then they can appear in the same relative order in the output.  For numbers, stability doesn't matter: one 42 is indistinguishable from another 42.  However, it would be good practice to write your merge() function so that it is stable anyway.  For that to happen, if you encounter a tie between left[l] and right[r], you must prefer to take left[l] first, so you should change if (left[0] > right[0]) to if (left[0] >= right[0]), and you must keep your left and right straight instead of swapping them carelessly when merge() calls itself.


Optimize this code of adapter's overriden method getItemCount() How can I optimize this code of adapter's overriden method getItemCount()  into simpler form?

@Override
    public int getItemCount() {
        return super.getItemCount() == 0 && data != null ? 1 : super.getItemCount();
    }

Code Beautification is suggestive. This is how I would write your code. There is also a possible performance optimization by only calling super.getItemCount() once.

be consistent with parentheses (I'm using a different rule for methods and conditions, but feel free to use your own preference as long as you are consistent)
create one variable to store the item count
get the item count you would like to return from the base class
determine predicate from the perspective of either

(A) the edge case value of item count, when itemCount == 0; an inner condition is used to determine the new value
(B) the combined condition that yields a different result itemCount == 0 && data != null

snippet A

@Override
    public int getItemCount() {
        int itemCount = super.getItemCount();
        if (itemCount == 0)
        {
            itemCount = data != null ? 1 : 0;
        }
        return itemCount;
    }

snippet B

@Override
    public int getItemCount() {
        int itemCount = super.getItemCount();
        if (itemCount == 0 && data != null)
        {
            itemCount = 1;
        }
        return itemCount;
    }


Directory Filter Class PHP I'd like feedback on the class below, from the perspective of wanting to write professional code that I would be paid to create. I would appreciate comments on details such as commenting, naming and layout as well as the actual functionality.

The code is a class which takes two arguments (directory and extension) and searches the directory for instances of files with the given extension.

The context is of me practicing writing PHP with a view to getting a job doing so. There is the possibility of "unknown unknowns" implied in this question.

<?php
/**
 * Filters a directory by file-type
 */
class DirectoryFilter{

  /**
   * Returns a list of files in a given directory,
   * filtered by the extension of the files
     *
   * @param string $dirname
   * @param string $extension
   *
   * @return array
   */
    public function filter(string $dirname, string $extension){

        // store results in an array
        $results = [];

        // set path
        $path = __dir__ . "/$dirname";

        // check directory exists
        if(is_dir(($path))){

            foreach(glob("$path/*.*") as $filename) {
                if(pathinfo($filename, PATHINFO_EXTENSION) == $extension){
                    array_push($results, $filename);
                }
            }

            return $results;
        }

        // Directory not found
        return false;
    }
}

$test = new DirectoryFilter();
print_r ($test->filter('testdir', 'php'));

I would probably refactor these things:

Return Type Declaration

You're using argument type declaration, which is a good thing. You can even take it one step further and add a return type declaration as well:

: array

This will force the function to return an array. So the final definition will look like this:

public function filter(string $path, string $extension) : array {

Return Values

I would return an array in any case. If no matches where found this array would simply be empty:

return $results;

Check-ups

I would test a few things:

Is the directory available?
Is the directory readable?
Is the extension empty?

If any of these test fail, throw an Exception with a message describing the error. So, other parts have to worry about the validity of the arguments.

An example test for readability of an existing directory:

if (!is_readable($path)) {
    $message = 'The directory "' . $path . '" is not readble.';
    throw new \Exception($message);
}

Case Sensitivity

Currently your code will fail if you look for PDF but the file is called example.pdf. Use strtolower to get rid of this problem:

$extension = strtolower($extension);
$pathinfoExtension = strtolower(pathinfo($fileInfo, PATHINFO_EXTENSION));

FilesystemIterator

Currently you're using glob to iterate over the files. Alternatively use a FilesystemIterator. It has the advantages1:

increases readability
it skips dot files .|.. (as opposed to an DirectoryIterator)
it's extensible

Result

The final result would look like this:2

class DirectoryFilter {
    public function filter(string $path, string $extension) : ?array {
        $results = [];

        if (!is_dir($path)) {
            $message = 'The directory "' . $path . '" doesn\'t exist.';
            throw new \Exception($message);
        }

        if (!is_readable($path)) {
            $message = 'The directory "' . $path . '" is not readble.';
            throw new \Exception($message);
        }

        if (!$extension) {
            $message = 'An empty string is not a valid extension.';
            throw new \Exception($message);
        }

        $extension = strtolower($extension);

        $iterator = new FilesystemIterator($path);

        foreach ($iterator as $fileInfo) {
            $pathinfoExtension = strtolower(pathinfo($fileInfo, PATHINFO_EXTENSION));

            if ($extension == $pathinfoExtension) {
                $results[] = $fileInfo->getFilename();
            }
        }

        return $results;
    }
}

var_dump( (new DirectoryFilter())->filter(__DIR__, 'php') );

Further Thoughts

To reduce the amount of false-positives, like files that have a wrong extension, you should probably use some sort of MIME-type guesser. A good one, based on the PECL-extension, is the one included in the Symfony framework. The guesser uses two components: FileBinaryMimeTypeGuesser and FileinfoMimeTypeGuesser. The Symfony source code is a good starting point to go more into detail:

http-foundation / File / MimeType / MimeTypeGuesser.php

1 You can find more details in the answers of this question "What exactly are the benefits of using a PHP 5 DirectoryIterator over PHP 4 “opendir/readdir/closedir”?".

2 I removed all comments/PHPDoc to keep the answer compact.


Removing extra spaces without using a regular expression I want to remove the extra spaces without using a regular expression. I'm using two while loops.

How can this become better / more elegant?

String str = "Hola  caracola  ";

char[] src = str.toCharArray(), dest = new char[src.length];
int i = 0, j = 0;
while (i < src.length - 1) {
    while (i < src.length - 1 && src[i] == ' ' && src[i + 1] == ' ') {
        i++;
    }
    dest[j++] = src[i++];
}

System.out.println(new String(dest, 0, j));

Your code is buggy: it fails to copy the last character.

As @Mat'sMug says, defining a function would be a good idea.

The task can be done with one array and one loop.

public static String compressConsecutiveSpaces(String str) {
    if (str.isEmpty()) return str;
    char[] chars = str.toCharArray();
    int dest = 1;
    for (int src = 1; src < chars.length; src++) {
        if (!(chars[src - 1] == ' ' && chars[src] == ' ')) {
            chars[dest++] = chars[src];
        }
    }
    return new String(chars, 0, dest);
}


Function to get daily metrics by fetching a record or evaluating a formula I have react-redux application that allows to track different metrics on daily basis. Each metric can either have Records or store math function to calculate values. State stores normalized Metrics and Records.

This function returns either a calculated value or stored value for certain date.

export const getMetricRecordForDate = (state, metric_id, date) => {
  const resultMetric = state.metrics[metric_id]
  let result = {decimal_value: 0, date: date}
  if (resultMetric.formula) { //formula format: '_idOfMetric_*_idOfAnotherMetric_'
    let args = {}

    const metrics = resultMetric.formula.scan(/_(.*?)_/g) //Array: [..,'_idOfMetric_']
    const l = metrics.length
    for (var i=0; i<l; i++) {
      const key = "_"+metrics[i]+"_"
      args[key] = getMetricRecordForDate(state, state.metrics[metrics[i]].id, date).decimal_value

    }

    let decimal_value
    try { // statements to try args format: {_idOfMetric_: value}
      decimal_value = math.eval(resultMetric.formula, args)
    }
    catch (e) {
      decimal_value = 0;
    }

    result.decimal_value = decimal_value
  } else {
    const records = getMetricRecords(metric_id, state)
    let metricRecord
    if (records.length > 0) {
      for (var i=0; i<records.length; i++) {
        if (moment(records[i].date).isSame(date, 'day')) {
          metricRecord = records[i]
          break
        }
      }
    }
    if (metricRecord) {
      result = metricRecord
    }
  }
  return result

}

The structure of state:

{state:
  {metrics: {1: {id: 1, name: 'name', records: [1, 2]} },
  {records: {1: {id: 1, decimal_value: 0, date: '01-01-2017', metric_id: 1},
            {2: {id: 1, decimal_value: 0, date: '02-01-2017', metric_id: 1}
  }
}

So, right now this function is very slow especially for recursive calls. I was wondering if it could be optimized.

Here is the getMetricRecords function:

export const getMetricRecords = (metric_id, state) => {
  return state.metrics[metric_id].records.map( id => state.records[id])
}

I would appreciate any help (I am new to Javascript).

Help you to figure out how to optimize this function is not easy from outside.

Optimization is hard staff and start by collecting numbers.

If you think your function is slow, you should first measure the amount of time that your function run, the size of data that processes, the memory involved and the CPU cycles used.

You could do this with a profiler included in all the modern browsers.

With this numbers you could understand if your function is really bad as you think it is or maybe not.

You said that it is slow, but how about the staff that have to do. In a part you speak about arrays of arrays.

Now if the function is just handling too much its helpful as told us we have to change our approach as the way we though to do is not fit the reality. So no need on wasting time to optimize a function rather find a way to reduce the problem in a manner that is faster to compute.

If the function is very bad, instead, according with our measurements, than it's time to profile inside and start to look where we consume all that time.

Keep the worst part and try to understand what is the problem and fix this. An then move on to the next less worse.

UPDATE

As was clarified later, the recursion is connected with complex formulas and not with stored values.

A part that is still not clear to me:

const resultMetric = state.metrics[metric_id]
...
const records = getMetricRecords(metric_id, state)

In getMetricRecords() you just look for resultMetric again. As it is an access by key is not an big issue, but for sake of code clarity, if it is the same thing you could consider to move the code of getMetricRecords(). It is not so big staff.

const records = resultMetric.records.map(id => state.records[id])

the following part could even be simplified:

let metricRecord
if (records.length > 0) {
  for (var i=0; i<records.length; i++) {
    if (moment(records[i].date).isSame(date, 'day')) {
      metricRecord = records[i]
      break
    }
  }
}
if (metricRecord) {
  result = metricRecord
}

Avoiding to check the length of the records as the for loop already do, and remove the variable metricRecord:

for (var i=0; i<records.length; i++) {
    if (moment(records[i].date).isSame(date, 'day')) {
      result = records[i]
      break
    }
}

Of course I don't think this will solve your performance issue, sorry. :(
I just like to remove not-needed code.

About records if is a huge list, and memory is not an issue, you could consider to have an object/map with date as the key so you could avoid the for loop.


Emulating C++ string input in C I did the following exercise:

  Write a C program that does the equivalent of C++ string s; cin>>s;; that is, define an input operation that reads an arbitrarily long sequence of whitespace-terminated characters into a zero terminated array of chars.

I wonder if it's good code. What could be improved?

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

struct String {
    char* signs;
    size_t size;
    size_t capacity;
};

void String_allocate_space(char **c, size_t *capacity)
{
    if (*capacity == 0) {   // allocate the first time
        *capacity = 1;
        *c = malloc(sizeof(**c) * ((*capacity)));
    }
    else {
        *capacity *= 2;     // double the new capacity
        *c = realloc(*c, sizeof(**c) * (*capacity));
    }
    if (*c == NULL)
        exit(-1);
}

void add_character(struct String* string, int ch)
{
    if (string->size == string->capacity) {     // if current letter sz = capacity
        String_allocate_space(&string->signs, &string->capacity);
    }
    string->signs[string->size++] = ch;    // append the sign in the array
}

void String_read(struct String* string)
{
    int ch = ' ';
    while (ch = getc(stdin)) {

        if (!isalpha(ch))
            break;

        add_character(string, ch);
    }
    add_character(string, '\0');
}

void String_print(struct String* string)
{
    printf("%s", string->signs);
}

void String_free(struct String* string)
{
    int i = 0;
    for (i = 0; i < string->capacity; ++i) {
        free(string[i].signs);
        string[i].signs = NULL;
    }
}

int main()
{
    struct String string = { 0 };

    String_read(&string);
    String_print(&string);
    String_free(&string);

    getchar();
    return 0;
}

Don't do this:
*c = realloc(*c, sizeof(**c) * (*capacity));

Once you have error handling that's more sophisticated than exit(1), this will become a liability.  You need a temporary:
char *tmp = realloc(*c, new_capacity);
if (!tmp) {
    /* error handling - c is still valid */
    /* ... */
}
*c = tmp;
*capacity = new_capacity;

If you always initialize the data pointer to start as a null pointer, you don't need to use malloc() instead of realloc().  String_allocate_space would be easier to write if it accepts a pointer to a struct String; that makes it closer to the object-oriented version:
void String_init(struct String* string)
{
    string->data = NULL;
    string->size = string->capacity = 0;
}

void String_allocate_space(struct String* string)
{
    size_t new_capacity = string->capacity ? 2 * string->capacity : 16;
    char *tmp = realloc(string->data, new_capacity);
    if (!tmp) {
        /* error handling - c is still valid */
        exit(1);                /* TODO: improve error reporting */
    }
    string->data = tmp;
    string->capacity = new_capacity;
}

I've also incorporated a change above to start with a larger initial size (16) instead of 1.  That lets us skip the first 4 reallocations for free.

The read() method has a flaw that will become apparent when you try to read another value into a string - unlike std::string, reading with >> will append to it, instead of replacing it.  We need to reset size at the beginning:
void String_read(struct String* string)
{
    string->size = 0;
    int ch;
    while (ch = getc(stdin)) {

        if (!isalpha(ch))
            break;

        add_character(string, ch);
    }
    add_character(string, '\0');
}

Also, the logic is slightly wrong - we want to finish when we see a space, rather than any non-alpha (which could be digits or punctuation characters).  (Well done for remembering that getch() returns int rather than char - that's one common error avoided).
void String_read(struct String* string)
{
    string->size = 0;
    int ch;
    while ((ch = getc(stdin)) != EOF && !isspace(ch)) {
        add_character(string, (char)ch);
    }
    add_character(string, '\0');
}

When we free the string, we don't need a loop.  Instead, we have a single free().  It's a good idea to reset the size and capacity so that the string object is consistent - it can be used again and/or freed again without harm:
void String_free(struct String* string)
{
    free(string->data);
    string->data = NULL;
    string->size = string->capacity = 0;
}

This is an important concept in object-oriented programming - objects have invariants that they guarantee are true at the start and end of every (public) operation.  In this case, the invariants are that

data points to valid storage of at least capacity if capacity > 0 and is a null pointer otherwise.
size is not greater than capacity.

We can improve the printing so that it outputs any embedded NUL characters, just like C++ strings do:
void String_print(struct String* string)
{
    if (fwrite(string->data, 1, string->size, stdout) != string->size) {
        exit(1);                /* TODO: improve error reporting */
    }
}

Note that this will now print the trailing NUL we added.  We no longer need that to mark the end of string, so we can remove that line.

Modified code
#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>

struct String {
    char* data;
    size_t size;
    size_t capacity;
};

void String_init(struct String* string)
{
    string->data = NULL;
    string->size = string->capacity = 0;
}

void String_allocate_space(struct String* string)
{
    size_t new_capacity = string->capacity ? 2 * string->capacity : 1;
    char *tmp = realloc(string->data, new_capacity);
    if (!tmp) {
        /* error handling - c is still valid */
        exit(1);                /* TODO: improve error reporting */
    }
    string->data = tmp;
    string->capacity = new_capacity;
}

void add_character(struct String* string, char ch)
{
    if (string->size == string->capacity) {     // if current letter exceeds capacity
        String_allocate_space(string);
    }
    string->data[string->size++] = ch;    // append it
}

void String_read(struct String* string)
{
    string->size = 0;
    int ch;
    while ((ch = getc(stdin)) != EOF && !isspace(ch)) {
        add_character(string, (char)ch);
    }
}

void String_print(struct String *restrict string, FILE *restrict stream)
{
    if (fwrite(string->data, 1, string->size, stream) != string->size) {
        exit(1);                /* TODO: improve error reporting */
    }
}

void String_free(struct String* string)
{
    free(string->data);
    string->data = NULL;
    string->size = string->capacity = 0;
}

int main()
{
    struct String string;

    String_init(&string);
    String_read(&string);
    String_print(&string, stdout);
    String_free(&string);
}


Python login system for a dice game This is a finished login system where 2 players have to enter their username and password to proceed onto the game. The usernames and passwords are stored in a text file in this formatusername: (username) password: (password). If one of the players enter their details in wrong it will loop it 3 times and if it is sill not correct they have to restart the code to try again.

import sys

counter = 0
check_failed = True
while check_failed:
    print("Could player 1 enter their username and password")
    username1=input("Please enter your username ")
    password=input("Please enter your password ")
    with open("accountfile.txt","r") as username_finder:
        for line in username_finder:
            if ("username: " + username1 + " password: " + password) == line.strip():
                print("you are logged in")
                check_failed = False
                check_failed = True
                while check_failed:
                    print("Could player 2 enter their username and password")
                    username2=input("Please enter your username ")
                    password=input("Please enter your password ")
                    with open("accountfile.txt","r") as username_finder:
                        for line in username_finder:
                            if ("username: " + username2 + " password: " + password) == line.strip():
                                print("you are logged in")
                                check_failed = False

                        else:
                            print("Sorry, this username or password does not exist please try again")
                            counter = counter + 1
                            if counter == 3:
                                print("----------------------------------------------------")
                                print("You have been locked out please restart to try again")
                                sys.exit()
        else:
            print("Sorry, this username or password does not exist please try again")
            counter = counter + 1
            if counter == 3:
                print("----------------------------------------------------")
                print("You have been locked out please restart to try again")
                sys.exit()

As I said this is a finished Login system and I just want suggestions on how I can improve this Login system.

One guiding principle in software development is Don't Repeat Yourself (DRY). Your code for player 1 and 2 are almost identical, so you should extract them into a login function:

import crypt

class AuthenticationError(Exception):
    pass

def encrypt(word, salt):
    # use some one-way hashing function here
    return crypt.crypt(word, salt)

def login(users, max_tries=3):
    for i in range(max_tries):
        username = input("Please enter your username ")
        password = input("Please enter your password ")
        if username in users and encrypt(password, username) == users[username]:
            print("You are logged in as", username)
            return username
        else:
            print(f"Login failed. You have {max_tries - i - 1} tries left.")
    raise AuthenticationError(f"You have been locked out please restart to try again.")

This uses a hashing function, like @ChadG suggested in their answer, with a salt to prevent rainbow table attacks.

By raising an Exception (here a custom one, to give it a nice name), the execution of the program is automatically halted, so no need to explicitly do sys.exit.

This code assumes that you have read the users file before into a dictionary (only need to do it once!), something like this:

import re

def get_users(file_name):
    line_fmt = re.compile(r'username: (.*) password: (.*)')
    with open(file_name) as f:
        return dict(line_fmt.match(line.strip()).groups() for line in f)

Or, this slightly expanded version which ensures that the entries not conforming to the format are skipped (including any trailing newline):

def get_users(file_name):
    line_fmt = re.compile(r'username: (.*) password: (.*)')
    users = {}
    with open(file_name) as f:
        for line in f:
            try:
                name, password = line_fmt.match(line.strip()).groups()
                users[name] = password
            except AttributeError:
                print("Malformed line:", line)
        return users

Use it like this:

if __name__ == "__main__":
    registered_users = get_users("accountfile.txt")

    print("Could player 1 enter their username and password")
    player1 = login(registered_users)

    print("Could player 2 enter their username and password")
    player2 = login(registered_users)

    # start dice game

This uses a if __name__ == "__main__": guard to allow importing from this script from another script.


Dynamic Treenode Creation I am trying to create a treenode structure based on a database table for a Dungeons and Dragons character creation program I am working on.  I have come up with an incredibly confusing way to do it, but I am wondering if anyone can give me some better ideas on how to do this more efficiently.  I have a tables called subfeats and feats.  Subfeats contains all feats from the table feats that have subfeats.  Really, all I want to do is create a parent/child treenode structure for all feats and their subfeats.  Here is my code:

foreach (DataRow subrow in ds.subfeats.Rows)
{
    subfeatSet.Add(Convert.ToString(subrow[ds.subfeats.Columns.IndexOf("subfeat")]));
}

foreach (DataRow row in ds.feat.Rows)
{
    TreeNode newNode = new  TreeNode(Convert.ToString(row[ds.feat.Columns.IndexOf("name")]));

    if (subfeatSet.Contains(newNode.Text))
    {
        // Do Nothing.
    }
    else
    {
        treeviewFeatsFeats.Nodes.Add(newNode);
        foreach (DataRow subrow in ds.subfeats.Rows)
        {
            if (Convert.ToString(subrow[ds.subfeats.Columns.IndexOf("feat")]).Equals(newNode.Text))
            {
                TreeNode subTreeNode = new TreeNode(Convert.ToString(subrow[ds.subfeats.Columns.IndexOf("subfeat")]));
                newNode.Nodes.Add(subTreeNode);

                foreach (DataRow subsubrow in ds.subfeats.Rows)
                {
                    if (Convert.ToString(subsubrow[ds.subfeats.Columns.IndexOf("feat")]).Equals(subTreeNode.Text))
                    {
                        TreeNode subsubTreeNode = new TreeNode(Convert.ToString(subsubrow[ds.subfeats.Columns.IndexOf("subfeat")]));
                                subTreeNode.Nodes.Add(subsubTreeNode);
                    }
                }
            }
        }
    }
}

I like the use of white space and indentation, makes the code easy to read.

I would start using var for your variable declarations.  It keeps the code cleaner, and easier for your eyes to scan.

Remove the // Do Nothing if statements.  It is redundant and adds extra lines of code that are unneeded.  This should work:

if (!subfeatSet.Contains(newNode.Text))
{
    treeviewFeatsFeats.Nodes.Add(newNode);
    foreach (DataRow subrow in ds.subfeats.Rows)
    {

    ...
}

I would also learn about linq, you can clean up most of your loops with linq.  I would also look into moving some of the inner loops out into their own function.  Moving to a function would remove some duplication you have in your code too.

private void ProcessNode(TreeNode parent, DataSet ds)
{
    var index = ds.subfeats.Columns.IndexOf("feat");
    foreach (var treeNode in (
            from row in ds.subfeats.Rows
            let nodeName = Convert.ToString(row[index])
            where rows.Contains(nodeName)
            select new TreeNode(nodeName)))
    {
        ProcessNode(treeNode, rows);
        parent.Add(treeNode);
    }
}

Now your function looks like this:

var index = ds.feat.Columns.IndexOf("name");
foreach (var treeNode in (
            from row in ds.feat.Rows,
            let nodeName = Convert.ToString(row[index])
            where rows.Contains(nodeName)
            select new TreeNode(nodeName)))
{
   ProcessNode(treeNode, ds);
}

This is a start.  I'm not happy with having to pass DataSet into the functions, but without fully understanding the data structure, I can't think of another way of doing it.


Check if an array value exists as part of the url I have the following bit of code in a service worker that checks to see it the url contains any of the values in an array and if so it breaks out of the function.  As it is run for every asset on a webpage, it can be run a lot of times so I was wondering if it could be optimised in any way:

const urlsToIgnore = [
  'boldchat.com',
  'pinimg.com',
  'responsetap.com',
  'facebook.net',
  'dwin1.com',
  'bing.com',
  'google-analytics.com',
  'googletagmanager.com',
  'hotjar.com',
  'pinterest.com',
];

self.addEventListener('fetch', event => {
  // don't load assets from these urls
  for (let i = 0; i < urlsToIgnore.length; i++) {
    if (event.request.url.indexOf(urlsToIgnore[i]) > -1) {
      return;
    }
  }

  // .... rest of code to cache the assets and page
});

If you can use ES6 Features in your code, use a Set for constant lookup time:
const urlsToIgnore = new Set([
  'boldchat.com',
  'pinimg.com',
  'responsetap.com',
  'facebook.net',
  'dwin1.com',
  'bing.com',
  'google-analytics.com',
  'googletagmanager.com',
  'hotjar.com',
  'pinterest.com',
]);

// later
if (uslstoIgnore.has(event.request.url) { ... }

Otherwise, just use a simple object as a map:
const urlsToIgnore = {
  'boldchat.com': 1,
  'pinimg.com': 1,
  'responsetap.com': 1,
  'facebook.net': 1,
  'dwin1.com': 1,
  'bing.com': 1,
  'google-analytics.com': 1,
  'googletagmanager.com': 1,
  'hotjar.com': 1,
  'pinterest.com': 1,
};

// later
if (uslstoIgnore[event.request.url]) { ... }

Both these options offer constant time lookup, as opposed to the linear time of your current solution.
The actual performance gain depends on the size of the list with ignored items, but since this list can be added to, I'd opt for an O(1) lookup.


Path Following Algorithm for Entity Within my game there are many entities, where each entity has a path (Array<Vector2>) assigned to it. No path to follow (indicating that the entity should move at all) is indicated by an empty path (the path is never null). updateMoveAttributes is called every frame for every entity, so it is important that it is efficient.

Note that my entity's direction is in radians, ranging from - Math.PI to Math.PI, since this is what Math.atan2 returns.

The below snippet contains updateMoveAttributes and its supporting methods.

private final Array<Vector2> path = new Array<>();
private int pathIndex = 0;

private static final float EPSILON = 1e-3f;

private float getXDifference(Vector2 pathComponent) {
    float xDifference = pathComponent.x - getSprite().getX();

    xDifference = Math.abs(xDifference) < EPSILON ? 0 : xDifference;

    return xDifference;
}

private float getYDifference(Vector2 pathComponent) {
    float yDifference = pathComponent.y - getSprite().getY();

    yDifference = Math.abs(yDifference) < EPSILON ? 0 : yDifference;

    return yDifference;
}

public float orient(Vector2 pathComponent) {
    float xDiff = getXDifference(pathComponent);
    float yDiff = getYDifference(pathComponent);

    return (float) Math.atan2(yDiff, xDiff);
}

void updateMoveAttributes() {
    // No path assigned
    if (getPath().size == 0)
        return;

    // No more path to travel
    if (getPathIndex() == getPath().size) {
        setSpeed(0);

        return;
    }

    Vector2 nextPathComponent = getPath().get(getPathIndex());
    float angleToComponent = orient(nextPathComponent);

    setDirection(angleToComponent);

    float xDiff = getXDifference(nextPathComponent);
    float yDiff = getYDifference(nextPathComponent);

    boolean angleInPositiveXQuadrants = (-Math.PI / 2 <= angleToComponent && angleToComponent <= Math.PI / 2);
    boolean pastX = angleInPositiveXQuadrants ? (xDiff <= 0) : (xDiff >= 0);

    boolean angleInPositiveYQuadrants = (0 <= angleToComponent) && (angleToComponent <= Math.PI);
    boolean pastY = angleInPositiveYQuadrants ? (yDiff <= 0) : (yDiff >= 0);

    // Indicates that we have passed the path component we are on route to, adjust course with respect
    // to next available path component.
    if (pastY && pastX)
        setPathIndex(getPathIndex() + 1);

    // In case user upgrades entity during travel of entity -> movement speed changes -> update required for actual speed
    setSpeed(getMovementSpeed());
}

public Array<Vector2> getPath() {
    return path;
}

Method getXDifference and getYDifference are very similar, you can convert this into a single method that accepts two variables.
you could also inline the return statement as the method name is sufficient to tell what it is returning.
Instead of calculating pastX and pastY, you can write methods to return the computed value.

boolean isPastX(variables...){
    // logic to calculate pastX and return it.
}

and the if statement will turn into if( isPastX() && isPastY()).
This will also help in short-circuiting the condition in case calculating pastX or pastY is very expensive(it isn't in this example, just a suggestion).
Curly braces {} should always be used after conditional statements or loops, even it's just a single line. It may avoid confusion in some scenarios.

if (getPath().size == 0){
    return;
}


Vector-to-direction method in Java I don't really have a problem, but my code bothers me, because I know it could be done in a much better way.

I have an enum of 4 directions: north west, north east, south east and south west. And I have a method that converts a vector to one of those directions. The method prefers North and East, so values of 0 would return those.

public enum Direction {
    NORTH_WEST, NORTH_EAST, SOUTH_EAST, SOUTH_WEST;

    public static Direction vectorToDirection(Vector2 vector) {
        if (vector.y >= 0) {
            if (vector.x >= 0) return NORTH_EAST;
            else return NORTH_WEST;
        } else {
            if (vector.x >= 0) return SOUTH_EAST;
            else return SOUTH_WEST;
        }
    }
}

If anyone can figure out a cleaner or more efficient method, I'd love to know!

Use Braces:
Consider using braces for more readable code:

public static Direction vectorToDirection(Vector2 vector) {
    if (vector.y >= 0) {
        if (vector.x >= 0) return NORTH_EAST;
        else return NORTH_WEST;
    } else {
        if (vector.x >= 0) return SOUTH_EAST;
        else return SOUTH_WEST;
    }
}

Becomes (with eclipse Ctrl + Shift + F):
public static Direction vectorToDirection (Vector2 vector) {
     if (vector.y >= 0) {
         if (vector.x >= 0) {
             return NORTH_EAST;
         } else {
             return NORTH_WEST;
         }
      } else {
         if (vector.x >= 0) {
             return SOUTH_EAST;
         } else {
             return SOUTH_WEST;
         }
     }
}

And suddenly it becomes extremely obvious and readable what happens here.
Naming:
Additionally I'd consider naming your method fromVector() instead. Compare:
Direction.vectorToDirection(someVector);

against
Direction.fromVector(someVector);

Documentation:
When I see the implementation I know, but how would anyone outside know you prefer east > west and north > south? Write some JavaDoc. Example:
/**
* @param vector the 2-dimensional vector to be converted to a direction
* @returns The Direction. hereby North is preferred over South and
* East is preferred over West.
* Example: Direction.fromVector(new Vector2(0, 0)); will return NORTH_EAST
* Direction.fromVector(new Vector2(-1, 0)); will return SOUTH_EAST
*/
public static Direction fromVector(Vector2 vector) {
// ...


Separate business logic from Data Access Logic in the repository I have a PupilService which is calling the PupilRepository.AttachPupil() method.

I have an N to M relation between SchoolclassCode and Pupil.

Technical logic:

A pupil can be related to many SchoolclassCode like "Math 7 a" or "English 8 c".

Business logic:

A pupil can be related only to SchoolclassCode like "Math 7 a" or "English 7 c". That means it's not possible for a pupil to belong to 2 different class numbers.
A pupil can only be attached to a smaller class number during the year (changes happen in mid-year) thus all higher class relations must be removed.

The business logic from 1) and 2) should be put elsewhere. It just does not fit into the PupilService or another Repository method because by splitting up the tasks in finer grained method It would causes more queries to do the same job. Further more if I split up the business logic the repo.AttachPupil would only work the half if the repository would be called directly. I do not like that either.

public async Task AttachPupil(int pupilId, int targetSchoolclassCodeId)
{
    var pupil = new Pupil { Id = pupilId };
    context.Pupils.Attach(pupil);
    var targetSchoolclassCode = await context.SchoolclassCodes.SingleAsync(s => s.Id == targetSchoolclassCodeId);
    await context.Entry(pupil).Collection(p => p.SchoolclassCodes).LoadAsync();
    var existingLowerSchoolclasses = pupil.SchoolclassCodes.Where( s => s.SchoolclassNumber < targetSchoolclassCode.SchoolclassNumber);
    if (existingLowerSchoolclasses.Any())
    {
        throw new PupilAdvancedNextSchoolclass("A pupil can only downgrade a class within a schoolyear");
    }
    else
    {
        foreach (var schoolclassCodeToDetach in existingLowerSchoolclasses)
        {
            // Detach pupil from higher SchoolclassCode classes e.g. class 9
            pupil.SchoolclassCodes.Remove(schoolclassCodeToDetach);
        }

        // Attach Pupil to a lower class becaue his grades/marks are too bad e.g. in the mid-year to class 8
        targetSchoolclassCode.Pupils.Add(pupil);
        await context.SaveChangesAsync();
    }
}

How would you refactor this AttachPupil method so that the business logic is better separated from the data access logic?

First of all I'd say that your foreach loop will never loop, because it is only reached when there are no elements in existingLowerSchoolclasses.

In general I would recommend to re-think your datastructure, especially with a view towards next year and the year after. Because then you'll wish you had identified classes like "English 8 c 2014" or "Math 7 d 2015".

This will make it easier to enforce the constraints you described.

If you're concerned about the number of queries you could use a stored transaction. Even that feels old-fashioned, in some cases it still makes sense to guard the access to N:M relations with those things. The question is always: Is it really a domain problem, or is it caused by the fact that your data structure doesn't adhere enough to the domain? If you can answer that, you'll know where to put the Data Access Logic.

Last of all let me be honest: Whenever I see a N:M relation I get very suspicious about it. In most real-world situations, there are no anonymous N:M relations. A student started a class at a certain time and left it at another, also different students could have different mentors within one course, and there might be much more attributes you might want to assign to that relationship in the future. Therefore I would advise you to remodel this relation, e.g. by introducing a ClassMembership entity.


Print lyrics of 99 Bottles of Beer I read 99 Bottles of OOP, and one of the offhand comments was that doing the 99 bottles problem with composition was another route that one could take (the book used inheritance).  Here is my attempt.  Here are the lyrics needed: Beer Song

Some difficulties I had:

1) Implementation of successor was tricky.  I could not simply pass in a successor object because then BottleNumber(99) needed to hold BottleNumber(98) which needed to hold... Instead I used successor_number and generated a successor when needed.

2) Factory seemed messy - the arguments for the initialize method stacked up and up.  Named arguments only made things longer. Sometimes I had to implement a default_object and other times I could use default named parameters.  Should this be standardized throughout?

Comments welcome

class BeerSong

  def verse(number)
    bottle_number = BottleNumber.for(number)
    "#{bottle_number} of beer on the wall, #{bottle_number} of beer.\n".capitalize +
    "#{bottle_number.action}, #{bottle_number.successor} of beer on the wall.\n"
  end

  def verses(starting,ending)
    starting.downto(ending).map do |number|
      verse(number)
    end.join("\n")
  end

  def song
    verses(99,0)
  end
end

class BottleNumber
  attr_reader :number, :container, :pronoun, :quantity, :action, :successor_number

  class << self
    def for(number)
      return number if number.is_a? BottleNumber
      case number
      when 0
        BottleNumber.new(number, quantity: 'no more', successor_number: 99, action: 'Go to the store and buy some more')
      when 1
        BottleNumber.new(number, container: 'bottle', pronoun: 'it')
      else
        BottleNumber.new(number)
      end
    end
  end

  def initialize(number, container: 'bottles', pronoun: 'one', quantity: nil, action: nil, successor_number: nil)
    @number = number
    @container = container
    @pronoun = pronoun
    @quantity = quantity || default_quanity
    @action = action || default_action
    @successor_number = successor_number || default_successor_number
  end

  def to_s
    "#{quantity} #{container}"
  end

  def default_successor_number
    number - 1
  end

  def default_quanity
    number.to_s
  end

  def default_action
    "Take #{pronoun} down and pass it around"
  end

  def successor
    BottleNumber.for(successor_number)
  end
end

Some comments when looking through your code:

Successor

I think that's fine - how else were you going to do it? Metz does exactly the same thing doesn't she? When successor is called a new bottle number - 1 is created - unless of course the bottle number is zero, in which case you start right back at 100.

Knowledge of the arguments and their order:

Consider this:

BottleNumber.new(number, quantity: 'no more', successor_number: 99, action: 'Go to the store and buy some more')

I don't like this. Why? Because every time you need to instantiante a bottle you need to KNOW what goes in there and you also need to know the order in which the arguments go in. You could eliminate the need to know the argument order by passing in a hash. That's probably the only bit of criticism i can add.

Inheritance

for this particular problem, inheritance seems like a better fit. it just seems a lot cleaner than dealing with the messiness of passing in those parameters.

anyways those are just my thoughts and i hope you find them of some use.


Starting MD5 hash like Pi I found a problem on here that ask to find an MD5 hash whose hexdigits starts like decimal digits of Pi. Is there a faster program or method to solve this than my program? This uses brute force search. The site says that a has with 12 first digits like Pi exists but I was unable to find 10 correct digits from the beginning. How can one optimize this program?

# -*- coding: utf-8 -*-
import calendar
import time
import hashlib

def next_string(s):
    strip_zs = s.rstrip('z')
    if strip_zs:
        return strip_zs[:-1] + chr(ord(strip_zs[-1]) + 1) + 'a' * (len(s) - len(strip_zs))
    else:
        return 'a' * (len(s) + 1)

starttime = calendar.timegm(time.gmtime())
fo = open("testfile.txt", "r+")
a = fo.readline()
print("Putkaposti Piinkova salasana thus 39b.")
print("Find a MD5-hash whose hexrepresentation starts like the decimal representation of pi.")
print("Let us continue from the string "+a+".")
print("Ctrl+C quits the program and saves the results to the file result.txt.")
try:
    while True:
# Let us save the results every half an hour.
     now = calendar.timegm(time.gmtime())
     if now - starttime > 30*60:
        f = open("testfile.txt","w")
        f.write(a)
        f.close()
        starttime = now

     if str(hashlib.md5(a.encode('utf-8')).hexdigest())[0:10] == "3141592653":
        f = open("result.txt","w")
        f.write(a)
        f.close()
        print("Found! Print result to the screen and to the file result.txt.")
        print(a)
     a = next_string(a)
except KeyboardInterrupt:
    f = open("testfile.txt","w")
    f.write(a)
    f.close()

When speeding up code, you need to measure the performance of the code on a standard test case, and then try many experiments focussing on the hot spots (the bits of code that are executed most often and take the majority of the time).

So the first thing to do is to refactor the code so that it is easily measurable.

def test1(key='a', target='3141592653'):
    """Return a string whose MD5 hexdigest starts with target. The search
    starts at the string key.

    """
    target_len = len(target)
    starttime = calendar.timegm(time.gmtime())
    while True:
        now = calendar.timegm(time.gmtime())
        if now - starttime > 30*60:
            starttime = now
        if str(hashlib.md5(key.encode('utf-8')).hexdigest())[0:target_len] == target:
            return key
        strip_zs = key.rstrip('z')
        if strip_zs:
            key = strip_zs[:-1] + chr(ord(strip_zs[-1]) + 1) + 'a' * (len(key) - len(strip_zs))
        else:
            key = 'a' * (len(key) + 1)

I've stripped the file-reading and writing code, and the exception handling, because it's not important for testing performance, and I've inlined the code for next_string so that we can improve that along with the rest of the code.

The refactored code is designed to be easy to measure — you can pass different values for target to get different levels of difficulty. Here's my chosen test case:

>>> from timeit import timeit
>>> timeit(lambda:test1(target='000000'), number=1)
10.682760871015489

The first thing we can try is to skip the code for periodic saving — if the KeyboardInterrupt handling is reliable, then there's no need for a second mechanism to save your progress.

def test2(key='a', target='3141592653'):
    """Return a string whose MD5 hexdigest starts with target. The search
    starts at the string key.

    """
    target_len = len(target)
    while True:
        if str(hashlib.md5(key.encode('utf-8')).hexdigest())[0:target_len] == target:
            return key
        strip_zs = key.rstrip('z')
        if strip_zs:
            key = strip_zs[:-1] + chr(ord(strip_zs[-1]) + 1) + 'a' * (len(key) - len(strip_zs))
        else:
            key = 'a' * (len(key) + 1)

This is about twice as fast as test1:

>>> timeit(lambda:test2(target='000000'), number=1)
5.272445386042818

This means that about half the time in test1 was being spent in calendar.timegm and time.gmtime!

The next thing to try is to avoid the encoding and decoding steps. The only characters you use are lower-case "a" to "z", and these all encode to UTF-8 as single bytes. So we could maintain a bytearray instead of a string.

def test3(key='a', target='3141592653'):
    """Return a string whose MD5 hexdigest starts with target. The search
    starts at the string key.

    """
    key = bytearray(key.encode('ascii'))
    key_indexes = range(len(key) - 1, -1, -1)
    md5 = hashlib.md5
    a, z = b'az'
    while True:
        if md5(key).hexdigest().startswith(target):
            return key.decode('ascii')
        for i in key_indexes:
            key[i] += 1
            if key[i] > z:
                key[i] = a
                if i == 0:
                    key.append(a)
                    key_indexes = range(len(key) - 1, -1, -1)
            else:
                break

Note that I've also cached hashlib.md5 in a local variable to avoid having to look it up on each iteration.

This is about three times as fast as test1:

>>> timeit(lambda:test3(target='000000'), number=1)
3.3292323709465563

Instead of calling the hexdigest method, we could call digest instead:

import binascii

def test4(key='a', target='3141592653'):
    """Return a string whose MD5 hexdigest starts with target. The search
    starts at the string key.

    """
    key = bytearray(key.encode('ascii'))
    key_indexes = range(len(key) - 1, -1, -1)
    target_digest = binascii.unhexlify(target)
    md5 = hashlib.md5
    a, z = b'az'
    while True:
        if md5(key).digest().startswith(target_digest):
            return key.decode('ascii')
        for i in key_indexes:
            key[i] += 1
            if key[i] > z:
                key[i] = a
                if i == 0:
                    key.append(a)
                    key_indexes = range(len(key) - 1, -1, -1)
            else:
                break

(Note that this imposes the limitation that the target has an even number of hexadecimal digits.)

This gives a small further improvement:

>>> timeit(lambda:test4(target='000000'), number=1)
3.105254566995427

That's the limit of piecewise improvements that I can spot, so it doesn't look as though this approach is going to get you anywhere near to 12 digits. We expect the time taken to grow by about 16 times for each additional hexadecimal digit, so the expected runtime of test4(target='314159265358') would be well over a year.


Formatting a list with commas and occasional line breaks I have an array mapped to make a typical result string separated by commas and with numbers a bit beautified with array.map(formatNumber).

Expected format is

number [quantity], number [quantity], number [quantity]

So

1234 [3], 1235 [2], 2234 [1]

My mapping function was like this:

function formatNumberSelected(number, i) {
    return number.number + " [" + number.amount() + "]" + (i % 5 === 4 ? ',
' : ', ');
}

But to avoid comma (or comma and
) in the last iteration now is:

function formatNumberSelected(number, i, array) {
  var toReturn = number.number + " [" + number.amount() + "]";
  if ((i + 1) < array.length) {
    if (i % 5 === 4) toReturn += ',
';
    else toReturn += ', ';
  }
  return toReturn;
}

There is a better way to make this code more readable? Seems like passing array to each iteration just to find the last index is a bit too much.

If not I find better this solution not using Array.prototype.map:

function formatNumberArray(array) {
  var SEPARATOR = '';
  var toReturn = '';
  for (var index in array) {
    toReturn += SEPARATOR + array[index].number + ' [' + array[index].amount() + ']';
    if (index % 5 === 4) SEPARATOR = ',
';
    else SEPARATOR = ', ';
  }
  return toReturn;
}

You have serval ways to achieve this task. You can pass the length of array as well.
About the implementation you can use forEach method like this.

 function formatNumberArray(arr) {
        let output = "";
        arr.forEach((v, i, a) => {
            output += v.number + "[" + v.amount + "]" + ((i + 1) % a.length == 0 ? "" : i % 5 == 4 ? ',
' : ', ');
        });
        return output;
    }

or you can iterate the array to the last index - 1 and then simply add the last value

function formatNumberArray(arr) {
        let output = "";
        for (let i = 0; i < arr.length - 1; ++i) {
            output += arr[i].number + "[" + arr[i].amount + "]" + ((i % 5 === 4) ? ',
' : ', ');
        }
        return output + arr[arr.length - 1].number + "[" + arr[arr.length - 1].amount + "]"
    }

I assume that the array is something like this.

  let arr = [{ number: 123, amount: 24 }, { number: 16, amount: 10 }, { number: 16, amount: 10 }, { number: 16, amount: 10 }, { number: 16, amount: 10 }, { number: 16, amount: 10 }, { number: 16, amount: 10 }, { number: 16, amount: 10 }, { number: 16, amount: 10 }, { number: 16, amount: 10 }, { number: 16, amount: 10 }, { number: 16, amount: 10 }, { number: 16, amount: 10 }]


Ruby on Rails Method ActiveRecord Query Optimizations So I have three methods and I'm not sure how to optimize them. I will be working on this and updating this post while I do but basically running these three methods is central to some parts of my application and they are causing a great deal of slowdown. I just have too many queries being ran because of them. If I could get some optimization help, I would appreciate it!

def self.appropriate_users(current_user)
    unless current_user.organization.nil?
      if current_user.has_role? :director
        @users = User.where(organization: current_user.organization)
      elsif current_user.has_role? :supervisor
        @users = current_user.subordinates
      elsif current_user.has_role? :leader
        @users = []
        groups = current_user.leading_groups.each do |g|
          g.users.each do |u|
            @users << u
          end
        end

        @users.uniq
      else
        @users = User.none
      end
    else
      @users = User.all if current_user.has_role? :admin
    end
    @users ||= []

    @users
  end

  def expired_trainings(current_user)
    retval = []
    Organization.appropriate_users(current_user).each do |user|
      Expiration.where("user_id = #{user.id} and expire_on <= '#{Date.today}'").each do |expir|
        retval << expir
      end
    end
    ttypes = []
    if current_user.max_role.downcase == 'leader'
      current_user.leading_groups.each do |g|
        g.training_types.each do |tt|
          ttypes << tt
        end
      end
      ttypes.flatten.uniq!
      retval.delete_if { |expir| !ttypes.include?(expir.training_type) }
    end
    retval.flatten
  end

  def current_trainings(current_user)
    retval = []
    Organization.appropriate_users(current_user).each do |user|
      Expiration.where("user_id = #{user.id} and expire_on >= '#{Date.today}' and expire_on <= '#{Date.today + 30.days}'").each do |expir|
        retval << expir
      end
    end
    ttypes = []
    if current_user.max_role.downcase == 'leader'
      current_user.leading_groups.each do |g|
        g.training_types.each do |tt|
          ttypes << tt
        end
      end
      ttypes.flatten.uniq!
      retval.delete_if { |expir| !ttypes.include?(expir.training_type) }
    end
    retval.flatten
  end

  def future_trainings(current_user)
    retval = []
    Organization.appropriate_users(current_user).each do |user|
      Expiration.where("user_id = #{user.id} and expire_on >= '#{Date.today + 31.days}'").each do |expir|
        retval << expir
      end
    end
    ttypes = []
    if current_user.max_role.downcase == 'leader'
      current_user.leading_groups.each do |g|
        g.training_types.each do |tt|
          ttypes << tt
        end
      end
      ttypes.flatten.uniq!
      retval.delete_if { |expir| !ttypes.include?(expir.training_type) }
    end
    retval.flatten
  end

You could benchmark and see if a INNER JOIN would be faster on the Expiration model w/ the User model. I'm assuming they are not one to one, since you are grabbing multiple expirations per user.

How big is the Users table? If it's relatively small, and not growing exponentially you might not need to use a join.

If it has millions of records, you are essentially doing a million SELECTs:

Benchmark.realtime  { User.all.each { |x| Expiration.where(:user_id => x.id) }

Which can be bad for performance and cause your MySQL server to be overloaded.

There is also something to look out for is the N + 1 problem, which is a very similar problem to what might be happening to you:

https://www.codemy.net/posts/optimizing-your-rails-app-part-1-n-1-queries

Edit:

N+1 queries occur when you write a piece of code that executes a SQL query many times (N times depending on the code), where one query would have been appropriate. It occurs commonly in loops, maps, and places where lazy loading is not enabled. It's very common in views that display tabular info that uses other tables, and is not noticeable in a development environment where you're not testing a ton of data.


Simple gas station calculator for Windows After reading some beginner textbooks on C++ programming, I decided to write a trivial unit conversion program. Since I am working on Windows, the console windows still use codepage 850 by default, and I wanted to output the Euro currency sign. I also wanted to get the error handling right, since that's one of the topics beginner books often ignore. And, since I'm German, I wanted to use the appropriate decimal separators.

Now my "trivial" program has become quite large. Does it have to be that large? Is there some way to make it smaller, without giving up one of its features?

#include <cstdlib>
#include <iomanip>
#include <iostream>
#include <locale>
#include <string>

#include <Windows.h>

class SwitchToUtf8 {
private:
    UINT in;
    UINT out;
public:
    SwitchToUtf8() {
        in = GetConsoleCP();
        out = GetConsoleOutputCP();
        SetConsoleCP(65001);
        SetConsoleOutputCP(65001);
    }
    ~SwitchToUtf8() {
        SetConsoleCP(in);
        SetConsoleOutputCP(out);
    }
};

int main() {
    SwitchToUtf8 temporarily;

    std::locale::global(std::locale(""));
    std::wcin.imbue(std::locale());
    std::wcout.imbue(std::locale());
    std::wcerr.imbue(std::locale());

    const double price_per_liter = 1.31;

    std::wcout << "How much gas did you refuel? ";
    std::wcin.sync();

    double liter;
    if (std::wcin >> liter) {
        double price = liter * price_per_liter;
        std::ios::fmtflags saved_flags(std::wcout.flags());
        std::wcout << "That sums up to " << std::fixed << std::setprecision(2) << price << " €.\n" << saved_flags;
        std::wcout << price << "\n";
    }
    else {
        std::wcin.clear();

        std::wstring line;
        if (std::getline(std::wcin, line)) {
            std::wcerr << "Input error: " << line << "\n";
        }
        else {
            std::wcerr << "Other error\n";
        }
    }
}

Firstly, I want to commend you on the neatness of your code. Appropriate spacing and newlines used make it very readable, even if some lines are slightly long. Also, you used the std:: namespace tag over the using namespace std; line, which is again, better practice in C++.

Now to answer your question, this code is not that large. When you compare it to the simplest possible program for a gas calculator, you must realize that your code has error-handling capabilities, it adds decimal seperators according to the German style, and it adds the ability to output the Euro sign to the screen. As you add more features, your code is naturally expected to get bigger, and once again, there is nothing that I can see in your code that is unnecessary, redundant, or unrequired (I'm not an expert on GetConsoleCP(), GetConsoleOutputCP(), etc..., so I may be wrong here).

**This is my general viewpoint of the code, and any further input for discussion is welcomed.


Using temporary file as backing object for a tarfile I have existing code that handles a tarball and would like to use that to process a directory, so I thought it would be reasonable to pack the directory up in a temporary file.  I ended up writing:

class temp_tarball( object ):
    def __init__( self, path ):
        self.tmp = tempfile.NamedTemporaryFile()
        self.tarfile = tarfile.open( None, 'w:', self.tmp )
        self.tarfile.add( path, '.' )
        self.tarfile.close()
        self.tmp.flush()
        self.tarfile = tarfile.open( self.tmp.name, 'r:' )
    def __del__( self ):
        self.tarfile.close()
        self.tmp.close()

I am looking for a cleaner way to reopen the tarfile with mode 'r'.

You could try:

def __init__(self, path):
    self.tmp = tempfile.TemporaryFile()
    self.tarfile = tarfile.open( fileobj=self.tmp, mode='w:' )
    self.tarfile.add( path, '.' )
    self.tarfile.close()
    self.tmp.flush()
    self.tmp.seek(0)
    self.tarfile = tarfile.open( fileobj=self.tmp, mode='r:' )

tarfile() will take a fileobj in the constructor instead of a name, so as long as you don't close it, and instead just seek() to 0 when you want to re-read it, you should be good.


View programming pattern in C++ without raw pointers I'm trying to achieve, for lack of a better term, a "view" pattern in C++. It's probably most comparable to "views" in the Database world, where one can make a query, and then perhaps aggregate the data, or turn otherwise interact with it without actually duplicating the data.

I've tried to reduce it down to a simple example, where the Data is any integer, and the View is the opposite of that integer (x, -x). This example is pretty contrived, but when we're talking about matrices or other complex forms of data, I could see this pattern become quite useful, especially with respect to generic programming

The code compiles, and fundamentally, works. Unfortunately, there are alternative implementations of this pattern, and that's why I'm asking for a code review. In the long term, are there any reasons that I should be concerned about using this design pattern in my code? Perhaps with respect to:

Avoiding, Identifying Bugs
Class Flexibility
Code Readability

Some alternative implementations I can think of:

Using raw pointers instead of const references for the Data class
Using inheritance and implementing the "view" functions within the Data class

There are some important things to note about this code as well. The View class can not be copied or moved, meaning that its lifespan is identical to that of the Data class. There is also the flexibility of being able to mutate the View class when Data::view() is called, for example, setting a scalar. The two classes are mutual friends, but the Data only really has a use for the constructor of the View.

Data.h

#ifndef DATA_H
#define DATA_H

#include "View.h"

class Data {
public:
  Data(const int value) :
    view_(*this),
    value_(value)
  {}

  const View& view() const { return view_; }

  int value() const { return value_; }

private:
  friend View;

  const int value_;

  const View view_;

};

#endif // DATA_H

View.h

#ifndef VIEW_H
#define VIEW_H

class Data;

class View {
public:
  View() = delete;
  View(const View&) = delete;
  View(View&&) = delete;
  View operator=(const View&) = delete;
  View operator=(View&&) = delete;

  int value() const;

private:
  friend class Data;

  const Data& data_;

  View(const Data& data) : data_(data) {}

};

#endif // VIEW_H

View.cpp

#include "View.h"

#include "Data.h"

int View::value() const {
  return -data_.value_;
}

Example.cpp

#include "Data.h"

#include <iostream>

int main() {
  Data data{ 5 };

  const View& view = data.view();

  std::cout << data.value() << std::endl;
  std::cout << view.value() << std::endl;
}

The intentions are clearly expressed, the code is clean and easily understood. As far as I can see there is no obvious problems.
clang-600.0.57 failed to compile the example:

./view.h:15:9: error: unknown type name 'Data'
  const Data& data_;
        ^
./view.h:16:14: error: unknown type name 'Data'
  View(const Data& data) : data_(data) {}
             ^

I don't know what Standard mandates here. Fix is simple: add a forward declaration class Data; to view.h.
In general I am not sure that this is a right approach. Data must be aware of View anyway (for example, to add another View you need to modify Data). So implementing views as member functions seems more direct.


I made a guess the number game in Python I just started programming a week ago. Online tutorials aren't my thing, so I decided to just start projects and learn what I need for the project.

My first project is a guess the number game. The user is asked to guess a number between 0 and 20. If the user's guess is too high, they will be told it's high and likewise if it's too low. This will go on until the user guesses correctly.

print("Enter a random number between 0 and 20")
userInput = int(input())
random_number= random.randint(0,20)

def random_num_checker(num,guess):
    if num== guess:
        print('CORRECT')
    else:
        while guess != num:
            if guess > 20:
                print('I said between 0 and 20')
            if guess < num:
                print('too small')
            elif guess > num:
                print('too big')
            guess=int(input())
        print('CORRECT')
random_num_checker(random_number,userInput)

Aside from the notes already given, here are two big ideas to think about:

Don't repeat yourself (DRY)

This is something you'll hear repeated a lot in discussions of code.  Any time you see the same "magic values" repeated more than one place (e.g. 0 and 20), or you see two lines of code that do exactly the same thing for exactly the same reason (e.g. your input() statements or the multiple CORRECT checks) it's a clue that you have an opportunity to share some code.

In the case of the magic numbers, one solution is to define them as constants at the top of the file.  The approach I'd prefer, personally, would be to make them parameters -- rather than hardcoding 0 and 20, have your function(s) take a range as a parameter.

In the case of the multiple "CORRECT" checks, this is just a matter of restructuring the loop a little.

Have each function do one thing well

This is significantly more subjective, but in looking at your function, I see two very separable things it's doing within its single main loop -- prompting the user for valid input, and giving them feedback on their guess.  If you have one piece of code that just has to get the input without having to worry about the game loop, and you have another that handles the game loop without having to worry about whether the input is valid, then both of them can be simpler.

When you start doing larger projects, breaking logic apart into simple functions makes it easier to test the individual units of your code independently.

I messed around with the code for a bit using those two principles and came up with this:

import random

def describe_range(numbers: range) -> str:
    """Nice English-language description of a range."""
    return f"between {numbers[0]} and {numbers[-1]}"

def input_number(numbers: range) -> int:
    """
    Prompt the user for a number within the range,
    retrying on invalid input.
    """
    while True:
        try:
            number = int(input())
            if not number in numbers:
                raise ValueError(f"The number needs to be {describe_range(numbers)}")
            return number
        except ValueError as e:
            print(e)

def guess_number(numbers: range) -> None:
    """
    Play a guessing game with the user within a range of numbers.
    Tell them whether their guess is too big or too small.
    They win when they get it right!
    """
    print("Enter a random number", describe_range(numbers))
    answer = random.choice(numbers)
    while True:
        guess = input_number(numbers)
        if guess > answer:
            print("too big")
        elif guess < answer:
            print("too small")
        else:
            print("CORRECT")
            break

if __name__ == "__main__":
    guess_number(range(21))

Note that I'm using Python 3's type annotations to say what type of argument each function takes and what it returns -- this is really handy because you can use the mypy tool to automatically check your code for errors (for example if you say a function returns an int and there's a line in that function that returns a str, mypy will throw an error), and therefore you don't need to worry as much about your code getting the wrong type at runtime.  The type annotations also serve as documentation for your functions so that human readers can easily see what sorts of values they should expect the code to be working with.


script to check sequence number in a binary file This is a script to check the sequence number (8 bit unsigned integer) in a binary file and make sure it is incrementing till the end and not missing any sequence number in between. Sequence number is some 'x' bytes from a sentinel found in the binary file. The fixed size packet (some audio data) in binary file gets repeated and this aforementioned packet also has the sentinel along with other audio data.

I am new to python so I'd appreciate a code review.

#!/usr/bin/python
import sys,getopt

def parse_arguments():
  def usage():
    print("python script to check missing sequence number in a binary file")
    print("pass filename, packet_size(optional) and sequence offset where sequence number is found in the packet")
    print("python %s -f filename -s packet_size -i sequence_offset"% (sys.argv[0]))

  filename, packet_size, sequence_offset = None, 0, None
  try:
      opts,args = getopt.getopt(sys.argv[1:],'ha:f:i:s:', ['help', 'filename', 'index'])
  except getopt.GetoptError as err:
      print(err)
      sys.exit(2)

  if len(opts) != 0:
     for (o, a) in opts:
        if o in ('-h', '--help'):
           usage()
           sys.exit()
        elif o in ('-f', '--filename'):
           filename = a
        elif o in ('-s', '--packet_size'):
           packet_size = int(a)
        elif o in ('-i', '--sequence_offset'):
           sequence_offset = int(a)
        else:
           usage()
           sys.exit(2)
  else:
     usage()
     sys.exit(2)
  return filename, packet_size, sequence_offset

def is_sequence_missing(filename, packet_size = 5392, sequence_offset = 11):
  def get_sequence_idx(filename, sentinel = ['0x4f', '0x5f', '0x4d', '0x41']):
    blob = None
    with open(filename,"rb") as f:
      blob = f.read()
      len_sentinel = len(sentinel)
      for i in range(len(blob)):
        if i + len_sentinel < len(blob) and sentinel == [hex(ord(j)) for j in blob[i:i+len_sentinel]]:
          return (blob, i)
    return (blob, None)

  missing, start, counter = False, True, 0

  blob, idx = get_sequence_idx(filename)
  if not idx:
    print("sentinel not found")
    return not missing
  print("sentinel starting at 0x%x"% (idx))

  for i in range(idx - sequence_offset, len(blob), packet_size):
    v = ord(blob[i])
    if start:
      print("started with sequence number 0x%x"% (v))
      start = False
      counter = v + 1
      continue
    if counter == v:
      counter += 1
      if counter == 256:
        counter = 0
    else:
      print("not matching expected: 0x%x receieved: 0x%x at 0x%x"% (counter, v, i))
      counter = v + 1
      missing = True
  return missing

if __name__ == "__main__":
  filename, packet_size, sequence_offset = parse_arguments()
  print("passed filename %s packet_size %d sequence_offset %d"% (filename, packet_size, sequence_offset))
  if is_sequence_missing(filename, packet_size, sequence_offset) == True:
    print("sequence is missing")
  else:
    print("sequence is not missing")

Style

Python comes with an official style guide (often just called PEP8) which is commonly accepted among Python programmers. Since you said you are new to Python, I wholeheartedly recommend to read and follow it.

E.g. one of the recommendations of the style guide is to indent code by 4 spaces per indentation level, while you have a mix of 2 and four spaces. Even if you decide to stick with 2 spaces: be consistent!

Another very widely accepted convention is to have no whitespace around = if used in keyword-arguments to functions, e.g. instead of

def is_sequence_missing(filename, packet_size = 5392, sequence_offset = 11):

use

def is_sequence_missing(filename, packet_size=5392, sequence_offset=11):

Outdated Python

Your code uses some parts of the Python language which have become a bit "rusty", i.e. have been superseded in later versions of the language. Especially in the light of soon to come end-of-life of Python 2.7, I would highly recommend to have a look at Python 3 or at least use "more current" features of Python 2.7.

A few examples:

Command-line arguments

Your code uses getopt to parse command-line arguments. getopt has been superseded by optparse, which in turn has been marked as deprecated starting from Python 2.7 to be replaced by argparse (see also this SO post from 2010!) from there on. Apart from being easier to use, argparse will e.g. also generate and show usage instructions if the user fails to give the correct arguments.

Further reading: The official and reasonably comprehensive tutorial on argparse.

String formatting

String output is generated using the old % formatting. As of Python 2.6, the recommended way is to use str.format for string formatting. If you happen to have access to Python 3.6 and later, also so called f-strings are available to you which make the whole thing even more comfortable. An example from your code:

"not matching expected: 0x%x received: 0x%x at 0x%x"% (counter, v, i)
"not matching expected: 0x{:x} received: 0x{:x} at 0x{:x}".format(counter, v, i)  # Python 2.6+
f"not matching expected: 0x{counter:x} received: 0x{v:x} at 0x{i:x}"  # Python 3.6+

Further reading: This blog post has a nice comparison of all three variants and their possibilities.

General recommendations

Variable definition

Usually it's always a good idea to define your variables as close as possible to where they are needed, and also arguably define one variable per line. This helps to avoid unused or uninitialized variables, although the later part is not so critical here in Python as it would be e.g. in C or C++.

Documentation

Also not strictly specific to Python: Do yourself (and possible also others) a favor and document your code. Maybe in a few months time where you have done something different you will (have to) come back and wonder why the default value for sequence_offset is 11. Python has a feature called docstrings which are also described in the style guide (and in more detail in PEP 257) and are very commonly found in Python code. docstrings are essentially """triple quoted strings""" placed immediately after the function definition, trying to describe what to expect from the function. As an example:

def is_sequence_missing(filename, packet_size=5392, sequence_offset=11):
    """Check file for gaps in the sequence number

    read up binray data from filename and check for ascending
    sequence numbers starting sequence_offset (default: 11) bytes away
    from the sentinel value
    """
    # rest of the code here

Documentation written using docstrings will be picked up by Python's built-in help(...) as well as by most Python IDEs.

That's all for now. Maybe I can spare some time tomorrow to have a closer look at the actual algorithm. Edit: See the second answer for a more detailed feedback on the actual algorithm.