Timer convenience class to time code execution for user output I implemented a convenience class to time code execution and print it to the user. This is part of a bigger project that will be distributed to external users. The timer however is only called by other procedures, not by the user directly. I was interested in implementing this myself, so I'm not looking for an external module or similiar. Docstrings are omitted here, the functions are rather straightforward.
from time import perf_counter

class Timer:
    def __init__(self, default_round_digits: int = 5):
        self.default_digits = default_round_digits
        self.start_timer()

    def start_timer(self) -> None:
        self.start_time = perf_counter()
        self.pause_start_time = None
        self.pause_length = 0

    def pause_timer(self) -> None:
        self.pause_start_time = perf_counter()

    def resume_timer(self) -> None:
        if self.pause_start_time is not None:
            self.pause_length += perf_counter() - self.pause_start_time
            self.pause_start_time = None

    @property
    def elapsed_seconds(self) -> float:
        # If timer is paused only consider time up to self.pause_start_time instead of now
        if self.pause_start_time is not None:
            return self.pause_start_time - self.start_time - self.pause_length
        else:
            return perf_counter() - self.start_time - self.pause_length

    def get_timer(self, round_digits: int = None, print_text: str = None, restart: bool = False) -> float:

        if round_digits is None:
            round_digits = self.default_digits

        elapsed_seconds = round(self.elapsed_seconds, ndigits=round_digits)

        if print_text is not None:
            print(f"{print_text}{elapsed_seconds} seconds")

        if restart:
            self.start_timer()

        return elapsed_seconds

    def __str__(self) -> str:
        state = "Running" if self.pause_start_time is None else "Paused"
        return f"{state} Timer at {self.get_timer()} seconds"

I'm still unsure about initializing attributes outside of __init__. I know it's not recommended, but copying start_timer into __init__ seemed like the worse option to me.

Your Timer class contains two timers.

The entire duration timer.
The timer for tracking pauses.

I'd move the core timer code out into a separate class.
And we can see you're starting to repeat yourself.

Initializing a timer shouldn't start the timer.
If I open my timer app should the timer start running straight away?
If I want to create some timers but I don't want the timer to be started your class gives me no options.
However just not calling start_timer in __init__ will allow everyone to use your class however is desired.
If you really really want to start the timer when building the object, you can write a class method.
class Timer:
    @classmethod
    def run(cls, *args, **kwargs):
        self = cls(*args, **kwargs)
        self.start_timer()
        return self

# option 1
timer = Timer()
timer.start_timer()

# option 2
timer = Timer.run()

start_timer restarts the timers.
You should rename the method or make the method do what is expected.

If you call pause_timer twice without calling resume_timer your code acts as if the timer was never paused the first time.

I don't really see the point of making elapsed_seconds a property.

get_timer is doing way too much and violates the SRP principle.

Rounds elapsed timings to a specified amount of decimal places.
Prints the elapsed timings.
Restarts the timer.
Returns the elapsed timings.

You should make a function to round the elapsed timings.
Then leave printing and restarting to the user.

I'd a couple more methods from functionality I'd expect from a timer.

from time import perf_counter

class CoreTimer:
    def __init__(self):
        self.stop()

    def __bool__(self):
        return bool(self.start_time)

    def start(self):
        self.start_time = perf_counter()

    def stop(self):
        self.start_time = 0

    def time_taken(self, now):
        return now - self.start_time

    def elapsed(self):
        return self.time_taken(perf_counter())

class Timer:
    def __init__(self, decimal_places=5):
        self._decimal_places = decimal_places
        self._timer = CoreTimer()
        self._pause = CoreTimer()
        self._pause_length = 0

    def __str__(self):
        state = "Paused" if self._pause else "Running"
        return f"{state} Timer at {self.elapsed_rounded()} seconds"

    @classmethod
    def run(cls, *args, **kwargs):
        self = cls(*args, **kwargs)
        self.start()
        return self

    def start(self):
        if not self._timer:
            self._timer.start()

    def stop(self):
        self._timer.stop()
        self._pause.stop()
        self._pause_length = 0

    def restart(self):
        self.stop()
        self.start()

    def pause(self):
        if not self._pause:
            self._pause.start()

    def resume(self):
        if self._pause:
            self._pause_length += self._pause.elapsed()
            self._pause.stop()

    def elapsed(self):
        if self._pause:
            duration = self._timer.time_taken(self._pause.start_time)
        else:
            duration = self._timer.elapsed()
        return duration - self._pause_length

    def elapsed_rounded(self, decimal_places=None):
        if decimal_places is None:
            decimal_places = self.decimal_places
        return round(self.elapsed(), ndigits=decimal_places)


Update only specific field of specific item (data) in List from YAML I don't like the logic to update [Repository] list here because I need to map whole list to change only specific field of specific node,  I especially don't like that I need to provide all other fields in changing node alike (location x) etc... Major trouble there is that if I will edit data model I will be forced to edit a lot of code where I use it like this.

hashupdate :: String -> String -> IO ()
hashupdate hash rep =
  withConfig $ \ymlx ->
    let ymlprocess = ifSo $ do
        rsdata <- yDecode ymlx :: IO [Repository]
        let ed = map enR rsdata
                        where enR x = if rep == (location x)
                                            then (Repository (location x)
                                                             (branches x)
                                                             (upstream x)
                                                             (enabled x)
                                                             (clean x)
                                                             (post_rebuild x)
                                                             (syncGroup x)
                                                             (Just hash))
                                            else x
        yEncode ymlx ed
    in doesFileExist ymlx >>= ymlprocess

The other definitions used there is here:

data Repository = Repository { location      :: String
                             , branches      :: [String]
                             , upstream      :: String
                             , enabled       :: Maybe Bool
                             , clean         :: Maybe Bool
                             , post_rebuild  :: Maybe [String]
                             , syncGroup     :: Maybe String
                             , hash          :: Maybe String
                             } deriving (Show, Eq)

yEncode :: ToJSON iToJSONable => FilePath -> iToJSONable -> IO()
yEncode fnm dat = do
  let bs = Data.Yaml.encode dat
  BS.writeFile fnm bs

I don't like that I need to provide all other fields in changing node
  alike (location x) etc...

You can "update" a field of a record like so:

where enR x = if rep == location x then x { hash = Just hash } else x

I would recommend against names like fnm, dat, ed, enR, etc. You should also use consistent casing (postRebuild instead of post_rebuild).


String/text manipulation utility class I'd like to get some expert's view on this first piece of code especially in the area of

performance - (e.g. avoidable copies)
good practice (e.g. static members?)

The final goal is the provide some abstractions for text manipulation like cleansing, stop-word removal and tokenization. The code base should grow substantially in the future so a lot more string manipulation methods should be supported. I understand that this should be bundled in a library in the end, however, I'd like to get feedback on the general code style first.
#include <fstream>
#include <iostream>
#include <regex>
#include <sstream>
#include <string>
#include <vector>

class TextManipulator {
 public:
  static std::string replace(std::string &string, const std::string pattern,
                             const std::string toreplace) {
    return std::regex_replace(string, std::regex(pattern), toreplace);
  }

  static std::string toLowerCase(std::string &string) {
    std::locale loc;
    std::string res;
    for (auto elem : string) {
      res += std::tolower(elem, loc);
    }
    return res;
  }

  static std::vector<std::string> split(const std::string &text,
                                        char seperator) {
    std::vector<std::string> tokens;
    std::size_t start = 0, end = 0;
    while ((end = text.find(seperator, start)) != std::string::npos) {
      std::string extr = text.substr(start, end - start);
      if (!extr.empty()) {
        tokens.push_back(extr);
      }
      start = end + 1;
    }
    tokens.push_back(text.substr(start));
    return tokens;
  }
};

int main() {
  std::ifstream input(
      "/example_docs/Pride_and_Prejudice.txt");
  std::stringstream buffer;
  buffer << input.rdbuf();
  std::string text = buffer.str();

  text = TextManipulator::replace(text, "\r\n", " ");
  text = TextManipulator::replace(text, ",", "");
  text = TextManipulator::replace(text, ":", "");
  text = TextManipulator::replace(text, ".", "");
  text = TextManipulator::toLowerCase(text);
  std::vector<std::string> tokens = TextManipulator::split(text, ' ');

  return 0;
}

Don't use a class
There is no need to create a class just to hold a collection of static functions. Instead, just declare the functions in the global namespace. If you want to avoid polluting the global namespace, consider putting them in a namespace instead, like so:
namespace TextManipulation {

std::string replace(...) {
    ...
};

...
}

Don't write unnecessary trivial wrapper functions
There is no need to create TextManipulator::replace(), when all it does is call std::regex_replace with almost exactly the same arguments. Why not call std::regex_replace() directly instead?
The name replace() also hides the fact that pattern is a regular expression.
Consider renaming toLowerCase() to tolower()
It's a bit annoying to have different naming conventions in one program, and since you already have tolower() for single characters, it would be very convenient to reuse that for whole strings as well. So:
std::string tolower(const std::string &str) {
    ...
}

Note: you forget to add const to the function parameter here, as well as for the first parameter of replace(), but you did it correctly for the other string parameters.


Collecting files from a particular directory I'm writing an object oriented package, and one problem I am trying to solve is a class that will get all PHP files in a particular directory and instantiate the classes but not abstracts or interface types.

From an object oriented perspective, is this class SOLID?

<?php namespace Massculator;

class Collector
{
    public function collect($path, $gatewayNamespace)
    {
        $classes = new \RecursiveDirectoryIterator(dirname(__FILE__) . $path);

        foreach(new \RecursiveIteratorIterator($classes) as $file) {
            $class = basename($file);
            $class = str_replace('.php', '', $class);
            if(class_exists($gatewayNamespace . $class)) {
                $constructedClass = $gatewayNamespace . $class;
                if((new \ReflectionClass($constructedClass))->isAbstract())
                    $return[] = new $constructedClass;
            }
        }

        return $return;
    }
}

The class has one method and that method has a couple of tasks:

get files
parse only PHP files
instantiate non abstracts

It seems like this breaks the single responsibility principle, but creating another class simply to do a str_replace seems pointless. What do you think?

README:
First off, I'll just focus on making this one class a tad more generic, and expandable and maintainable and... well, generally more OO. Then, though I'm not going to go into too much detail on the matter, I'll just list a few quick tips of how you could go about separating concerns even more and create an entire File namespace.
Code-review can be harsh, but I do not intend to be hurtful or patronizing. My intentions are simply to convey, to the best of my abilities my views on what might be a preferable approach. I base this on experience, as well as personal preference and. While I try to be as objective as possible, I'm only human, so it stands to reason that the code I suggest might not be to your liking. But the code listed here is untested, written off the top of my head and only serves to illustrate my point.

This class/method definitely could do with a couple of additional methods. For example: I might want to set a path, and then -at various points in time- want to get reflection classes for a given "gateway namespace".
Your code, as it now stands would imply calling the collect method with the same $path argument, which will create an all-new RecursiveDirectoryIterator instance, only to get those files I'm after.

If, however, your class would look like this:

class Collector
{
    protected $iterator = null;
    public function __construct($path = null)
    {
        if ($path) $this->setPath($path);
    }
    public function setPath($path)
    {
        $this->iterator = new \RecursiveDirectoryIterator(
            dirname(__FILE__) . $path
        );
        return $this;
    }
    public function collect($namespace, $path = null)
    {
        $return = array();
        if ($path !== null) $this->setPath($path);
        foreach(new \RecursiveIteratorIterator($this->iterator) as $file)
        {
            $class = str_replace('.php','', basename($file));
        }
    }
}

You allow users to set the base-path when creating the instance, and then use that instance to get whatever reflection classes they will be needing at any given moment.
Moreover, you can expand on this "theme" even more, by allowing them to set a property that you can use instead of using dirname(__FILE__), which -if we're honest- isn't really improving the reusability of your current code.

I also struggle to see the point in a method that only allows reflection on abstract classes only:

if((new \ReflectionClass($constructedClass))->isAbstract())

Why not create reflection classes for all matched files, and assign them to a property, so you can then use a getter where the user can specify which classes he/she needs?

class Collector
{
    const COLLECT_ABSTRACT = 0;
    const COLLECT_REGULAR = 1;
    const COLLECT_BOTH = 2;
    //properties:
    $iterator = null;
    $collection = array();
    public function getCollection($gatewayNS, $which = self::COLLECT_ABSTRACT)
    {
        if (!isset($this->collection[$gatewayNS]))
            $this->getNS($gatewayNS);
        if (!isset($this->collection[$gatewayNS][$which]))
            throw new \InvalidArgumentException('invalid offset (0,1,2 are allowed)');
        return $this->collection[$gatewayNS][$which];
    }

    private function getNS($ns)
    {
        $this->collection[$ns] = array(
            self::COLLECT_ABSTRACT => array(),
            self::COLLECT_REGULAR  => array(),
            self::COLLECT_BOTH     => array()
        );
        foreach(new \RecursiveIteratorIterator($this->iterator) as $file)
        {
            $class = $ns.str_replace('.php','', basename($file));
            if (class_exists($class))
            {
                $class = new \ReflectionClass($class);
                $this->collections[$ns][self::COLLECT_BOTH] = $class;
                if ($class->isAbstract())
                    $this->collection[$ns][self::COLLECT_ABSTRACT] = $class;
                else
                    $this->collection[$ns][self::COLLECT_REGULAR] = $class;
            }
        }
        return $this->collections[$ns];
    }
}

That should help you on your way to turn this one-method-function-in-OOP-drag thing into a bona-fide class, that you can use in future projects, as well as your current one.
This is, of course, just a start. This class still needs some more work, and actually, could do with some helper classes, too. As I said, I'm not going to deal with this in great detail, but just as a primer/list of suggestions. The first one, though, is not a suggestion, but an appeal:

Code should document itself
Looking at your code, I'm not at all aware of all the other classes you're using, lest I browse through the actual code. Namespaced class files should start with a series of use statements, that tell me what other classes they depend upon. That way, I can get a rough Idea of what classes I can expect to see in the return values, and which objects are going to be valid as arguments, too.
That, and of course: nobody likes to see all those backslashes throughout the code.

Clear Interface is not optional
A clear-cut, consistent interface isn't just a nice-to-have thing. It's vital. If you want to churn out code you can pass on to others, or re-use without having to refactor half of it, you need to stop and think about what data is likely to be passed to your classes
You're iterating directories, in search of files. So yes, strings are likely candidates for arguments. As are SplFileInfo instances, don't you think?

Now, to add support for these instances, you have 2 choices: Either check the type of the argument that is passed ($arg instanceof \SplFileInfo) or create a wrapper.
IMO, the wrapper is the way to go, as it normalizes your API, allows for type-hints and centralizes all code concerning how you deal with file-paths in a couple of classes. If ever the SplFileInfo class changes, you can update those classes and be done with it all, certain that all other code will work fine.

Read up on what the SplFileInfo class can do for you. If you're interested, write a constructor that accepts either an instance of this class or a string, and -if you receive a string- use that to construct an SplFileObject asap.

Collector is a container
If you do set about writing a File-managing lib, this Collector shouldn't contain anything but processed files. It's a data-container, not a data processor.
You should consider writing some file-parsers (or "Adapters"), that each deal with a specific type of file. They can implement various interfaces, like the ParserInterface for the (and I'm just making these names up as I type) DOMFile, PHPFile and YamlFile classes). This signals users that these classes can process a files content.
Another interface could then be CompressorInterface, which might be useful when dealing with gzipped text files.
Of course, all these classes can extend from a generic abstract class FileAdapter or something, that specifies generic read-write methods, and perhaps allows the user to change the mode to binary, read-only, or even stream... Just go wild here :)

Remember, remember: files are files
After all this talk and suggestions of what you can do, it's important to stress that, while you can do a lot, you can take things to far: these are files, and files mean I/O. If you build an API that lends itself to file usage a bit too well, you could end up with people using files as a replacement for DB storage. Don't.
If people might use your API as an alternative to caching, or simple DB's, then you might have over-done it.

Copy with pride, but credit
A lot of open-source projects do offer some classes to read/process files already. There's nothing wrong with looking at those for inspiration. Copy what you need, provided the license permits it. And of course, feel free to contribute, too :)


Configuring MVC 5 project with service layer and DI and UOW In the past I have always called the repositories directly from the controller, but that is a bad practice and now I am implementing a "Business Layer" to my project.

Would I have two UnitOfWorks?  One for the Service and then one for the repositories?

BaseController:

public class BaseController : Controller
{
    protected IUnitOfWorkService UnitOfWorkService;
    private readonly IEmployeeService _employeeService;

    protected BaseController(IUnitOfWorkService unitOfWorkService)
    {
        UnitOfWorkService = unitOfWorkService;
        _employeeService = unitOfWorkService.EmployeeService;
    }
}

HomeController:

public class HomeController : BaseController
{
    private readonly IEmployeeService _employeeService;

    public HomeController(IUnitOfWorkService unitOfWorkService) : base(unitOfWorkService)
    {
        _employeeService = unitOfWorkService.EmployeeService;
    }

    public ActionResult Index()
    {
        var emp = _employeeService.GetEmployee(employeeId);
        ...
    }

    ...
}

EmployeeService:

public class EmployeeService : IEmployeeService
{
    private IEmployeeRepository _employeeRepo;

    public EmployeeService(IUnitOfWork unitOfWork)
    {
        _employeeRepo = unitOfWork.EmployeeRepository;
    }

    public Employee GetEmployee(int employeeId)
    {
        return _employeeRepo.GetEmployee(employeeId);
    }

    ...
}

EmployeeRepository

public class EfEmployeeRepository : EfRepository, IEmployeeRepository
{
    public EfEmployeeRepository(SqlContext context) : base(context) { }

    public Employee GetEmployee(int employeeId)
    {
        var employee = Context.Employees
                    .SingleOrDefault(e => e.EmployeeId == employeeId);

        return employee != null ? employee.ToDomain() : null;
    }
}

Unit of Work

IUnitOfWorkService:

public interface IUnitOfWorkService : IDisposable
{
    IEmployeeService EmployeeService { get; }
}

UnitOfWorkService:

public class UnitOfWorkService : IUnitOfWorkService
{
    private readonly IUnitOfWork _unitOfWork;

    private EmployeeService _employeeService;

    public UnitOfWorkService()
    {
        _unitOfWork = new EfUnitOfWork();
    }

    public IEmployeeService EmployeeService
    {
        get { return _employeeService ?? (_employeeService = new EmployeeService(_unitOfWork)); }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
            _unitOfWork.Dispose();
    }
}

IUnitOfWork:

public interface IUnitOfWork : IDisposable
{
    IEmployeeRepository EmployeeRepository { get; }
}

UnitOfWork:

public class EfUnitOfWork : IUnitOfWork
{
    private readonly SqlContext _context;

    private EfEmployeeRepository _employeeRepository;

    public IEmployeeRepository EmployeeRepository
    {
        get { return _employeeRepository ?? (_employeeRepository = new EfEmployeeRepository(_context)); }
    }

    public EfUnitOfWork()
    {
        _context = new SqlContext();
    }

    public void Save()
    {
        _context.SaveChanges();
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
            _context.Dispose();
    }
}

Register Dependencies

builder.RegisterType<EfUnitOfWork>().As<IUnitOfWork>().InstancePerRequest();
builder.RegisterType<UnitOfWorkService>().As<IUnitOfWorkService>().InstancePerRequest();

public class BaseController : Controller
{
    protected IUnitOfWorkService UnitOfWorkService;
    private readonly IEmployeeService _employeeService;

    protected BaseController(IUnitOfWorkService unitOfWorkService)
    {
        UnitOfWorkService = unitOfWorkService;
        _employeeService = unitOfWorkService.EmployeeService;
    }
}
public class HomeController : BaseController
{
    private readonly IEmployeeService _employeeService;

    public HomeController(IUnitOfWorkService unitOfWorkService) : base(unitOfWorkService)
    {
        _employeeService = unitOfWorkService.EmployeeService;
    }

    public ActionResult Index()
    {
        var emp = _employeeService.GetEmployee(employeeId);
        ...
    }

    ...
}

I don't see a reason here to have private readonly IEmployeeService _employeeService; in the HomeController. It would be better to make the _employeeService variable of the BaseController protected so it can be used by the objects which inherits the BaseController.
Why is the protected IUnitOfWorkService UnitOfWorkService; not readonly ?

Style

Using abbreviations or naming variables is bad practice. Does it hurt to name the variable IEmployeeRepository _employeeRepo like IEmployeeRepository _employeeRepository ?
It does not hurt to use braces {} for single if statements but will make the code less error prone. I would like to encourage you to always use them.


Customer search using LINQ followed by conditional filtering of results I have a search function in my class which checks various class property values for null or certain values and queries the database based on those properties. This how my function looks like now:

 public object searchCustomerList()
    {
        object partner = null;
        try
        {
            using (var rep = new OrderEntities())
            {
                var results = (from par in rep.Partners
                               join term in rep.CustomerTerms on par.TERMID equals term.TERMID
                               select new
                               {
                                   par.ID,
                                   par.COMPANY,
                                   par.CONTACT,
                                   par.PRICECAT,
                                   par.STATE,
                                   par.TERMID,
                                   par.PHONE,
                                   term.TERMDESC
                               });
                if (customerDetails.priceCategory != Prog.allObjectsIndex) {
                    results = results.Where(c => c.PRICECAT == customerDetails.priceCategory);
                }
                if (customerDetails.termID != Prog.allObjectsIndex) {
                    results = results.Where(c => c.TERMID == customerDetails.termID);
                }
                if (customerDetails.customerState != Prog.allObjectsIndex)
                {
                    results = results.Where(p => p.STATE == customerDetails.customerState);
                }
                if (!string.IsNullOrWhiteSpace(customerDetails.companyName))
                {
                    results = results.Where(p => p.COMPANY.Contains(customerDetails.companyName));
                }
                if (!string.IsNullOrWhiteSpace(customerDetails.contactPerson))
                {
                    results = results.Where(p => p.CONTACT.Contains(customerDetails.contactPerson));
                }
                partner = results.AsEnumerable()                                                                        // Execute method call in Linq-to-Objects
                            .Select(x => new                                                                            // The rest of the query is evaluated in memory
                            {
                                x.ID,
                                x.COMPANY,
                                x.CONTACT,
                                PRICECAT=Prog.getPriceCategory()[(int) x.PRICECAT],
                                STATE = Prog.getStatus()[(int)x.STATE],
                                x.TERMDESC
                            }).OrderBy(x => x.COMPANY).ToList();

            }
        }
        catch (Exception ex)
        {
            saveError("Failed to extract customer information. Error Description:" + ex.Message);
        }
        return partner;
    }

From the little that I know about LINQ, this function is ineffective as it first searches the database and then searches subsets of the result.

Is there a more effective way to re-write this function?

Filtering is actually done correctly, using the best and recommended approach. All LINQ queries are executed lazily, that is they are not executed until you start enumerating them. So all those .Where calls actually just register additional filtering of the records which would translate to entries in the where clause in SQL.

One thing to notice - the call to AsEnumerable() method before .OrderBy is potentially ineffective because it forces the ordering to be done on the client side instead of SQL. I would rewrite it as

    results
        .OrderBy(x => x.COMPANY)
        .AsEnumerable()
        .Select(x => new
        {
            x.ID,
            x.COMPANY,
            x.CONTACT,
            PRICECAT=Prog.getPriceCategory()[(int) x.PRICECAT],
            STATE = Prog.getStatus()[(int)x.STATE],
            x.TERMDESC
        }).ToList();


Finding most common contiguous sub-lists in an array of lists Objective: Given a set of sequences ( eg: step1->step2->step3, step1->step3->step5) ) arranged in an array of lists, count the number of times every contiguous sub-lists occur

Where I need your help:

The code below works, but is very slow on my original dataset (100M sequences, with 100 unique steps). Can you help me make this more efficient? For larger datasets, is there a more efficient programming method than just brute force?
My code currently depends on each element in the list being a single character. How can I adapt this code to handle multiple-character elements?

Working code:

from collections import Counter

sequences = [['A','B'],['A','B','B'],['A','C','A','B']]
counts = Counter()

for sequence in sequences:
    input = "".join(sequence)

    for j in range(1,len(input)+1):
        counts = counts + Counter(input[i:i+j] for i in range(len(input)-(j-1)))
        print counts

for x in counts:
    print x,":",counts[x]," times"

1. Write a test case

When working on performance of code, the first thing to do is to make a reproducible test case. We'll need to have your code in a function:

from collections import Counter

def subsequence_counts_1(sequences):
    counts = Counter()
    for sequence in sequences:
        input = "".join(sequence)
        for j in range(1,len(input)+1):
            counts = counts + Counter(input[i:i+j] for i in range(len(input)-(j-1)))
    return counts

and then we need some test data:

import random
import string

def test_data(n, m, choices):
    """Return a list of n lists of m items chosen randomly from choices."""
    return [[random.choice(choices) for _ in range(m)] for _ in range(n)]

So let's try a small example:

>>> data = test_data(50, 50, string.ascii_uppercase)
>>> from timeit import timeit
>>> timeit(lambda:subsequence_counts_1(data), number=1)
102.42408156394958

2. Don't build data structures by repeated addition

The main problem with the code is this line:

counts = counts + Counter(...)

which has essentially the same effect as:

new_counts = counts + Counter(...)
counts = new_counts

That is, it creates a new Counter object and populates it with the counts from both of the addends. In particular, this involves copying across the whole contents of counts into the new object.

We can avoid all that copying by using the update method:

def subsequence_counts_2(sequences):
    counts = Counter()
    for sequence in sequences:
        input = "".join(sequence)
        for j in range(1,len(input)+1):
            counts.update(input[i:i+j] for i in range(len(input)-(j-1)))
    return counts

and this is a thousand times faster:

>>> timeit(lambda:subsequence_counts_2(data), number=1)
0.08853063406422734

 3. Further improvements

There are few more minor improvements that we could make:

Instead of having separate iterations over i and j, we can iterate over both at the same time using itertools.combinations.
Instead of calling Counter.update for each sequence, we could do all the work in one comprehension.

This results in the following:

from itertools import combinations

def subsequence_counts_3(sequences):
    return Counter(seq[i:j]
                   for seq in map(''.join, sequences)
                   for i, j in combinations(range(len(seq) + 1), 2))

which is about 60% faster than subsequence_counts_2:

>>> timeit(lambda:subsequence_counts_3(data), number=1)
0.052610712591558695

But it's still not going to be able to solve your problem in a reasonable amount of time:

>>> data2 = test_data(100, 100, string.ascii_uppercase)
>>> timeit(lambda:subsequence_counts_3(data2), number=1)
0.5441978382878006

So to process 100 million sequences of 100 characters would take more than half a million seconds, which is more than six days.

4. Using tuples

If you want to handle other kinds of data, convert the sequences to tuples:

def subsequence_counts_4(sequences):
    return Counter(seq[i:j]
                   for seq in map(tuple, sequences)
                   for i, j in combinations(range(len(seq) + 1), 2))

and then you can use any hashable items:

>>> data3 = test_data(50, 50, list(range(10)))
>>> subsequence_counts_4(data3)[(8, 8, 8)]
5


Group-wise deviations from means / centering with dplyr vs. data.table I am trying in R to "center" a matrix (i.e. remove mean) group-wise (i.e. remove group means for each variable).

Input is a matrix (including the grouping variable), and output is the same matrix, where columns are now group-wise centered (possibly excluding now grouping variable).

I tried 3 solutions:

Using dplyr: use group_by(cell) and mutate_all(funs(. - mean(.)))
Using data.table: dt[, lapply(.SD, function(x) x - mean(x)), by= cell]
Using data.table: same as above, but in 2 steps: instead of over-writing variables, 1) add mean variables as new columns, 2) then compute differences, splitting the original matrix as two different matrices.

Results in terms of speed are: 3 < 2 < 1. (3) being faster than (2) is surprising (as (2) is just overwriting variables, not adding)... I suspect it could be from the fact that using function(x) x - mean(x) prevents data.table to use an optimised version of the mean function.

My questions are:

Any way I can speed up some of the code?
How can I understand (3) is faster than (2)? Can I get (2) to be faster?

library(tidyverse)
library(data.table)

## function (1)
center_dplyr <- function(x) {
  x %>%
    group_by(cell) %>%
    mutate_all(funs(. - mean(.))) %>%
    ungroup() %>%
    dplyr::select(-cell)
}

## function (2)
center_dt_1 <- function(x) {
  x <- as.data.table(x)
  setkey(x, cell)
  res <- x[, lapply(.SD, function(x) x - mean(x)), by= cell][, -"cell"]
  as.data.frame(res)
}

## function (3)
center_dt_2 <- function(x) {
  x <- as.data.table(x)
  x_names <- colnames(x)[colnames(x) !="cell"]
  x_names_new <-  paste(x_names, "mean", sep="_")

  setkey(x, cell)
  x[,  paste(x_names, "mean", sep="_"):= lapply(.SD, mean, na.rm = TRUE), by = cell]
  res <- x[, x_names, with=FALSE] - x[, x_names_new, with=FALSE]
  as.data.frame(res)
}

## Data
T = 6;
N = 10^4
set.seed(123)
sim_df <- data.frame(A = sample(c(0,1), N * T, replace = TRUE),
                      B1 = sample(c(0,1), N * T, replace = TRUE),
                      B2 = rnorm(N),
                      cell = rep(1:N, each = T))

ans_dplyr <- center_dplyr(x=sim_df)
ans_dt1 <- center_dt_1(sim_df)
ans_dt2 <- center_dt_2(sim_df)

all.equal(ans_dt1, ans_dplyr, check.attributes = FALSE)
all.equal(ans_dt2, ans_dplyr, check.attributes = FALSE)

### Benchmark:

library(microbenchmark)

## small
sim_df_s <- sim_df[1:1000,]
bench_small <-  microbenchmark(ans_dplyr = center_dplyr(sim_df_s),
                           ans_dt1 = center_dt_1(sim_df_s),
                           ans_dt2 = center_dt_2(sim_df_s),
                           times = 10)

bench_large <- microbenchmark(ans_dplyr = center_dplyr(sim_df),
                              ans_dt1 = center_dt_1(sim_df),
                              ans_dt2 = center_dt_2(sim_df),
                              times = 10)

bench_all <- rbind(bench_small %>% summary %>% mutate(data_size = "small"),
                   bench_large %>% summary %>% mutate(data_size = "large")) %>%
  select(data_size)

bench_all

This answer only goes to how to speed up processing and doesn't get at the intricacies of data.table vs dplyr under the hood.

Below is a slightly faster version (at least for larger datasets).

Rather than create additional columns in the main DT, I created a new DT that had the mean values for each. As that had a 6-fold smaller size, the second DT has to be keyed and then joined back to the original.

This produces a bunch of extra columns (with values from x), so that's why we only selection columns of [,1:length...].  Note that subtraction of two DTs ignores column labels and assumes the same order of all the columns - i.e without the setcolorder, mean_x has "cell" being the first column, which results in the wrong columns being subtracted.

center_dt_3 <- function(x) {
  setDT(x,key="cell")
  x_fullnames <- names(x)
  mean_x <- x[,lapply(.SD,mean, na.rm=TRUE),by=cell]
  setcolorder(mean_x,x_fullnames)
  setkey(mean_x,cell)
  res<-x-mean_x[x,][,1:length(names(mean_x))]
  res[,cell:=NULL]
  setDF(res)
  return(res)
}

Perhaps your package version of microbenchmark is different than mine

bench_all <- rbind(bench_small %>% summary %>% mutate(expr = paste0(expr,"_small")),
                   bench_large %>% summary %>% mutate(expr = paste0(expr,"_large")))

bench_all

             expr        min        lq       mean     median         uq        max neval
1 ans_dplyr_small  14.016470  14.63970  16.057286  14.919529  15.477332  26.772788    10
2   ans_dt1_small   2.033062   2.13456   2.349156   2.189441   2.638336   2.754370    10
3   ans_dt2_small   2.217810   2.27483   2.438935   2.407972   2.628640   2.661144    10
4   ans_dt3_small   2.925432   3.00355   3.896746   3.093642   3.514454  10.440436    10
5 ans_dplyr_large 660.563300 677.73756 709.718870 695.432125 707.791010 848.521906    10
6   ans_dt1_large  54.344144  55.17835  59.058532  56.304646  62.696206  69.227890    10
7   ans_dt2_large  21.906666  22.25135  23.184892  22.716924  23.423408  28.072566    10
8   ans_dt3_large   8.380004   8.94536   9.065873   9.136237   9.218774   9.665814    10

Doing this a bit more natively in data.table (unfortunately I couldn't figure out how to dereference in x and i, as with=FALSE assumes the name of each list is e.g. x.x_names)

center_dt_4 <- function(t) {
  setDT(t,key="cell")
  x_fullnames <- names(t)
  x_names <- x_fullnames[which(x_fullnames != "cell")]
  mean_x <- t[,lapply(.SD,mean,na.rm=TRUE),by=cell]
  res<-t[mean_x,.(A=x.A-i.A,B1=x.B1-i.B1,B2=x.B2-i.B2),on="cell"]
  setDF(res)
  return(res)
}

> bench_all
                        expr        min         lq       mean     median         uq        max neval
1            ans_dplyr_small  14.366576  14.494300  15.392998  14.925517  15.385244  20.094272    10
2              ans_dt1_small   2.029644   2.216670   2.384766   2.381316   2.554228   2.729282    10
3  ans_dt1_precompiled_small   3.341396   3.495352   3.804031   3.781878   4.003972   4.439324    10
4              ans_dt2_small   2.155372   2.321018   2.463510   2.405263   2.584166   2.936550    10
5              ans_dt3_small   2.638620   2.760072   2.859517   2.873259   2.965346   3.063422    10
6              ans_dt4_small   1.925294   1.930712   2.961612   2.119307   2.510894  10.003090    10
7            ans_dplyr_large 672.502800 677.593296 692.047965 685.753199 706.827080 716.847560    10
8              ans_dt1_large  55.923322  56.490104  59.640225  60.677824  62.484374  63.247306    10
9  ans_dt1_precompiled_large 135.373544 139.275732 141.147599 141.397891 142.505084 150.496206    10
10             ans_dt2_large  21.825126  22.328332  23.785973  22.710226  23.469594  29.990162    10
11             ans_dt3_large   8.644578   8.912002   9.775805   9.052987   9.578002  15.339058    10
12             ans_dt4_large   7.787560   8.263110   8.517451   8.644150   8.788840   8.970734    10


Ruby price calculator for groceries I am good at logic and most of the time I write working code. But I want to learn writing code which follows best practices and is very efficient. I tried to implement some of them in my code but it still looks very unclean.
Could anyone please explain how I can make this code better and make it production standard code.

This is my ruby code:

class Items
  @@items = {}
  def initialize(name, price)
    @@items[name] = price
  end

  def self.all
    @@items
  end
end

class SaleItems
  @@sale_items = {}
  def initialize(name, units, price)
    @@sale_items[name] = { 'units' => units, 'price' => price}
  end

  def self.all
    @@sale_items
  end
end

class PriceCalculator

  def start_billing
    input = get_input
    @purchased_items = input.split(',').map(&:strip)
    if !@purchased_items.empty?
      quantity = count_items
      price = calculate_bill(quantity)
      billing_items = quantity.each_with_object(price) { |(k,v), billing_items|
        billing_items[k] = {'units' => v, 'price' => price[k]}
      }
      display_bill(billing_items, quantity)
    else
      puts "Sorry! no items were given to process the bill"
    end
  end

  private

    def get_input
      puts "Please enter all the items purchased separated by a comma"
      input = gets.chomp
    end

    def count_items
      @purchased_items.inject(Hash.new(0)){ |quantity, item|
          quantity[item] += 1
          quantity
      }
    end

    def calculate_bill quantity
      price = {}
      quantity.each { |item,value|
        if SaleItems.all[item].nil?
          price[item] = quantity[item]*Items.all[item]
        else
          price[item] = (((quantity[item]/SaleItems.all[item]['units']).floor)*SaleItems.all[item]['price']) + ((quantity[item]%SaleItems.all[item]['units'])*Items.all[item])
        end
        }
        price
    end

    def display_bill billing_items, quantity
      total_price = billing_items.inject(0){ |tot, (item,v)|
        tot + v['price']
      }

      actual_price = quantity.inject(0){ |tot, (item,units)|
        tot + (units * Items.all[item])
      }

      puts "Item          Quantity          Price"
      puts "------------------------------------------"

      billing_items.each{ |item, v|
        puts "#{item.ljust(20)} #{v['units']}           $#{v['price']}"
      }
      puts "Total price : #{total_price.round(3)}"
      puts "You saved #{(actual_price - total_price).round(3)} today."
    end

end

begin
  # creating inventory of items
  Items.new('milk', 3.97)
  Items.new('bread', 2.17)
  Items.new('banana', 0.99)
  Items.new('apple', 0.89)

  # creating sale items
  SaleItems.new('milk',2,5.00)
  SaleItems.new('bread',3,6.00)

  pc = PriceCalculator.new
  puts pc.start_billing
end
```

In addition to what @dfhwze has said, I wanted to give some suggestions on general ruby best practice and readibility:

Multiline Blocks

In PriceCalculator#start_billing, PriceCalculator#count_items and in a couple other methods you use muliline blocks like so:

foobar.each { |*args|
  # do stuff
  # more stuff
}

The use of {} is really only intended for single line blocks -- when you have more than one line inside of the block, it's best to use do ... end. This will make your code a bit easier to read. From my example:

foobar.each do |*args|
  # do stuff
  # more stuff
end

Method Definitions

In ruby, while it's valid syntax to define methods like def some_method argument, it's really easy to misread that as def some_method_argument, and so it's considered better practice to put parentheses around your method definitions, like so: def some_method(argument).

Other

I just wanted to take a look at this method:

def calculate_bill quantity
  price = {}
  quantity.each { |item,value|
    if SaleItems.all[item].nil?
      price[item] = quantity[item]*Items.all[item]
    else
      price[item] = (((quantity[item]/SaleItems.all[item]['units']).floor)*SaleItems.all[item]['price']) + ((quantity[item]%SaleItems.all[item]['units'])*Items.all[item])
    end
    }
    price
end

Specifically, in the if statement, you could take advantage of a common ruby idiom for generating hashes, and instead of generating a price hash just return the hash you want:

def calculate_bill quantity
  quantity.map { |item,value| [item, SalesItems.all[item].nil? ? quantity[item]*Items.all[item] : (((quantity[item]/SaleItems.all[item]['units']).floor)*SaleItems.all[item]['price']) + ((quantity[item]%SaleItems.all[item]['units'])*Items.all[item])] }.to_h
end

(note how I used the {} for a one line block). If you wanted to expand this to make it a bit more readable:

def calculate_bill quantity
  quantity.map do |item,value|
    v = if SalesItems.all[item].nil?
      quantity[item]*Items.all[item]
    else
      (((quantity[item]/SaleItems.all[item]['units']).floor)*SaleItems.all[item]['price']) + ((quantity[item]%SaleItems.all[item]['units'])*Items.all[item])
    end
    [item, v]
  end.to_h
end

I'd also recommend breaking up that massive formula into some smaller pieces with better variable names to make it easier to tweak it later if you need to.


Simple builder pattern implementation for building immutable objects This is a builder pattern implementation to build immutable Person objects:

Person class

public final class Person {

    // All are set to final to make Person Immutable
    private final int id;
    private final String name;
    private final String surname;
    private final boolean isOccupied;

    // Constructor is private, so that only static
    // PersonBuilder can initiate the Person class instance
    private Person(PersonBuilder builder) {
        this.id = builder.getId();
        this.name = builder.getName();
        this.surname = builder.getSurname();
        this.isOccupied = builder.isOccupied();
    }

    public static class PersonBuilder {

        // Member variables of PersonBuilder are temporary storage
        // to create Immutable Person class instance
        private int id;
        private String name;
        private String surname;
        private boolean isOccupied;

        PersonBuilder() { }

        // The only method to initiate Person class
        Person build() {
            return new Person(this);
        }

        // Multiple Constructors for each member variable
        public PersonBuilder id(int id) {
            this.id = id;
            return this;
        }

        public PersonBuilder name(String name) {
            this.name = name;
            return this;
        }

        public PersonBuilder surname(String surname) {
            this.surname = surname;
            return this;
        }

        public PersonBuilder setOccupied(boolean isOccupied) {
            this.isOccupied = isOccupied;
            return this;
        }

        // getters, these will be used in private constructor
        // of Person class
        public int getId() {
            return id;
        }

        public String getName() {
            return name;
        }

        public String getSurname() {
            return surname;
        }

        public boolean isOccupied() {
            return isOccupied;
        }
    }

    // getters
    public int getId() {
        return id;
    }

    public String getName() {
        return name;
    }

    public String getSurname() {
        return surname;
    }

    public boolean isOccupied() {
        return isOccupied;
    }

    @Override
    public String toString() {
        return String.format(
                "Id:\t\t%d\nName:\t\t%s\nSurname:\t%s\nIsOccupied:\t%s\n"
                , id, name, surname, isOccupied);
    }

}

Person demo

public class PersonDemo {

    public static void main(String[] args) {
        // PersonBuilder instance
        Person.PersonBuilder builder = new Person.PersonBuilder();

        // builder builds the instance
        builder.name("Levent").surname("Divilioglu").id(666).setOccupied(true);

        // Immutable Person object is initiated with build() method
        Person person = builder.build();

        System.out.println(person);
    }

}

Demo output

Id:           666
Name:     Levent
Surname:  Divilioglu
IsOccupied:   true

I did write this implementation to learn the GoF builder pattern. Immutability is important for thread-safety, thus set the class and it's members final. I also set the Person constructor private to guarantee that it is only instantiated with inner class method, and only once. On each build() method, a new unique Person instance will be created.

I'm aware that builder patter is used for classes that have a lot of many member variables but I did only used 4 member variables for the simplicity.

First of all, very good job.

Now I will point out a few things that I think can be improved on.

Code Duplication

You have copied over all the private fields from Person to the Builder, which is necessary when the fields are declared final. As coderodde pointed out, using a final declaration is not necessary here when the fields are private and there are no setters.

Now that the fields are no longer declared final, it is possible to hold an unfinished version of your Person object in your builder. Now all the fields from Person no longer need to be copied over.

This also prevents the need for the getter methods on your Builder and the weird constructor of Person that takes a Builder as a parameter. The Person class should not be initializing itself via the state of the Builder object. The Builder should be building the Person, hence the name of the pattern.

Excessive commenting

You use comments a lot, even at places where they aren't needed and are generally just noisy. For example:

// getters

and

// getters, these will be used in private constructor
// of Person class`.

The first one is simply redundant, since all the methods below it are called get..., clearly indicating a getter function. The second one also provides little useful information. Note that generally, someone will read a code file from top to bottom, unless they might be looking for the details on a specific method. This would mean that they have already scrolled past your constructor and seen the getters being used there.

But there is a bigger point I want to make here with regards to the comments. In general, using comments to express what your code does, means that you have failed to express yourself in code, which can document itself when names are chosen carefully.

Coming back to the second comment I mentioned. You probably wrote that because you felt that you had to explain yourself for having public getters on your Builder class. When placing a comment like this, take a step back and ask yourself why you placed that comment and if your code would still seem logical without it. If your code seems weird without the comment, which I think public getters on a Builder class do, then you need to fix that issue instead of commenting about why you did it.

Now by moving the initialization of the Person to the Builder class where it belongs, as mentioned above, the Builder no longer needs these methods and the comment can be removed.

The same goes for the comment about the temporary member variables, which can now be removed as the Builder simply holds an instance of the Person class, which makes perfect sense on its own.

I went on a bit of a rant about comments, but I hope you can see what I mean.

Naming

Overall, job well done with the naming. A few remarks though:

I'd rename PersonBuilder to Builder. It's an inner class of Person so there really is no need for it. Person.Builder is perfectly fine.
I'd also rename the individual building methods for id, name, surname and isOccupied to setId, setName, setSurname and setOccupied, since they are all effectively setters.

Reworked Code

When incorporating these points into your code, it should look a little something like this.

public final class Person {
    private int id;
    private String name;
    private String surname;
    private boolean isOccupied;

    private Person() {}

    public static class Builder {
        private Person personToBuild;

        Builder() {
            personToBuild = new Person();
        }

        Person build() {
            Person builtPerson = personToBuild;
            personToBuild = new Person();

            return builtPerson;
        }

        public Builder setId(int id) {
            this.personToBuild.id = id;
            return this;
        }

        public Builder setName(String name) {
            this.personToBuild.name = name;
            return this;
        }

        public Builder setSurname(String surname) {
            this.personToBuild.surname = surname;
            return this;
        }

        public Builder setOccupied(boolean isOccupied) {
            this.personToBuild.isOccupied = isOccupied;
            return this;
        }
    }

    public int getId() {
        return id;
    }

    public String getName() {
        return name;
    }

    public String getSurname() {
        return surname;
    }

    public boolean isOccupied() {
        return isOccupied;
    }

    @Override
    public String toString() {
        return String.format(
                "Id:\t\t%d\nName:\t\t%s\nSurname:\t%s\nIsOccupied:\t%s\n"
                , id, name, surname, isOccupied);
    }
}


Bash Code to backup the history file in Linux How is the logic, efficiency and can it be improved?

This is a snippet from my .bashrc file, I just wrote it.

The code is supposed to do this:

Backup my .bash_history file if it is 1000 lines or less from the defined size of HISTSIZE=100000 and HISTFILESIZE=100000 lines, then backup the file.

Check if .bash_history is 99000 lines or more.
Check if ~/.bash_history.old exists if it doesn't use that filename.
Increment i one digit larger than is already used by filenames.(filename-4.old)
Check if there are more than 20 files already backed up and warn if it is.
Set the new filename into a variable.
Check if the last file with a digit in it's filename is older than the original.
cp the file to new_name

Here is the code:

# Count the number of lines in `/.bash_history and check if it's equal or more than a thousand
if (( $(wc -l < ~/.bash_history) >= 99000 ))
then
    name=~/.bash_history
    # Here is the suffix of the new files and the ones I check for
    old=.old
    # -e FILE ==> True if FILE exists.
    if [[ ! -e ~/.bash_history.old ]]
    then
       printf "%s\n" "##################################################" ".bash_history will be cleared soon, backing up....!" "##################################################"
       # Here I copy $name which is ~/.bash_history and create backup file
       cp $name ~/.bash_history.old
    else
        # i is the increment in the filenames to be checked and created
        i=0
        # $name$old is ~/.bash_history.old
        if [[ -e $name$old ]]
        then
            # Here I count how many copies there are with digits in the filename
            while [[ -e "$name-$i$old" ]]
            do
                let i++
            done
        fi
        # if there are 20 files already backed up then I need to archive them somewhere else
        if [[ "$i" -ge 20 ]]
        then
            printf "%s\n" "********************************************************" "You need to arhive your history files they are mounting up!!!" "**************************************************************"
        fi
        new_name=$name-$i$old
        minus=$(( i - 1 ))
        if [ $name -nt "$name-$minus$old" ]
        then
            printf "%s\n" "##################################################" ".bash_history will be cleared soon, backing up....!" "##################################################"
            cp ~/.bash_history "$new_name"
        fi
    fi
fi

This is the result from shellcheck.net:

Line 16:
                let i++
                ^-- SC2219: Instead of 'let expr', prefer (( expr )) .

First, logrotate is a tool built to do exactly this sort of thing. I would recommend using that to achieve the effectively the same extremely reliably and with lots of configuration options. That said, some suggestions on the code as written:

ShellCheck is great; I would recommend following its recommendation to use (( i++ )).
wc -l counts the number of lines, not bytes or kilobytes, which seems to be what you want.
old, minus and i are not helpful names; I have to read and understand all the code in the context in order to understand what they mean.
It looks like you only ever replace the last of the 20 files once you have 20 backups.
You have five instances of ~/.bash_history, even though one of them is the value of a variable. I would pull that variable out and reuse it everywhere.
Use More Quotes™ - it's good for you and the code.
Rather than a special unnumbered backup file (~/.bash_history.old), why not just start numbering the backups immediately? That way you can get rid of at least two checks for whether that file exists ([[ ! -e ~/.bash_history.old ]] and its inverse, [[ -e $name$old ]]).


Most efficient dict filter on key tuple? I have a dict wherein a value needs to be retrievable by any one of several keys. It seemed that making multiple dictionaries or multiple entries in a single dictionary pointing to the same value object (ref type) was more maintenance than I wanted to commit to.

I decided to use a tuple for the key. I would then use filter(...) with a lambda to determine if the given provider_lookup was in the keyed tuple. I am not concerned that a value may be duplicated across tuples (this will be guarded against as the code moves forward). Here are the two methods:

def register_block_provider(self, provider):
    block = provider()
    self.__block_providers.update({
        (block.block_id, block.name): block
    })

def get_block_provider(self, provider_lookup):
    for provider_key in filter(lambda p: (p[0], p[1]), self.__block_providers.keys()):
        if provider_lookup in provider_key:
            print("we've found the key: {key}".format(key=provider_key))
            return self.__block_providers[provider_key]

    return None

Are there specific improvements that can be made to the get_block_provider method? This works fine so I'm just asking for some feedback on the details of the implementation.

filter

Is useless here. It's purpose is to filter out values based on a predicate:

list(filter(f, data))

is the same as:

[d for d in data if f(d)]

Given that, in your case, f is lambda p: (p[0], p[1]), it will always evaluate to True in a boolean context. Thus never filtering anything. I’m not sure what it was you were trying to achieve, but you can remove filter without changing the behaviour:

def get_block_provider(self, provider_lookup):
    for provider_key in  self.__block_providers:
        if provider_lookup in provider_key:
            print("we've found the key: {key}".format(key=provider_key))
            return self.__block_providers[provider_key]

    return None

get_block_provider

I believe this method is to be used in the same kind of way than __getitem__ on dicts. Thus you need to act accordingly; it's the principle of least attonishment: you claim to be somewhat a dict then you get to act like a dict.

First of, remove that print. There is nothing justifying that line in a getter. If the user want to be sure it got something, it's its job to print something. It's not your classe's job.

Second, it might be better to raise a KeyError as a dict would have instead of returning None:

def get_block_provider(self, provider_lookup):
    for provider_key in  self.__block_providers:
        if provider_lookup in provider_key:
            return self.__block_providers[provider_key]

    raise KeyError("No block corresponding to lookup '{}'".format(provider_lookup))

Be a dict

You said that

  It seemed that making multiple dictionaries or multiple entries in a single dictionary pointing to the same value object (ref type) was more maintenance than I wanted to commit to.

But I disagree. Especially since looking at the code and your comments, you’re only expecting 2 different kind of lookups for your blocks: ID and name.

You do not have to entirely expose a dict interface. But you should use its strength. Your for loop in get_block_provider is a complete waste of time: you’re performing an \$O(n)\$ lookup where a dict would have provided an \$O(1)\$ one.

Instead, your lookup should rely on the underlying dictionary one. And you should focus on having only one entry point to update both keys at once:

def register_block_provider(self, provider):
    block = provider()
    blocks = self.__block_providers
    # update both keys at once
    blocks[block.block_id] = blocks[block.name] = block

def get_block_provider(self, provider_lookup):
    return self.__block_providers[provider_lookup]

You can even define

__getitem__ = get_block_provider

to mimic a dict interface.


An utterly pointless jQuery program This is the first jQuery program I've written, and it doesn't do much, but I'd appreciate the feedback. Just click on the different colored buttons to do things.

Codepen

var $greenElement = $(
  '
 \

Hello
 \

'
);

var $redElement = $(
  '
 \

Goodbye
 \

'
);

/* Main JQuery loop */
$(document).ready(function() {

  /* Add a red item and fade to the right value*/
  $('.add-item-red').mouseenter(function() {
    $(this).fadeTo('fast', 0.4);
    $(this).click(function() {
      $('.output-start').after($redElement);
    });
  });

  /* Check if the mouse is no longer on red button */
  $('.add-item-red').mouseout(function() {
    $(this).fadeTo('fast', 1);
  });

  /* Add a green item and fade to the right amount */
  $('.add-item-green').mouseenter(function() {
    $(this).fadeTo('fast', 0.4);
    $(this).click(function() {
      $('.output-start').after($greenElement);
    });
  });

  /* Check if mouse is no longer on green button */
  $('.add-item-green').mouseout(function() {
  $(this).fadeTo('fast', 1);
  });

  /* Reset all the items */
  $('.reset-items').mouseenter(function() {
    $(this).fadeTo('fast', 0.4);
    $('.reset-items').click(function() {
      $('.green-bar').remove();
     $('.red-bar').remove();
    });
  });

  /* Check if mouse is no longer on white button */
  $('.reset-items').mouseout(function() {
    $(this).fadeTo('fast', 1);
  });
});
body {
  background-color: black;
}

.button-bar div{
  display: table;
  display: table-cell;
}

.add-item-red {
  width: 50px;
  height: 50px;
  background-color: red;
  border-color: black;
  border-style: solid;
  border-width: 3.5px;
}

.add-item-green {
  width: 50px;
  height: 50px;
  background-color: green;
  border-color: black;
  border-style: solid;
  border-width: 3.5px;
}

.reset-items {
  width: 50px;
  height: 50px;
  background-color: white;
  border-color: black;
  border-style: solid;
  border-width: 3.5px;
}

.output-start {
  width: 50px;
  height: 15px;
}

Ok, so there's two different things here:

Hover effects (fade in/out)
Actions

Now, #1 is the same for all three elements, and thus shouldn't need to be repeated in code. But #2 is expressly different for each button, which means it probably shouldn't mixed with the hover effects.

Aside: Speaking of duplication, your CSS is full of it too. You should look into that. You should also avoid naming things for how they look. E.g. add-item-red is fine until that item is no longer red; usually it'll have some purpose other than being red. Its color is a presentation detail. Name stuff for what it does, not how it looks.

Anyway, these days (read: modern browsers), I'd probably just use CSS transitions to handle the hover effect; no javascript necessary. For instance:

div {
  background: green;
  padding: 0.3em;
  display: inline-block;
  transition-duration: 0.5s;
}

div:hover {
  opacity: 0.4;
}

Hover

But since this is about jQuery, here's how I'd handle it with a common class name for all the buttons (which, incidentally, I'd use <button> tags for), and jQuery's appropriately named .hover() event handler.

$(".fade").hover(function () {
  $(this).fadeTo('fast', 0.4);
}, function () {
  $(this).fadeTo('fast', 1.0);
});
body { background: black; }

button {
  border: none;
  padding: 1em;
}

#helloButton   { background: green; }
#goodbyeButton { background: red; }
#clearButton   { background: white; }

<button type="button" id="helloButton" class="fade">One</button>
<button type="button" id="goodbyeButton" class="fade">Two</button>
<button type="button" id="clearButton" class="fade">Three</button>

Now, you're using mouseenter combined with mouseout. But that's not really "symmetrical". The opposite of mouseenter is mouseleave (and the opposite of mouseout is mouseover). The enter/leave events are originally Internet Explorer-only events which jQuery has replicated, and they behave differently than the standard over/out events with regard to nested elements. jQuery's docs have a discussion of this and a demo to illustrate. It happens to work in your case, but it's a bit like saying that the opposite of multiplying is subtracting; it just happens to work out for certain numbers, but that doesn't make it right.

Next, there's the click actions. You're adding those inside the mouseenter event. This is a big no-no. It means that every time you mouse over a button, a new click event handler gets added! You just don't notice it because you're inserting and re-inserting the same element. But if you mouse over the red button 3 times before clicking it, the effect is that you're clicking it 3 times because now you've got 3 separate but identical event handlers attached.

Yet another reason to keep the actions separate from the hover effects.

You can either attach the event handlers to an element by its ID (as below), or you can use some other means of linking an element to an action.

$(".fade").hover(function () {
  $(this).fadeTo('fast', 0.4);
}, function () {
  $(this).fadeTo('fast', 1.0);
});

var hello = $("

").text("Hello").css({background: "green"});
var goodbye = $("

").text("Goodbye").css({background: "red"});

$("#helloButton").on("click", function () {
  $("#output").prepend(hello);
});

$("#goodbyeButton").on("click", function () {
  $("#output").prepend(goodbye);
});

$("#clearButton").on("click", function () {
  $("#output").empty();
});
body { background: black; }

button {
  border: none;
  padding: 1em;
}

#helloButton   { background: green; }
#goodbyeButton { background: red; }
#clearButton   { background: white; }

<button type="button" id="helloButton" class="fade">One</button>
<button type="button" id="goodbyeButton" class="fade">Two</button>
<button type="button" id="clearButton" class="fade">Three</button>

You'll notice a few things here:

I'm prepending the elements to the output div, rather than inserting them next to it. This makes more sense to me since now, all the output is, well, in the output. Not beside it. And it means I can just call empty to clean up.
I'm constructing the elements using jQuery rather than with strings of HTML. I generally don't like a ton of hardcoded HTML in my JS. I also don't need IDs for them, because, again, I just need to call empty.

I might go a step further and add the elements in the HTML itself, and simply hide them until needed, which would be even simpler in a lot of ways, and keep the JS free of hardcoded elements of any kind.

In the end, I'd probably replicate your code with something like this:

$("#helloButton").on("click", function () {
  $("#hello").show();
});

$("#goodbyeButton").on("click", function () {
  $("#goodbye").show();
});

$("#clearButton").on("click", function () {
  $("#output").children().hide();
});
body { background: black; }

button {
  border: none;
  padding: 1em;
  display: inline-block;
}

.fade { transition-duration: 0.5s; }
.fade:hover { opacity: 0.4; }

#helloButton   { background: green; }
#goodbyeButton { background: red; }
#clearButton   { background: white; }

#hello { background: green; }
#goodbye { background: red; }

#output p { display: none; }

<button type="button" id="helloButton" class="fade">One</button>
<button type="button" id="goodbyeButton" class="fade">Two</button>
<button type="button" id="clearButton" class="fade">Three</button>

Hello

Goodbye

It doesn't make the paragraphs switch places, but I don't think that's the most salient feature either, to be honest.


Char* hex string to BYTE Array The idea is that i want any size string to put the corresponding hex value into a byte array. I've seen a million ways to do it. Some of them didn't look to clean. My needs are speed over memory, so chose to try and implement a lookup table. Tell me what you think

const BYTE HEX[0x80] = //This is the ASCII table in number value form
{   //0     1     2     3     4     5     6    7      8     9     A     B     C     D     E     F
    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,//0
    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,//1
    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,//2
    0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,//3
    0x00, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,//4
    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,//5
    0x00, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,//6
    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 //7
};
void ByteUtil::StringToHex(const char* str, BYTE* hex)
{
    int len = strlen(str)/2;
    for(int i=0; i<len; i++)
    {
        hex[i] =  (HEX[*str++] << 4);
        hex[i] += (HEX[*str++]);
    }
}

This method assumes you've already figured out the length of the array. So to use it it would look somethign like this.

char* gky = "55D38577093A88F3B5EA40BBF11158813A2C662EB71FBAB9";
int len = strlen(gky)/2;
BYTE* GKY = new BYTE[len];
ByteUtil::StringToHex(gky, GKY);
ByteUtil::LogArray("GKY", GKY, len);
delete[] GKY;

this particular code snippet outputted this

2013-02-15.10:03:19     GKY(24)- 55:D3:85:77:09:3A:88:F3:B5:EA:40:BB:F1:11:58:81:3A:2C:66:2E:B7:1F:BA:B9:

Some minor comments:

you are assuming there are no invalid chars in the input string.
why not pass the string length into the function; the caller has to know it in order to allocate the hex buffer.
len could be const
unnecessary brackets around RHS expressions in hex[i] assignments
I would prefer the second assignment to be |= not +=
it is usual to use uppercase names only for #defined constants


For a query over multiple databases, is it faster to do GROUP BY first or UNION first? Having several databases with the same table, I need to group the union of the records.

The xTables are stored in separate databases named by the year (this is by third-party design and I cannot change it). There are 3 databases in my case but it could be say up to 10. Each of the tables contains about 2500 records with about 300 distinct xID values.

I have tried two approaches but I do not know which is better. I need to calculate the MIN and SUM of the grouped values. The first approach calculates the values for the years (databases) first, and then calculates it again from the agregated values.

CREATE VIEW dbo.view1 AS
    SELECT
        id,
        MIN(minA) AS a,
        CONVERT(numeric(19, 2), SUM(sumB)) AS b
    FROM (
        SELECT
            xID AS id,
            MIN(xA) AS minA,
            SUM(xB) AS sumB
        FROM x2014.dbo.xTable
        GROUP BY xID
        UNION
        SELECT
            xID AS id,
            MIN(xA) AS minA,
            SUM(xB) AS sumB
        FROM x2013.dbo.xTable
        GROUP BY xID
        UNION
        SELECT
            xID AS id,
            MIN(xA) AS minA,
            SUM(xB) AS sumB
        FROM x2012.dbo.xTable
        GROUP BY xID
        ) AS u
    GROUP BY id

The second approach unions all the record first and groups the flatted records later:

CREATE VIEW dbo.view2 AS
    SELECT
        xID AS id,
        MIN(xA) AS a,
        CONVERT(numeric(19, 2), SUM(xB)) AS b
    FROM (
        SELECT
            xID,
            xA,
            xB
        FROM x2014.dbo.xTable
        UNION
        SELECT
            xID,
            xA,
            xB
        FROM x2013.dbo.xTable
        UNION
        SELECT
            xID,
            xA,
            xB
        FROM x2012.dbo.xTable
        ) AS u
    GROUP BY id

The execution plans differ and they show only percentage of the calculation time. I am not that good to read them. By intuition, the first approach may take advantage of statistics bound to the separate databases. However, I am not sure how it is implemented.

The view may be used intensively, and I need to decide which form is more efficient.

The Second is Faster

Having just tested a slightly simpler version of this, using only 1 column to be aggregated, I found that the second version is faster. I would also argue that the second is much easier to read.

Here is the code I used to test it:

CREATE TABLE TEST1 (id INT, a INT)

INSERT INTO TEST1
SELECT 1,23 UNION ALL
SELECT 1,26 UNION ALL
SELECT 1,47 UNION ALL
SELECT 2,13 UNION ALL
SELECT 2,31

CREATE TABLE TEST2 (id INT, a INT)

INSERT INTO TEST2
SELECT 1,18 UNION ALL
SELECT 1,45 UNION ALL
SELECT 2,24 UNION ALL
SELECT 2,42 UNION ALL
SELECT 2,64

/*
    Single Group
*/
SELECT
    id,
    MIN(a) AS minA
FROM
(
    SELECT id,a
    FROM TEST1

    UNION ALL

    SELECT id,a
    FROM TEST2
)Data
GROUP BY id

/*
    Multiple Groups
*/
SELECT
    id,
    MIN(a) AS minA
FROM
(
    SELECT id,MIN(a) AS a
    FROM TEST1
    GROUP BY id

    UNION ALL

    SELECT id,MIN(a) AS a
    FROM TEST2
    GROUP BY id
)Data
GROUP BY id

DROP TABLE TEST1
DROP TABLE TEST2

General Feedback

You've consistently capitalised your keywords and your indentation is also consistent. Both of these help readability.

I recommend using the second query, it doesn't require any modification as far as I can see. You've got a pretty nice query there.


Initializing events on class I wrote a simple class that substitutes all file inputs in an HTML for a more complex template and set some actions on it. The code is rather small, but I'm not used to write JavaScript.

I wonder if there is a design pattern or extensively used way to set the callbacks? Of course, overall improvements are welcome as well.

The full code is here:

$(document).ready ->
  $('input[type="file"]').each ->
    new PhotoInput(this)

class PhotoInput
  constructor: (originalInput) ->
    @renderTemplate(originalInput)
    @setClickActions()
    @setPreviewRender()

  renderTemplate: (originalInput) ->
    @template = ich.image_file_input_template # this is ICanHazJS call - it loads a mustache template there.
      fileInput: $(originalInput)[0].outerHTML
    @fileInput = @template.find('input[type=file]').first()
    @previewImage = @template.find('.file-input-preview img')
    $(originalInput).replaceWith(@template)

  setClickActions: ->
    @template.find('#select-file').click =>
      @fileInput.focus().click()
    @template.find('.file-input-preview').click =>
      @fileInput.focus().click()

  setPreviewRender: ->
    @fileInput.change =>
      @previewImage.attr('alt', @fileInput.val())
      if(@fileInput[0].files && @fileInput[0].files[0])
        reader = new FileReader()
        reader.onload = (e) =>
          @previewImage.attr('src', e.target.result)
        reader.readAsDataURL(@fileInput[0].files[0])

You're instantiating a PhotoInput object but never assigning it to a variable. This implies that there's no need for a class at all. You're only instantiating it for its side-effects; you're not interested in the instance itself.

This works, but it's bad form.

Even if you stored the instance somewhere, you still can't really use it for anything. Sure, you could call its methods, like, say, setPreviewRender, but there'd be no point since it's already been called - in fact, it'd probably be a bad idea to call it again.

So don't use a class, just write a function and use closures instead of instance variables:

createPhotoInput = (originalInput) ->
  originalInput = $ originalInput

  # replace original input
  template = ich.image_file_input_template
    fileInput: originalInput.get(0).outerHTML

  originalInput.eq(0).replaceWith template

  # set click handlers
  input = template.find 'input[type=file]:first'
  template.find('#select-file, .file-input-preview').click (event) ->
    input.focus().click()

  # set preview handling
  preview = template.find '.file-input-preview img'
  input.change (event) ->
    preview.attr 'alt', input.val()
    file = input.get(0).files?[0]
    if file?
      reader = new FileReader
      reader.onload = (event) -> previewImage.attr 'src', e.target.result
      result.readAsDataURL file

  # return the template, just in case
  template

$ ->
  $('input[type="file"]').each ->
    createPhotoInput this

You'll note that it also does away with all the scope-handling and fat arrows (=>), since it's just closures.


Killing a tunnel and establishing a tunnel through SSH I have two functions right now:

TUNNEL_COMMAND = "ssh -L 27017:localhost:27017 username@ip -f -N"

def kill_tunnel():
    p = subprocess.Popen(["ps", "aux"], stdout=subprocess.PIPE)
    out, err = p.communicate()
    for line in out.splitlines():
        if TUNNEL_COMMAND in line:
            pid = int(line.split()[1])
            print "killing pid", pid
            os.kill(pid, signal.SIGKILL)

def connect_tunnel():
    subprocess.Popen(TUNNEL_COMMAND)

connect_tunnel() establishes the connection, which I am ok with, but kill_tunnel() looks very ugly. Is there a better way to find the PID of a process based off the command?

How about using pgrep with the -f option to match the whole command line so that we can match the desired process precisely?

For example, in you case something like:

pgrep -f 'ssh.*27017:localhost:27017'

might give the accurate PID. If you desire, you can modify it to make it more robust.

Implementing this in Python:

import subprocess
import os
import signal

def kill_tunnel():
    try:
        tunnel_pid = subprocess.check_output(['pgrep', '-f', 'ssh.*27017:localhost:27017'])
    except subprocess.CalledProcessError:
        return 'No Such Process'
    os.kill(tunnel_pid, signal.SIGKILL)


PHP OOP MVC Project Structure I'm a beginner in OOP and PHP frameworks.  I used slim skeleton to create a project and designed the project in this way.

Project structure is given by the slim skeleton. Object Mappers are added by my self. I want to know whether this is a good practice. What I'm doing wrong here. Is there any better way to do this?

src
   Controllers
      PostController.php
      CommentController.php
   Models
      Post.php
      Comment.php
   ObjectMappers
      PostMapper.php
      CommentMapper.php
   Dependencies.php
   Middleware.php
   Routes.php
   Settings.php

Model files are used to include queries for relevant class.
Mappers are the real classes that used to map query results to necessary class.
Controllers are for necessary operations before sending it to the user.

Post.php

namespace App\Models;

class Post {

   public function getPostById($id){

      $sql = "SELECT `post_id`, `post` FROM `posts` WHERE `post_id` = :id";

      try {
         $stmt = $this->db->prepare($sql);
         $stmt->execute(['id' => $id]);
         $post = $stmt->fetchObject('\App\ObjectMappers\PostMapper');
         if($post){
            return $post;
         }
         return false;
      } catch (\PDOException $e){
         return false;
      }
   }

   // Other queries
}

PostMapper.php

namespace App\ObjectMappers;

class PostObject {

    public $post_id;
    public $post;

    public function __construct(){}

    public function getPost_id()
    {
        return $this->post_id;
    }

    public function setPost_id($post_id)
    {
        $this->post_id= $post_id;
        return $this;
    }

    public function getPost()
    {
        return $this->post;
    }

    public function setPost($post)
    {
        $this->post= $post;
        return $this;
    }
}

PostController.php

namespace App\Controllers;

class PostController {

   public function ($request, $response, $args){

      $postId = $request->getAttribute("post_id");

      if(!$postId){
         return $response->withJSON("post id not found");
      }
      $post = $this->Post->getPostById($postId);

      if(!$post ){
         return $response->withJSON("post not found");
      }

      return $response->withJSON($post);
   }
}

I'll be glad if you can help me. I'm trying to avoid using existing ORMs here.

This is good in general. The main point is that you confused a model and a mapper. Your mapper must be a model (or, to avoid a confusion, better to call it an entity) whereas your model is actually a mapper.

People often mistake  the term "model", so I had to write an article to explain it.

So

src
   Controllers
      PostController.php
      CommentController.php
   Entities
      Post.php
      Comment.php
   Mappers
      PostMapper.php
      CommentMapper.php
   Dependencies.php
   Middleware.php
   Routes.php
   Settings.php

where Post.php would be

namespace App\Entities;

class Post {

    protected $postId;
    protected $post;

    public function __construct(){}

    public function getPostId()
    {
        return $this->postId;
    }

    public function setPostId($postId)
    {
        $this->postId= $postId;
        return $this;
    }

    public function getPost()
    {
        return $this->post;
    }

    public function setPost($post)
    {
        $this->post= $post;
        return $this;
    }
}

and PostMapper would be

namespace App\Mappers;

class PostMapper {

   public function getPostById($id){

      $sql = "SELECT `post_id`, `post` FROM `posts` WHERE `post_id` = :id";
      $stmt = $this->db->prepare($sql);
      $stmt->execute(['id' => $id]);
      return $stmt->fetchObject('\App\Entity\Post');
   }
}

Please note that a lot of code has been removed from PostMapper class. First, a harmful try .. catch has been removed. A try .. catch that silently returns false is as bad as any other error suppression operator, and shouldn't be used such casually. As a programmer, you crave to know what the error was, so you'll be able to fix it. So always let errors go, unless you have a certain handling scenario. See more in my article, PHP error reporting basics.

Also, rather useless condition is removed too, as fetchObject() already returns false if a row not found, so we can return its result right away.

Also, consider making your PHP code style to conform with the standard


Laravel Dependency Injection : Perfomace Issue And Architecture Style I have a TransportationController which uses six repositories. The repositories are injected to the controller via constructor and method.

TransportationController.php

use Illuminate\Http\Request;
use App\Repositories\TransportationRepository;
use App\Repositories\TruckRepository;
use App\Repositories\AccountRepository;
use App\Repositories\SiteRepository;
use App\Repositories\EmployeeRepository;
use App\Repositories\EmployeeWageRepository;
use App\Http\Requests\TransportationRegistrationRequest;
use App\Http\Requests\TransportationFilterRequest;
use \Carbon\Carbon;

class TransportationController extends Controller
{
    protected $transportationRepo;
    public $errorHead = 5, $noOfRecordsPerPage = 15;

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

    /**
     * Display a listing of the resource.
     *
     * @return \Illuminate\Http\Response
     */
    public function index(TransportationFilterRequest $request, TruckRepository $truckRepo, AccountRepository $accountRepo, SiteRepository $siteRpepo, EmployeeRepository $employeeRepo)
    {
        //some filtering stuff

        $transportations = $this->transportationRepo->getTransportations($params, $noOfRecords);

        return view('transportations.list', [
                'accounts'          => $accountRepo->getAccounts(),
                'sites'             => $siteRpepo->getSites(),
                'trucks'            => $truckRepo->getTrucks(),
                'drivers'           => $employeeRepo->getEmployees(),
                'materials'         => $this->transportationRepo->getMaterials(),
                'transportations'   => $transportations,
                'rentTypes'         => $this->transportationRepo->rentTypes,
                'params'            => $params,
                'noOfRecords'       => $noOfRecords,
            ]);
    }
}

Is there any architecture faults in injecting many(about 5 or 6)
repositories to a controller?
Is there any performance issue related to injecting many(about 5 or 6)
repositories to a controller?
What are the difference in injecting repositories through constructor
vs injecting through method?
What about injecting repositories in another repositories

I'll be curious to see if anyone else has an opinion here, but while we wait here is mine:

Architecture

I don't see anything obviously wrong with this code.  I presume that your view actually needs all of that data in order to generate all the necessary HTML/etc.  If that is the case you only have two options anyway: inject all your repositories or completely rebuild the actual HTML page so you don't need so much data.  Again, presuming that your page simply needs all this data on it, the only way to do that would be with a completely different architecture: namely, a front end application that hits up a variety of endpoints to get each piece of data separately and then builds the actual page.  That will substantially simplify your controllers (presumably they would just need one repository each), but it would also mean more controllers and a more complicated front-end, so I don't really see that as being much of an improvement.  All that to say this: If your page needs a lot of different "kinds" of data then you have no choice but to fetch lots of different "kinds" of data.

Performance

4 or 5 repositories doesn't seem like a terribly large number to me.  You could always find out by doing some profiling.  The best way to answer "does this cause performance problems" is by profiling and finding out how it actually performs.  Laravel has some easy-to-add-on tools for that.  As a (very) rough guideline, if your page can completely render in a tenth of a second or so, then it isn't terrible.  Ultimately though, performance is relative to page load and other factors.  A high traffic page obviously needs to have better performance than one visited once a month by a single person.  In essence, poor performance + high traffic = crashed server.  Ultimately, it is up to you to decide whether or not a given page is loading fast enough (i.e. performing well enough) given your available resources and the level of traffic the page receives.

Constructor injection vs method injection

Although I don't know enough about the innards of Laravel here to say for sure, I doubt there is a measurable difference in performance between injecting a repository in the constructor vs the method.  However, the way there can be a difference is because any constructor-dependencies are always injected, where as method ones are only injected when the particular endpoint is called.  So if you have a dependency injected in your constructor that is only used in one endpoint (or not at all), then that would be wasteful.  If, however, a dependency is used in all the endpoints in the controller, I would just inject it in the constructor and be done with it.  If you only have one method in your constructor, then it effectively shouldn't matter whether you inject in your constructor or method.

Injecting repositories in repositories

Whether or not that is a good idea or bad idea depends on the use case.  There isn't an example of that here, so I would suggest another question to address that.


Brute-force string generator I have created a brute-force algorithm class in C# and I was wondering if I could speed up the process of creating a new string. There are two methods inside the class: one returns the brute-force string and one returns the number of strings created.

public class BruteForce
{
    int IntPasswordsGenerated = 0;
    public char[] CharList = new char[] { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9' };
    public List<int> IntList = new List<int> { 0 };

    public string GenerateString()
    {
        int Number = IntList.Count - 1;

        IntPasswordsGenerated++;
        if (IntPasswordsGenerated == 1) return CharList[0].ToString();

        do
        {
            IntList[Number]++;
            if (IntList[Number] == CharList.Length && Number == 0)
            {
                IntList[Number] = 0;
                IntList.Add(0);
                break;
            }
            else if (IntList[Number] == CharList.Length)
            {
                IntList[Number] = 0;
                Number--;
                continue;
            }
            else
            {
                break;
            }
        } while (true);

        string BruteForceString = "";

        foreach (int CurrentInt in IntList)
            BruteForceString += CharList[CurrentInt];

        return BruteForceString;
    }
    public int PasswordsGenerated()
    {
        return IntPasswordsGenerated;
    }
}

What you have implemented is a back-to-basics number-system in base 62 Your IntList List is a mechanism of having the equivalent of "units", "tens", "hundreds", except you are in base 62 so it's "units", "sixty-twos", .....

When you increment a value, and it overflows, you then set it back to zero, and increment the next column instead.

In addition, you have a special case for the first time around, where you have to bypass the logic to ensure you have the right initial value returned.

So, there's a trick for the first time around that's easy to implement. The trick is to think of the array as holding the "next" value to return. Your code then becomes:

    string BruteForceString = "";

    foreach (int CurrentInt in IntList)
        BruteForceString += CharList[CurrentInt];

    // Now calculate the **next** password

    ......

    return BruteForceString;

Note how your initial array already contains the "next" password when initialized. Then, you can convert that to a string, and then increment the array to be ready for the next call.

But, I want to suggest that you are doing it all horribly wrong... ;-)

What you should be doing is using a simple long value to store your next value, and then using integer division and modulo to work your base-62 system.

Additionally, your variable names are quite horrible.... but that's OK, we will get rid of them all except the worst....

private ulong IntPasswordsGenerated = 0;

public string GenerateString()
{

    string password = "";
    ulong base = CharList.Length;

    // get the current value, increment the next.
    ulong current = IntPasswordsGenerated++;

    do
    {
        password = CharList[current % base] + password;
        current /= base;
    } while (current != 0);

    return password;
}

The magic is almost entirely in this line here:

   ulong current = IntPasswordsGenerated++;

hat takes a copy of the "next" password, and then afterwards increments what the next password will be.

Using division and modulo is likely a lot faster than the multiple loops required to check and validate the multiple array elements in your custom number system.

Finally, by taking a copy of the next password, and by keeping the password as a single ulong instance variable, it becomes relatively simple to lock on a small section of code and then make your method able to run in multiple threads... allowing you to brute force more passwords at once though parallel processing. All you need to do that is drop the ulong to a long, and then use the Interlock.increment() method to do a thread-safe, atomic increment (assuming you have a 64-bit machine).


Shuffle array elements by rearranging the key value associations PHP's shuffle() function destroys the array keys, so I decided to write a shuffle function that doesn't do that and instead rearranges key-value associations. As I would be using the function in place of shuffle() most of the time, I'd like it to be as fast and memory efficient (if a time-space tradeoff is needed, I think I would prioritise time) as possible:

<?
    function swap(&$a, &$b)
    {
        $tmp = $a;
        $a = $b;
        $b = $tmp;
    }

    function shuffleX(&$arr)    #Shuffles the key-value associations in an array.
    {
        $keys = array_keys($arr);   #extract the keys from the array.
        $length = count($keys);
        $i = 0; #Index.
        while ($i < $length-1)
        {
            $target = rand(($i+1), $length-1);  #This ensures that no value ends up mapped to the same key.
            swap($arr[$keys[$i]], $arr[$keys[$target]]);    #Swap each element of the array with another.
            $i++;
        }
    }
?>

Rather than making iterated rand() calls, you should randomize the data once for best efficiency.  To do this, just isolate the keys, shuffle them, then rejoin the values to the appropriate keys in the new order.

Code: (Demo)

function preserve_shuffle(&$arr)    #Shuffles the key-value associations in an array.
{
    $keys = array_keys($arr);   #extract the keys from the array.
    shuffle($keys);
    for ($index = 0, $length = count($keys); $index < $length; ++$index) {
        $result[$keys[$index]] = $arr[$keys[$index]];
    }
    $arr = $result;
}

$arr = ["a" => "apple", "b" => "banana", "c" => "capsicum", "d" => "dill"];
preserve_shuffle($arr);
var_export($arr);

Or, if you prefer greater brevity or a functional syntax, you could use the following inside your custom function:

$keys = array_keys($arr);   #extract the keys from the array.
shuffle($keys);
$arr = array_merge(array_flip($keys), $arr);

Proof that it also works: https://3v4l.org/eLMo6

My earlier snippets only reorder the associative data.  The following will shuffle the associations without shuffling the key orders.  It does not guarantee that all initially associations will be destroyed -- which I feel is beneficial / less predictable in a randomized result.

function random_disassociate(&$assoc_array)
{
    if (sizeof($assoc_array) < 2) {
        return;  // data cannot be disassociated
    }
    $keys = array_keys($assoc_array);
    shuffle($assoc_array);
    $assoc_array = array_combine($keys, $assoc_array);
}

$arr = ["a" => "apple", "b" => "banana", "c" => "capsicum", "d" => "dill"];
random_disassociate($arr);
var_export($arr);