Python object-oriented pipe cooling simulations Here is my code simulating liquid in a pipe cooling under different conditions. How did I do approaching this in an object orientated way? Is there any way I can improve this code?

import numpy as np
import scipy
from scipy.optimize import minimize
from matplotlib import pyplot as plt

class pipeSimulator:
    R_w = 0.654#  W/mk water heat loss ocefficient
    # R_w = 0.028# W/mk air heat loss ocefficient
    R_f = 0.042  #  W/mk pipe heat loss coefficient
    inner_radius = 0.2  #  m pipe radius
    T_0 = 50  #  C initial temperature
    T_a = 20  #  C ambiant temperature
    dx = 0.01  #  m differential distance
    Length = 25  # m length of pipe

    def heat_balance(self,T_1,T_2):
       heat_in = (T_1-T_2) * scipy.pi * self.inner_radius**2 * self.R_w
       heat_out = ( (T_1 + T_2) / 2 - self.T_a ) * 2 * self.inner_radius * scipy.pi * self.dx * self.R_f
       return abs(heat_in-heat_out)

    def simulation(self):
       T = [self.T_0]
       X = [0]
       for x in np.arange(self.dx,self.Length,self.dx):
         X.append(x)
         fun = lambda s: self.heat_balance(T[-1],s)
         T.append(float(minimize(fun,T[-1]-self.dx,method="SLSQP")['x']))
       return([T,X])

trial = pipeSimulator()
trial.Length = 30
diameters = [0.02,.2,1,3]
R_values = [100,400]
T_a_values = [20]
cols = ['red', 'blue', 'black', 'purple', 'brown']
mylines = ['-','--','-.',':']
i=0
labs = []
for rval in R_values:
    trial.R_f = 1.0/rval
    for dval in diameters:
         trial.inner_radius = dval/2.0
         for tval in T_a_values:
              trial.T_a = tval
              ans = trial.simulation()
              plt.plot(ans[1],ans[0],c=cols[i%len(cols)],ls=mylines[i%len(mylines)],lw=2*(i%len(mylines)+1))
              labs.append(str(rval)+' R-value (EU), '+str(dval)+' meter diameter, '+str(tval)+" degree C ambient")
              i+=1

plt.legend(fontsize=20,title='',labels=labs,prop={'size':12},loc=3)
plt.xlabel('Distance (m)',fontsize=20)
plt.ylabel('Average Temperature (C)',fontsize=20)
plt.show()

Immediately you have bad names of constants. R_w, R_f, T_0? These are meaningless. If they correspond to technical terms, then make a note of that in a single comment, ideally with a link to somewhere online that they could be roughly explained. But the best response would be actual words:

class PipeSimulator:
    INNER_RADIUS = 0.2  # m
    DIFFERENTIAL_DISTANCE = 0.01  # m
    LENGTH = 25  # m

    # Coefficients (in W/mk)
    WATER_HEAT_LOSS = 0.654
    # AIR_HEAT_LOSS = 0.028
    PIPE_HEAT_LOSS = 0.042

    # Temperature C
    INITIAL_TEMPERATURE = 50
    AMBIENT_TEMPERATURE = 20

Now, you'll notice I changed the names but also the naming style to make it clearer that they're constants. It's important to use this convention. It's useful to note that these are coefficients and what unit they're in (I assume that's what W/mk is) but instead of noting it individually put a single comment header under which they all fall. For units on single constants, simply leaving the unit notation at the end is sufficient.

However, I question whether these should all be constants. Wont you have pipes of differing lengths and inner radius? I'm not familiar with the physics here, but will the coefficients change contextually? It seems like at least some of these values should be initiated as instance attributes with __init__. Indeed, some of them are set by you again later. You'd save some lines by setting up __init__.

To give a simple demo of how that would look, I'll just use inner radius and length:

class PipeSimulator:

    def __init__(self, inner_radius=0.2, length=25):
        self.inner_radius = inner_radius
        self.length = length

pipe1 = PipeSimulator(0.2, 25)
pipe2 = PipeSimulator(0.6, 40)
pipe3 = PipeSimulator()

This way, you can pass different parameters when creating PipeSimulator to get different attributes on your object. I'm also using default parameters, in case you'd want the class to fall back on a default radius on length for when none are supplied (like in pipe3). If this is completely unfamiliar to you, leave a comment and I can explain how it works.

Note that __init__ can have other lines of code too, so if some attributes need to be calculated it's perfectly fine to have them determined in there rather than calculating externally to then assign to your PipeSimulator object.

heat_balance has a complex equation to determine the result. I'd suggest putting in a docstring where you make reference to what this equation is and where it could be found. It would be more readable too if you split the long lines up, calculating only a bit at a time instead of everything thrown together in an unreadable manner. If you wanted to be really neat, heat_in and heat_out could each have their own function that just get called in heat_balance.

The same applies to simulation except that I don't think it's a formula so much as your methods and intentions are not the clearest. What are T and X for? If you gave them better names it would be much easier to follow what they mean. Add a docstring too, explain what is simulated here. Though your question title explains it, nothing in the code itself does.


fork_call every 'n' seconds in AnyEvent I need to run fork workers every 5 seconds. Is there a way to do it more ideomatic? I'm embarrassed about $running variable.

use 5.010;
use strict;
use warnings;

use AnyEvent;
use AnyEvent::Util;

my $running;

sub work1 {
    say "work1";
    sleep 1;
}

sub work2 {
    say "work2";
    sleep 1;
}

sub do_work {

    my $worker_cv = AnyEvent->condvar;

    $worker_cv->begin;
    fork_call {
        work1();
    } sub {
        $worker_cv->end;
    };

    $worker_cv->begin;
    fork_call {
        work2();
    } sub {
        $worker_cv->end;
    };

    $worker_cv->cb(sub {
        $running = 0;
    });
}

my $timer = AnyEvent->timer(
    interval => 5,
    cb => sub {
        unless ($running) {
            $running = 1;
            do_work();
        }
    }
);

my $loop = AnyEvent->condvar;

$loop->recv;

Ok, here is a smaller scope for $running and do_work() doesn't need to bother with it,

sub do_work {

    my $worker_cv = AnyEvent->condvar;

    $worker_cv->begin;
    fork_call {
        work1();
    } sub {
        $worker_cv->end;
    };

    $worker_cv->begin;
    fork_call {
        work2();
    } sub {
        $worker_cv->end;
    };

    return $worker_cv;
}

my $running;
my $timer = AnyEvent->timer(
    interval => 5,
    cb => sub {
        return if $running;

        $running = 1;
        do_work()->cb(sub{
            $running = 0;
        });
    }
);


The game of life with a truly infinite board The game of life is often implemented by representing the board as a 2D boolean array.  This doesn't scale very well to larger boards -- it starts to consume lots of memory, and without some separate mechanism to keep track of a list of live cells, you have to visit each board cell on each iteration.  This implementation just keeps a list of live cells to represent the board state; the board "size" is limited only by the maximum of an integer.

import Data.List as L
import Data.Map  as M

type Coo = (Int,Int)
type Board = Map Coo Int

moveBoard::Coo->Board->Board
moveBoard (dx,dy) = M.mapKeysMonotonic (\(x,y)->(x + dx, y + dy))

countNeighbors::Board->Board
countNeighbors b =
  unionsWith (+) [ moved (-1, -1),  moved (0, -1),  moved (1, -1),
                   moved (-1,  0),                  moved (1,  0),
                   moved (-1,  1),  moved (0,  1),  moved (1,  1) ]
    where moved (dx, dy) = moveBoard (dx, dy) b

lifeIteration::Board->Board
lifeIteration b = M.union birth survive
  where neighbors = countNeighbors b
        birth     = M.map (const 1)  (M.filter (==3) neighbors)
        survive   = M.intersection b (M.filter (==2) neighbors)

glider = M.fromList $ L.map (\(x,y)->((x,y),1::Int)) ([(1,1),(1,2),(1,3),(2,3),(3,2)]::[(Int,Int)])

Edit: This answer was given when the reviewed code looked quite differently.

Any specific questions? Here's what stands out for me:

Why do toList in emptyNeighbors, then go back to Set again? You could simply use Data.Set.map there.
countNeighbor is very inefficient: the filter operation always iterates over all life cells, and you are calling it three times per existing cell! That's unneeded, as you only ever care about a handful of neighbourhood cells.

My idea to fix issue 2 would be to build a Map of the neighbour count of every cell. If you represent the board as a Map with only 1 cells in it, that can be done pretty efficiently using mapKeysMonotonic and unionsWith:

type Coo = (Int, Int)
type Board = Map Coo Int

moveBoard :: Coo -> Board -> Board
moveBoard (dx,dy) = M.mapKeysMonotonic (\(x, y) -> (x+dx, y+dy))

countNeighbours :: Board -> Board
countNeighbours b =
  unionsWith (+) [ moved (-1) (-1), moved 0 (-1), moved 1 (-1)
                 , moved (-1)   0 ,               moved 1  0
                 , moved (-1)   1 , moved 0   1 , moved 1  1   ]
 where moved dx dy = moveBoard (dx,dy) b

Note that usage of mapKeysMonotonic is only safe because the order of coordinates doesn't change when we add a constant. Effectively, this means that the library can simply replace the concrete coordinates without any internal resorting.

The iteration is then a simple matter of using filter, map and intersection over the result:

lifeIteration :: Board -> Board
lifeIteration b = M.union birth survive
 where neighbours = countNeighbours b
       birth      = M.map (const 1)  (M.filter (==3) neighbours)
       survive    = M.intersection b (M.filter (==2) neighbours)

Changing your formulation slightly by having a life cell with 3 neighbours "rebirth" instead of survive, as that's a bit simpler to write.

Also note that this is a bit "clever" by taking advantage of the fact that intersection always returns the value of the first Map, therefore I don't need to do another M.map (const 1) step in there.

I hope this is helpful to you.


Self populating view models So I have a view model (asp.net mcv 3 app) that when instantiated with a particular parameter, populates it's properties via a couple of calls to a web service in it's constructor.

public class QuizPreviewViewModel
{
    private IQuizServiceWrapper _service;
    public string QuizName { get; set; }
    public List<QuizQuestion> QuizQuestions { get; set; }
    public List<AnswerChoice> AnswerChoices { get; set; }
    public int QuizId { get; set; }

    public QuizPreviewViewModel()
    {
    }

    public QuizPreviewViewModel(int quizId )
    {
        _service = new QuizServiceWrapper();
        var quiz = _service.GetQuizHeader(quizId);
        var questions = _service.GetQuizQuestionsByQuiz(quizId).OrderBy(x => x.Order).ToList();
        QuizId = quizId;
        QuizName = quiz.QuizName;
        QuizQuestions = questions;
    }
}

I know I could just as easily perform the population in the controller, or possibly extract the code to populate the properties into a separate method or helper class.

I have read in various books/articles that both view models and controllers should be kept as clean as possible.  So I'm wondering what you all think of populating the view model in it's constructor.  Is this a decent practice, or a horrible idea?  Where is the best place to populate the view model?

Personally I try to keep the view models as close to DTO's as possible.  That way you can have any number of objects populating the data from any number of sources.  And, as you mentioned, it helps keep them lean and free of domain logic.

If there was only one particular method of doing the transformation I might look at existing mapping solutions i.e. AutoMapper.  Alternatively as you suggested I might create a separate builder class that was responsible for doing the building.

The main problem I have with doing it your way (apart from those mentioned by Larry already) is that it tightly couples the ViewModel with your model.  Re-using the viewModel in different contexts becomes harder.

Also, if you were to to TDD, I might find it a bit strange writing a test like:

public void TestMyViewModel()
{
   try
   {
      // Act
      var viewModel = QuizPreviewViewModel(1);

      // nothing to arrange???

      // Assert
      Assert.AreEqual(1, viewModel.QuizId);
   }
   catch(Exception ex)
   {
      Assert.Fail();
   }
}

I think I would prefer something like:

public class QuizPreviewViewModelBuilder
{
    private readonly IQuizServiceWrapper _service;

    public QuizPreviewViewModel(IQuizServiceWrapper service)
    {
        _service = service;
    }

    public QuizPreviewViewModel GetViewModel(int quizId)
    {
        var viewModel = new QuizPreviewViewModel();
        var quiz = _service.GetQuizHeader(quizId);

        viewModel.QuizId = quizId;
        viewModel.QuizName = quiz.QuizName;
        viewModel.QuizQuestions = quiz.questions;

        return viewModel;
    }
}

then your test class would look like:

public void TestMyViewModelBuilder()
{
      // Act
      var mockService = // new mock service however this works
      var builder = QuizPreviewViewModelBuilder(mockService);

      // arrange
      var viewModel = builder.GetViewModel(1);

      // Assert
      Assert.AreEqual(1, viewModel.QuizId);
}

Even this has a potential downside I guess in the fact that you have to have a bit of knowledge as to how to build up the service in order for the GetViewModel to work, so I'm not completely sold on that idea.

Although this really just moves the logic of the building from the controller to another class I think it better allows for re-useability of the ViewModel while also providing a DRY approach to doing the actual building.

Just a couple of different thoughts for you to mull over.


HackerRank itertools.combinations() I was solving the following problem on HackerRank.
Here's my solution -
from itertools import combinations
a = input()
ls = list(map(str, a.split()))
for i in range(1, int(ls[1])+1):
    ls2 = []
    ls2 = list(map("".join, combinations(ls[0], i)))
    for elem in ls2:
        ls2[ls2.index(elem)] = "".join(sorted(elem))
    ls2.sort()
    for char in ls2:
        print(char)

But I feel it's not very efficient as it uses 2 Lists and 2 nested loops.
Any suggestions as to how can I improve the code?

Nice solution, these are my suggestions:

Parsing the input: in Python 3 the function input always returns a string, so the mapping can be removed. In Python 2, you can use raw_input.
Naming: ls is too generic. In this case, a suggestion is to take the names from the problem description. From:
a = input()
ls = list(map(str, a.split()))

To:
S, k = input().split()

Sort in advance: the problem requires to output the combinations in lexicographic sorted order. The doc says:

The combination tuples are emitted in lexicographic ordering according
to the order of the input iterable. So, if the input iterable is
sorted, the combination tuples will be produced in sorted order.

So, instead of sorting each combination, it's enough sorting the input S at the beginning and all the combinations will be automatically sorted.

Use the output of itertools.combinations: the output of itertools.combinations is an iterable or tuples, which can be used in a for-loop to print the combinations one by one. The only change is to convert the tuple to a string, and can be done with string.join.

Revised code
from itertools import combinations

S, k = input().split()
S_sorted = ''.join(sorted(S))
for i in range(1, int(k)+1):
    for c in combinations(S_sorted, i):
        print(''.join(c))


Is this binary search in TypeScript correct? I need your help to see if the following binary search code is correct. I did my best to cover the corner cases.  I wonder if I missed anything.

The code as it is with tests (you can play with it online here):

function binarySearch<a, r>(
    hay: a[],
    needle: a,
    compare:(one: a, another: a, oneGreaterThanAnother: () => r, oneLessThanAnother: () => r, oneEqualToAnother: () => r) => r,
    haveStaightHit: (hay: a[], index: number) => r,
    haveExpectedToBeAt: (hay: a[], index: number) => r
) {
    if (hay.length > 0) {
        return binarySearchUnsafe(hay, needle, 0, hay.length - 1, compare, haveStaightHit, haveExpectedToBeAt);
    } else {
        return haveExpectedToBeAt(hay, 0);
    }
}
function binarySearchUnsafe<a, r>(
    hay: a[],
    needle: a,
    from: number,
    to: number,
    compare:(one: a, another: a, oneGreaterThanAnother: () => r, oneLessThanAnother: () => r, oneEqualToAnother: () => r) => r,
    haveStaightHit: (hay: a[], index: number) => r,
    haveExpectedToBeAt: (hay: a[], index: number) => r
): r {
    if (from < to) {
        var at = from + (~~((to - from) / 2));
        return compare(
            needle,
            hay[at],
            function needleIsOnTheRight() {
                return binarySearchUnsafe(hay, needle, at + 1, to, compare, haveStaightHit, haveExpectedToBeAt);
            },
            function needleIsOnTheLeft() {
                return binarySearchUnsafe(hay, needle, from, at, compare, haveStaightHit, haveExpectedToBeAt);
            },
            function needleIsFound() {
                return haveStaightHit(hay, at);
            }
        );
    } else if (from > to) {
        throw new Error('From index ' + from + ' is bigger than the to index ' + to + '.');
    } else {
        var at = from /* === to */;
        return compare(
            needle,
            hay[at],
            function needleIsOnTheRight() {
                return haveExpectedToBeAt(hay, at + 1);
            },
            function needleIsOnTheLeft() {
                return haveExpectedToBeAt(hay, at);
            },
            function needleIsFound() {
                return haveStaightHit(hay, at)
            }
        );
    }
}

function areEqual(actual: any, expected: any) {
    if (actual !== expected) throw new Error('Expected: ' + expected + ', Actual: ' + actual);
}

function compareStrings<r>(one: string, another: string, oneGreaterThanAnother: () => r, oneLessThanAnother: () => r, oneEqualToAnother: () => r) : r {
    if (one > another) {
        return oneGreaterThanAnother();
    } else if (one < another) {
        return oneLessThanAnother();
    } else {
        return oneEqualToAnother();
    }
}

function failOver(message: string) : any {
    return function() {
        throw new Error(message);
    }
}

try {
    binarySearch([], 'x', compareStrings, failOver('Direct hit is not expected.'), (hay, index) => areEqual(index, 0));
    binarySearch(['b', 'd', 'f'], 'b', compareStrings, (hay, index) => areEqual(index, 0), failOver('Direct hit is expected.'));
    binarySearch(['b', 'd', 'f'], 'd', compareStrings, (hay, index) => areEqual(index, 1), failOver('Direct hit is expected.'));
    binarySearch(['b', 'd', 'f'], 'f', compareStrings, (hay, index) => areEqual(index, 2), failOver('Direct hit is expected.'));
    binarySearch(['b', 'd', 'f'], 'g', compareStrings, failOver('Direct hit is not expected.'), (hay, index) => areEqual(index, 3));
    binarySearch(['b', 'd', 'f'], 'a', compareStrings, failOver('Direct hit is not expected.'), (hay, index) => areEqual(index, 0));
    binarySearch(['b', 'd', 'f'], 'c', compareStrings, failOver('Direct hit is not expected.'), (hay, index) => areEqual(index, 1));
    alert('You are all good!');
} catch (e) {
    alert(e.message);
}

That looks jolly complicated.  If you haven't read it, look for Jon Bentley's Programming Pearls article on binary search.  Here's my version:

var bSearch = <T>(xs: T[], x: T, cmp: (p: T, q: T) => number): number => {
    var bot = 0;
    var top = xs.length;
    while (bot < top) { // If x is in xs, it's somewhere in xs[bot..top).
        var mid = Math.floor((bot + top) / 2);
        var c = cmp(xs[mid], x);
        if (c === 0) return mid;
        if (c < 0) bot = mid + 1;
        if (0 < c) top = mid;
    }
    return -1;
}

var cmp = (p: number, q: number) => p - q;

console.log(bSearch([], 3, cmp) === -1);
console.log(bSearch([3], 3, cmp) === 0);
console.log(bSearch([1, 3], 3, cmp) === 1);
console.log(bSearch([3, 4], 3, cmp) === 0);
console.log(bSearch([3, 3], 3, cmp) !== -1);
console.log(bSearch([1, 2, 3], 3, cmp) === 2);
console.log(bSearch([1, 3, 4], 3, cmp) === 1);
console.log(bSearch([3, 4, 5], 3, cmp) === 0);
console.log(bSearch([4, 5, 6], 3, cmp) === -1);


Recursive bytearray hash function I need a function that hashes a list of different inputs (integral datatypes, strings, bytearrays) in a recursive manner.

def RecHash(v):
    isSingleElementOfList = False

    if type(v) is list and len(v) == 1:
            isSingleElementList = True

    if type(v) is not list or isSingleElementOfList:   # if v is a single element
        v0 = v
        if isSingleElementOfList: v0 = v[0]

        if type(v0) is bytearray or v0.__class__.__name__ == 'bytes':
            return hash(v0)
        if type(v0) is int:
           return hash(ToByteArray(v0))
        if type(v0) is str:
            return hash(v0.encode('utf-8'))

        return bytes()
    else:                               # if v is a list
        res = bytearray()
        for vi in v:
            res +=  RecHash(vi)     # recursion
        return hash(res)            # hash the concatenated hashes

and the helper function:

def ToByteArray(x):
    q, r = divmod(BitAbs(x),8)
    q += bool(r)
    return ToByteArrayN(x, q)

def ToByteArrayN(x, n):

    B = bytearray()

    for i in range(0, int(n)):
        b = x % 256
        x = x // 256                  # // = integer division => floor
        B.insert(0, b)

    return bytes(B)

def BitAbs(i):
     return i.bit_length()

It is working fine, however, it's very slow (and interestingly, it's not the hash() function that is slowing it down.).

Performance is worst when the list contains mostly numbers, so the ToByteArray function also doesn't seem to perform well.

ISTR that type(x) is relatively slow.  Try saving it in a local variable:

typev = type(v)
if typev is list and len(v) == 1:
        isSingleElementList = True

if typev is not list or isSingleElementOfList:   # if v is a single element
    v0 = v[0] if isSingleElementOfList else v

    type_v0 = type(v0)
    if type_v0 is bytearray or v0.__class__.__name__ == 'bytes':
        return hash(v0)
    if type_v0 is int:
       return hash(ToByteArray(v0))
    if type_v0 is str:
        return hash(v0.encode('utf-8'))
else:                               # if v is a list
    res = bytearray()
    for vi in v:
        res +=  RecHash(vi)     # recursion
    return hash(res)            # hash the concatenated hashes

Also, btw, your code will choke on nested single element lists. Consider [[[1]]]


Uploading photos: Domain Model and Repository classes In the database, I have a table with name photographs with the following columns:

id
filename
type
size
caption

I want to create two classes - one is Domain Model named photo.php and the other one is a Repository class (photorepository.php).

So, I started like this:

photo.php:

class Photo
{
    private $id;
    private $filename;
    private $type;
    private $size;
    private $caption;

    public function __construct($filename, $type, $size, $caption, $id = 0)
    {
        $this->id = $id;
        $this->filename = $filename;
        $this->type = $type;
        $this->size = $size;
        $this->caption = $caption;
    }

   // setters & getters

}

Repository class (photorepository.php):

class PhotographRepository
{
    private $connection;
    private $statements = [];

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

    public function create($file, $caption) {
        if(!$file || empty($file) || !is_array($file)) {
            return false;
        } elseif($file['error'] != 0) {
            return false;
        } else {
            $temp_path = $file['tmp_name'];
            $filename = basename($file['name']);
            $type = $file['type'];
            $size = $file['size'];
            if(empty($filename) || empty($temp_path)) {
                return false;
            }
            $target_path = __DIR__ . "/../public/images/" . $filename;
            if (file_exists($target_path)) {
                return false;
            }
            if(move_uploaded_file($this->temp_path, $target_path)) {

                $photo = new Photograph($filename, $type, $size, $caption);

                if (!isset($this->statements[__METHOD__])) {
                    $this->statements = $this->connection->prepare("INSERT INTO photographs (filename, type, size, caption) VALUES (:filename, :type, :size, :caption)");
                }
                $stmt = $this->statements[__METHOD__];
                if ($stmt->execute([':filename'=>$photo->getFilename(), ':type'=>$photo->getType(), ':size'=>$photo->getSize()])) {
                    $photo->setId($this->connection->lastInsertId());
                    $stmt->closeCursor();
                    unset($this->temp_path);
                    return true;
                } else {
                    $stmt->closeCursor();
                    return false;
                }
            }
        }
    }

}

$file is $_FILES["input_name"]. Is this the good way of doing it?

Working with images is always a messy adventure. The PhotographRepository#create method is doing a number of things:

Creating a new record in the database
Testing to see if the file upload exists
Moving the uploaded temp file into a permanent location

The challenge in this scenario is how to handle database problems as well as file I/O problems in a manner that allows the user to try again.

Database operations are inherently safe in that if an INSERT fails on commit, the transaction gets rolled back. File operations are permanent and therefore destructive in nature. You can "undo" a file move operation, but it is not as simple is issuing a "rollback" to the file system. Due to the increased risk of losing information in I/O operations, I would recommend ensuring the database INSERT succeeds first, then attempt to move the file.

While a "photograph" seems to be just one thing, at a code level it is two things: A record in the database and a file on disk. The I/O operation should be placed in its own repository. What exists on a shared drive today might need to move to The Cloud® tomorrow. You actually have a need for two repositories:

PhotographRepository to handle the CRUD operations in the database
ImageFileRepository to handle the CRUD operations of storing the file on disk (or in The Cloud®)

The question becomes how to marry the two.

Dependent Repositories

There are times when one repository requires another repository in order to make one complete "transaction" (INSERT a record into a database, move a file on disk). There are two approaches here:

Use Constructor Injection to pass the ImageFileRepository object into PhotographRepository

$photos = new PhotographRepository(new PDO(...), new ImageFileRepository());

Create a "repository context" object that gives you easy access to all the other repositories in your system

Which approach you take depends on how dependent each repository is on the others. If the PhotographRepository is the only one that needs another "repository" object, then go for option #1. If you have a number of repository objects that need each other, for example you need to save a parent record in the database and insert child records in other tables, then go for option #2.

Whichever direction you take, you want the file storage decoupled from the meta data stored in the database.

Using Constructor Injection for Dependent Repositories

This is the easiest to implement, but doesn't scale well as your application grows and repositories increasingly become dependent on one another. Part of our goal is to decouple the file storage from the database operations, which is a prime example of how interfaces are beneficial. The PhotographRepository class will require a second constructor argument:

class PhotographRepository
{
    private $connection;
    private $images;

    public function __construct(PDO $connection, ImageRepository $images) {
        $this->connection = $connection;
        $this->images = $images;
    }

The ImageRepository object is actually the following interface:

interface ImageRepository
{
    public function create($file);
}

The PhotographRepository#create method gets a refactoring job so the database INSERT occurs first, and then the image is moved:

class PhotographRepository
{
    // ...

    public function create($file, $caption) {
        $imageInfo = new SplFileInfo($file['tmp_name']);
        $photo = new Photograph($imageInfo->getFileName(), $file['type'], $file['size'], $caption);

        if (!isset($this->statements[__METHOD__])) {
            $this->statements = $this->connection->prepare("INSERT INTO photographs (filename, type, size, caption) VALUES (:filename, :type, :size, :caption)");
        }

        $stmt = $this->statements[__METHOD__];

        if ($stmt->execute([':filename'=>$photo->getFilename(), ':type'=>$photo->getType(), ':size'=>$photo->getSize()])) {
            $photo->setId($this->connection->lastInsertId());
            $stmt->closeCursor();
            $this->images->create($file);

            return $photo;
        } else {
            $stmt->closeCursor();

            throw new RuntimeException('Failed to save Photograph');
        }
    }

Two differences:

The database operation is completed first, then the file operation is performed
If the database operation fails, an exception gets thrown. being unable to insert into the database is a catastrophic problem, so throw an exception

Now let's implement the ImageRepository interface to perform the file move operation:

class ImageFileRepository implements ImageRepository
{
    private $root;

    public function __construct($root) {
        if (!file_exists($root)) {
            throw new InvalidArgumentException('Root image directory does not exist: ' . $root);
        } elseif (is_writable($root)) {
            throw new InvalidArgumentException('Root image directory "' . $root . '" is not writable');
        }

        $this->root = $root;
    }

    public function create($file) {
        $this->ensureValidFile($file);
        $targetPath = $this->root . basename($file['name']);

        if (file_exists($targetPath)) {
            throw new InvalidArgumentException('The file "' . $targetPath . '" already exists');
        }

        if (!move_uploaded_file($file['tmp_name'], $targetPath)) {
            throw new RuntimeException('Failed to move upload file from "' . $file['tmp_name'] . '" to "' . $targetPath . '"');
        }

        return new SplFileInfo($targetPath);
    }

    private function ensureValidFile($file) {
        if (!$this->isValidFile($file)) {
            throw new InvalidArgumentException('File is not valid');
        } elseif (!is_readable($file['name'])) {
            throw new InvalidArgumentException('File is not readable');
        } elseif ($file['error'] != 0) {
            throw new InvalidArgumentException('File error: ' . $file['error']);
        }
    }

    private function isValidFile($file) {
        return $file
            && !empty($file)
            && is_array($file);
    }
}

If the any file operation cannot be completed (reads or writes) throw an exception. This is a hard-stop in your application. If your application cannot write to the public/images directory, fail early and fail loudly. A server admin needs to fix this.

Now it only takes 3 lines of code to create the photograph record and move the image:

$photos = new PhotographRepository(
    new PDO(...),
    new ImageFileRepository(__DIR__ . '/../public/images/')
);
$caption = $_POST['caption'];
$photo = $photos->create($_FILES['input_name'], $caption);

The first line of code is where you glue the two repositories together. This works well, architecturally, but if you need PhotographRepository objects in multiple places, you'll have a lot of repeated code. This is where a "Repository Context" comes in handy.

Using a Repository Context

When interdependent relationships between repositories become more commonplace, an object wrapper for your repositories is beneficial:

class RepositoryContext
{
    public $photos;
    public $images;

    public function __construct() {
        $this->images = new ImageFileRepository($this, __DIR__ . '/../public/images/');
        $this->photos = new PhotographRepository($this, new PDO(...)));
    }
}

The key feature is that each repository gets passed a reference to its $context.

Now ImageFileRepository becomes:

class ImageFileRepository implements ImageRepository
{
    private $context;
    private $root;

    public function __construct(RepositoryContext $context, $root) {
        if (!file_exists($root)) {
            throw new InvalidArgumentException('Root image directory does not exist: ' . $root);
        } elseif (is_writable($root)) {
            throw new InvalidArgumentException('Root image directory "' . $root . '" is not writable');
        }

        $this->context = $context;
        $this->root = $root;
    }

And PhotographRepository becomes:

class PhotographRepository
{
    private $context;
    private $connection;
    private $images;

    public function __construct(RepositoryContext $context, PDO $connection) {
        $this->context = $context;
        $this->connection = $connection;
        $this->images = $images;
    }

    public function create($file, $caption) {
        // ...

        if ($stmt->execute([':filename'=>$photo->getFilename(), ':type'=>$photo->getType(), ':size'=>$photo->getSize()])) {
            $photo->setId($this->connection->lastInsertId());
            $stmt->closeCursor();
            $this->context->images->create($file);

            return $photo;
        } else {
            $stmt->closeCursor();

            throw new RuntimeException('Failed to save Photograph');
        }
    }
}

After the database insert, we delegate to $this->context->images->create($file); to access the image file repository. Then it's just a slight change to the code creating the photograph:

$context = new RepositoryContext();
$caption = $_POST['caption'];
$photo = $context->photos->create($_FILES['input_name'], $caption);

Moving Image Files To The Cloud®

Later, when the Business decides to spend money on the next big buzzword, you can write another image repository to store those images on a cloud-based hosting provider:

class CloudImageRepository implements ImageRepository
{
    private $url;
    private $apiKey;

    public function __construct(RepositoryContext $context, $url, $apiKey) {
        $this->context = $context;
        $this->url = $url;
        $this->apiKey = $apiKey;
    }

    public function create($file, $caption) {
        // ...
    }
}

And just create a different repository in your context:

class RepositoryContext
{
    public $photos;
    public $images;

    public function __construct() {
        $this->images = new CloudImageRepository($this, $url, $apiKey);
        $this->photos = new PhotographRepository($this, new PDO(...)));
    }
}

Nothing else in your code needs to change.


customSignupForm code to allow signup for only the invited user I am developing a feature where the user has to request for invitation if he/she wants to sign up on the site. When the request is accepted by the admin, the user gets to sign up, otherwise it shows them an error saying they are not invited at this time, and saves a email so we can follow up with him/her. To be generic, I used the condition INVITE_MODE = True in settings, so if we disable the invitation feature using INVITE_MODE=False, all the users should get to sign up.

Here is my signup form and I am using Django allauth for that:

class CustomSignupForm(forms.Form):
    def __init__(self, *args, **kwargs):
        super(CustomSignupForm, self).__init__(*args, **kwargs)

    def clean(self):
        email = self.cleaned_data.get('email')
        print ('email', email)
        email = get_adapter().clean_email(email)
        # if email and app_settings.UNIQUE_EMAIL:
        #     email = self.validate_unique_email(email)
        if email and settings.INVITE_MODE:
            try:
                Invitation.objects.get(email=email, request_approved=True)
            except Invitation.DoesNotExist:
                invitation, created = Invitation.objects.get_or_create(email=email)
                self.add_error('email', 'Sorry at this time you are not invited. But we have added you to our invite list')
        elif email is None:
            self.add_error('email', 'Email field cannot be empty')
        else:
            return email

    def signup(self, request, user):
        # email = self.cleaned_data.get('email')
        # user.email = email
        user.save()

Can anyone suggest a to do this in a better/efficient way?

A good motivation to overwrite the Form's clean() method is when you need to modify/check multiple fields (reference):

  The form subclass’s clean() method can perform validation that
  requires access to multiple form fields. This is where you might put
  in checks such as “if field A is supplied, field B must contain a
  valid email address”.

And, the overwritten clean() method usually returns the modified cleaned_data.

In your case, you only work with a single field, use the clean_<fieldname>() method - clean_email() to be specific.

I would also handle the "email is falsy" case beforehand (though, I am not completely sure if you actually need to worry about it here and having required=True on your email form field is not enough).

Also, I'm not sure you actually need to call clean_email() on the adapter - if you are using the default account adapter, I think it is doing nothing and is simply a "stub" for custom adapters (source).

I think using get_or_create() first and then checking the request_approved value could be a bit cleaner.

Here is a variation of the code with applied assumptions and suggestions (described above):

def clean_email(self):
    email = self.cleaned_data.get['email']

    if settings.INVITE_MODE:
        invitation, _ = Invitation.objects.get_or_create(email=email)
        if not invitation.request_approved:
            self.add_error('email', 'Sorry at this time you are not invited. But we have added you to our invite list')

    return email

One concern with that though - I am not sure if creating an invitation record from a form field's clean form method is a good thing and is not a sign of a bad design..

Another note is that if a new user would submit the form multiple times with the same email address, the user would get the same "Sorry at this time you are not invited. But we have added you to our invite list" message. Instead, may be, if email does not exist, you can say that the user was added to the invite list, but if email exists but the invitation is not approved, you can say something more specific, like "Your invitation is waiting for approval".


Short Ruby program that opens a random video file from a directory I am currently learning to code in ruby. I have created a small program that iterates through a directory of video files and opens one at random. The program is fully functioning (with the odd issue). The aim of my question is to hopefully gain some insight into how I can better structure the code, or simplify the process - I think this will really aid me in my learning!

The random selection method I have created, creates 2 random numbers and converts them to a string:

def random_num_generator
  first_num = rand(2..9) #NOTE: generates a random number that is firt name and last name. s
  second_num = rand(1..25)
  second_num < 10 ? "0#{first_num}0#{second_num}" : "0#{first_num}#{second_num}"
  #this returns value that, for example is 0101, relative to whether or not there is a requirement for the additional "0"
  #value is equal to a string
end

The second part of the program iterates through the directory where the video files are stored. As all the file names are written as "0101.mkv" for example, the iteration will then stop and open a file if it is equal to the number generated in the "random_num_generator" method.

 def episode_picker
  Dir.foreach("/GitHub/videos") do |x|
    next if x == "." or x == ".."
    if x == "#{random_num_generator}.mkv"
      system %{open "/GitHub/videos/#{x}"}
    elsif x == "#{random_num_generator}.mp4"
      puts "You are watching Season: #{x[0..1]} Episode: #{x[2..3]}!"
      system %{open "/GitHub/videos/#{x}"}
    elsif x == "#{random_num_generator}.avi"
      puts "You are watching Season: #{x[0..1]} Episode: #{x[2..3]}!"
      system %{open "/GitHub/videos/#{x}"}

    end
  end
end

Any advice is greatly appreciated and a big thanks in advance!

I'll try to help you on how you could improve your code here.
Keep in mind that I have no idea what is your current level in programming in general. You state that you started learning ruby, but you don't tell us about other languages.

Random number generator

This method is fine, but what you want is to avoid having to comment every line. Comments are usually needed when the code is not clear. We'll try to make its meaning more obvious.

Let's try first to find better names here. It can seem like a little detail, but it is pretty important if you want to be able to read it in the future.

Names are very hard to find, you want names that give the meaning of what you are doing, and not how you are doing it.

For instance first_num is not as good as season, or season_number, because it gives less information.

This is true for functions names too. The result of your random_number_generator is a string that you will use as a file name. Based on the function's name, you would expect to get an object that generates numbers.

Let's rename it to random_file_name for instance.

This would make us write this function

def random_file_name
  season = rand(2..9)
  episode = rand(1..25)
  "#{two_chars(season)}#{two_chars(episode)}"
end

Did I mention it would be good to extract the logic of turning a number into a 2 characters string?

Let's create this two_chars function

def two_chars(number)
  number.to_s.rjust(2, '0')
end

Calling two_chars(1) will take 1, convert it to a string with to_s and fill the string with 0s until the string's size is 2. This way we won't need this test second_num < 10.

Episode Picker

In this function you are looping through all files in your folder; there is a more efficient way of doing it.

Plus you are calling random_num_generator several times in the same loop, and it will generate different filenames. This is probably not what you want: it could not find any file, or find several different files.

What you want is something looking like

def pick_episode
  file = file_with_name(random_file_name)
  puts "You are watching Season: #{filename[0..1]} Episode: #{filename[2..3]}!"

  system %{open "/GitHub/videos/#{file}"}
end

I changed the function's name to pick_episode since that what it does.
What we do is find a random file, display that we are watching it, and open it.

Since you ask what is in this file_with_name function, here is how you could write it

def file_with_name(filename)
  files = Dir["/GitHub/videos/#{filename}.{mkv,mp4,avi}"]
  raise "No file named #{filename}" if files.empty?
  raise "Several files: #{files}" if files.size > 1

  files.first
end

Instead of looping through all files in the folder, we try to find a file that starts with the right name, and that has an extension amongst mkv, mp4 and avi.

Then if no file is found, we raise an error.
If several files share the same prefix, we raise an error.

In the case we found exactly one file, we return its name.

If you find good names for your variables and functions, and you try to break your code in smaller pieces (functions), you will be able to look at it in 2 months and understand it clearly.

You could even go further and use some classes to have be even more explicit with the season and the episode numbers: for now, we compute them, turn them into a string, and extract them from a string later on when displaying it. But I think it would be a bit out of scope here. Tell me if you want me to go further this path.

Well, that is it. Asking questions here is a good way to improve your coding skills, the rest is just practice!


Finding (and counting) duplicate JS/Java files I have the following script which takes minutes to give output.

printf "\nDuplicate JS Filenames...\n"
(
  find . -name '*.js' -type f -exec basename {} \; | sort | uniq -c | grep -v "^[ \t]*1 ";
  echo "$(find . -type f -name '*.js' | wc -l) JS files in search directory";
  echo "$(find . -name '*.js' -type f -exec basename {} \; | sort | uniq -c | grep -v "^[ \t]*1 " | wc -l) duplicates found";
)

printf "\nDuplicate Java Filenames...\n"
(
  find . -name '*.java' -type f -exec basename {} \; | sort | uniq -c | grep -v "^[ \t]*1 ";
  echo "$(find . -type f -name '*.java' | wc -l) Java files in search directory";
  echo "$(find . -name '*.java' -type f -exec basename {} \; | sort | uniq -c | grep -v "^[ \t]*1 " | wc -l) duplicates found";
)

I know that I make the same request, or similar ones, a couple of times.

How could I optimize this, and maybe already the base command - I'm surprised that find takes so long, or is it due to sort, uniq, and grep?

Aside from running essentially the same find command three times, the main issue is that you run a separate basename instance for every single found file.

If you are using GNU find (verify with find --version), you can get find to print the basenames directly:

find . -name '*.js' -type f -printf '%f\n'

On my system this is about 900  times faster than calling basename when run on a directory with about 200,000 files in it.

If your system does not come with GNU find (e.g. MacOS, OpenBSD, FreeBSD) and you do not want to install it (the package is usually called findutils), you can use sed to do the same as basename but for all found files at once:

find . -name '*.js' -type f | sed 's@.*/@@'

On my system this is only slightly slower than using -printf.

If you want to reduce the amount of times you run find, you can just save the output in a variable:

filelist="$(find . -name '*.js' -type f -printf '%f\n' | sort)"
echo "$filelist" | uniq -c | grep -v "^[ \t]*1 ";
echo "$(echo "$filelist" | wc -l) JS files in search directory";
echo "$(echo "$filelist" | uniq -c | grep -v "^[ \t]*1 " | wc -l) duplicates found"

Note that on bash you need to put double-quotes around $filelist so that the newlines are not squashed.


JS Simple Image Slider I'm currently learning some javascript and I've been getting up to scratch with ES6. I have learned the basics and continued to build things that I've wanted to build in Javascript for a long time.

I am looking for someone to review the code, and to hopefully aid me in becoming a better developer in learning from mistakes.

So the image slider is working from an array which is a div with 4 hidden img elements. The is an img element which is being created within the div #image-display id. The JS then swaps the src attribute based off the number within the counter and the key from the array.

The working copy is on codepen: https://codepen.io/syncoed/pen/WmpEQJ

HTML

        &#10096;
        &#10097;

CSS

 #image-display{
 width:500px;
 height: 100%;
 border: 3px solid orange;
 z-index: -1;
 padding:3;
 margin:0;
}

#img-size{
 height:100%;
 width:100%;
 z-index: -1;
 margin:0;
 position: relative;
}

/* left arrow */
#left{
 z-index: 0;
 position: fixed;
 left:21.5%;
 top:10%;
 cursor: pointer;
 color:#ffffff;
 padding:0;
}

/* right arrow */
 #right{
 z-index: 0;
 position: fixed;
 right:53.5%;
 top: 10%;
 cursor: pointer;
 color:#ffffff;
 padding:0;
}

/*Desktop Media Queries*/

@media(min-width: 1573px) {

#right{
    right:35%;
    top: 12%;
}

#left{
    left:35.5%;
    top:12%;
 }
}

@media(min-width: 1613px) {

#right{
    right:35.5%;
    top: 11%;
}

#left{
    left:35.5%;
    top:11%;
 }
}

@media(min-width: 1140px) {

#right{
    right:37.5%;
    top: 11%;
}

#left{
    left:37.5%;
    top:11%;
 }
}

Javascript

 const imgDisplay = document.querySelector('#image-display');
 const allImages = document.querySelector('#img-container');

 //gets all images
 let all = allImages.querySelectorAll('img');

var imageContainer = document.querySelector('.selector');
let counter = 0;
var newImg = document.createElement('img');
newImg.setAttribute('id', 'img-size');

newImg.setAttribute('src', all[0].src);
imgDisplay.appendChild(newImg);

let imageSelected = all[counter];

imgDisplay.addEventListener('click', (e) => {

  if (counter == all.length - 1 && e.target.id == "right") {

    newImg.setAttribute('src', all[0].src);
    counter = 0;

 } else if (counter == 0 && e.target.id == 'left') {

    counter = all.length - 1;
    newImg.setAttribute('src', all[counter].src);

 } else {

    if (e.target.tagName == 'SPAN' && e.target.id == 'right') {
        counter++;

        newImg.setAttribute('src', all[counter].src);
    }

    if (e.target.tagName == 'SPAN' && e.target.id == 'left') {

        counter--;
        newImg.setAttribute('src', all[counter].src);

    }
 }

});

Explanation:

If you're going to take the source of an image and set it to another image, why even bother having those images as HTML? It would be much simpler to simply store the image urls in an array like so:

const imageUrls = [
  "https://i.ytimg.com/vi/E6CYI3Xb1tQ/maxresdefault.jpg",
  "https://upload.wikimedia.org/wikipedia/commons/9/9b/Dragon_Ball_Z_Logo.png",
  "https://encrypted-tbn0.gstatic.com/images?q=tbn:ANd9GcQDWVk-v3DR5j0pHabogu6i3n1KLhqJWNd-Z54qevISiCoHKF88",
  "https://media.comicbook.com/2017/02/vegeta-dragon-ball-z-235088-1280x0.png"
];

Secondly, you're using a counter variable and an imageSelected variable... but their end goal is the same thing. Stick with the counter.

You're spending a lot of time writing conditions, why not use % (modulo) to determine which is the next image.

When the right button is clicked simply do:

counter = (counter + 1) % imageUrls.length

This way, once you get to the last image (counter = 3) and wish to go to the next one, then (counter + 1) = (3+1) = (4). So the new position is 4 however the imageUrl length is 4.. and 4 % 4 = 0. So, it's an easy solution to reset the counter to 0 when it gets to the end of the image url list.

However, this doesn't work when you wish to go left. Because module doesn't work on negative numbers. To solve this do the following.

counter = (counter + -1 + imageUrls.length) % imageUrls.length

Now, to figure out if it's 1 or -1 and to avoid setting a condition for it, we're going to store those values in the data attributes in the left and right HTML elements like so:

&#10096;
&#10097;

Then simply to access them in the event callback:

Number(this.dataset.direction) //returns a string so reason why use Number to convert it.

Now, all there is left to do is to complete the formula.

counter = (counter + Number(this.dataset.direction) + imageUrls.length) % imageUrls.length;

Avoid setting the onclick function to a container and actually set it to the elements that need to be clicked.

imgDisplay.querySelectorAll("#left, #right").forEach(element=>{
  //first time this is called element = left
  //second time this is called element = second
  //for each element add an event listener.
});

Solution:

const imageUrls = [
  "https://i.ytimg.com/vi/E6CYI3Xb1tQ/maxresdefault.jpg",
  "https://upload.wikimedia.org/wikipedia/commons/9/9b/Dragon_Ball_Z_Logo.png",
  "https://encrypted-tbn0.gstatic.com/images?q=tbn:ANd9GcQDWVk-v3DR5j0pHabogu6i3n1KLhqJWNd-Z54qevISiCoHKF88",
  "https://media.comicbook.com/2017/02/vegeta-dragon-ball-z-235088-1280x0.png"
];

const imgDisplay = document.getElementById('image-display');

let counter = 0;
const img = new Image();
img.src = imageUrls[counter];
imgDisplay.appendChild(img);

imgDisplay.querySelectorAll("#left, #right").forEach(element=>{
  element.addEventListener('click', function(e){
      counter = (counter + Number(this.dataset.direction) + imageUrls.length) % imageUrls.length;
      img.src = imageUrls[counter];
  });
});

Working Example:

const imageUrls = [
  "https://i.ytimg.com/vi/E6CYI3Xb1tQ/maxresdefault.jpg",
  "https://upload.wikimedia.org/wikipedia/commons/9/9b/Dragon_Ball_Z_Logo.png",
  "https://encrypted-tbn0.gstatic.com/images?q=tbn:ANd9GcQDWVk-v3DR5j0pHabogu6i3n1KLhqJWNd-Z54qevISiCoHKF88",
  "https://media.comicbook.com/2017/02/vegeta-dragon-ball-z-235088-1280x0.png"
];

const imgDisplay = document.getElementById('image-display');

let counter = 0;
const img = new Image();
img.src = imageUrls[counter];
imgDisplay.appendChild(img);

imgDisplay.querySelectorAll("#left, #right").forEach(element=>{
  element.addEventListener('click', function(e){
      counter = (counter + Number(this.dataset.direction) + imageUrls.length) % imageUrls.length;
      img.src = imageUrls[counter];
  });
});
#image-display {
  width: 500px;
  height: 100%;
  border: 3px solid orange;
  z-index: -1;
  padding: 3;
  margin: 0;
}

#img-size {
  height: 100%;
  width: 100%;
  z-index: -1;
  margin: 0;
  position: relative;
}

/* left arrow */

#left {
  z-index: 0;
  position: fixed;
  left: 21.5%;
  top: 10%;
  cursor: pointer;
  color: #ffffff;
  padding: 0;
}

/* right arrow */

#right {
  z-index: 0;
  position: fixed;
  right: 53.5%;
  top: 10%;
  cursor: pointer;
  color: #ffffff;
  padding: 0;
}

/*Desktop Media Queries*/

@media(min-width: 1573px) {
  #right {
    right: 35%;
    top: 12%;
  }
  #left {
    left: 35.5%;
    top: 12%;
  }
}

@media(min-width: 1613px) {
  #right {
    right: 35.5%;
    top: 11%;
  }
  #left {
    left: 35.5%;
    top: 11%;
  }
}

@media(min-width: 1140px) {
  #right {
    right: 37.5%;
    top: 11%;
  }
  #left {
    left: 37.5%;
    top: 11%;
  }
}

  <meta charset="utf-8">
  <meta http-equiv="X-UA-Compatible" content="IE=edge">
  <title></title>
  <meta name="viewport" content="width=device-width, initial-scale=1">

  <link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css" integrity="sha384-ggOyR0iXCbMQv3Xipma34MD+dH/1fQ784/j6cY/iJTQUOhcWr7x9JvoRxT2MZw1T" crossorigin="anonymous">

      &#10096;
      &#10097;


Project Euler Problem 3: Largest prime factor The problem statement is as follows. I feel as if my code can be improved but I'm not able to see how to do so. Is there merit in removing {} on the if blocks? Since I'm new, could that unintentionally lead to bugs if I forget that only a single instruction can be executed?

The prime factors of 13195 are 5, 7, 13 and 29.
What is the largest prime factor of the number 600851475143 ?

class Program
{
    static void Main(string[] args)
    {
        Console.WriteLine(LargestPrimeFactor(600851475143));
        Console.ReadLine();
    }

    static long LargestPrimeFactor(long number)
    {
        long largestFactor = 1;

        for (long i = 2; i<= Math.Sqrt(number); i++)
        {
            if (IsFactorOf(number, i))
            {
                if (IsPrime(i))
                {
                    largestFactor = i;
                }

                while (number > 1 && IsFactorOf(number, i))
                {
                    number /= i;
                }
            }
        }

        if (number == 1)
        {
            return largestFactor;
        }
        else
        {
            return number;
        }

    }

    static bool IsFactorOf(long numerator, long denominator)
    {
        return numerator % denominator == 0 || numerator == denominator;
    }

    static bool IsPrime(long value)
    {
        int maxPossibleRoot = (int)Math.Sqrt(value);
        for (int i = 2; i <= maxPossibleRoot; i++)
        {
            if (value % i == 0)
            {
                return false;
            }
        }
        return true;
    }
}

Is there merit in removing {} on the if blocks?

If you are the kind of person who is likely to write bugs without the braces, then leave them in.

Other notes:

static long LargestPrimeFactor(long number)

Good naming.

Long will do you for the first few Euler problems but you might have to install System.Numerics and use BigInteger for some problems coming up soon.

long largestFactor = 1;

More good naming.

for (long i = 2; i<= Math.Sqrt(number); i++)

i is not descriptive.  possibleFactor perhaps?

i <= Math.Sqrt(number) is much slower than i * i <= number.

static bool IsFactorOf(long numerator, long denominator)
{
    return numerator % denominator == 0 || numerator == denominator;
}

Why numerator and denominator? There are no fractions.

What is the point of the right side of the ||? How can !(n%d==0) and (n==d) both be true?

static bool IsPrime(long value)
{

Your implementation is adequate now, but will be too slow for later problems.  Think about how you might optimize this. For example: if you've already checked divisibility by 2, then why are you checking 4, 6 and 8?

But more generally: Under what circumstances in your existing implementation is IsPrime ever false?  Suppose you are factorizing 840.

You start by throwing out the twos -- a prime -- and get down to 105.  Then you throw out the threes -- a prime -- and get down to 35. Then you check fours -- why? You've already checked twos, so four cannot be a factor.  Then you check fives, you have a factor, and surprise, 5 is a prime.

Your algorithm only finds prime factors because it has already thrown out all the composite factors who have smaller factors! So you don't have to check for primality at all in your algorithm.

Finally: you don't need it for this problem, but for later problems you will benefit greatly from having a method:

static IEnumerable<long> PrimeFactors(long number)

Can you implement such a method? If you can, then you can reimplement

static long LargestPrimeFactor(long number)
{
  return PrimeFactors(number).Max();
}

and you're done. In Euler problems it often pays to solve a more general problem, and then write a query against the more general solution.


Effecient Use Sieve of Eratosthenes algorithm Given this as a the Problem Statement

  Given two numbers, find the sum of prime numbers between them, both
  inclusive.

  Input:

  The first line contains the number of test cases T. Each test case
  contains two space separated integers.

  Output:

  Print the answer on a new line for each case.

  Constraints:

  1 <= T <= 100

  1 <= a,b < 10^6

. Here is the working code of mine, that has been accepted.

class TestClass {

    public static void main(String args[] ) throws Exception {
        BufferedReader keyboard= new BufferedReader(new InputStreamReader(System.in));
        int t=Integer.parseInt(keyboard.readLine());

        while(t>0 && t<=100){
            String[] tempInt=keyboard.readLine().split(" ");
            int n=Integer.parseInt(tempInt[0]);
            int m=Integer.parseInt(tempInt[1]);

            int sum=primeSum(n,m);
            System.out.println(sum);
            t--;
        }
    }

private static int primeSum(int n, int m) {
        int sum=0;
        int maxFactor= (int)Math.sqrt(m);
        boolean[] isPrime=new boolean[m + 1];
        int len=isPrime.length;
        Arrays.fill(isPrime,true);
        isPrime[0]=false;
        isPrime[1]=false;
        for(int i=0;i<=maxFactor;i++){
            if(isPrime[i]){
                for(int j=i+i;j<len;j+=i){
                    isPrime[j]=false;
                }
            }
        }
        for(int i=n;i<=m;i++){
            if(isPrime[i]){
                sum=sum+i;
            }
        }
        return sum;
    }

}

But what is really bugging me is this line boolean[] isPrime=new boolean[m + 1];. Where i Have to use really large block of space. How could i avoid such large allocation while being effiecient too.? What are futher way of improvement of this code apart from array allocation ?

Ways to decrease the allocation
Since your main question was about the allocation length, I'll suggest two ways to decrease the allocation (although 1 MB really isn't that much to begin with).

Since the only even number that is prime is 2, you can only allocate half the amount by only considering odd numbers.  When you perform the sieve, you skip all even numbers and use index [j/2] for each odd number.  Just remember at the end to count 2 as one of the primes if the range includes it.

You can use 1 bit per number instead of 1 boolean per number.  You can use BitSet or your own bit array.

Combining both of the above allows you to allocate 1/16 the amount that you originally allocated (so about 64KB instead of 1MB).  I don't think it will help at all though.  If anything, your program will only get slower.
Reuse the allocation
Also, since you are running up to 100 test cases, you might as well just run the sieve once on the maximum input (10^6).  Then you can compute the 100 answers quickly without redoing all the work each time.


Recursive flatten with lazy evaluation iterator I'm trying to rewrite this:

def flatten(lst):
    flat = []
    for x in lst:
        if hasattr(x, '__iter__') and not isinstance(x, basestring):
            flat.extend(flatten(x))
        else:
            flat.append(x)
    return flat

In [21]: a=[1, [2, 3, 4, [5, 6]], 7]

In [22]: flatten(a)
Out[22]: [1, 2, 3, 4, 5, 6, 7]

..into a version that would flatten the iterable, but return values in a lazy manner. Now, this works:

def flat_fugly(s):
    if iterable(s):
        for x in s:
            yield chain.from_iterable(flat_fugly(x))
    else:
        yield takewhile(lambda x: True, [s])

list(islice(chain.from_iterable(flat_fugly(a)), 0, 6))
Out[34]: [1, 2, 3, 4, 5, 6]

But, as the name suggests... Is there a cleaner/better way?

Not to mention that I have to apply chain.from_iterable to flat_fugly anyway while I'd prefer to have plain iterator (I could wrap it in another function that would use chain.from_iterable of course, but still if it could be all made to fit in one more elegant function that would be preferable).

Not my idea, but I think this is better:

import collections

def flatten(lst):
    for item in lst:
        if isinstance(item, collections.Iterable) and not isinstance(item, basestring):
            for sublst in flatten(item):
                yield sublst
        else:
            yield item


Graph and Node classes with BFS and DFS functions I've created a Node class and a Graph class

class Node:
    def __init__(self, val):
        self.val = val
        self.edges = []

class Graph:
    def __init__(self, nodes=[]):
        self.nodes = nodes

    def add_node(self, val):
        newNode = Node(val)
        self.nodes.append(newNode)

    def add_edge(self, node1, node2):
        node1.edges.append(node2)
        node2.edges.append(node1)

I have also added functions to the Graph class for performing breadth first search and depth first search on a given graph.

def bfs(self):
    if self.nodes is None:
        return []
    visited, toVisit = [], [self.nodes[0]]
    while toVisit:
        node = toVisit.pop()
        visited.append(node)
        print(node.val)
        for nd in node.edges:
            if nd not in visited and nd not in toVisit:
                toVisit.insert(0,nd)
    return visited

def dfs(self):
    if self.nodes is None:
        return []
    visited, toVisit = [], [self.nodes[0]]
    while toVisit:
        node = toVisit.pop()
        visited.append(node)
        print(node.val)
        for nd in node.edges:
            if nd not in visited and nd not in toVisit:
                toVisit.append(nd)
    return visited

Here is an example implementation

graph = Graph()
graph.add_node(5)
graph.add_node(3)
graph.add_node(8)
graph.add_node(1)
graph.add_node(9)
graph.add_node(2)
graph.add_node(10)

#            2
#           /
# 5 - 3 - 8 -  9 - 10
#  \    /
#     1

graph.add_edge(graph.nodes[0], graph.nodes[1])
graph.add_edge(graph.nodes[0], graph.nodes[3])
graph.add_edge(graph.nodes[1], graph.nodes[2])
graph.add_edge(graph.nodes[0], graph.nodes[1])
graph.add_edge(graph.nodes[2], graph.nodes[3])
graph.add_edge(graph.nodes[2], graph.nodes[5])
graph.add_edge(graph.nodes[2], graph.nodes[4])
graph.add_edge(graph.nodes[4], graph.nodes[6])

graph.dfs()
graph.bfs()

The depth first search returns 5,1,8,9,10,2,3

The breadth first search returns 5,3,1,8,2,9,10

From what I can tell, this is a correct implementation.  However, I'm curious if there are more efficient ways to do some of these things.  Or maybe ways that make more logical sense.  For example, am I storing the edge list in a reasonable way?  Is this generic enough that it could easily be extended to work with directed vs undirected graphs?  Any feedback would be much appreciated.

Algorithm

Both your BFS and DFS run in time \$\mathcal{O}(EV)\$ due to this:

if nd not in visited and nd not in toVisit:

Since toVisit is a list, finding whether nd not in toVisit will have to iterate over all elements in that list. Since the above if will be iterated around \$\Theta(E)\$ times, and len(toVisit) \$\leq |V|\$, the total work may be as large as \$\mathcal{O}(EV)\$.

What you could do above in order to

if nd not in visited and nd not in toVisit:

in constant time, is to add a set that stores the graph nodes already reached by the search. In order for that to happen, you have to add a couple of special methods to your Node class:

class Node:
    def __init__(self, val):
        self.val = val
        self.edges = []

    # Do this and other represent the same node?
    def __eq__(self, other):
        return self.val == other.val

    # Used for finding the collision chain for this node.
    def __hash__(self):
        return self.val

Note that set() is implemented as a hash table that runs both insertion and query operations in constant time.

Also, I strongly suggest that you do not print to standard output from an algorithm. Instead, arrange an output from the algorithm that may be printed.

Also, taking a look at bfs, there is an improvement opportunity as well: you use a list for representing the search frontier queue. Appending is efficient, yet removing the head node of that "queue" runs in linear time. Instead, use collections.deque(); it allows both pops and pushes in constant time. [1] https://wiki.python.org/moin/TimeComplexity

Naming

Python suggest new_node instead of newNode. Same applies to toVisit.

Misc

The test if self.nodes is None: may be rewritten more succintly:

if not self.nodes:

The above will deal with self.nodes == None and len(self.nodes) == 0.

Summa summarum

All in all, I had this in mind:

from collections import deque

class Node:
    def __init__(self, val):
        self.val = val
        self.edges = []

    def __eq__(self, other):
        return self.val == other.val

    def __hash__(self):
        return self.val

class Graph:
    def __init__(self, nodes=[]):
        self.nodes = nodes

    def add_node(self, val):
        new_node = Node(val)
        self.nodes.append(new_node)

    def add_edge(self, node1, node2):
        node1.edges.append(node2)
        node2.edges.append(node1)

    def bfs(self):
        if not self.nodes:
            return []
        start = self.nodes[0]
        visited, queue, result = set([start]), deque([start]), []
        while queue:
            node = queue.popleft()
            result.append(node)
            for nd in node.edges:
                if nd not in visited:
                    queue.append(nd)
                    visited.add(nd)
        return result

    def dfs(self):
        if not self.nodes:
            return []
        start = self.nodes[0]
        visited, stack, result = set([start]), [start], []
        while stack:
            node = stack.pop()
            result.append(node)
            for nd in node.edges:
                if nd not in visited:
                    stack.append(nd)
                    visited.add(nd)
        return result

graph = Graph()
graph.add_node(5)
graph.add_node(3)
graph.add_node(8)
graph.add_node(1)
graph.add_node(9)
graph.add_node(2)
graph.add_node(10)

#            2
#           /
# 5 - 3 - 8 -  9 - 10
#  \    /
#     1

graph.add_edge(graph.nodes[0], graph.nodes[1])
graph.add_edge(graph.nodes[0], graph.nodes[3])
graph.add_edge(graph.nodes[1], graph.nodes[2])
graph.add_edge(graph.nodes[0], graph.nodes[1])
graph.add_edge(graph.nodes[2], graph.nodes[3])
graph.add_edge(graph.nodes[2], graph.nodes[5])
graph.add_edge(graph.nodes[2], graph.nodes[4])
graph.add_edge(graph.nodes[4], graph.nodes[6])

dfs_result = graph.dfs()
bfs_result = graph.bfs()
print("DFS")
for i in range(len(dfs_result)):
    print(dfs_result[i].val)
print("BFS")
for i in range(len(bfs_result)):
    print(bfs_result[i].val)

Hope that helps.


Removing duplicates from an array quickly Array.prototype.unique = function() {
    var a = [], // uniques get placed into here
        b = 0; // counter to test if value is already in array 'a'

    for ( i = 0; i < this.length; i++ ) {
        var current = this[i]; // get a value from the original array

        for ( j = 0; j < a.length; j++ ) { // loop and check if value is in new array
            if ( current != a[j] ) {
                b++; // if its not in the new array increase counter
            }
        }

        if ( b == a.length ) { // if the counter increased on all values
                               // then its not in the new array yet
            a.push( current ); // put it in
        }

        b = 0; // reset counter
    }

    this.length = 0; // after new array is finished creating delete the original array
    for ( i = 0; i < a.length; i++ ) {
        this.push( a[i] ); // push all the new values into the original
    }

    return this; // return this to allow method chaining
}

I'm expecting this to be a slow checker especially because there is no sorting first.  I'm interested in improving my sorting abilities etc so I thought I would get an early review.

You can use the indexOf function to check if a value exists in an array. This would simplify your code greatly:

Array.prototype.unique = function() {
    var a = [];
    for ( i = 0; i < this.length; i++ ) {
        var current = this[i];
        if (a.indexOf(current) < 0) a.push(current);
    }

    this.length = 0;
    for ( i = 0; i < a.length; i++ ) {
        this.push( a[i] );
    }

    return this;
}

At the end you are replacing the array's content. It would be better to not mutate the original array, but to return the new array instead with unique elements:

Array.prototype.unique = function() {
    var a = [];
    for ( i = 0; i < this.length; i++ ) {
        var current = this[i];
        if (a.indexOf(current) < 0) a.push(current);
    }
    return a;
}

The name a is a very poor choice. unique would have been better.

You should declare the loop variable i using let, to limit its scope to the current block, so the code becomes:

Array.prototype.unique = function() {
    var unique = [];
    for (let i = 0; i < this.length; i++) {
        let current = this[i];
        if (unique.indexOf(current) < 0) unique.push(current);
    }
    return unique;
}

Finally, there is a much more elegant solution to this problem, using the reduce function:

Array.prototype.unique = function() {
    return this.reduce(function(accum, current) {
        if (accum.indexOf(current) < 0) {
            accum.push(current);
        }
        return accum;
    }, []);
}

UPDATE (to answer your follow-up question)

If you want the function to take a parameter to decide whether it should modify the original array, you could try something like this:

Array.prototype.unique = function(mutate) {
    var unique = this.reduce(function(accum, current) {
        if (accum.indexOf(current) < 0) {
            accum.push(current);
        }
        return accum;
    }, []);
    if (mutate) {
        this.length = 0;
        for (let i = 0; i < unique.length; ++i) {
            this.push(unique[i]);
        }
        return this;
    }
    return unique;
}


Selecting all movies where specified actors worked together Here's a query I wrote trying to find all movies that three actors have been in together:

select distinct
    Movie
from
    CastMember
where
    Movie in (
        select Movie
        from CastMember
        where Actor = 1
    )
    and Movie in (
        select Movie
        from CastMember
        where Actor = 2
    )
    and Movie in (
        select Movie
        from CastMember
        where Actor = 3
    )

Movie and Actor are FKs to a Movie and Actor table. CastMember just has two columns: Movie and Actor.

Is there a better way to write this?

Update: thinking of just using INTERSECT

select Movie
from CastMember
where Actor = 1
intersect
select Movie
from CastMember
where Actor = 2
intersect
select Movie
from CastMember
where Actor = 3

Will this do the trick?

select m.Title
from Movie m
    inner join CastMember cm on cm.MovieId = m.Id
    inner join Actor a on cm.ActorId = a.Id
where a.Name in ('John Travolta', 'Uma Thurman')
group by m.Title
having count(m.Id) > 1 -- this should be replaced with the number of items in the "in" list minus 1.


Remove sections of content from Quora.com I wrote a Chrome browser extension to hide Top Content posts from Quora, which bother users following too few topics.

A user's feed in Quora.com starts as a list of posts relevant to the user, including a set of "Top Content" posts. As the user scrolls down the page, it loads more posts for them to view.

My extension triggers its initial removal when the DOM loads, and removes all Top Content posts currently visible to the user. This in itself triggers Quora to load in more posts.

My extension responds to the new load by running again and removing the Top Content posts again.

This seems to work well, but I'm concerned about Performance. In some cases, Quora goes through and loads the entire user's feed over a much shorter time, even without action from the user.

Github is available here.

main.js

var QuoraExtension =
{
/* Initialize Quora Extension */
main: function()
{
    QuoraExtension.hideContent();
},

/* Hide Selected Content From Quora */
hideContent: function()
{
    $(document).ready(
        function()
        {
            //Run the clean out as soon as possible
            QuoraExtension.hideTopContent();

            /*
                Now, we add a listener for when the DOM tree is updated,
                when it is, we check for more content to hide.

                Timeouts are used to prevent infinite loops of removals
                Removal of content can sometimes trigger another check,
                so timeouts prevent us from using too much processing
             */
            var hideTopContentTimeout = null;

            document.addEventListener("DOMSubtreeModified", function() {
                if(hideTopContentTimeout) {
                    clearTimeout(hideTopContentTimeout);
                }
                hideTopContentTimeout = setTimeout(QuoraExtension.hideTopContent, 500);
            }, false);
        }
    );
},

/* Hide Top Content sections */
hideTopContent: function()
{
    var $topContent = jQuery("span:contains('Top content on Quora')").closest('.pagedlist_item');
    $topContent.hide();
}
};

// Initialize QuoraExtension if JQuery is available
if(!!jQuery)
{
    QuoraExtension.main();
}

manifest.json

{
"manifest_version": 2,

"name": "Quora Extension by 2NickPick",
"description": "Utilities for using Quora.com",
"version": "0.1",

"permissions": [
    "activeTab"
],
"content_scripts": [
    {
        "matches": ["https://*.quora.com/*"],
        "js": ["lib/jquery/1.7.2/jquery.min.js", "main.js"],
        "runat": "document_start"
    }
]
}

Interesting question,

I would place the bulk of your comments on top, they stop the flow of the reading
I would experiment with $.remove() instead of $.hide();, otherwise you might actually run into memory and performance problems. I am thinking that with $.remove() you might not even need the timeouts any more.
Manifest looks fine
Your indenting is off, use something like jsbeautifier
This is shorter and more readable

$("span:contains('Top content on Quora')").closest('.pagedlist_item').hide();

than

var $topContent = jQuery("span:contains('Top content on Quora')").closest('.pagedlist_item');
$topContent.hide();


Twiddle table computing I'm trying to optimize this twiddle computing function:

void twiddle(int N)
{
  int i;
  for (i=0;i<N;i++)
  {
     twiddle_table[i].re = (float)   cos((float)i * 2.0 * PI /(float)N);
     twiddle_table[i].im = (float) - sin((float)i * 2.0 * PI /(float)N);
  }
}

where N = 4096 size of twiddle table and could be bigger!

And then, I did the following:

void twiddle(int N)
{
   int i;
   float Tconst;
   Tconst = 2.0 * PI /(float)N;
   for (i=0;i<N;i++)
   {
      twiddle_table[i].re = (float)   cos((float)i * Tconst);
      twiddle_table[i].im = (float) - sin((float)i * Tconst);
   }
}

But, I'm getting a performance of 340,000 cycles for the for loop, which I think is bad.

Any hints that could enhance the performance of this function?

The optimization you made manually is most likely already noticed and implemented by the compiler.

Instead, you should notice that \$\cos\frac{2\pi k}{N} - i \sin\frac{2\pi k}{N} = e^{-\frac{2\pi i}{N}k}\$, and replace multiple invocations of sin and cos with multiplications. Naively:

    complex base = { cos(2.0 * PI / N), -sin(2.0 * PI / N) };
    twiddle_table[0] = { 1.0, 0.0 };
    for (i = 1; i < N; ++i) {
        twiddle_table[i] = twiddle_table[i - 1] * base;
    }

In the production code you need to pay attention to the accumulation of numerical errors, and once in a while fall back to direct computation of sin and cos (or cexp).

Of course, if your compiler doesn't support complex type, you's have to implement complex multiplication manually.

Another optimization comes from the inherent symmetry of the table, e.g. \$cos\frac{2\pi k}{N} = \cos\frac{2\pi (N - k)}{N}\$, so you only need to compute half of the table; there are more symmetries you may exploit.

PS: That said, I strongly recommend to precompute it once for largest possible N. Notice that it can be reused for any power of 2.


Python 3 Simple Xor Pipe/CLI Program I wrote a simple Python 3 program that takes data from either a file or standard input and xor encrypts or decrypts the data. By default, the output is encoded in base64, however there is a flag for disabling that --raw. It works as intended except when I am using the raw mode, in which case an extra line and some random data is appended to the output when decrypting xor data.

#!/usr/bin/env python3
from itertools import cycle
import argparse
import base64

import re
def xor(data, key):
    return ''.join(chr(ord(str(a)) ^ ord(str(b))) for (a, b) in zip(data, cycle(key)))

# check if a string is base64 encoded.
def is_base64(s):
    pattern = re.compile("^([A-Za-z0-9+/]{4})*([A-Za-z0-9+/]{4}|[A-Za-z0-9+/]{3}=|[A-Za-z0-9+/]{2}==)$")
    if not s or len(s) < 1:
        return False
    else:
        return pattern.match(s)

if __name__ == '__main__':
    parser = argparse.ArgumentParser()
    parser.add_argument('-i', '--infile', type=argparse.FileType('r'), default='-', dest='data', help='Data to encrypt'
                                                                                'or decrypt')
    parser.add_argument('-r', '--raw', dest='raw', default=False, action='store_true', help='Do not use base64 encoding')
    parser.add_argument('-k', '--key', dest='key', help='Key to encrypt with', required=True)
    args = parser.parse_args()

    data = args.data.read()
    key = args.key
    raw = args.raw
    if raw:
        ret = xor(data, key)
        print(str(ret))
    else:
        if is_base64(data):
            # print('is base64')
            decoded = base64.b64decode(data).decode()
            ret = xor(decoded, key)
            print(ret)
        else:
            # print('is not base64')
            ret = xor(data, key)
            encoded = base64.b64encode(bytes(ret, "utf-8"))
            print(encoded.decode())

When running without the --raw flag, everything performs as intended:

$ echo lol|./xor.py -k 123
XV1fOw==
echo lol|./xor.py -k 123 |./xor.py -k 123
lol

However, if I disable base64, something rather odd happens. It's easier to demonstrate then it is to explain:

$ echo lol|./xor.py -k 123 -r |./xor.py -k 123 -r
lol
8

Does anyone know why I am seeing the character 8 appended to the output of xor decrypted data? I have a c program called xorpipe that I use for this exact use case, and it does not suffer this bug. I wanted to rewrite it in Python.

I am looking for other constructive criticism, suggestions, or reviews as well.  Particular, I would like argparse to be able to determine whether the supplied input is either a file, string, or data piped from standard input. This is easy to accomplish bash or C, but I am not sure how best to do this in Python.

Does anyone know why I am seeing the character 8 appended to the output of xor decrypted data?

The statement echo lol pipes lol\n\r to Python, which encodes the line breaks as ;_ which is decoded into an 8. Unfortunately echo -n doesn't work here but adding .strip() to the input data in the Python script fixes this issue.

PEP8 is Python's internal coding standards which specify how line breaks and code should be structured: https://www.python.org/dev/peps/pep-0008/ . The guide is very long; you can use autopep8 to auto-format the code.

    if raw:
        ret = xor(data, key)
        print(str(ret))
    else:
        if is_base64(data):
            # print('is base64')
            decoded = base64.b64decode(data).decode()
            ret = xor(decoded, key)
            print(ret)
        else:
            # print('is not base64')
            ret = xor(data, key)
            encoded = base64.b64encode(bytes(ret, "utf-8"))
            print(encoded.decode())

I would simplify the nested if-statements and add a return to print(str(ret)) then the is_base64 could be unindented, or I would set a variable called decoded to the final string to print, then print it out at the end of the if/elif loop.

is_base64(s) could just run base64.b64decode(data).decode() and return False if any exceptions were thrown during decoding instead of the regex.

I would remove the commented out code such as # print('is base64').


AdventOfCode Day 1 : find X element matching a criteria in haskell This problem is from https://adventofcode.com/2020/ day 1.
I need to find the N entries of inputs that sum to 2020 and then multiply those N numbers together.
inputs = [1721, 979, 366, 299, 675, 1456]

I have 2 cases N = 2 and N = 3
I have done this with list comprehension
N = 2 :
main = print $ head [ i * j |
                      i <- inputs ,
                      j <- inputs ,
                      i+j == 2020]

N = 3 :
main = print $ head [ i * j * k  |
                      i <- inputs ,
                      j <- inputs ,
                      k <- inputs ,
                      i+j+k == 2020]

But it scales badly, if y need N = 100 that's a lot of <-.
So I have done this :
solution n =  foldl1 (*) $ head [ x | x <- sequence $ replicate n inputs , (foldl1 (+) x == 2020 )]

main = print $ solution 3

I wonder if there is a more idiomatic way to do this and if there is a way of using list comprehension with N variables ?
Note : I know that with these solutions, the same entries can be used multiple times, and I am ok with it.

While the code certainly works, I wouldn't necessarily call it idiomatic. Haskell's safety and robustness stems from its types, and the type signatures are completely missing. Sure, it works fine. However, all integers are Integer by default, a type that has some nice properties (arbitrary large numbers) and some drawbacks (sub par performance compared to Int).
However, to introduce type signatures, we need a function. The third variant introduces a fine candidate: solution.
solution :: Int -> Int
solution n =  foldl1 (*) $ head [ x | x <- sequence $ replicate n inputs , (foldl1 (+) x == 2020 )]

Now that we have a safe foundation and get type errors if we do anything unexpected, let's have a look at the code and the used functions. foldl1 (*) is product and foldl1 (+) is sum, although both alternatives act slightly different on an empty list.. Yes, both functions will use foldr internally, but that is fine. If we are going for strictness, then we want to use foldl1' instead, but that's not necessary with numbers from my experience. Also, sequence . replicate is replicateM (from Control.Monad), and the parentheses around fold1 (+) x == 2020 aren't necessary. Note that hlint will report those changes, too.
So without changing the original algorithm, we end up at
import Control.Monad (replicateM)

inputs :: [Int]
inputs = [ ... ]

solution :: Int -> Int
solution n =  product $ head [ xs | xs <- replicateM n inputs
                                  , sum xs == 2020]

main :: IO ()
main = print $ solution 3

Note that we changed x to xs, as we're not dealing with a single element, but instead with lists. This new variant ticks all the boxes:

it has explicit types on its top-level structures ✔
it uses named variants of folds (product for foldl (*) 1, sum for foldl (+) 0) ✔
it uses typical naming (xs for lists) ✔
it splits functionality into reasonable parts (main and solution) ✔

However, there's nothing wrong with your old variant, as it was semantically equivalent (except for empty intermediate lists). If I was to solve AoC, I would have used a similar script-style Haskell format, except for solution's type; I'd use [Int] -> Int or even [Int] -> Maybe Int instead to interact with map read . lines <$> readFile "inputs". But that's personal preference.
And last but not least: Your choice to use the list monad to easily check all \$n\$ combinations was really nice!


Reading denormalized data using a stateful loop body I'm reading denormalized data with a SqlDataReader, the data has this form:

SELECT
    Objects.ObjectName, Objects.OwnerId, Owners.Name
FROM
    Objects
    INNER JOIN Owners ON Objects.OwnerId = Owners.OwnerId
ORDER BY
    Objects.OwnerId

Which has this output:

OjectName      OwnerId    OwnerName
Foo            1          Mr. Fu
Bar            1          Mr. Fu
Baz            2          Mrs. Daz

My reading loop is this:

using( SqlDataReader rdr = cmd.ExecuteReader() )
{
    Int64 lastOwnerId = -1;
    List<String> objectNames = new List<String>();
    while( rdr.Read() )
    {
        String objName   = rdr.GetString( 0 );
        Int64  ownerId   = rdr.GetInt64 ( 1 );
        String ownerName = rdr.GetString( 2 );

        if( ownerId != lastOwnerId && objectNames.Count > 0 )
        {
            // logic to populate and yield-return new Owner w/ ObjectNames

            objectNames.Clear();
            lastOwnerId = ownerId;
        }
        else
        {
            objectNames.Add( objName );
        }
    }

    if( ownerId != lastOwnerId && objectNames.Count > 0 )
    {
        // logic to populate and yield-return new Owner w/ ObjectNames
    }
}

The logic to populate and yield-return new Owner w/ObjectNames  exists twice. Short of moving it to its own function, is there another loop pattern that is DRY?

In essence your code is currently:

while (rdr.Read()) {
    if (new owner) {
       // do duplicated logic yield stuff
       // Prepare for new owner
    } else {
       // Add owner names
    }
}

if (new owner) {
    // do duplicated logic yield stuff
}

To avoid the duplicated logic, the nicest solution would be to use an extra function, but alternatively you could also change the loop logic into being an eternal loop, and breaking out of it when reaching the end.

In other words something similar to the following:

while (True) {
    if (!rdr.read()) {
        last_record == true;
    }

    if (new owner || last_record ) {
       // do duplicated logic yield stuff
       if (!last_record) {
           // Prepare for new owner
       } else {
          break; // Break out of loop
       }
    } else {
       // Add owner names
    }
}

In this way you call the duplicated logic block both if there are data and you have a new owner, and if you've read the last record. Note that the !rdr.read() will read records even though the if condition is negated afterwards.

Whether you feel this is a better approach or not, is left for you to decide, but it does avoid the duplication of logic you are concerned about.


Reading file and print it in hex format really slow I'm trying to read a file and print its byte in hex form, using the below code. The code those it but i think optimization can be done to make it read and print faster, because if i remove the print the timestamp is 53ms+/-,
but printing, it takes 18 minutes to read a file of 29mb.

import java.io.IOException;
import java.io.RandomAccessFile;
import java.nio.MappedByteBuffer;
import java.nio.channels.FileChannel;
import java.util.concurrent.TimeUnit;

public class cOnvert {
public static void main(String[] args) {
    long startTime = System.nanoTime();
    try {
        try (RandomAccessFile aFile = new RandomAccessFile("filePath", "r")) {
            FileChannel inChannel = aFile.getChannel();
            MappedByteBuffer buffer = inChannel.map(FileChannel.MapMode.READ_ONLY, 0, inChannel.size());
            String set;
            for (int i = 0; i < buffer.limit(); i++) {
                byte read = buffer.get();
                set = (String.format("%02X ", (byte) read));
                System.out.print(set);
            }
        }
    } catch (IOException ioe) {
    }
    long endTime = System.nanoTime();
    long elapsedTimeInMillis = TimeUnit.MILLISECONDS.convert((endTime - startTime), TimeUnit.NANOSECONDS);
    System.out.println("Total elapsed time: " + elapsedTimeInMillis + " ms");
}
}

Thank you.

Writing to the console is incredibly slow.... it also bit me in the past :)

Console is not really meant to print such big data anyway. What's your use case ?

Performance tuning

To reduce the impact on performance, you should try to reduce the number of call to System.out.print(set) to only one, at the end of the run....

You may end with a very big String however, so you should use a StringBuilder instead.

Combining both those things should reduce the time taken by your application by a very noticeable amount and you should end up with something looking like this :

StringBuilder sb = new StringBuilder();
// some more code
try (RandomAccessFile aFile = new RandomAccessFile("filePath", "r")) {
        FileChannel inChannel = aFile.getChannel();
        MappedByteBuffer buffer = inChannel.map(FileChannel.MapMode.READ_ONLY, 0, inChannel.size());
        for (int i = 0; i < buffer.limit(); i++) {
            final byte read = buffer.get();
            sb.append((String.format("%02X ", read)));
            sb.append("\n");
        }
}

To ((very) slighlty) improve performance you may give your StringBuilder a big starting size like this :

StringBuilder sb = new StringBuilder(1_000_000);

Of course, you should prefer a well named constant over this ugly 1000000.

Other things in the code

Now that we are done with the performance aspect :
1. the name aFile is not really good looking IMO.
2. there is two nested try clause within each other but it's useless here.
3. please never user empty catch block :'(

After all those tweaks : it takes my computer 1 minute and a half to read a 28.5 mb file.


Simple bash script for opening a repository in the browser from the command line I'm relatively new to bash scripting, and I'm wondering how I've done on this. Basically it's a script where if I'm in a github or bitbucket repository in my terminal, I can type browse-repository to open it up in the browser. I can also type something like browse-repository issues to visit the issues page, or browse-repository issues 19 to see issue # 19 for the repository.

I alias the command as br to save on typing, and can type things like br i 19 to get to the page for issue # 19.

I'm mainly looking for tips on organization and scope, and of course if there's any other improvements as well. I couldn't figure out any sort of namespacing, so i prefixed commands with _browse_repository

The file as it currently stands is simple sourced from my .bash_profile, if that matters.

#!/bin/bash -e

function browse-repository() {
  local URL="https://$(git config remote.origin.url | sed 's/\.git//' | sed 's/https:\/\///' | sed 's/ssh:\/\///' | sed 's/git:\/\///' | sed 's/git@//' | tr ':' '/')"

  if _browse_repository_is_bitbucket_repo "$URL" || _browse_repository_is_github_repo "$URL"
  then
    case "$1" in
      "")
        URL="$URL"
        _browse_repository_go
        ;;
      i|issues|-i|--issues)
        if [[ -n $2 ]]
        then
          if _browse_repository_is_bitbucket_repo "$URL"
          then
            URL="$URL/issue/$2"
          else
            URL="$URL/issues/$2"
          fi
        else
          URL="$URL/issues"
        fi
        _browse_repository_go
        ;;
      p|pulls|-p|--pulls)
        if _is_bitbucket_repo "$URL"
        then
          URL="$URL/pull-requests"
        else
          URL="$URL/pulls"
        fi
        _browse_repository_go
        ;;
      w|wiki|-w|--wiki)
        URL="$URL/wiki"
        _browse_repository_go
        ;;
      h|help|-h|--help)
        _browse_repository_usage
        ;;
      *)
        _browse_repository_usage
        ;;
    esac
  else
    case "$1" in
      h|help|-h|--help)
        _browse_repository_usage
        ;;
      *)
        echo "Not a valid bitbucket or github repository"
        ;;
    esac
  fi
}

_browse_repository_is_bitbucket_repo() {
  [ "$URL" != "${URL#https:\/\/bitbucket.org\/}" ]
}

_browse_repository_is_github_repo() {
  [ "$URL" != "${URL#https:\/\/github.com\/}" ]
}

_browse_repository_usage() {
  cat <<EOF
usage: browse-repository [<command>] [<number>]

The following commands are performed on the current project.

Available commands:

  -i --issues [<number>] Open issues, optionally pass a number to open a specific issue
  -p --pulls             Open pull requests, optionally pass a number to open a specific pull request
  -w --wiki              Open wiki

When no argument is supplied the main project page will be opened.
EOF
}

_browse_repository_go() {
  open "$URL"
}

Some things:

You don't need the function keyword to define a function.
You can combine sed commands by separating them by ;: sed 's/\.git//;s/https:\/\///;s/ssh:\/\///;s/git:\/\///;s/git@//'
This can be shortened down to a single statement: sed 's/\(\.git\|https:\/\/\|ssh:\/\/\|git:\/\/\|git@\)//'
Fortunately you can get rid of picket fences in sed by using a different separator: sed 's#\(\.git\|https://\|ssh://\|git://\|git@\)##'
I'd use getopt or getopts for option handling. Which one to use is a bit of a religious issue, but getopt supports long options (--help and the like), while getopts is more portable.
It is recommended to use upper case only for variables exported in the shell, and lower case for other variables.
This is maybe more of a personal preference, but rather than creating a function called I'd create a script with the same name and move the contents of the browse-repository function to the end of the script. That way there's no pollution of the function name space, and no need to source the script. Just put it for example at /usr/local/bin/browse-repository and run. An added advantage of this is that you don't need the function name prefixes anymore.
I'm not sure how portable open is. On my system:

$ open http://www.example.org
Couldn't get a file descriptor referring to the console

Alternatively you could use x-www-browser, but I fear that's even less portable (Debian & derivatives only IIRC). I don't know if there's something more portable available.

All in all a very nice script.


I want to run the timer at 2:00 a.m. and shutdown the application I want to run the timer at 2:00 a.m. and shutdown the application.
This is my code:

  var ShutdownTimer = new Timer();
  var currentTime = DateTime.Now;

  int hoursUntilShutdown;

  switch (currentTime.Hour)
  {
    case 1:
      hoursUntilShutdown = 1;
      break;

    case 2:
      hoursUntilShutdown = 0;
      break;

    default:
      hoursUntilShutdown = (24 - currentTime.Hour) + 2;
      break;
  }

  var shutdownTimeSpan = new TimeSpan(0, hoursUntilShutdown, 0, 0);
  ShutdownTimer.Interval = (int)shutdownTimeSpan.TotalMilliseconds;
  ShutdownTimer.Tick += this.OnShutdownTimer;
  ShutdownTimer.Start();

   private void OnShutdownTimer(object sender, EventArgs e)
   {
       Environment.Exit(0);
   }

Any help will be greatly appreciated!

The whole switch can be replaced by

hoursUntilShutdown = (26 - currentTime.Hour) % 24

where % is the remainder of the division, as in C (I don't use C#, so I can only assume it's the same).