Vectorizing a complex nested for loop in R (running models on different subsets of a data set, subsetting the data differently for each loop) This is my first time posting so hopefully I manage to get my question across in a clear and concise manner. I'm not sure this is the right place but someone on StackOverflow suggested I ask here.

I am running a cross-validation experiment which involves training a model on a subset of a data set and then validating it against the rest of the data set. This is done by randomly subsetting the data into "folds" and then running the model for each fold. This is then replicated in order to produce standard errors. This is the code I have written in R:

reps = 10
folds = 10
rep.Res<-list()

for(i in 1:reps){
  #i=1
  phen2<-phen
  phen2$fold<-sample(1:folds,nrow(phen),replace=T)

  fold.Res<-list()

  for(j in 1:folds){
    #j=1

    print(paste(i," ",j))

    phen3<-phen2
    phen3[which(phen3$fold==j),2]<-NA

    ETA<-list(list(X = geno_centred, model = 'BayesA'))

    model <- BGLR(y = phen3[,2], ETA = ETA, nIter = 20000,
                  burnIn = 1500,  thin=10)

    model.output<-data.frame(ID = phen3[which(phen2$fold == j), 1],
                             PBV = as.numeric(model$yHat[which(phen2$fold == j)]),
                             phen = phen2[which(phen2$fold == j), 2], rep = i, fold = j)

fold.Res[[j]]<-model.output

  }

  rep.Res[[i]]<-do.call(rbind,fold.Res)

}

"phen" is a data frame with a a column of IDs and a column of values. For each replicate, each sample is assigned to a fold randomly. Then, for each fold individuals in the fold are given an NA value, i.e. excluded from the model.

The model is run using the Bayesian Generalised Linear Regression (BGLR) package. Unfortunately, I can't really give a reproducible example. X is a scaled centred genotype matrix of -1's, 0's and 1's (not critical for my problem here). The output of the model is then written out for each fold, and then finally for each replicate.

My question is, is it possible to vectorize these loops? Using 10 folds and 10 reps means that the model runs 100 time, at 20 000 MCMC iterations each time. This causes the system to slow down significantly before it's even half done. I know that there are many possible approaches using basic R or packages in the tidyverse suite, but I am really a bit lost with such a complex loop. I can imagine that I would try to write at least one of the loops into a function enclosing the other loop but I'm not even sure how that would work...

Again, first time posting so please let me know if I need to add anything to my question! Thanks!

Edit: Here are a small, random centred genotype matrix (X in the call to BGLR) and an accompanying phen file.

X (rows are sample IDs matching those in phen, columns are genomic locations (SNPs)):

    A  B  C  D  E  F  G  H  I  J  K  L  M  N  O  P  Q  R  S  T
1   1  1  1 -1 -1  0 -1 -1  0  1 -1  0  0  0  0 -1  0  0  1  1
2  -1  1  0 -1  0  0  1 -1  1  0 -1  1 -1  0  0  0 -1 -1 -1 -1
3  -1 -1  1 -1  0  0  1  0  0 -1 -1  0 -1  0  0 -1  1 -1  0 -1
4   0  0 -1  1 -1  0 -1  1  1  1 -1  0  0  0  1  1 -1 -1  0  1
5   1  0  1  1  0  0  1  1  0  0 -1  1  1  1  1 -1 -1  0 -1  0
6   1 -1 -1  0  1 -1  1  1  1 -1  0  0  0 -1  0  1  0 -1 -1  0
7   1 -1 -1  1  1  0 -1  1 -1  1  0 -1  1  1 -1 -1 -1  1 -1  1
8   0  1  0  1 -1 -1  0 -1 -1 -1  1 -1  0  0  0  0  0 -1  0 -1
9  -1  1 -1 -1  1  0  0  0  0  0  1  0 -1 -1 -1  0  1 -1  0 -1
10  0  0 -1  1  0  0 -1  0 -1  1  1  1  0  0  1  1  0 -1  0  0

Phen:

ID  phenotype
1   1000
2   1500
3   1200
4   500
5   700
6   2000
7   1500
8   1000
9   1300
10  900

I'm not familiar with BGLR but I think I can give some pointers.

First to your direct question - yes you can vectorise.  A simple approach is to use one of the apply functions.  lapply usually is a good fit for such loops.  However this is not a magic bullet and I would strongly suggest to review the code.

I think the main thing that slows you down - besides the time BGLR needs - is how R deals with memory.  The relevant properties are:

most objects need to be placed continuous in memory
memory isn't immediately freed - but R waits till it is necessary and does garbage collect then
the time when R creates a copy of memory is a bit tricky.  It is not when one writes an assignment - at this point the variable just holds a reference to the original data.  But as soon as a change on the data is performed it creates a full copy.

All this effects don't appear in a "clean" R session, but as memory clogs things get significantly slower - agreeing with your observation.

I see following issues in the code:

Growing lists
fold.Res and rep.Res are growing lists.  As grow them they need to be written anew.  This means first all the previous entries need to be copied (which can take some time) - but more important they can not be written in the same place as the new list takes more space.  This will create fragmented memory - meaning whenever R can't fit the new list where a whole from a previous was created it will first need to garbage collect and then maybe defragment memory.  I think this is the serious slow down you are experiencing.
Allocating lists unfortunately doesn't help, but with the apply functions like lapply this is also taken care of.

creating modified copies
You create phen2 and phen3 as copies - but you only modify and use column 2.  Meaning there is some unnecessary copying going on.  Also this will lead faster to some extra garbage collects as memory fills up.

possible side effects of BGLR
While the first runs are relatively free where to put their data, later may run into troubles finding suitable spots - causing again some superfluous memory cleanups.  This is not so easy to deal with.  I worked with stan which is quite the hog - and the best solution was to run it in separate R processes and save the results and the kill it - it was way faster than having these cleanups going on.  The main scripts would then just collect the files.  I described how I do it in my answer to: Using doParallel to cycle through *.rds files (specifying how to do it outside doParallel).

to style
It is a bit confusing that you sometimes refer to columns by name and sometimes by column number.  This makes things harder to read than necessary.  I would suggest always using the name approach.

Those apply functions are awkward to write.  Personally I would consider to rewrite it to use dplyr - in particular piping and do.  However that may be to much work.


Simple MAC converter I'm new to Python and experimenting with some simple scripts. I would appreciate if you could give me some suggestions on how the following code can be improved or made simpler or anything in general.

#Coverts MAC Addresses into cisco format

def mac_conv(mac, sep='-'):
  #split mac address into 3 groups
  splitted_mac = mac.split(sep)
  gr1 = ''.join(splitted_mac[0:2])
  gr2 = ''.join(splitted_mac[2:4])
  gr3 = ''.join(splitted_mac[4:])

  #join groups into a single MAC
  mac = gr1, gr2, gr3
  final_mac = '.'.join(mac)

  #print final MAC address
  print(final_mac)

#Open file with MAC addresses and convert them
with open('mac.txt', 'r') as f:
  for mac in f.readlines():
    mac_conv(mac)

First of all, you should be using return rather than print to get a value out of a function, otherwise you'll have problems when you try to expand the program.

def mac_conv(mac, sep='-'):

  #etc

  #return final MAC address
  return final_mac

#Open file with MAC addresses and convert them
with open('mac.txt', 'r') as f:
  for mac in f.readlines():
    print(mac_conv(mac))

I would also avoid reusing variables, especially if its a function argument (mac).  You can actually eliminate that usage of the variable all together:

#join groups into a single MAC
final_mac = '.'.join([gr1, gr2, gr3])

If I was writing this, however, I would probably do it in just a couple of lines, as you know exactly how the input and output strings are going to be formatted:

def mac_conv(mac, sep='-'):
    mac_parts = mac.split(sep)
    return "{0[0]}{0[1]}.{0[2]}{0[3]}.{0[4]}{0[5]}".format(mac_parts)

If this was going into a product, I would probably add some error handling too, to allow for input that isn't a mac address - too short, too long, wrong separators, alternate groupings (a cisco-formatted address is still a valid mac address, but this function won't handle it.)


Attendance tracker design MySQL The task is to create a website which allows the admin to create classes, students and teachers. He/She should be able to assign students to classes and one teacher. The admin should also be able to register students attendance using a register. Classes should have specified dates as to when they are running.

The teachers that have been provided a login should be able to login, see the classes assigned to them and take the register. Any feedback would be appreciated!

CREATE TABLE IF NOT EXISTS admin_login (
    id INT UNSIGNED AUTO_INCREMENT PRIMARY KEY,
    user_name VARCHAR(20) NOT NULL,
    password VARCHAR(40) NOT NULL
);

INSERT INTO admin_login (user_name, password) VALUES
('admin', SHA1('password')) ;

CREATE TABLE IF NOT EXISTS classes (
    class_id INT UNSIGNED AUTO_INCREMENT PRIMARY KEY,
    class_name VARCHAR(30) NOT NULL,
    subject_name VARCHAR(30) NOT NULL,
    meeting_dates VARCHAR(900) NOT NULL,
    student_ids VARCHAR(900) NOT NULL,
    teacher_id VARCHAR(50) NOT NULL,
    completed BOOL,
    FOREIGN KEY (teacher_id)
        REFERENCES teachers (teacher_id)
);

CREATE TABLE IF NOT EXISTS students (
    student_id INT UNSIGNED AUTO_INCREMENT PRIMARY KEY,
    first_name CHAR(30) NOT NULL,
    last_name CHAR(30) NOT NULL,
    profile_picture LONGBLOB NOT NULL
);

CREATE TABLE IF NOT EXISTS teachers (
    teacher_id INT UNSIGNED AUTO_INCREMENT PRIMARY KEY,
    first_name CHAR(30) NOT NULL,
    last_name CHAR(30) NOT NULL,
    profile_picture LONGBLOB NOT NULL,
    username VARCHAR(20) NOT NULL,
    password VARCHAR(40) NOT NULL
);

CREATE TABLE IF NOT EXISTS attendance (
    id INT PRIMARY KEY AUTO_INCREMENT,
    student_id INT REFERENCES students (student_id),
    class_id INT,
    attendence_outcome CHAR,
    date CHAR
);

A few thoughts:

You shouldn't have the student ID's as a varchar(900) in the classes table. This is a classic many to many relationship. You should create a bridge entity that will have a record for each class-student relationship. This will allow you to get all the student's in a class without needing to parse a varchar.
Instead of a varchar with all of the meeting date, you would do something similar to what I mentioned in regards to the students. In a separate table create a separate record for each meeting date.
What I mentioned in 1 and 2 will now help make the attendance table a bit more compact. The date and class_id can be removed and replaced using the unique id from the bridge entity that has all the classes meeting date.
In the classes table, the BOOL completed can be a calculated field by checking if the the last meeting date has already passed or not (ie. the current date > last meeting date).


Initializing a window in Cocoa This is some of the code I have:

[window setLevel:kCGScreenSaverWindowLevel];
[window setOpaque:NO];
[window setStyleMask:0];
[window setBackgroundColor:[NSColor colorWithCalibratedWhite:0.0 alpha:0.3]];
[window setAlphaValue:0];

[window setFrame:[window frameRectForContentRect:[[window screen] frame]] display:YES animate:YES];

[window makeKeyAndOrderFront:self];
[[window animator] setAlphaValue:1.0];

I was just wondering if there was any way to compact it, all of those commands to my window. Any ways to improve it too?

This is a highly readable style, and simple. You might be able to make a loop and run through the list in some fashion, but it's unlikely to actually lower complexity, just shift it around a bit.


Login system created with Python This is a program that I created using Python. Please help me optimize the code.

import time
import sys
import socket
from time import localtime, strftime

# start up system

def start():
    print("Welcome, please choose what to do")
    choice = input('register/login ')
    if choice == 'register':
        register()
    elif choice == 'login':
        username()
    elif choice == 'admin':
        username()
    else:
        problems()

def register():
    registeragain = False
    global user
    global passs
    user = input('Please register your username : ')
    passs = input('Please register your password : ')
    register = input('Do you want to register another account? Y/N : ')
    if register == 'Y':
        registeragain = True
        register()
    elif register == 'N':
        username()
    else:
        print("Error")
        problems()

# login system

def username():
    global admin
    admin = 'Max'
    userinputname = input('Please enter the username : ')
    if userinputname == admin or userinputname == user:
        password()
    elif userinputname != user:
        print('Wrong username, please try again')
        username()
    else:
        problems()

def password():
    x = 1
    global adminpass
    adminpass = 'Luo'
    userinputpass = input('Please enter the password : ')
    if userinputpass == adminpass:
        adminlogin()
    elif userinputpass == passs:
        loading()
    elif userinputpass != passs:
        print('Wrong password, please try again')
        x += 1
        if x > 2:
            exit

    else:
        problems()

# main menu

def loading():
    toolbar_width = 40
    # setup toolbar
    sys.stdout.write("[%s]" % (" " * toolbar_width))
    sys.stdout.flush()
    # return to start of line, after '['
    sys.stdout.write("\b" * (toolbar_width+1))

    for i in range(toolbar_width):
        time.sleep(0.1)  # do real work here
        # update the bar
        sys.stdout.write("-")
        sys.stdout.flush()

    sys.stdout.write("]\n")  # this ends the progress bar
    main()

def main():
    time.sleep(3)
    print("Welcome to the main screen")
    time.sleep(3)
    print("Menu: current time, ipchecker, quit")
    time.sleep(2)
    mainmenu = input('Enter: ')
    if mainmenu == 'current time':
        telldate()
    elif mainmenu == 'ipchecker':
        ipchecker()
    elif mainmenu == 'quit':
        leave()
    elif mainmenu == 'cal':
        cal()
    else:
        problems()

# solutions

def problems():
    print("We are truly sorry that you have experienced a problem, please contact us at 123pythonlogin@gmail.com")
    option = input('Would you like to return to the main menu? Y/N ')
    if option == 'Y':
        main()
    else:
        exit()

# admin
def adminlogin():
    print("Welcome admin")
    mainadmin = input('Enter: ')
    if mainadmin == 'current time':
        telldate()
    elif mainadmin == 'ipchecker':
        ipchecker()
    elif mainadmin == 'quit':
        leave()
    elif mainadmin == 'cal':
        cal()
    else:
        problems()

# functions

def telldate():
    localtime = time.asctime(time.localtime(time.time()))
    print("Local current time :", localtime)
    main()

def ipchecker():
    hostname = socket.gethostname()
    IPAddr = socket.gethostbyname(hostname)
    print("Your Computer Name is:" + hostname)
    print("Your Computer IP Address is: " + IPAddr)
    main()

def cal():
    numx = int(input('Enter the first number: '))
    numy = int(input('Enter the second number: '))
    op = input('Enter the operation(+, -, *, /): ')
    if op == '+':
        sum = numx + numy
        print('Your sum is ' , sum)
        main()
    elif op == '-':
        diff = numx - numy
        print('The difference is ',diff)
        main()
    elif op == '*':
        mult = numx * numy
        print('The multiple is ' ,mult)
        main()
    elif op == '/':
        divd = numx / numy
        print('The result is ' ,divd)
        main()
    else:
        problems()

def leave():
    exit()

start()

Some general notes:

Global variables are usually a bad idea; state that can be accessed from everywhere is hard to read and debug because you have no idea what random function might modify it.  It's extra difficult here because your globals are declared in different places.
The standard way of handling errors in Python is to raise an exception that can be caught and handled.
Long sequences of if/elif are usually an indication that your logic can either be simplified or split up.

Here's how I might suggest writing your start and register functions, using exceptions to handle errors, a lookup table to handle dispatching your different functions, and a single state object that collects all the random global variables:

from typing import NamedTuple

class LoginState(NamedTuple):
    admin: str
    adminpass: str
    user: str
    pass: str

def start() -> None:
    state = LoginState("Max", "Luo", "", "")
    print("Welcome, please choose what to do")
    menu = {
        'register': register
        'login': username
        'admin': username
    }
    choice = input('register/login ')
    try:
        menu[choice](state)
    except:
        problems()

def register(state: LoginState) -> None:
    state.user = input('Please register your username : ')
    state.pass = input('Please register your password : ')
    menu = {
        "Y": register
        "N": username
    }
    choice = input('Do you want to register another account? Y/N : ')
    menu[choice](state)

Defining your state as a type means that it's easier to refactor -- for example, suppose you realize that your register logic overwrites the username and what you actually want to do is be able to have more than one!  You could change user and pass into a collection (like a Dict, say), and use mypy to tell you all the places in your program that those attributes are used so you can make the appropriate updates.  If they're defined as globals, it's a lot more difficult to make sure you've updated all the places where they're used.

The menu dict is IMO a little easier to expand (and read) than the big chain of if/elif, by providing a single obvious place where the menu options are defined.

Having problems() be an exception handler means that not only does it catch the case where the user inputs an invalid command (this will cause menu[choice] to raise an IndexError), but it will also catch any unhandled exceptions raised by your other functions, so those functions don't need to call problems() themselves; all they have to do is raise an exception and the control flow will end up right here.  In the case of register() I didn't have to do anything with the error case, because if an exception is raised there I know that it will be caught in start().

Since the "present menu, make selection" pattern is probably going to be repeated a few times in this program, I might want to make that into a function as well:

def menu_prompt(
    menu: Dict[Text, Callable[[State], None]],
    prompt: str,
    state: LoginState
) -> None:
    choice = input(prompt)
    menu[choice](state)

and now this:

    menu = {
        'register': register
        'login': username
        'admin': username
    }
    choice = input('register/login ')
    try:
        menu[choice](state)
    except:
        problems()

becomes:

try:
    menu_prompt({
        'register': register
        'login': username
        'admin': username
    }, 'register/login ', state)
except:
    problems()


Dealing with Entity Framework's lack of enum support I'm using Entity Framework Code First CTP 5 for a current project. There is still no enum support in Entity Framework so this is what I am using in order to avoid having magic numbers or casts littered throughout my code.

Simplified Example:

Domain Model Consumed by Code First

public class Address
{
    // This would normally be an enum
    public int Type { get; set; }
}

Repository

public class AddressRepository
{
    public Add(Address address)
    {
        // Compare using enum semantics
        if (address.Type == AddressType.Home)
            // Do something

        context.Add(address);
    }
}

View Model

public class AddressVM
{
    // Using the AddressType class for the view model
    public AddressType Type { get; set; }

    public AddressVM(Address address)
    {
        this.Type = address.Type;
    }
}

Enum Replacement Class

public interface IEnumClass
{
    List<string> Properties { get; }
}

public class AddressType : IEnumClass
{
    public static int Home { get { return 0; } }
    public static int Work { get { return 1; } }

    public List<string> Properties { get { return properties; } }

    private List<string> properties;
    private int id;

    public AddressType()
    {
        SetProperties();
    }
    public AddressType(int id)
    {
        this.id = id;
        SetProperties();
    }

    private void SetProperties()
    {
        properties = this.GetType().GetProperties(BindingFlags.Public|BindingFlags.Static)
            .Select(x => x.Name).ToList();
    }

    public static implicit operator AddressType(int id)
    {
        return new AddressType(id);
    }

    public static implicit operator String(AddressType addressType)
    {
        return addressType.properties[addressType.id];
    }

    public static implicit operator int(AddressType addressType)
    {
        return addressType.id;
    }
}

My Personal Assessment

Pros:

Allows me to write business rules without using magic numbers
Uses the same semantics/syntax as enums for easy refactoring down the road
Allows for an implicit conversion to a string for the view model
My select list factory class can easily be made to accept any IEnumClass and generate a select list for the view model

Cons:

A little verbose
As far as I can tell it is impossible to use a nice abstract generic class because all the actual work uses implicit type casting

I'm asking for feedback here because I'm a total novice programmer so I want to solicit some critiques before implementing something I just came up with!

I keep seeing all these really complex solutions to the lack of enum support, but most make things tricky when taking into account that next entity framework release is probably going to have enum support.

Here's my solution. A sample class, before our workaround:

public class Widget
{
    public int ID { get; set; }
    public Status Status { get; set; }
}

Obviously Status won't be saved as it's an enum, and if we provide a fancy workaround, things will probably break when we upgrade to the next entity framework release, as it'll suddenly start trying to read and write that property. So we do the following:

public class Widget
{
    public int ID { get; set; }
    [NotMapped] // stop future entity frameworks from accidentally using this column
    public Status Status { get; set; }

    [Column("Status")]
    private int StatusEnum
    {
        get { return (int)Status; }
        set { Status = (Status)value; }
    }
}

The new property is private so we can't see it externally, so it doesn't exist to the outside world for all intents and purposes. The entity framework can see it as you've marked it with a [Column] attribute and so it'll be found via reflection. The original enum column will be skipped even if we upgrade to a later version of the framework, and then all we have to do is remove those few lines of workaround code and things should still work seamlessly.

The result?

The class looks the same to anything consuming it, but now enums are saved and loaded
No additional code complexity
Future-proofed against entity framework upgrades
Super easy to remove the additional code when upgrading to a version of the framework that supports enum persistence


SICP exercise 1.28 - miller-rabin primality test From SICP

  Exercise 1.28:

  One variant of the Fermat test that cannot be fooled is
  called the Miller-Rabin test (Miller 1976; Rabin 1980). This starts
  from an alternate form of Fermat’s Little Theorem, which states that
  if \$n\$ is a prime number and \$a\$ is any positive integer less than \$n\$, then
  \$a\$ raised to the \$n-1\$-st power is congruent to \$1 \mod n\$.

  To test the
  primality of a number \$n\$ by the Miller-Rabin test, we pick a random
  number \$a < n\$ and raise \$a\$ to the \$n-1\$-st power \$\mod n\$ using the expmod
  procedure. However, whenever we perform the squaring step in expmod,
  we check to see if we have discovered a “nontrivial square root of \$1
\mod n\$,” that is, a number not equal to \$1\$ or \$n-1\$ whose square is
  equal to \$1 \mod n\$.

  It is possible to prove that if such a nontrivial
  square root of \$1\$ exists, then \$n\$ is not prime. It is also possible to
  prove that if \$n\$ is an odd number that is not prime, then, for at least
  half the numbers \$a < n\$, computing \$a^{n-1}\$ in this way will reveal a
  nontrivial square root of \$1 \mod n\$. (This is why the Miller-Rabin
  test cannot be fooled.)

  Modify the expmod procedure to signal if it
  discovers a nontrivial square root of \$1\$, and use this to implement the
  Miller-Rabin test with a procedure analogous to fermat-test. Check
  your procedure by testing various known primes and non-primes.

  Hint:
  One convenient way to make expmod signal is to have it return \$0\$.

According to the book this is a probabalistic algorithm. My tests gave mostly correct results.

Here's my code.

;; modified expmod procedure
(define (expmod base exp m)
  (cond ((= exp 0) 1)
        ((even? exp)
         (if (and (not (= base 1))
                  (not (= base (- m 1)))
                  (not (= exp (- m 1)))
                  (= (remainder base m) 1))
             0
             (remainder
              (expmod (square base) (/ exp 2) m)
              m)))
        (else
         (remainder
          (* base (expmod base (- exp 1) m))
          m))))

(define (miller-rabin-test n)
  (define (try-it a)
    (= (expmod a (- n 1) n) 1))
  (try-it (+ 1 (random (- n 1)))))

(define (fast-prime? n times)
  (cond ((= times 0) true)
        ((miller-rabin-test n)
         (fast-prime? n (- times 1)))
        (else false)))

How can I improve this code and make it faster?

To ensure I understood the SICP exercise correctly, I wrote my own version of the Miller-Rabin algorithm. You can look at my code and compare against yours.

With this in mind, here's my correctness-related feedback:

Your test expression, (and (not (= base 1)) (not (= base (- m 1))) (not (= exp (- m 1))) (= (remainder base m) 1)), seems to muddle up the relationship between the previous and current values. Really, what you want to find out is whether the previous value is between 2 and (- m 2) (mod m) and the current value is 1 (mod m).
Your (remainder (expmod (square base) (/ exp 2) m) m) is not optimal. All values returned by expmod should already be mod m so doing a further remainder call is pointless. You should, however, clamp the result of square to mod m.

Other style notes:

Not sure why you didn't just use a one-liner for the miller-rabin-test function:

(define (miller-rabin-test n)
  (= (expmod (+ 1 (random (- n 1))) (- n 1) n) 1))

or, if you're trying to break up the expression a bit,

(define (miller-rabin-test n)
  (define a (+ 1 (random (- n 1))))
  (= (expmod a (- n 1) n) 1))

No need for an internal function!
You can rewrite the main loop as a single or/and expression:

(define (fast-prime? n times)
  (or (zero? times)
      (and (miller-rabin-test n)
           (fast-prime? n (- times 1)))))


FIFO queue for a 2D array in ANSI C I have a 2D array of fixed size and I've implemented a FIFO function. So, the array will always be full before it reaches the FIFO function, then the FIFO will remove the oldest array, move all of the arrays up by one and then add the newest array to the other end. Basically a queue. Now, this code will actually be running on a microcontroller, an array will come from an ADC and then I store the ADC array in a 2D array. To design the FIFO algorithm in simple C first, I have written a program that generates random numbers and then stores them into a 2D buffer. The program then asks the user what new values should be stored, user puts in the latest array of values and then the program removes the oldest array, shuffles all the remaining arrays up by one and then the latest user numbers get added to the end. The program does this 10 times. This is designed to simulate what is happening in the microcontroller code. The 2D array is already full with values and then the ADC adds the latest set of values to the array, and the oldest set are forgotten.
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
int fixed_number[5] = { 0 };
uint16_t big_array[10][5] = { 0 };
uint16_t big_array_copy[10][5] = { 0 };
uint16_t small_array_1[5] = { 0 };
uint16_t k[1] = { 0 };
uint8_t count = 0;
int main()
{
    for (int j = 0; j < 10; j++)
    {
        for (int i = 0; i < 5; i++)
        {
            big_array[j][i] = rand() % 255;
        }
    }
    for (int j = 0; j < 10; j++)
    {
        for (int i = 0; i < 5; i++)
        {
            big_array_copy[j][i] = big_array[j][i];
        }
    }
    printf("These are the original numbers in the big array:\n");
    for (int j = 0; j < 10; j++)
    {
        for (int i = 0; i < 5; i++)
        {
            k[0] = big_array[j][i];
            printf("%d,", k[0]);
                    }
        printf("\n");
            }
    while (count < 10)
    {
        printf("Please enter values for queue:\n");
        for (int i = 0; i < 5; i++)
        {
            scanf_s("%d", &fixed_number[i]);
        }
           printf("\n\n");
        printf("Array values have been swapped:\n ");
        for (int k = 0; k < 5; k++)
        {
            big_array_copy[10 - 1][k] = 0;
        }
                for (int j = 9; j > 0; j--)
        {

            for (int i = 0; i < 5; i++)
            {
                //small_array_1[i] = big_array[j + 1][i];
                big_array_copy[j][i] = big_array[j - 1][i];
            }
        }
        for (int i = 0; i < 5; i++)
        {
            big_array_copy[0][i] = fixed_number[i];
        }
        for (int j = 0; j < 10; j++)
        {
            for (int i = 0; i < 5; i++)
            {
                k[0] = big_array_copy[j][i];
                printf("%d,", k[0]);
            }
            printf("\n");
        }
        for (int j = 0; j < 10; j++)
        {
            for (int i = 0; i < 5; i++)
            {
                big_array[j][i] = big_array_copy[j][i];
            }
        }
        count++;
    }
}

Now this code works perfectly fine, but there is a lot of loops and it also requires twice the memory space as the buffer, big_array[][] must be copied in to big_array_copy[][]. In the real application, the buffer is uint16_t somebuffer[100][45] which is 4.5kb. I've got 512kb of SRAM to use, but I'd rather use as little as I can.
The above method can be implemented on my target platform (STM32F767 for those wondering) but is there a more simple method to implementing the above, that uses fairly portable c with no special functions, that could reduce the memory required and the processing time required?
thanks

We're missing an include of <stdint.h>.  On the other hand, we include  <time.h> which we're not using.
The code doesn't compile here because of the use of Annex K scanf_s() without checking for its availability.  There's no reason not to use ordinary scanf() here (but do examine its return value - that's important!)
There are a lot of magic constants sprinkled throughout the code, with no indication whether they relate to each other.  That makes this an unmaintainable program, because you can't change any of those individually without understanding the whole program.
Also, with all the functionality thrown together in main(), that makes it harder to understand the purpose of each part of the code.  Please divide the code into well-named functions (probably with static linkage).
I recommend fixing those before posting another review request to look at this in any depth.


Smart pointer mark II - my answer to C++ 11 shared_ptr I am writing this smart pointer as a learning exercise.  Any feedback would be most appreciated.

Any flaws? Have I missed any test cases?

smart_pointer.hpp:

#ifndef SMARTPOINTER_HPP_
#define SMARTPOINTER_HPP_

#include <iostream>

template<typename T>
class sp {
public:
   sp(T* ptr) : ptr_(ptr), ref_cnt_(new unsigned(1)) {
      std::cout << "sp ctor. Address: " << ptr_ << ", ref_cnt_: " << *ref_cnt_ << '\n';
   }

   T& operator*() { return *ptr_; }
   const T& operator*() const { return *ptr_; }

   T* operator->() { return ptr_; }
   const T* operator->() const { return ptr_; }

   // access to raw ptr
   T* get() { return ptr_; }

   sp(const sp<T>& rhs) : ptr_(rhs.ptr_), ref_cnt_(rhs.ref_cnt_) {
      addref();
      std::cout << "sp copy ctor: " << ptr_ << ", ref_cnt_: " << *ref_cnt_ << '\n';
   }

   sp& operator=(const sp& rhs) {
      if(this != &rhs) {
         try_delete();
         ptr_ = rhs.ptr_;
         ref_cnt_ = rhs.ref_cnt_;
         addref();
         std::cout << "sp assignment: " << ptr_ << ", ref_cnt_: " << *ref_cnt_ << '\n';
      }
      return *this;
   }

   ~sp() {
        std::cout << "~sp. Address: " << ptr_ << ", ref count before remove_ref: " << *ref_cnt_ << '\n';
        try_delete();
   }

private:
   unsigned addref() {
      return ++*ref_cnt_;
   }
   unsigned remove_ref() {
      return --*ref_cnt_;
   }

   void try_delete() {
      if(remove_ref() == 0) {
         std::cout << "try_delete(): finally deleting ptr_ at address: " << ptr_ << ", ref count: " << *ref_cnt_ << '\n';
         delete ref_cnt_;
         delete ptr_;
      }
   }

   T* ptr_;                    // raw ptr
   unsigned* ref_cnt_;  // shared ownership
};

#endif //SMARTPOINTER_HPP_

Code to exercise:

#include <iostream>
#include <vector>

#include "smart_pointer.hpp"

// useless class just used to test with object more complex than int
class tester {
public:
   tester(const int id, const char* name) : id_(id), name_(name) {}

   const char* const get_name() const { return name_; }
   const int get_id() const { return id_; }

private:
   const int id_;
   const char* const name_;
};

// global vector to hold smart pointer
std::vector< sp<int> > gvs;

int main() {

   {
      std::cout << "Test with ints\n";
      sp<int> myptr1 = new int(3);
      *myptr1 = 4;
      std::cout << "number: " << *myptr1 << '\n';

      {
         // test usage in collection
         std::vector< sp<int> > vs;
         vs.push_back(myptr1);
      }

      // global collection
      gvs.push_back(myptr1);

      // copying
      sp<int> myp2(myptr1);
      sp<int> myp3(myptr1);
      sp<int> myp4 = myptr1;
   }

   {

      std::cout << "Test with more complex objects\n";
      sp<tester> spt = new tester(1, "Keira Knightley");

      tester* p = spt.get();
      std::cout << "Name: " << p->get_name() << ", id: " << p->get_id() << '\n';

      // copying
      sp<tester> spt2 = spt;
      sp<tester> spt3(spt);

      sp<tester> spt4(new tester(2, "Cate Blanchett"));

      // re-assign
      spt = new tester(3, "Natalie Portman");

   }

   return 0;
}

(1) Style nit: Prefer #pragma once to fragile #ifndef-based include guards. #pragma once isn't yet part of the standard; but any compiler that supports C++11 will definitely have supported #pragma once for years already.

(2) Obviously, remove the std::cout usage from your constructors when using this class for real. :)

(3) This is a MAJOR issue, and a common one, which is why I want to make a big deal of it:

T& operator*() { return *ptr_; }
const T& operator*() const { return *ptr_; }

No no no no no. You should provide only one operator*, and it should look like this:

T& operator*() const { return *ptr_; }

The act of dereferencing a (smart) pointer does not modify the pointer object, and therefore should be a const method of that pointer object. Also, the result of dereferencing a (smart) pointer to T had darn well better be a reference to T! Adding const to the return type is just plain wrong.

This is the same confusion that people often have between int * const and int const *, or between std::vector<int>::iterator const and std::vector<int>::const_iterator. In your case the confusion is between sp<int> const and sp<int const>.

(4) As above, you should have only one operator-> and one get(), and both should be const methods:

T* operator->() const { return ptr_; }
T* get() const { return ptr_; }

(5) Your constructor should almost certainly be explicit:

explicit sp(T* ptr) : ptr_(ptr), ref_cnt_(new unsigned(1))

The exception-safety guarantees here look good to me. I would argue that size_t would be more appropriate than unsigned (you're probably paying for a 16-byte allocation anyway, so why not use that space?), but that's a nitpick.

If you really care about memory allocations, and want to keep learning about how the standard shared_ptr does things, you should try to implement the equivalent of std::make_shared for your smart pointer type, so that the memory allocations can be wrapped up together into a single call to new. I'd imagine that the tricky parts of make_shared have to do with alignment requirements.

(6) Your ref_cnt_ doesn't point to a std::atomic<unsigned>, so addref() is not atomic; copying the same sp from two threads at once may result in undefined behavior.

(7) In C++11, it would be advisable to implement a move constructor and a move assignment operator in order to avoid a pointless addref() and removeref() when moving an rvalue. However, there's no problem with correctness in C++11; because you implement a user-defined copy constructor, the compiler will correctly suppress the generation of default move constructor and assignment functions (which would do the Wrong Thing if they were generated, so it's a good thing they're not).

(8) Your copy assignment operator performs unnecessary operations in the case that x is being assigned from a copy of x; for example:

sp<int> x(new int);
sp<int> y = x;  // adds a ref, correctly
x = y;  // removes a ref, then adds a ref; could be implemented as a no-op instead

(9) You don't use remove_ref except inside try_delete. Personally, I would inline the one call to remove_ref, and then rename try_delete to remove_ref. I would also rename addref to add_ref for consistency, and remove its unused return value.

(Future) To continue building your smart pointer into something like the standard's std::shared_ptr, consider the following use-cases:

sp<int> s(nullptr);  // or (int *)NULL
s = nullptr;
s.reset();  // same as =nullptr

Consider inheritance, upcasting and downcasting. This will require fleshing out your ref_cnt_ pointer from unsigned* to struct ref_cnt*, and careful consideration of what metadata you need to keep in there.

class Mother { int m; };
class Father { int f; };
class Child : public Mother, public Father { int c; };

sp<Child> ch(new Child);
sp<Father> fa = ch;  // casting derived to base should work
ch.reset();
fa.reset();  // this should call ~Child on the correct pointer

Consider providing "weak references", which the standard library provides as std::weak_ptr. Weak references (weak_ptr) and strong references (shared_ptr) work tightly together, using the same infrastructure.

Consider providing the ability to pass in a custom deleter:

sp<char> buf(malloc(1024), (void(char*))free);
buf.reset();  // this should call free on the correct pointer


Integer quicksort in Java I have this Quicksort implementation that sorts arrays of int (not Integer). It has comparable performance to Java's DualPivotQuicksort, especially when the size of the range is below one million elements or so.
IntegerQuicksort.java
package net.coderodde.util;

import java.util.Arrays;
import java.util.Random;

public class IntegerQuicksort {

    public static void sort(int[] array) {
        sort(array, 0, array.length);
    }

    public static void sort(int[] array, int fromIndex, int toIndex) {
        if (toIndex - fromIndex < 2) {
            return;
        }

        int pivot = array[fromIndex];
        int leftPartitionLength = 0;
        int rightPartitionLength = 0;
        int index = fromIndex;

        while (index < toIndex - rightPartitionLength) {
            int current = array[index];

            if (current > pivot) {
                ++rightPartitionLength;
                int tmp = array[toIndex - rightPartitionLength];
                array[toIndex - rightPartitionLength] = current;
                array[index] = tmp;
            } else if (current < pivot) {
                int tmp = array[fromIndex + leftPartitionLength];
                array[fromIndex + leftPartitionLength] = current;
                array[index] = tmp;

                ++index;
                ++leftPartitionLength;
            } else {
                ++index;
            }
        }

        sort(array, fromIndex, fromIndex + leftPartitionLength);
        sort(array, toIndex - rightPartitionLength, toIndex);
    }

    private static final int SIZE = 500_000;
    private static final int FROM = 100;
    private static final int TO = SIZE - 100;

    public static void main(final String... args) {
        long seed = System.nanoTime();
        Random random = new Random(seed);
        int[] array1 = getRandomArray(SIZE, -1000, 1000, random);
        int[] array2 = array1.clone();

        System.out.println("Seed: " + seed);
        long startTime = System.nanoTime();
        sort(array1, FROM, TO);
        long endTime = System.nanoTime();

        System.out.printf("IntegerQuicksort.sort in %.2f milliseconds.\n",
                          (endTime - startTime) / 1e6);

        startTime = System.nanoTime();
        Arrays.sort(array2, FROM, TO);
        endTime = System.nanoTime();

        System.out.printf("Arrays.sort in %.2f milliseconds.\n",
                          (endTime - startTime) / 1e6);

        System.out.println(Arrays.equals(array1, array2));
    }

    public static int[] getRandomArray(int size,
                                       int minimum,
                                       int maximum,
                                       Random random) {
        int[] array = new int[size];

        for (int i = 0; i < size; ++i) {
            array[i] = random.nextInt(maximum - minimum + 1) + minimum;
        }

        return array;
    }
}

Some performance figures:

Seed: 347202193766632
IntegerQuicksort.sort in 61.21 milliseconds.
Arrays.sort in 131.72 milliseconds.

I am well aware that the current pivot selection rule will make the algorithm degrade to quadratic running-time on sorted input; please ignore this, I wanted to experiment a little bit.
So, is there room for improvement? Naming? Coding style? Please, tell everything that comes to mind.

First of all, your benchmark is not really correct because it doesn't take into account any VM warmup and has only a single iteration.

Using JMH 1.11.3, I rewrote your benchmark: two integer arrays of length 10.000, 100.000, 1.000.000 and 10.000.000 are created with random values and then sorted with your implementation and Arrays.sort. The results are (Windows 10, JDK 1.8.0_66 64 bits, i5-3230M CPU @ 2.60GHz):

Benchmark            (length)  Mode  Cnt     Score    Error  Units
SortTest.arraysSort     10000  avgt   30     0,557 ±  0,015  ms/op
SortTest.arraysSort    100000  avgt   30     7,369 ±  0,267  ms/op
SortTest.arraysSort   1000000  avgt   30    86,435 ±  3,273  ms/op
SortTest.arraysSort  10000000  avgt   30  1039,120 ± 49,706  ms/op
SortTest.customSort     10000  avgt   30     0,905 ±  0,029  ms/op
SortTest.customSort    100000  avgt   30    11,152 ±  0,391  ms/op
SortTest.customSort   1000000  avgt   30   132,986 ±  5,581  ms/op
SortTest.customSort  10000000  avgt   30  1530,132 ± 42,771  ms/op

This shows that while Arrays.sort is indeed a little faster, your implementation has a very good performance, for small and large arrays.

I find your code is really good and easy to read. Only a few remarks if you intent to keep this code around:

The method sort(int[] array, int fromIndex, int toIndex) does not do any sanity checks on fromIndex and toIndex. This is well understandable because it introduces another level of complexity. You should then consider making that method private instead of public: this way, you ensure that no out of bounds exception can occur through the public API.
Consider perhaps renaming the utility class IntQuicksort to make it clear that is operates on primitive int array and not Integer objects. Also, since this is a utility class, consider making it final and adding a private constructor.
Regarding the code itself, I would add a swap method to refactor the code a little

private static void swap(int[] array, int firstIndex, int secondIndex) {
    int tmp = array[firstIndex];
    array[firstIndex] = current;
    array[secondIndex] = tmp;
}

and use it like

if (current > pivot) {
    ++rightPartitionLength;
    swap(array, toIndex - rightPartitionLength, index);
} else if (current < pivot) {
    swap(array, fromIndex + leftPartitionLength, index);
    ++index;
    ++leftPartitionLength;
}

Code of benchmark for completeness:

import java.util.Arrays;
import java.util.concurrent.ThreadLocalRandom;
import java.util.concurrent.TimeUnit;

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Level;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Param;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;

@Warmup(iterations = 10, time = 700, timeUnit = TimeUnit.MILLISECONDS)
@Measurement(iterations = 10, time = 700, timeUnit = TimeUnit.MILLISECONDS)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.MILLISECONDS)
@Fork(3)
public class SortTest {

    @State(Scope.Benchmark)
    public static class ArrayContainer {

        @Param({ "10000", "100000", "1000000", "10000000" })
        private int length;

        private int[] array;
        private int[] arrayToSort;

        @Setup(Level.Iteration)
        public void setUp() {
            ThreadLocalRandom random = ThreadLocalRandom.current();
            array = random.ints(length).toArray();
        }

        @Setup(Level.Invocation)
        public void cloneArray() {
            arrayToSort = array.clone();
        }

    }

    public static void sort(int[] array) {
        sort(array, 0, array.length);
    }

    public static void sort(int[] array, int fromIndex, int toIndex) {
        if (toIndex - fromIndex < 2) {
            return;
        }

        int pivot = array[fromIndex];
        int leftPartitionLength = 0;
        int rightPartitionLength = 0;
        int index = fromIndex;

        while (index < toIndex - rightPartitionLength) {
            int current = array[index];

            if (current > pivot) {
                ++rightPartitionLength;
                int tmp = array[toIndex - rightPartitionLength];
                array[toIndex - rightPartitionLength] = current;
                array[index] = tmp;
            } else if (current < pivot) {
                int tmp = array[fromIndex + leftPartitionLength];
                array[fromIndex + leftPartitionLength] = current;
                array[index] = tmp;

                ++index;
                ++leftPartitionLength;
            } else {
                ++index;
            }
        }

        sort(array, fromIndex, fromIndex + leftPartitionLength);
        sort(array, toIndex - rightPartitionLength, toIndex);
    }

    @Benchmark
    public int[] customSort(ArrayContainer container) {
        sort(container.arrayToSort);
        return container.arrayToSort;
    }

    @Benchmark
    public int[] arraysSort(ArrayContainer container) {
        Arrays.sort(container.arrayToSort);
        return container.arrayToSort;
    }

}


From procedural to OOP I recently discover by reading some blog topics on web that 90% of code lines I produced in my career was procedural and not OOP oriented (big slap in the head). So now, I try to lobotomy my brain in order to produce only OOP codes. The problem is that I have a lot of questions about OOP practice.

Here is one :

I got a factory RawLineCollectionFactory that take string lines and create from it a domain entity called RawLineCollection. My question is about argument type pass to the Create method.

Should I pass it the object (IDataSource) that allows me to retrieve the string lines like that :

public class RawLineCollectionFactory : IRawLineCollectionFactory
{
    public IRawLineCollection Create(IDataSource dataSource)
    {
        ICollection<RawLine> lineGroup = new List<RawLine>();

        if (dataSource.GetLines() != null)
        {
            foreach (string line in dataSource.GetLines())
            {
                lineGroup.Add(new RawLine(line));
            }
        }

        return new RawLineCollection(lineGroup);
    }
}

Or call getLines() of the dataSource object on higher abstraction level and pass the result as argument of Create method ?

public class RawLineCollectionFactory : IRawLineCollectionFactory
{
    public IRawLineCollection Create(IEnumerable<string> lines)
    {
        ICollection<RawLine> lineGroup = new List<RawLine>();

        if (lines != null)
        {
            foreach (string line in lines)
            {
                lineGroup.Add(new RawLine(line));
            }
        }

        return new RawLineCollection(lineGroup);
    }
}

Edit :

The context is very close to the application entry point.
RawLine is an object representation of a line in a file that contain data settings for a game creation.

    public class RawLine
{
    private string value;

    public RawLine(string value)
    {
        this.value = value;
    }

    public IEnumerable<string> Split(string separator)
    {
        if (string.IsNullOrEmpty(separator))
        {
            throw new ArgumentException("The separator is required");
        }

        if (!string.IsNullOrEmpty(value))
        {
            return value
                .Replace(" ", "", StringComparison.CurrentCulture)
                .Split(separator)
                .Where(item => !string.IsNullOrEmpty(item));
        }

        return Array.Empty<string>();
    }
}

The behavior of this class is to split an actual representation of his state for the application-specific rules purpose while RawLineCollection is a collection of this entity type.

    public class RawLineCollection : IRawLineCollection
{
    private readonly IEnumerable<RawLine> lines;
    private readonly string separator = "-";

    public RawLineCollection(IEnumerable<RawLine> lines)
    {
        this.lines = lines;
    }

    public SplittedLineCollection Split()
    {
        ICollection<IEnumerable<string>> result = new List<IEnumerable<string>>();

        if (IsValidStringGroup(lines))
        {
            foreach (RawLine line in lines)
            {
                if (line.Split(separator).Any())
                {
                    result.Add(line.Split(separator));
                }
            }
        }

        return new SplittedLineCollection(result);
    }

    private bool IsValidStringGroup(IEnumerable<RawLine> settingsToSplit)
    {
        return settingsToSplit != null && settingsToSplit.Any();
    }
}

The RawLineCollectionFactory is used just one time like DataSource.GetLines() method.

Thanks for your question. IMHO, converting procedural code to OO code is an excellent thing to practice. Congratulations on your epiphany!

In general, the problem of persisting your game's data is a serialization problem. I would invite you to explore whether .NET's native "settings" functionality might be able to meet your needs. And beyond that, standard serialization to JSON or XML might be worth a look.

If you insist on "rolling your own" serialization, you can certainly do so in an object-oriented way. My take on getting started is below.

If you have complex data like game level definitions, you might be serializing quite a bit of data, which would be all the more reason to use a standard format like JSON or XML.

For serializing and deserializing a relatively simple config file, you can probably skip the factory pattern. But, if the config spans multiple files, including image blobs, etc., then a "Config Factory" might be worth considering.

I would recommend building one (or more) classes to properly model the actual config properties, and serializing/deserializing that instead of processing everything as strings.

For example:

public class Config
{
    public string PlayerName {get; set;}
    public DateTime LastPlayed {get; set;}
    public int HighScore {get; set;}
}

The below example takes a "hybrid" approach. It goes further than processing raw strings by using KeyValuePair, but stops short of a fully-typed Config class (as shown above).

Another quick tip: Instead of RawLineCollection, I'd call it RawLines, or even better, Lines.

Welcome to the wide world of OOP. In some of my other code review answers I go deeper into the principles that guide my OOP practice.

Here's the sample (this code compiles, but I didn't test it):

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;

public class App_ConfigFile
{
    /*  Config file example

        player1Name:Jane
        player2Name:Joe
        difficulty:medium
        lastPlayed:04/20/2020
        lastLevel:5
        highScore:31000
     */

    public void Run()
    {
        var file = new ConfigFile(@"c:\myGame\config.txt");
        var config = file.ToConfigData();
        var player1name = config.GetByName("player1Name");
        var lastPlayed = DateTime.Parse(config.GetByName("lastPlayed"));
        var highScore = int.Parse(config.GetByName("highScore"));
    }
}

public class ConfigFile
{
    private string path;
    private List<Line> lines;
    public List<Line> Lines => lines ?? (lines = getLines());
    public bool HasContent => Lines.Count > 0;

    public ConfigFile(string path) => this.path = path;

    public ConfigData ToConfigData(char separator = ':') => new ConfigData(Lines.Select(l => l.ToKvp(separator)).ToList());

    private List<Line> getLines() => File.ReadAllLines(path).Select(l => new Line(l)).ToList();
}

public class Line
{
    public string Raw { get; private set; }
    public Line(string line) => Raw = line;

    public KeyValuePair<string, string> ToKvp(char separator)
    {
        var tokens = Raw.Split(separator);
        return new KeyValuePair<string, string>(tokens.First(), tokens.Last());
    }
}

public class ConfigData
{
    private List<KeyValuePair<string, string>> data;
    private Dictionary<string, string> _dictionary;
    private Dictionary<string, string> dictionary => _dictionary ?? (_dictionary = data.ToDictionary(d => d.Key, d => d.Value, StringComparer.OrdinalIgnoreCase));

    public ConfigData(List<KeyValuePair<string, string>> data) => this.data = data;

    public string GetByName(string key) => dictionary[key];
    public bool TryGetByName(string key, out string value) => dictionary.TryGetValue(key, out value);
}

}


Adjusting the height of a div to match that of another column This code checks whether an image whose parent is .papers.left has completely loaded into DOM and if yes then the background of its right content's height has to be same as the background of this images parent. So below is html to understand the structure

Lorem Ipsum is simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's standard dummy text ever since the 1500s, when an unknown printer took a galley of type and scrambled it to make a type specimen book. It has survived not only five centuries, but also the leap into electronic typesetting, remaining essentially unchanged. It was popularised in the 1960s with the release of Letraset sheets containing Lorem Ipsum passages." Jogn De, Birthday Event

So to set same height I have written below js code which will set the height of div.papers.right same as div.papers.left but only when the image is loaded completely

$('img').each(function () {
    if ($(this).closest('.papers.left').length)
    {
        $(this).on('load', function () {
            $(this).closest('.row').find('.papers.right').height($(this).closest('.papers.left').height());
        })
    }
})

Can the above piece of jQuery code be optimized? I am not sure whether the above code can be optimized to some simple one.

Update
I just realized one more requirement so that whichever's height is greater assign that height to other one and I've made below changes to code and I would like to know if there is any improvement that can be done to the below code. Extremely sorry for the inconvenience.

$('img').each(function () {
    if ($(this).closest('.papers.left').length)
    {
        $(this).on('load', function () {
            if ($(this).closest('.papers.left').height() > $(this).closest('.row').find('.papers.right').height())
             //if left side has greater height assign it to right side
                $(this).closest('.row').find('.papers.right').height($(this).closest('.papers.left').height());
            else
            //vice versa
            $(this).closest('.papers.left').height($(this).closest('.row').find('.papers.right').height());
        })
    }
})

You can save $(this).closest('.papers.left') into a local variable since you access it twice. This might improve performance. It can also enhance code readability:

$('img').each(function () {
    var parent = $(this).closest('.papers.left');
    if (parent.length)
    {
        $(this).on('load', function () {
            $(this).closest('.row').find('.papers.right').height(parent.height());
        })
    }
})

As an add on to your code modification, I don't know that this is faster, but why not try assigning max height to both without checking which is taller? Something like:

$('img').each(function () {
    var parent = $(this).closest('.papers.left');
    if (parent.length)
    {
        $(this).on('load', function () {
            var sibling = $(this).closest('.row').find('.papers.right');
            var height = Math.max(parent.height(), sibling.height());
            parent.height(height);
            sibling.height(height);
        })
    }

})

It's certainly shorter and more readable. Why not test it out to see how the efficiency compares?


What's wrong with static utility classes, versus beans? My inclination is to make these methods static:

package net.bounceme.dur.usenet.driver;

import java.util.List;
import java.util.logging.Logger;
import javax.mail.Folder;
import javax.mail.Message;
import javax.persistence.*;
import net.bounceme.dur.usenet.model.Article;
import net.bounceme.dur.usenet.model.Newsgroup;

class DatabaseUtils {

    private static final Logger LOG = Logger.getLogger(DatabaseUtils.class.getName());
    private EntityManagerFactory emf = Persistence.createEntityManagerFactory("USENETPU");
    private EntityManager em = emf.createEntityManager();

    public int getMax(Folder folder) {
        int max = 0;
        String ng = folder.getFullName();
        String queryString = "select max(article.messageNumber) from Article article left join article.newsgroup newsgroup where newsgroup.newsgroup = '" + ng + "'";
        try {
            max = (Integer) em.createQuery(queryString).getSingleResult();
        } catch (Exception e) {
            LOG.info("setting max to zero");
        }
        LOG.severe(folder.getFullName() + "\t" + max);
        return max;
    }

    public void persistArticle(Message message, Folder folder) {
        em.getTransaction().begin();
        String fullNewsgroupName = folder.getFullName();
        Newsgroup newsgroup = null;
        int max = getMax(folder);
        TypedQuery<Newsgroup> query = em.createQuery("SELECT n FROM Newsgroup n WHERE n.newsgroup = :newsGroupParam", Newsgroup.class);
        query.setParameter("newsGroupParam", fullNewsgroupName);
        try {
            newsgroup = query.getSingleResult();
            LOG.fine("found " + query.getSingleResult());
        } catch (javax.persistence.NoResultException e) {
            LOG.fine(e + "\ncould not find " + fullNewsgroupName);
            newsgroup = new Newsgroup(folder);
            em.persist(newsgroup);
        } catch (NonUniqueResultException e) {
            LOG.warning("\nshould never happen\t" + fullNewsgroupName);
        }
        Article article = new Article(message, newsgroup);
        em.persist(article);
        em.getTransaction().commit();
    }

    public void close() {
        em.close();
        emf.close();//necessary?
    }
}

However, I'm quite sure that I'm in the minority!  Why?

A quick look at Math shows that this isn't a strange or odd approach.  The object itself keeps no state, really, so why would a bean or POJO be preferred to static methods?

Static helper classes makes testing harder. Nick Malik has a good article on this topic: Killing the Helper class, part two.

Mocking non-static methods is closer to OOP and easier than static ones. Another gain is that you'll have simple tests: simple tests for the utility/helper class and simple tests for the clients of the utility class; the tests of the client classes don't have to know the internals of the utility class, and you don't need a DatabaseUtilsTestHelper class to keep in one place the mocking logic for the helper class. The linked article is worth the reading.

Some other notes:

Try not using short variable names like ng. They are hard to read.
persistArticle does not close the transaction in every path. You should rollback it in the finally block if it's still active:

final EntityTransaction transaction = em.getTransaction();
transaction.begin();
try {
    ...
    transaction.commit();
} finally {
    if (transaction.isActive()) {
        transaction.rollback();
    }
}

persistArticle does not use the value of max, therefore the getMax call seems unnecessary.


Merge Sort implementation in Java Can you check my merge sort implementation?  From all of my testing it seems like it works, but I just wanted to make sure I am doing it as good as possible, and it couldn't be optimized any further.

private static void merge(int[] c,int[] a, int[] b){
    int i = 0;//index for a
    int j = 0;//index for b
    for(int k = 0; k < c.length; k++){
        //check a[] edge case
        if(i >= a.length){
            c[k] = b[j];
            j++;
        }else if(j >= b.length){
            c[k] = a[i];
            i++;
        }else if(a[i] < b[j]) {
            c[k] = a[i];
            i++;
        } else {
            c[k] = b[j];
            j++;
        }
    }
}

public static void mergeSort(int[] A){
    if(A.length > 1){
        int pivot = A.length / 2;
        int[] left = Arrays.copyOfRange(A, 0, pivot);
        int[] right = Arrays.copyOfRange(A, pivot,A.length);
        mergeSort(left);
        mergeSort(right);
        merge(A,left,right);
    }
}

Mostly LGTM, with the following objections.

Names. Avoid which require a comment to be understood. I'd have no problem with int ax; because it clearly conveys the goal of being an index of a.
Conditionals.
You can statically prove that i > a.length is not possible. If by any chance it happens, it means that something really weird is going on. A good cause to raise an exception. Bottomline is, separate a == test from > one.
Once one of the ranges is exhausted, there's no need to test it again and again. Normally one would break a main loop and copy the remaining data separately

along the lines of:

while (A has data) and (B has data)
    calculate what to copy
    copy it to C

while (A has data)
    copy A to C

while (B has data)
    copy B to C

Types. The methods cry to be generic. If you didn't cover generics yet, disregard the objection.


Speed and ID concerns converting DOM object into JQuery objects First of all, I am not using JQuery but Cheerio. I have never actually used JQuery in "real life" -- I started with Cheerio, which is a subset and doesn't actually work in a browser environment.

(Note: I don't have enough reputation here to create tags, somebody might want to tag this question as "cheerio" and delete this sentence...)

Here is my code. Questions are inline:

$ = cheerio.load( self.formPage.toString() );

$( 'form' ).each( function( index, form ){

  // *****************
  // QUESTION:
  // Is this the right way to do this?
  // I am "converting" each found "DOM" object into a "JQuery" object (although
  // it's not real DOM, and it's not real JQuery either).
  // But, if there is no ID set for form, for example, is this _guaranteed_ to always find
  // the right one?
  var $form = $( form );

  // Work out fullAction, which is the form's action relative to the fetched URL
  var action;
  var baseUrl = self.config.url.substr(0, self.config.url.lastIndexOf("/") + 1);
  var action = $form.attr( 'action' );
  var fullAction = require( 'path' ).normalize( baseUrl + action );
  console.log("FULL ACTION:", fullAction );

  stores.workspacesNinjaFormCodes.apiGetQuery( { filters: { workspaceId: self.workspaceId }, skipHardLimitOnQueries: true }, function( err, codesData ){

  //stores.workspacesNinjaFormCodes.dbLayer.select( { conditions: { and: [ { field: 'workspaceId', type: 'eq', value: self.workspaceId } ] } }, { skipHardLimitOnQueries: true }, function( err, codesData ){

    codesData.forEach( function( codeData ){
      var code = codeData.code;
      var dataSubmitHash = {};

      // *****************
      // QUESTION:
      // Is this the right way to do this? I want to find all 'input' fields
      // in this particular form. So, I pass $form as context and look for
      // all 'input' tags. Is this what you are meant to do?
      // ******************
      $( "input", $form ).each( function( index, inputField ){

        // *****************
        // QUESTION:
        // Once again, will this always work, regadrless of IDs etc.? I am passing `form` as the
        // context.
        // ******************
        var $inputField = $( inputField, form );

        // See if the field name is already assigned
        var fieldValue = $inputField.val() ? $inputField.val() : code;

        // Assign values
        dataSubmitHash[ $inputField.attr('name') ] = fieldValue;
      });

      console.log("dataSubmitHash:", dataSubmitHash );
    });

  });

});

Questions are inline, and they are focused on this: how expensive is the "conversion" of cheerio's "DOM" objects into Cheerio "JQuery" objects? And is it always guarantee to get the right one?

Here's what I did, comments in the code.

$ = cheerio.load(self.formPage.toString());
path = require('path'); //Move out path from the loop to avoid re-requiring it
$('form').each(function (index, form) {

  var $form = $(form);

  // Removed action here. You seem to use it after the next line.

  var baseUrl = self.config.url.substr(0, self.config.url.lastIndexOf("/") + 1);
  var action = $form.attr('action');
  var fullAction = path.normalize(baseUrl + action);

  // In the next block, input is in the context of this form
  // We can pull it out of the loop to avoid fetching it on every iteration
  var input = $('input', form);

  // You don't seem to use fullAction in the next block?

  stores.workspacesNinjaFormCodes.apiGetQuery({
    filters: {
      workspaceId: self.workspaceId
    },
    skipHardLimitOnQueries: true
  }, function (err, codesData) {
    codesData.forEach(function (codeData) {
      var code = codeData.code;
      var dataSubmitHash = {};

      // here's the input earlier
      input.each(function (index, inputField) {

        // Let's keep it readable, place then in variables
        var $inputField = $(inputField, form);
        var inputFieldName = $inputField.attr('name');

        // we could use the || operator to check for the first and use it, or use the second if not first.
        var fieldValue = $inputField.val() || code;

        dataSubmitHash[inputFieldName] = fieldValue
      });

      console.log("dataSubmitHash:", dataSubmitHash)
    })
  })
});

In addition, one should not create functions inside loops. Consider moving them out of the loops.


Weaving multiple lists into one I have this code to weave the split_input ['5,4 4,5 8,7', '6,3 3,2 9,6 4,3', '7,6', '9,8', '5,5 7,8 6,5 6,4'] together. However I feel like this can be done more efficient (especially the weave function) but I have no clue how to improve it without importing any modules.

class Class1:
    def __init__(self, row):
        self.row = row

    def append(self, coordinate):
        self.row += [coordinate]

    def extend(self, row):
        for coordinate in row:
            self.row += [coordinate]

    def weave(self, row2):
        result = Class1([])
        for i in range(len(self.row)):
            if i < len(self.row):
                result.append(self.row[i])
            if i < len(row2.row):
                result.append(row2.row[i])
        if len(row2.row) > len(self.row):
            result.extend(row2.row[len(self.row):])
        return result

def get_route(split_input):
    rows = []
    for i in range(len(split_input)):
        rows += [Class1(split_input[i].split())]
    previous_row = rows[0]

    for row in rows[1:]:
        previous_row = previous_row.weave(row)
    return previous_row

woven_rows = get_route(split_input)
print woven_rows

Do you have any good advice?

The output of the code is one list. First, the first and second list are weaved into one list. Then this list is weaved with the third list etc.

I'm not sure what Class1 is for, but you can weave 2 lists together using a list comprehension to build the first part of the list.  Then add on the rest of the longer list.  Like so:

def weave2(seq1, seq2):
    result = [item for pair in zip(seq1, seq2) for item in pair]

    if len(seq1) < len(seq2):
        result += seq2[len(seq1):]
    else:
        result += seq1[len(seq2):]

    return result

To weave more than two lists, weave the first two together, then weave in the third list, and so on.  Like so:

def weave(*args):
    args = iter(args)
    result = next(args)

    for arg in args:
        result = weave2(result, arg)

    return result


Python read/write pickled file Is there a way in Python 2.7 to improve reading/writing speed (or memory consumption of the file) compared to this version?

import gzip
import cPickle
import io

# save zipped and pickled file
def save_zipped_pickle(obj, filename):
    # disable garbage collector (hack for faster reading/writing)
    gc.disable()
    with gzip.open(filename, 'wb') as f:
        cPickle.dump(obj, io.BufferedWriter(f), -1)
        # enable garbage collector again
        gc.enable()

# load zipped and pickled file
def load_zipped_pickle(filename):
    # disable garbage collector (hack for faster reading/writing)
    gc.disable()
    with gzip.open(filename, 'rb') as f:
        loaded_object = cPickle.load(io.BufferedReader(f))
        # enable garbage collector again
        gc.enable()
        return loaded_object

This looks really efficient and potentially the best you can do, but here are some thoughts:

try specifying the highest protocol (for Python 2.7 that would be protocol 2):

with gzip.open(filename, 'wb') as f:
    cPickle.dump(obj, io.BufferedWriter(f), protocol=cPickle.HIGHEST_PROTOCOL)

re-iterate over the serialization format itself - if JSON is an option, ujson might win the performance battle against cPickle
see if PyPy is an option
you can also get Cython into play here - there are famous stories how just specifying types with Cython leads to serious performance gains
there is also this undocumented .fast option/flag, but I am not at all sure it would actually help to speed things up - too good to be true :)
as far as improving on memory, you can try chunking with shutil

FYI, the gc.disable/gc.enable() pair is planned to be implemented as a context manager in Python 3.7. You can borrow a Python sample from the issue tracker:

@contextmanager
def gc_disabled():
    if gc.isenabled():
        gc.disable()
        yield
        gc.enable()
    else:
        yield


Autodetecting monitors in XFCE I got sick of manually xrandering things on my computers (especially since I always just sequence monitors from left to right and set each at the highest resolution) so I wrote this:

#!/usr/bin/ruby

def xrandrPairs (xList)
## Takes a split list of xrandr output and returns [[<display name>, <max-resolution>], ...]
  pairs = [[matchDisplay(xList[0]), matchOption(xList[1])]]
  (2..xList.length-1).to_a.each do |i| # kind of hacky, but I need to reference car and cadr here, so a call to .map won't do it
    if xList[i] =~ /^\S/
      pairs.push([matchDisplay(xList[i]), matchOption(xList[i+1])])
    end
  end
  pairs
end

def matchDisplay (dispString)
## Matches a display name
  dispString.match(/^([^\s]*)/)[1]
end

def matchOption (optString)
## Matches a resolution string (since they have whitespace preceding them)
  optString.match(/^\s*([^\s]*)/)[1]
end

def xrandrString (xPairs)
## Takes [[<display name>, <max-resolution>] ...] and returns an xrandr command string
  s = "xrandr --output #{xPairs[0][0]} --mode #{xPairs[0][1]}"
  if xPairs.length >= 2
    (1..xPairs.length-1).to_a.each do |i| # same as above
      s += " --output #{xPairs[i][0]} --mode #{xPairs[i][1]} --right-of #{xPairs[i-1][0]}"
    end
  end
  s
end

exec xrandrString(xrandrPairs(`xrandr`.split("\n")[1..-1]))

The key is that each computer I use has different displays (they're named differently and they have different maximum resolutions), so as far as I know, I have to either parse xrandr output or write a different script for each machine.

I don't care that it's inefficient (traversing the xrandr output multiple times and doing some looped string formatting) because it only runs once at startup time, and deals with a list of 30 elements at the outside. I'm using Ruby 1.8.7 straight out of the Squeeze repos for ease of installation (this is also why I wouldn't mind being shown how this works in Python/Perl; those come with the system).

Can I get some comments on it?

First of all, the convention in ruby is to use snake_case, not camelCase for variable and method names. It's generally a good idea to adhere to a language's naming conventions - if only so that the code looks consistent when you're calling standard library methods as well as your own.

In your xrandrPairs method, you mention that you're using an index to iterate because you have to go through the array in pairs. You can avoid this by using each_cons(2) which will yield each item together with the item after it (e.g. [1,2,3].each_cons(2) will yield 1,2 in the first iteration and 2,3 in the second).

However there's a better way to do this then to iterate through the lines. You can use scan to find all the lines that start without spaces and extract the information you want in one go:

def xrandr_pairs (xrandr_output)
## Returns [[<display name>, <max-resolution>] ...]
  display_re = /^(\S+)/
  option_re = /^\s+(\S+)/
  xrandr_output.scan(/#{display_re}.*\n#{option_re}/)
end

Since scan returns an array containing one subarray per match where each item in the subarray corresponds to one capturing group in the regex, this will produce the output you want. Note that xrandr_pairs now takes xrandr's output as a string, not an array of lines.

In addition to using scan I also changed the regexen a bit: I replaced [^\s] with \S, which is equivalent but shorter, and used + instead of *, so it does not match empty strings.

The xrandr_string method can also be rewritten to be much nicer by using the each_cons method like this:

def xrandr_string (x_pairs)
## Takes [[<display name>, <max-resolution>] ...] and returns an xrandr command string
  s = "xrandr --output #{x_pairs[0][0]} --mode #{x_pairs[0][1]}"
  x_pairs.each_cons(2) do |(previous_output, previous_mode), (output, mode)|
      s += " --output #{output} --mode #{mode} --right-of #{previous_output}"
    end
  end
  s
end

You don't need to check that the size is at least 2 because each_cons simply doesn't do anything if the array is smaller than the given chunk-size.

I also used the destructuring bind of block arguments to assign the elements of the subarrays to variables directly.

Instead of building up the string imperatively you could also use map and join like this:

def xrandr_string (x_pairs)
## Takes [[<display name>, <max-resolution>] ...] and returns an xrandr command string
  cmd = "xrandr --output #{x_pairs[0][0]} --mode #{x_pairs[0][1]}"
  args = x_pairs.each_cons(2).map do |(previous_output, previous_mode), (output, mode)|
    "--output #{output} --mode #{mode} --right-of #{previous_output}"
  end
  [cmd, *args].join(" ")
end


Format double to 3 decimal places without decimal separator to a certain length I need to format a double value to three decimal places to a string with length of 9 with leading spaces and no decimal separator.

My approach is this

NumberFormatInfo nfi = new NumberFormatInfo();
nfi.NumberDecimalSeparator = "a";
nfi.NumberGroupSeparator = "";

double a = 334.44554;

Console.WriteLine($"{a.ToString("####0.000", nfi).Replace("a", ""), 9}");

Is there a more elegant way to do this?

To truncate a double to 3 decimal places and remove the decimal separator, is I think more easily done by multiplying by 1000 and casting to an int.  Padding the result to 9 digits is easily done with a format specifier in the ToString method.  It could look something like this:

static string FormatNum(double num) => ((int)(num * 1000)).ToString("D9");

Note I've also put it in a method with an expression body.

I just noticed that you wanted the padding with spaces not zeros here's a version that does that:

static string FormatNum(double num) => string.Format("{0,9:D}",((int)(num * 1000)));

As was pointed out, your solution produced a rounded off number.  If that is required here's one way:

static string FormatNum(double num) => string.Format("{0,9:D}",((int)(Math.Round(num * 1000))));


Customer MapReduce implementation I would love a second opinion / another pair of eyes on this.

db.customers.mapReduce(
    function() {
        Array.prototype.getUnique = function() {
            var u = {}, a = [];
            for (var i = 0, l = this.length; i < l; ++i) {
                if (u.hasOwnProperty(this[i])) {
                    continue;
                }
                a.push(this[i]);
                u[this[i]] = 1;
            }
            return a;
        }
        var customer = this;
        this.tags.getUnique().forEach(function(tag) {
            if (customer.status) {
                emit(customer.shop, {
                    "kind": "unique_customers_submitted",
                    "tag": tag.tag,
                });
            } else {
                emit(customer.shop, {
                    "kind": "unique_customers_withheld",
                    "tag": tag.tag,
                });
            }
            emit(customer.shop, {
                "kind": "unique_customers_total",
                "tag": tag.tag,
            });
        })
        this.tags.forEach(function(tag) {
            if (customer.status) {
                emit(customer.shop, {
                    "kind": "total_customers_submitted",
                    "tag": tag.tag,
                });
            } else {
                emit(customer.shop, {
                    "kind": "total_customers_withheld",
                    "tag": tag.tag,
                });
            }
            emit(customer.shop, {
                "kind": "total_customers",
                "tag": tag.tag,
            });
        });
    },
    function(key, sets) {
        var forms = {};
        sets.forEach(function(set) {
            if (typeof forms[set.tag] == "undefined") forms[set.tag] = {};
            if (typeof forms[set.tag][set.kind] == "undefined") forms[set.tag][set.kind] = 0;
            forms[set.tag][set.kind] += 1;
        });
        return forms;
    }, {
        out: "form_numbers",
    },
    function(err, results) {
        console.log(err);
        //console.log(results);
        results.find().toArray(function(err, numbers) {
            console.log(err);
            console.log(JSON.stringify(numbers));
            db.server.close();
        });
    }
);

I'm not familiar with MR best practices, but here are some JS-specific comments.

Key Points

DRY - that emit code is repeated all over the place and has only slight changes - parameterize what varies and put it all in a function
You are repeating your loops - either do everything in one loop for better performance (and keep in mind that for is faster than forEach, or go fully functional (see next)
You are essentially doing a filter within your loop - use filter to remove the if logic from within the loop (or see previous). If you want to go even more functional, maybe you should use underscorejs or lo-dash, which both have a unique function already.
Never change the prototype of a builtin unless you have already checked if the method exists (like a polyfill) and/or you have a really, really good reason to do so. Mostly, just don't.
Declare functions so that they are not re-created (unless necessary)
You should probably close your db connection ASAP
Oh, and name your inline functions to provide better documentation

A simple refactoring hosted as a gist for easy forking

function getUnique() {
    var u = {}, a = [];
    for (var i = 0, l = this.length; i < l; ++i) {
        if (u.hasOwnProperty(this[i])) {
            continue;
        }
        a.push(this[i]);
        u[this[i]] = 1;
    }
    return a;
};
function emitHelper(kind, shop, tag) {
    emit(shop, {
        "kind": kind,
        "tag": tag,
    });
};
db.customers.mapReduce(
    function mapCustomerToShopWithKindAndTag() {
        var customer = this;
        var kind;
        getUnique(this.tags).forEach(function(tag) {
            kind = customer.status ? "unique_customers_submitted" : "unique_customers_withheld";
            emitHelper(kind, customer.shop, tag.tag);
            emitHelper("unique_customers_total", customer.shop, tag.tag);
        });
        this.tags.forEach(function(tag) {
            kind = customer.status ? "total_customers_submitted" : "total_customers_withheld";
            emitHelper(kind, customer.shop, tag.tag);
            emitHelper("total_customers_total", customer.shop, tag.tag);
        });
    },
    function reduceToCountOfKindsPerTag(key, sets) {
        var forms = {};
        sets.forEach(function(set) {
            if (typeof forms[set.tag] == "undefined") forms[set.tag] = {};
            if (typeof forms[set.tag][set.kind] == "undefined") forms[set.tag][set.kind] = 0;
            forms[set.tag][set.kind] += 1;
        });
        return forms;
    }, {
        out: "form_numbers",
    },
    function(err, results) {
        console.log(err);
        //console.log(results);
        results.find().toArray(function(err, numbers) {
            db.server.close(); // Maybe wrap in a try/catch too
            console.log(err);
            console.log(JSON.stringify(numbers));
        });
    }
);


Linked List Interview Code methods, runtime, and edge cases refactored In my previous post I had a number of steps given to me from the accepted answer on how I can write more production ready code. In this post I want this code to be reviewed as near production code I'd write in an interview. I'd like to know how well optimized my code is now from this improvements I've made to it. If there are other optimizations or c++11 features I could be using here please share that as well.

I also included comments on worst case run time complexity for my member functions, I'd like some feedback on this and my space complexity too.

#include <iostream>
#include <string>

class LinkedList
{
    private:
        struct Node
        {
            int data;
            Node* next;
            Node(int data_) : data(data_), next(nullptr) { }
        };
        Node* _head;
        Node* _tail;
    public:
        LinkedList() : _head(nullptr) { }
        LinkedList(int data) { // O(1)
            Node* n = new Node(data);
            _head = n;
            _tail = n;
        }
        ~LinkedList() { // O(n)
            Node* current = _head;
            Node* n;
            while(current) {
                n = current;
                current = current->next;
                delete n;
            }
            _head = nullptr;
        }
        void append(int new_data) { // O(1)
            auto n = new Node(new_data);
            if(!_head) {
               _head = n;
               _tail = n;
            }
            else {
                _tail->next = new Node(new_data);
                _tail = _tail->next;
            }
        }
        void remove(int target_data) { // O(n)
            if(!_head) { return; }
            Node* previous = nullptr;
            auto current = _head;
            for(; current->data != target_data; current = current->next) {
                if(!current->next) {
                    return;
                }
                previous = current;
            }
            if(!current->next) {
                if(previous) {
                    delete current;
                    previous->next = nullptr;
                }
                else {
                     delete current;
                    _head = nullptr;
                }
            }
            else {
                auto dangling_ptr = current->next;
                current->data = current->next->data;
                current->next = current->next->next;
                delete dangling_ptr;
            }
        }
        friend std::ostream& operator<<(std::ostream& os, const LinkedList& linked_list) { // O(n)
            Node* current = linked_list._head;
            if(!current) {
                os << "Linked List is empty" << std::endl;
                return os;
            }
            else {
                for(; current; current = current->next) {
                    os << '(' << std::to_string(current->data) << ")->";
                }
                os << "nullptr";
                return os;
            }
        }
};

int main()
{
    LinkedList linked_List(0);
    linked_List.append(1);
    linked_List.append(2);
    linked_List.append(3);
    linked_List.append(4);

    std::cout << linked_List << std::endl;

    linked_List.remove(0);
    linked_List.remove(4);
    linked_List.remove(2);
    std::cout << linked_List << std::endl;

    linked_List.remove(3);
    linked_List.remove(1);
    std::cout << linked_List << std::endl;

    LinkedList linked_List_two;
    linked_List_two.append(1);
    linked_List_two.append(2);
    linked_List_two.append(3);
    linked_List_two.append(4);

    std::cout << linked_List_two << std::endl;

    linked_List_two.remove(0);
    linked_List_two.remove(4);
    linked_List_two.remove(2);
    std::cout << linked_List_two << std::endl;

    linked_List_two.remove(3);
    linked_List_two.remove(1);
    std::cout << linked_List_two << std::endl;
}

I see a number of things that could help you improve your code.

Don't define a default constructor that only initializes data

Instead of writing this:

class LinkedList
{
    Node* _head;
    Node* _tail;
 public:
    LinkedList() : _head(nullptr) { }
    // etc.
};

write this:

class LinkedList
{
    Node* _head = nullptr;
    Node* _tail = nullptr;
 public:
    // no need to explicitly write default constructor
    LinkedList() = default;
};

See Cpp Core Guidelines C.45 for details.  Note also that this initializes both member data items.

Think about whether the destructor should be virtual

For an interview, you should be prepared to answer why the class destructor is not virtual and to explain circumstances in which it should be virtual.

Eliminate redundant variables

The current code contains this constructor:

LinkedList(int data) { // O(1)
    Node* n = new Node(data);
    _head = n;
    _tail = n;
}

I'd prefer to see n eliminated and to see it written instead like this:

LinkedList(int data) :
    _head{new Node{data}}, _tail{_head} {}

Use for rather than while where appropriate

The desructor currently looks like this:

~LinkedList() { // O(n)
    Node* current = _head;
    Node* n;
    while(current) {
        n = current;
        current = current->next;
        delete n;
    }
    _head = nullptr;
}

Rather than introducing two extra variables, I'd write it using a for loop like this:

~LinkedList() { // O(n)
    if (_head == nullptr) {
        return;
    }
    for (auto current{_head->next}; current; current = current->next) {
        delete _head;
        _head = current;
    }
}

Fix the memory leak

The current code has this member function:

void append(int new_data) { // O(1)
    auto n = new Node(new_data);
    if(!_head) {
       _head = n;
       _tail = n;
    }
    else {
        _tail->next = new Node(new_data);
        _tail = _tail->next;
    }
}

The problem is that we create two nodes but only use one unless the list is empty.  I'd fix that like this:

void append(int new_data) { // O(1)
    auto n{new Node(new_data)};
    if(_head == nullptr) {
       _head = _tail = n;
    } else {
        _tail->next = n;
        _tail = _tail->next;
    }
}

Simplify your code

The remove function is much more longer than it needs to be.  Right now it looks like this:

    void remove(int target_data) { // O(n)
        if(!_head) { return; }
        Node* previous = nullptr;
        auto current = _head;
        for(; current->data != target_data; current = current->next) {
            if(!current->next) {
                return;
            }
            previous = current;
        }
        if(!current->next) {
            if(previous) {
                delete current;
                previous->next = nullptr;
            }
            else {
                 delete current;
                _head = nullptr;
            }
        }
        else {
            auto dangling_ptr = current->next;
            current->data = current->next->data;
            current->next = current->next->next;
            delete dangling_ptr;
        }
    }

It could be made somewhat simpler by creating a temporary Node that points to _head:

void remove(int target_data) { // O(n)
    Node headptr{0};
    headptr.next = _head;
    for (Node* curr{&headptr}; curr && curr->next; curr = curr->next) {
        if (curr->next->data == target_data) {
            auto victim = curr->next;
            if (victim == _tail) {
                _tail = curr;
            }
            if (victim == _head) {
                _head = victim->next;
            }
            curr->next = victim->next;
            delete victim;
            return;
        }
    }
}

Rethink the interface

It's not unreasonable to expect that one might want to actually do something with the things inserted into a list.  Right now, there's no way to do anything except print which suggests an incomplete interface.

Don't use std::endl if '\n' will do

Using std::endl emits a \n and flushes the stream.  Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.

Make test success obvious

The current test code exersizes the list, but it doesn't indicate what is expected to be printed.  I'd instead write both test scenarios and also the expected result so that it would be clear to anyone running the code whether everything was working as expected or not.

Consider using a template

A linked list is a fairly generic structure that could hold any kind of data if the class were templated, and not just an int.

Consider possible uses

For any code production, but especially if you're in an interview, think about how the class is being used and whether there are any restrictions or limits inherent in the design.  For example, think about copy and move operations.  If you write this, does the code do the right thing?

LinkedList a;
a.append(1);
a.append(2);
a.append(3);
a.append(4);
auto b{a};
std::cout << "a: " << a << '\n';
std::cout << "b: " << b << '\n';
a.remove(1);
std::cout << "a: " << a << '\n';
std::cout << "b: " << b << '\n';

Also consider multithreaded code.  Would it be thread-safe to insert nodes from one thread and remove them from another?  If not, what would be needed to make that work?


PHP PDO login with session index.php,general message.php, logout.php, site life.php (this page for session and put it in the other pages by required)

Database:

connection.php

<?php
$dsn = "mysql:host=localhost;dbname=mg";
$username = "root";
$password = "";
$options = array(
PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8',
);

try{
$conn = new PDO($dsn,$username,$password,$options);
} catch (PDOException $e){
echo "Error!".$e->getMessage();
}

?>

index.php:

<?php
session_start();
if(isset($_SESSION['user'])){
header("location: general message.php");
}
require "connection.php";

if(isset($_POST['login'])){
$user = $_POST['username'];
$pass = md5($_POST['password']);
$messeg = "";

if(empty($user) || empty($pass)) {
    $messeg = "Username/Password con't be empty";
} else {
    $sql = "SELECT username, password FROM users WHERE username=? AND
  password=? ";
    $query = $conn->prepare($sql);
    $query->execute(array($user,$pass));

    if($query->rowCount() >= 1) {
        $_SESSION['user'] = $user;
        $_SESSION['time_start_login'] = time();
        header("location: general message.php");
    } else {
        $messeg = "Username/Password is wrong";
    }
}
}

?>

Site life.php (and I will put it in the the other pages by require "site life.php")

//The lives of session is one hour 60*60=3600

<?php
session_start();

if(isset($_SESSION['user'])){
if((time() - $_SESSION['time_start_login']) > 3600){
    header("location: logout.php");
} else {
    $_SESSION['time_start_login'] = time();
}
} else {
header("location: logout.php");
}
 ?>

logout.php

<?php
session_start();
session_destroy();
header("location: index.php");
?>

General message.php

I put this in the header (to make a refresh every hour):

// 60*60=3600 one hour

<meta http-equiv="Refresh" content="3600" >

<?php
require ('site life.php');
?>

Starting a database session
You are starting a dabatase session with the following code:
try{
$conn = new PDO($dsn,$username,$password,$options);
} catch (PDOException $e){
echo "Error!".$e->getMessage();
}

It is unclear to me why you let the request live on. If the application needs the database connection, letting the request live on produces all kinds of behaviour from code that assumes the connection is there. Consider killing the request right there and then.
Besides this, don't print the error message to the user. It is not useful to the average user, and might give an adversary additional information to find weaknesses in your site.
Keep in mind that if you include a php file, the variables inside it are available even outside that file. If an adversary can trick any other file that includes connection.php to display the contents of the $password variable, they can obtain the password this way. Arguably, the chance of this happening, but the adversary not being able to execute arbitrary code, is very small. You could defend yourself against it by unsetting the variable, or define those variables only in a function.
Storage of password
You are storing an unsalted MD5 hash from the password. MD5 has not been crytographically secure for a long time now, and any password can easily be brute-forced in it. Since your password is unsalted, cracking the passwords is as simple as looking it up in multiple already existing Rainbow tables. For example, the MD5 hash for your account is "Mohamed" (http://md5decoder.org/bc2ab28e4cda984ca76646874371e864).
I recommend to use bcrypt instead as hash function. It is designed to be slow, and the algorithm can not easily be sped up by implementing it on a GPU. See this question on security.SE for more information.
You should always add a per-user salt. By using an unique salt for each user, brute-forcing the password of one user will not yield any useful results for any other user. If no such salt were to be implemented, we could build a rainbow table for each password that we tried, and simply look up in the table for other users. A salt should be reasonably random, and sufficiently long. See this question on Stackoverflow for more information.
Checking of password
There are a few things I want to say about the following code.
if($query->rowCount() >= 1) {

->rowCount() is not guaranteed to return the number of rows in the result for select statements as per the documentation, quoted below. A possible safer alternative is count($query->fetch_all()).

If the last SQL statement executed by the associated PDOStatement was a SELECT statement, some databases may return the number of rows returned by that statement. However, this behaviour is not guaranteed for all databases and should not be relied on for portable applications.

I do not understand why you check for >= 1 in this code. Only one user/password should ever match. If this is not the case, something very bad happened. Either rowCount returns something that has nothing to do with this select statement, someone tries to get the query to return more information or your data is corrupt somehow. In either case, you want to know about this, not silently log in someone when more than one row is returned.
Misc
Use consistent indentation in your code. Inside your try block you don't indent. Similarly, your nested if-statements are not indented nicely.