Tracking 1400+ client codes, multi-threading nightmare I am looking for a bit of help. I am having to cycle through 1400 clients code on my server, I have to check what Version of the software they are on and check for customization's to the code so when we apply updates, those aren't lost. I tried to do it serialized, as you might have guessed, that took forever. I then Tried running it parallel, as you might have expected, I am getting "Os Error, too many open files" I tried to resolve this by throwing in "sleep(random())" Which Works,  But this seems convoluted and slows down the process considerably, and it could also be my code is complete garbage, any insight would be great.
I am using git in CLI to get version, this seems to be heavy? Is there a better way to pull tags?
Here, I get a list of the clients, I do some editing to get the pathing
def full_check(client_list, cur_vers):
    for c in client_list:
        if sc.venue_check(c):
            url = sc.surl(c[0])
            if url is not None:
                path = f'/home/ubuntu/site-files/{url}'
                # Maximizing output, this cut run time by 90%
                # Hard to log though...

                nschools.append({
                    'path': path,
                    'cur_vers': cur_vers,
                    'client': c[0],
                    'schema_version': c[2]
                })
# Here, I am then looping through the paths I get, I put a sleep to stop it from bugging out
# about OS File open Limits
    for s in nschools:
        print(s['path'])
        sleep(random())
        t = threading.Thread(target=vers_check, args=(s['path'], s['cur_vers'], s['client'], s['schema_version']))
        t.start()

Here I am going through doing some version checking
def vers_check(path, cur_vers, client, schema_version):
    """
    This Checks the Schools Release.
    TODO Add logging and Return Functionality.
    :param client:
    :param schema_version:
    :param path:
    :param school:
    :param cur_vers:
    :return:
    """
    os.chdir(path)
    try:
        vers = subprocess.check_output(["git", "describe", "--tags"]).strip().decode('utf-8')

        raw = str(vers).strip('-')
        rawsplit = raw.split('-')
        st = rawsplit[0]

        if str(st) != cur_vers:
            cschools.append({
                'client': client,
                'schema_version': schema_version,
                'sw_version': st,
                'path': path,
                'is_update': False
            })
            log.log('warn', 'Version Check', f'The School {client} is on Software Version {st} and Schema Version {schema_version} Which is Out Of Date. Current Version is {cur_vers}')
        else:
            cschools.append({
                'client': client,
                'schema_version': schema_version,
                'sw_version': st,
                'path': path,
                'is_update': True
            })
            log.log('info', 'Version Check', f'The School is on Software Version {st} and Schema Version {schema_version} Which is Up To Date. Current Version is {cur_vers}')
    except subprocess.CalledProcessError as e:
        log.log('err', 'Failed Version Check', f'We were unable to get the version for {client}', e)
```

First, I'm pretty sure that it's impossible for
os.chdir(path)

to meaningfully apply different working directories per thread; and even if it were possible it would be a bad idea. subprocess supports a cwd parameter directly that you should use.
If you were opening files yourself, you would want to make a semaphore where the lock only applies to the section of the code where the file is opened. However, you aren't opening the files - it's probably your invocation of git that opens the most files.
I think the only practical way around this is to just limit the number of concurrent threads. You could either estimate this ahead of time, or keep spinning up threads until you hit the first OSError and start no new threads, running one-out-one-in until your work pool is complete.
If you deeply care about performance, subprocess-ing out to git should be replaced with in-process computation; either calling into a library or manually processing entries in .git (I have not researched either).


best way to apply across an xts object As xts objects are arrays, getting apply functions to work is a little tricky if you want to preserve the dates. For example, take the xts object xx below:

xx <- xts(replicate(6, sample(c(1:10), 10, rep = T)),
          order.by = Sys.Date() + 1:10)

Say we wish to apply a function to each column (to keep it simple say i wish to add 100 to each element of each column).

sapply(xx, function(col) col + 100)

Doing with with sapply loses the row names.

Using apply works -- but it is slow (see below).

apply(xx, 2, function(col) col + 100)

To address this, I wrote the following helper:

mapXts <- function(Xts, cFUN) {
        if(!is.xts(Xts)) stop("Must supply function with xts object")
        Z <- Xts
        for (j in 1:ncol(Xts)) {
            Z[,j] <- do.call(cFUN, list(Xts[,j]))
        }
        Z
}

The obligatory horse race:

set.seed(1)
xz <- xts(replicate(6, sample(c(1:100), 1000, rep = T)),
          order.by = Sys.Date() + 1:1000)

apFun <- function() apply(xz, 2, function(col) col + 100)
sapFun <- function() sapply(xz, function(col) col + 100)
mapXf <- function() mapXts(xz, function(col) col + 100)

op <- microbenchmark(
               app = apFun(),
               sap = sapFun(),
               mXf = mapXf(),
               times = 1000L)

This results in speedups of about 5x over apply and is about 3x faster than sapply (and it gives me back the data in the formate i most value).

Unit: microseconds
 expr      min        lq    median        uq      max neval
  app 3083.425 3206.0295 3287.1725 3496.9720 30678.90  1000
  sap 1669.384 1763.7945 1827.0265 1952.7490 24173.88  1000
  mXf  615.279  677.0005  709.4725  787.1635 24390.62  1000

Are there any further tricks i might use to speed things up? Also, any dark corners in the case that i've missed?

You can use the function vapply. The help page of ?vapply says:

  vapply is similar to sapply, but has a pre-specified type of return
  value, so it can be safer (and sometimes faster) to use.

You can use

vapply(xz, function(col) col + 100, FUN.VALUE = numeric(nrow(xz))

The argument FUN.VALUE is used to tell the vapply what the function returns. Since all columns are numeric, the function returns a numeric vector of length nrow(xz) for each column of xz. Unfortunately, vapply does not preserve the dates of the xts object.

You can use the following command to generate a new object based on xz and replace all values with the matrix returned by vapply. This is very easy with the following command:

"[<-"(xz, , vapply(xz, function(col) col + 100, FUN.VALUE = numeric(nrow(xz))))

The function "[<-" copies the xz object and replaces all its values.

Alternatively, you can create a new xts object based on the matrix returned by vapply and the time information in your original xts object.

xts(vapply(xz, function(col) col + 100, FUN.VALUE = numeric(nrow(xz))),
    order.by = time(xz))

Now, we test the approaches:

vapFun <- function() "[<-"(xz, , vapply(xz, function(col) col + 100,
                                        FUN.VALUE = numeric(nrow(xz))))

vapFun2 <- function() xts(vapply(xz, function(col) col + 100,
                                 FUN.VALUE = numeric(nrow(xz))),
                          order.by = time(xz))

library(microbenchmark)

op <- microbenchmark(
  app = apFun(),
  sap = sapFun(),
  mXf = mapXf(),
  vap = vapFun(),
  vap2 = vapFun2(),
  times = 1000L)

Unit: microseconds
 expr      min       lq   median       uq        max neval
  app 4066.612 4117.860 4167.324 4270.200 364484.879  1000
  sap 3337.001 3428.798 3463.680 3532.317   4310.336  1000
  mXf 1232.070 1263.531 1275.663 1295.014   2320.361  1000
  vap  984.732 1013.248 1022.726 1035.330 100172.130  1000
 vap2 1326.899 1368.562 1389.672 1428.608 126477.354  1000

As you can see, the approach with vapply and "[<-" is the fastest one.

An important information: if the function you want to apply to each column is a mathematical operation, you can apply it to the whole xts object at once, e.g., xz + 100. This is considerably faster and returns an xts object, i.e., the dates are preserved.

simpleFun <- function() xz + 100

op <- microbenchmark(
  app = apFun(),
  sap = sapFun(),
  mXf = mapXf(),
  vap = vapFun(),
  vap2 = vapFun2(),
  simple = simpleFun(),
  times = 1000L)

Unit: microseconds
   expr      min        lq   median        uq      max neval
    app 4063.972 4119.6995 4145.120 4243.7955 85454.87  1000
    sap 3347.558 3429.6725 3450.778 3515.3745 86508.52  1000
    mXf 1236.142 1267.2030 1276.455 1289.7095 84349.75  1000
    vap  992.792 1017.7425 1024.992 1033.7785 82275.26  1000
   vap2 1336.300 1372.8755 1390.553 1423.9875 83118.79  1000
 simple   92.770   98.4305  101.080  103.9995 81233.70  1000


Pseudorandom telephone generator Was tinkering around in Python after a long hiatus, and decided to do a very simple fake telephone generator.

Anything I can do to improve it?

import random

areacode = input("Enter your desired area code prefix: ")
print(areacode + "-", end='')
for i in range(3):
    print(random.randint(0,9), end='')

print("-",end='')

for j in range(4):
    print(random.randint(0,9), end='')

What are some of the things I should keep in mind syntax wise? I feel like I'm using unnecessary looping here.

Assuming you're looking for numbers of the format XXX-XXX-XXXX (keeping in mind that this excludes international numbers):

I wouldn't separate the printing onto separate lines. I would build up the number (perhaps extracting this logic into a function) and then print the whole thing. You can also eliminate those unnecessary loops by generating numbers in different ranges and using different formatting strings to add the leading zeros (ex. a routing code of 2 should be 002).

You'll also probably want to handle the case that an invalid area code is input. For this, I pulled out the prompt into a separate function that can be used in a loop (until a valid area code is given).

Finally, for scripts you should wrap the main action in if __name__ == '__main__':

#!/bin/env python3
from random import randint

def random_phone_number(area_code):
    """Returns a random phone number (XXX-XXX-XXXX) with a given
    area_code."""
    prefix = randint(0, 999)
    line_number = randint(0, 9999)
    return f'{area_code:03d}-{prefix:03d}-{line_number:04d}'

def prompt_area_code():
   """Interactively prompts the user for an area code, returning it."""
   while True:
       try:
           area_code = int(input('enter an area code: '))
       except ValueError:
           pass

       if 0 <= area_code <= 999:
           return area_code

def main():
    print(random_phone_number(prompt_area_code()))

if __name__ == '__main__':
    main()

Things of note:

f-strings (new in Python 3.6)
Handling exceptions
int converts a string to an integer or raises a ValueError


Method for gathering the correct data to a specific user I have quite a lengthy method that I know for sure can be optimized, yet I am not sure how to do it. The method is in my User model, and essentially gets the questions assigned to the user in the current week. Questions can be assigned to a variety of intervals, so it's important that for each week we get the right ones.

  def collect_right_questions_for_this_week
    current_week = Time.zone.now.strftime('%V').to_i

    odd_or_even_week = current_week.odd? ? :odd_weeks : :even_weeks

    beginning_week_of_month =
      Time.zone.now.beginning_of_month.strftime('%V').to_i
    end_week_of_month       =
      Time.zone.now.end_of_month.strftime('%V').to_i

    beginning_week_of_quarter =
      Time.zone.now.beginning_of_quarter.strftime('%V').to_i
    end_week_of_quarter       =
      Time.zone.now.end_of_quarter.strftime('%V').to_i

    frequency_values = [questions.frequencies[:weekly]]
    frequency_values << questions.frequencies[odd_or_even_week]

    frequency_values << questions.frequencies[:start_of_month] if
      current_week == beginning_week_of_month

    frequency_values << questions.frequencies[:end_of_month] if
      current_week == end_week_of_month

    frequency_values << questions.frequencies[:start_of_quarter] if
      current_week == beginning_week_of_quarter

    frequency_values << questions.frequencies[:end_of_quarter] if
      current_week == end_week_of_quarter
    frequency_values
  end

As you can see here, it's quite lengthy and repetitive. Essentially what I'd like to achieve is for it to be reduced, and better written, it doesn't have this nice flow to it.

I also recall that a method should only do 1 thing, while at least in my opinion this method does two things. It sets dates, and then gets questions.

Some notes:

Use local variables for values that are used many times.
time = Time.zone.now: This local variable prevents race conditions from occurring if the day changed in the middle of the method (unlikely, but why risk it?).
All those methods you are calling on a Time object are also available on Date. This will simplify your code.
You can send the time/date as (an optional) argument so the method works for any moment of the year (also, it will be easier to test).
time.strftime("%V").to_i is ok, but there is a better way:  Date#cweek.
There are some ways to build arrays with conditional items. I like this functional pattern: values = [value1, (value2 if condition), value3].compact.

I'd write:

def questions_for_week(date: Time.zone.today)
  frequencies = questions.frequencies
  week = date.cweek
  [
    frequencies[:weekly],
    frequencies[week.odd? ? :odd_weeks : :even_weeks],
    (frequencies[:start_of_month] if week == date.beginning_of_month.cweek),
    (frequencies[:end_of_month] if week == date.end_of_month.cweek),
    (frequencies[:start_of_quarter] if week == date.beginning_of_quarter.cweek),
    (frequencies[:end_of_quarter] if week == date.end_of_quarter.cweek),
  ].compact
end


Properties File MRU Cache I made an attempt to implement a Properties File MRU Cache with a limited cache size which are of the most recently used properties can only be held in.  A read miss from this cache leads to a read from the file for that particular property and keep it in the cache until it goes out of the limit where other recently properties gets read into the cache from the file.

import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.util.LinkedHashMap;

public class PropertiesFileMRUCache {

    private LinkedHashMap<String, String> propertiesMap = new LinkedHashMap<>(
            10);

    private int limit = 8;

    private String propertiesFile = "default.properties";

    public PropertiesFileMRUCache() {

    }

    public PropertiesFileMRUCache(String propertiesFile) {
        this.propertiesFile = propertiesFile;
    }

    public PropertiesFileMRUCache(int limit, String propertiesFile) {
        this(propertiesFile);
        this.limit = limit;
    }

    public String getCapital(String country) {
        String capital = propertiesMap.get(country);
        if (capital == null) {
            capital = readFromFile(country);
            if (propertiesMap.size() == limit) {
                propertiesMap.remove(propertiesMap.keySet().iterator().next());
            }
        } else {
            propertiesMap.remove(country);
        }
        propertiesMap.put(country, capital);
        return capital;
    }

    private String readFromFile(String country) {
        try (BufferedReader br = new BufferedReader(new FileReader(
                propertiesFile))) {
            for (String line; (line = br.readLine()) != null;) {
                if (line.startsWith(country)) {
                    return line.substring(line.indexOf('=') + 1);
                }
            }
        } catch (FileNotFoundException e) {
            e.printStackTrace();
        } catch (IOException e) {
            e.printStackTrace();
        }
        return null;
    }

    public static void main(String[] args) {
        PropertiesFileMRUCache pfc = new PropertiesFileMRUCache("res\\CountryCapital.properties");
        pfc.getCapital("India");
        pfc.getCapital("USA");
        pfc.getCapital("UK");
        pfc.getCapital("Kuwait");
        pfc.getCapital("Iraq");
        pfc.getCapital("Canada");
        pfc.getCapital("Australia");
        pfc.getCapital("India");
        pfc.getCapital("India");
        pfc.getCapital("India");
        pfc.getCapital("Germany");
        pfc.getCapital("India");
        pfc.getCapital("Germany");
        pfc.getCapital("China");
        pfc.getCapital("Pakistan");
        System.out.println(pfc.propertiesMap);
    }
}

For testing purpose I provided a user specified property as res\CountryCapital.properties with the default cache limit as 8.  The content of the CountryCapital.properties is provided below for reference:

India=New Delhi
USA=Washington
Egypt=Cairo
Srilanka=Columbo
South Korea=Seoul
Tailand=Bangkok
Qutar=Doha
Lebanon=Beirut
Canada=Toronto
Norway=Oslo
Sweden=Stockholm
Finland=Helsinki
China=Beiging
Japan=Tokyo
Bangladesh=Dhaka
Pakistan=Karachi
Afganistan=Kabul
Australia=Sydney
Newzeland=Wellington
Ireland=Dublin
Germany=Berlin
Itlay=Rome
Nepal=Kadmantu
North Korea=Pyongyang
Brazil=Brazilia
South Africa=Cape Town
France=Paris
Czech Repulic=Prague
Austria=Vienna
Israel=Jerusalem
Kuwait=Kuwait City
Greece=Athens
Iraq=Bagdad
Syria=Demascaus
Denmark=Copengahen
UK=London

There's a little-known, though powerful feature of Java's LinkedHashMap code, and it is purpose-built for managing caches like yours. In your case, you should create the Map with the access-order constructor:

private static final class MyCache extends LinkedHashMap<String, String> {

    private final int limit;

    public MyCache(int limit) {
        super(limit, 0.75f, true);
        this.limit = limit;
    }

    @Override
    protected boolean removeEldestEntry(Map.Entry<String, String> eldest) {
        return size() > limit;
    }
}

Note how you can override the removeEldestEntry method?

If you put this all together in an application, it looks like:

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.LinkedHashMap;
import java.util.Map;

public class PropertiesFileMRUCache {
    private static final class MyCache extends LinkedHashMap<String, String> {

        private final int limit;

        public MyCache(int limit) {
            super(limit, 0.75f, true);
            this.limit = limit;
        }

        @Override
        protected boolean removeEldestEntry(Map.Entry<String, String> eldest) {
            return size() > limit;
        }
    }

    private final MyCache cache;

    private final Path propertiesFile;

    public PropertiesFileMRUCache(int limit, String file) {
        cache = new MyCache(limit);
        propertiesFile = Paths.get(file);
    }

    public String getCapital(String country) {
        return cache.computeIfAbsent(country, c -> readFromFile(c));
    }

    private String readFromFile(String country) {
        final String prefix = country + "=";
        try (Stream<String> lines = Files.lines(propertiesFile)){
            return lines.filter(line -> line.startsWith(prefix))
                    .map(line -> line.substring(prefix.length()))
                    .findFirst()
                    .orElse(null);
        } catch (IOException e) {
            e.printStackTrace();
        }
        return null;
    }

    public static void main(String[] args) {
        PropertiesFileMRUCache pfc = new PropertiesFileMRUCache(8, "ccap.properties");
        pfc.getCapital("India");
        pfc.getCapital("USA");
        pfc.getCapital("UK");
        pfc.getCapital("Kuwait");
        pfc.getCapital("Iraq");
        pfc.getCapital("Canada");
        pfc.getCapital("Australia");
        pfc.getCapital("India");
        pfc.getCapital("India");
        pfc.getCapital("India");
        pfc.getCapital("Germany");
        pfc.getCapital("India");
        pfc.getCapital("Germany");
        pfc.getCapital("China");
        pfc.getCapital("Pakistan");
        System.out.println(pfc.cache);
    }
}

Note how I have also hacked around in your readFromFile method. I have made it NIO2-based using Paths, and Files. This is the best practice way to do it in Java 8. I have also streamed the lines, which I believe will help. A try-with-resources cleanly closes the stream too. Additionally, a computeIfAbsent is the right way to reuse an existing entry, and create a missing one.

There are a number of things you should identify as being different. Consider the extension of the LinkedHashMap to be the most significant, and the improved use of the computeIfAbsent to be second most important.


Async tcp socket client: send multiple requests with one connection I'm new to socket programming and c++. I have a the following method that sends an array to server and receives sum of the array, I tried to make it async.
std::future<void> AsyncClient::ReqSum(const std::vector<double>& numbers)
{
    return std::async(
        std::launch::async,
        [&, numbers]()
        {
            // Setup connection. Can move to seperate method?
            boost::asio::io_service _ioservice;
            tcp::socket _socket(_ioservice);
            _socket.connect(m_endpoint);

            std::string requestJson = Serializer::ArraySumRequest(numbers);
            boost::system::error_code err;

            boost::asio::write(_socket, boost::asio::buffer(requestJson), err);
            if (err)
            {
                std::stringstream ss;
                ss << "Couldn't write to socket! error code: " << err;
                throw ss.str();
            }

            // getting response from server
            boost::asio::streambuf receiveBuffer;
            boost::asio::read(_socket, receiveBuffer, boost::asio::transfer_all(), err);

            if (err && err != boost::asio::error::eof)
            {
                std::stringstream ss;
                ss << "Receiving from the server failed! error code: " << err.message();
                throw ss.str();
            }

            const char* data = boost::asio::buffer_cast<const char*>(receiveBuffer.data());

            rapidjson::Document doc;
            doc.Parse<rapidjson::kParseDefaultFlags>(data);

            if (!doc.HasMember("result"))
                throw "No result found!";

            double result = doc["result"].GetDouble();

            std::cout << "Sum of the array: " << result << std::endl;

            _socket.shutdown(boost::asio::socket_base::shutdown_both);
            _socket.close();
        });
}

Actually I want to know is it possible to use one connection to for multiple requests(i.e move that socket and connection to another method)?
And if there are any fixes to my code please let me know.

I want to know is it possible to use one connection to for multiple requests(i.e move that socket and connection to another method)?

In principle, yes. The easiest way would be to not create a socket in this method, but leave it up to the caller to create a socket, and pass a reference to it to AsyncClient::ReqSum(), so that the socket can be reused for multiple requests.
However, as you mentioned this doesn't work, and that is because you rely on the server to close the connection right after sending the response to a single request:
boost::asio::streambuf receiveBuffer;
boost::asio::read(_socket, receiveBuffer, boost::asio::transfer_all(), err);

The read() command here doesn't know how large the response is, but you told it to transfer all data sent by the server. The only way to signal the end of all data is for the server to close the connection. If the server would support multiple requests on a connection, then you need some way to deliniate responses. And in that case, you will have to use a function like boost::asio::read_until() to read data up to a certain point.
Don't throw strings
First of all, constructing a string using std::stringstream is quite expensive. But the main issue is that you are throwing a plain std::string. The problem is that any error handling code now has no idea what kind of error happened, unless it parses that string. That will also be inefficient.
You should instead throw a type that inherits from std::exception. In this case, either throw a std::runtime_error, or if you wish some custom type that inherits from that.
Avoid using std::endl
Prefer using "\n" instead of std::endl; the latter is equivalent to the former, but also forces a flush of the output stream, which is usually not necessary and might be bad for performance.


Autowiring publishers/listeners with reflection in Unity Framework I'm currently building an application with the Microsoft Unity Framework. I also use a simple system of message publishers/listeners inspired from MVVM light and caliburn:

The listener interface

public interface IMessageListener<T>
{

    void Handle(T Message);
}

The messenger interface

public interface IMessagePublisher<T>
{

    void Subscribe(IMessageListener<T> subscriber);

    void Unsubscribe(IMessageListener<T> subscriber);

    void Publish(T message);
}

I started to find it boring to have to inject my messengers to my listeners so they can subscribe, so I thought: "Hey, my Unity container can do that". I've coded a UnityExtension which autowires all resolved listener objects with their appropriate messenger(s) by using reflection. Of course, my messengers are ContainerLifeTimeManaged.

public override void PostBuildUp(IBuilderContext context)
        {
            base.PostBuildUp(context);

            foreach (Type interf in context.Existing.GetType().GetInterfaces().Where(i => i.IsGenericType))
            {
                //If the interface is of type IMessageListener
                if (interf.GetGenericTypeDefinition() == typeof(IMessageListener<object>).GetGenericTypeDefinition())
                {
                    //Get the generic parameter type
                    Type genericParameter = interf.GetGenericArguments().FirstOrDefault();
                    //Build the messenger type
                    Type messengerType = typeof(IMessagePublisher<object>).GetGenericTypeDefinition().MakeGenericType(genericParameter);
                    //Resolve the messenger
                    object messenger = _container.Resolve(messengerType);
                    //Find the subscribe method
                    MethodInfo subscribeMethod = messenger.GetType().GetMethod("Subscribe");
                    //Subscribe the listener (the array is ugly, but i just keep getting an error without it)
                    subscribeMethod.Invoke(messenger, new []{context.Existing});
                }
            }
       }

I've removed the exception handling on purpose. I've struggled a bit to achieve this, and I'm not sure that it's the most efficient way.

typeof(IMessageListener<object>).GetGenericTypeDefinition()

You can simplify this to just:

typeof(IMessageListener<>)

Why are you combing Where() with an if? I would stick with one or the other.

interf.GetGenericArguments().FirstOrDefault()

Don't use OrDefault when you're not going to deal with the null. That way, when an error happens, you're going to get a descriptive exception right away, instead of a confusing NullReferenceException later.

If this is part of the exception handling that you removed, then that makes it hard to review your code properly.

// […] the array is ugly, but i just keep getting an error without it

You have to use an array here, MethodInfo.Invoke() is not a params method.


Display the difference between DataFrames' dtypes? I recently updated Pandas and noticed that it is no longer possible to merge DataFrames with mismatched dtypes. I have a script which relied on merging and seemed to work in the past despite having mismatched dtypes. I need to display to the user which columns in two dataframes are causing problems, so the user can then adjust the types accordingly. (Specifically, one dataframe is read in from a database and represents what is currently in the DB, while the second dataframe includes any changes/new data to be applied to the database. Once the the user finds the problem columns they can determine if the DataFrame that contains the changes was meant to change the database's type or whether there is an error in the changes). The following code appears to work, but I feel like pandas must have a built in better way to deal with this problem.

def get_mismatched_dtypes(self, df1, df2):
    mismatch = {}
    for key, val in df1.dtypes.iteritems():
        if key in df2.dtypes and df2.dtypes[key] != val:
            mismatch[key] = (f"df1:{val}, df2: {df2.dtypes[key]}")
    print(mismatch)
    return mismatch

self

Why put this method on a class? the lack of use of self in the method should act as a flag

string result

you format the mismatch as a string (f"df1:{val}, df2: {df2.dtypes[key]}"). This way you can not do anything about it any more. Better would be to use a tuple or dict here

generator

generally, when I see

def method():
    result = {}
    for key in ...:
        result[key] = ...
    return key

it is more clear to work with a generator

def method():
    for key in ...:
        yield key, ...

If the user of the method needs a dict, he can do dict(method()). If he wants to iterate over the items, there is no need to instantiate the dict

alternative approach

you can use sets to see which types are different:

def get_mismatched_dtypes(df1, df2):
    df1_types = set(df1.dtypes.items())
    df2_types = set(df2.dtypes.items())
    for column_name, df1_type in df1_types - df2_types:
        yield column_name, (df1_type, df2.dtypes[column_name])

df1 = pd.DataFrame({"col1": [1, 2, 3], "col2": [4, 5, 6]}, index=list(range(3)))
df2 = pd.DataFrame({"col1": list("abc"), "col2": [4, 5, 6]}, index=list(range(3,6)))
dict(get_mismatched_dtypes(df1, df2))

{'col1': (dtype('int64'), dtype('O'))}

for clarification: df1_types - df2_types is {('col1', dtype('int64'))}

If you want it formatted like in your code:

{
    column_name: f"df1: {df1_type}, df2: {df2_type}"
    for column_name, (df1_type, df2_type) in get_mismatched_dtypes(df1, df2)
}

{'col1': 'df1: int64, df2: object'}


Calculating the minimum distance of a linear code This is a solution for the problem named Codes on the kattis platform:

  Given a generator matrix of a linear code, calculate the minimum distance of the code.

I'm looking for reviews on optimization and best practices. Code readability should be preserved.

public static class LinearCode {
   private int[][] generator;
   private int n;
   private int k;

   public LinearCode(int[][] generator, int n, int k) {
      this.generator = generator;
      this.n = n;
      this.k = k;
   }

   public int getMinimumDistance() {
      int minDistance = n;
      int maxKString = 1 << k;

      // generate all k-strings
      for (int i = 1; i < maxKString; i++) {
          int kstring = i;
          int distance = 0;

          // matrix multiplication
          for (int j = 0; j < n; j++) {
              int p = 0;
              for (int z = 0; z < k; z++) {
                  int a = generator[j][k-z-1];
                  int b = (kstring >>> z) & 1;
                  p = (p + a * b) & 1;
              }
              // distance as number if 1 bits
              distance += p;
          }

          minDistance = Math.min(minDistance, distance);
      }

      return minDistance;
   }
}

The n and k parameters seem pointless: you can derive them from the dimensions of the received generator matrix:

int n = generator.length;
int k = generator[0].length;

Instead of this:

for (int j = 0; j < n; j++) {

You could replace using for-each, which is just more natural:

for (int[] row : generator) {

I don't really see a reason to store the parameters as fields.
The class doesn't need to hold data.
You need just a pure function,
so this can be a public static method.

Putting it together

Applying the above suggestions (+ a few minor things):

public static int getMinimumDistance(int[][] generator) {
    int n = generator.length;
    int k = generator[0].length;
    int minDistance = n;
    int maxKString = 1 << k;

    // generate all k-strings
    for (int i = 1; i < maxKString; i++) {
        int distance = 0;

        // matrix multiplication
        for (int[] row : generator) {
            int p = 0;
            for (int z = 0; z < k; z++) {
                int a = row[k - z - 1];
                int b = (i >>> z) & 1;
                p = (p + a * b) & 1;
            }
            // distance as number if 1 bits
            distance += p;
        }

        minDistance = Math.min(minDistance, distance);
    }

    return minDistance;
}

Unit testing

I recommend adding unit tests to verify your solution, for example:

@Test
public void testSampleExercise1() {
    assertEquals(3, LinearCode.getMinimumDistance(new int[][]{
            {1, 0, 0, 0},
            {0, 1, 0, 0},
            {0, 0, 1, 0},
            {0, 0, 0, 1},
            {0, 1, 1, 1},
            {1, 0, 1, 1},
            {1, 1, 0, 1},
    }));
}

@Test
public void testSampleExercise2() {
    assertEquals(0, LinearCode.getMinimumDistance(new int[][]{
            {1, 1},
            {0, 0},
            {1, 1},
    }));
}


There's not a lot of code here so there's not a lot to say. ^_^

Implementations like this are generally outside of the day-to-day knowledge of most programmers. You may wish to include a reference in the code to how the algorithm was derived and how it works.
The chosen variable names are pretty useless. I have no idea what an or sm mean. Because I don't know what they mean, I didn't change them, but you should. (Looking back at the Ada implementation, I see that sm should be sum and an is just the current value of the lookup table, but it shouldn't be required to find 2 other implementations to understand this one).
Leading underscores in a variable name means the variable is unused (but cannot be removed for some reason). Don't use a underscore and use the variable at the same time.
If something is a constant, make it so. Constants use const or static and are named with SCREAMING_SNAKE_CASE.
There is inconsistent alignment in the lookup table. The first column is aligned on the decimal point, the second and third columns are not.
The sm variable doesn't need an explicit type as type inference will cover it.
There are inplace operations like *= which are shorter than foo = foo * bar.
There's a f64::recip method which may be more intuitive.
Operators like / should have space on both sides.
Is there a benefit to iterating in reverse every time? I'd just reorder the table and iterate forwards. Computers generally like going forwards.
The loop can be simplified by using Iterator::fold.

const TAYLOR_COEFFICIENTS: [f64; 29] = [
    -0.00000000000000000023,  0.00000000000000000141,  0.00000000000000000119,
    -0.00000000000000011813,  0.00000000000000122678, -0.00000000000000534812,
    -0.00000000000002058326,  0.00000000000051003703, -0.00000000000369680562,
     0.00000000000778226344,  0.00000000010434267117, -0.00000000118127457049,
     0.00000000500200764447,  0.00000000611609510448, -0.00000020563384169776,
     0.00000113302723198170, -0.00000125049348214267, -0.00002013485478078824,
     0.00012805028238811619, -0.00021524167411495097, -0.00116516759185906511,
     0.00721894324666309954, -0.00962197152787697356, -0.04219773455554433675,
     0.16653861138229148950, -0.04200263503409523553, -0.65587807152025388108,
     0.57721566490153286061,  1.00000000000000000000,
];

const INITIAL_SUM: f64 = 0.00000000000000000002;

fn gamma(x: f64) -> f64 {
    TAYLOR_COEFFICIENTS.iter().fold(INITIAL_SUM, |sum, coefficient| {
        sum * (x - 1.0) + coefficient
    }).recip()
}

fn main() {
    for i in 1..11 {
        let f = i as f64;
        println!("{}", gamma(f / 3.0));
    }
}

Notes based on the original Ada implementation:

The lookup table was defined inside the gamma function, and the same could be done here. It's a matter of personal preference at this point in time.
Long numbers could be separated with underscores: 0.111_222_333.
The loop invariant 1.0 - x could be hoisted out of the loop, but I'd trust the optimizer to do that until proven otherwise.


HackerRank: Left Array Rotation in Python Here is the problem on HackerRank. I want to know how I can improve this code. I am mainly trying to improve the following skills: Documentation, functional programming (top-down approach), accurately estimating Big O Complexity, Space Complexity, doc-testing, and readable code.

Sometimes I feel that my code is simply too clustered with documentation, so I am not sure how much or how little to add. Should I write param in my documentation how I have it written? Can I improve my space/time complexity for this algorithm? I tried to focus on changing the list in place so as not to increase the space complexity. Is my estimate of O(2N) correct for my main array_left_rotation() function?

If I want to write about the worst case complexity, is how I wrote it the correct way to write it and is that where I am supposed to write it?

The mysterious no return functions and other funky stuff under the main function are the courtesy of HackerRank. Just in case you want to view it on Github

I wish to improve code style, space/time complexity analysis, and readability.

"""
Author: Rafeh Qazi
Program: Array Left Rotation (Hackerrank Challenge)
Modified: October 2016
Submit this code at: https://www.hackerrank.com/challenges/ctci-array-left-rotation
"""

def array_left_rotation(int_list, shift):
    """
    Given an array of n integers and a number, d, perform d left rotations on the array.
    Then print the updated array as a single line of space-separated integers.
    :param int_list: list
    :param shift: int

    Worst Case Time Complexity: O(2N)

    Worse Case Space Complexity: O(N)

    >>> array_left_rotation([1, 2, 3, 4, 5], 1)
    2 3 4 5 1

    >>> array_left_rotation([2, 3, 4, 5, 1], 3)
    5 1 2 3 4
    """
    pairs = new_index_value_pairs(int_list, shift)
    replace_int_list(int_list, pairs)
    print(*int_list, sep=' ')

def new_index_value_pairs(int_list, shift):
    """
    Create a list of shifted index and value tuples.
    :param int_list: list
    :param shift: int
    :return: list

    Worst Case Complexity: O(N)

    >>> new_index_value_pairs([10, 20, 30], 1)
    [(2, 10), (0, 20), (1, 30)]
    """
    return [((index - shift) % len(int_list), value) for index, value in enumerate(int_list)]

def replace_int_list(int_list, pairs):
    """
    Map the the shifted index value pairs onto the original int_list.
    :param int_list: list
    :param pairs: list

    Worst Case Complexity: O(N)

    >>> int_list = [10, 20, 30]
    >>> replace_int_list(int_list, [(0, 20), (1, 30), (2, 10)])
    >>> int_list
    [20, 30, 10]
    """
    for index, value in pairs:
        int_list[index] = value

if __name__ == '__main__':
    import doctest

    doctest.testmod()
    _, shift = map(int, input().strip().split())
    int_list = list(map(int, input().strip().split()))
    array_left_rotation(int_list, shift)

Update:  I am aware of the deque data structure from collections module in Python. However, I wanted to challenge myself.

This is the easiest way I can think of to do a "shift" on a list in Python:

def shift_list(array, s):
    """Shifts the elements of a list to the left or right.

    Args:
        array - the list to shift
        s - the amount to shift the list ('+': left-shift, '-': right-shift)

    Returns:
        shifted_array - the shifted list
    """
    # calculate actual shift amount (e.g., 11 --> 1 if length of the array is 5)
    s %= len(array)

    # uncomment this line to reverse the shift direction
    # s *= -1

    # shift array with list slicing
    shifted_array = array[s:] + array[:s]

    return shifted_array

my_array = [1, 2, 3, 4, 5]

# negative numbers
shift_list(my_array, -7)
>>> [4, 5, 1, 2, 3]

# no shift on numbers equal to the size of the array
shift_list(my_array, 5)
>>> [1, 2, 3, 4, 5]

# works on positive numbers
shift_list(my_array, 3)
>>> [4, 5, 1, 2, 3]

If you need this to shift backwards (to the left) with negative numbers, add the following to the beginning of the shift list function:

s *= -1

I think the above documentation is sufficient to describe everything without being too wordy.

I have added this to the Stack Overflow Python docs.


Render arbitrary jade in express We've got a "catch-all" in app.js that renders a .jade file if it exists:

app.get('*', routes.render);

  index.js

render: function(req, res) {
  fs.exists('views' + req.url + '.jade', function(exists) {
    if (exists) {
      //substring: "/admin" -> "admin"
      res.render(req.url.substring(1));
    } else {
      res.status(404);
      res.render('404', {url: req.url});
    }
  });
}

Problem is, we've either got to move everything we don't want rendered this way into another folder, which is a hassle, or create a manual blacklist.  Is there a better way of doing this?

Here are some examples of how most apps do:

Apache renders everything in the folder. If you want to hide access to a subfolder, you do that with an htaccess in this subfolder.
Define each route yourself. This'd mean having a whitelist in your case. (In some array or something.)
Keep the routes in the database. This allows you to handle permissions easily.

I haven't often seen the blacklist way. Why? Because of a simple security principle: blacklists may accidentally treat bad input as safe.


Is there a cleaner/better way to write this Contact Manager in React? I am learning React by doing a Contact Manager, I am trying to write this code more efficiently. Any ideas...good practise, component splitting, formatting tips are accepted too. Thanks in advance
P.S. Sorry for my English...I am from Italy.
First I used a separate function for everything, then I transformed person variable in an object to use the id and to map the array, then I used the person id to remove people by clicking the side button. Is it possible to do something else?
import React, { Component } from "react";
import "./App.css";

export default function App() {
  return (

React Contact Manager

      <Content />

  );
}

class Content extends Component {
  state = {
    people: [],
    name: "",
    age: "",
    telephone: "",
    searchTerm: "",
  };

  id = 1;
  people = [];

  getName = (e) => {
    this.setState({ name: e.target.value });
  };
  getAge = (e) => {
    this.setState({ age: e.target.value });
  };
  getTelephone = (e) => {
    this.setState({ telephone: e.target.value });
  };
  getSearchTerm = (e) => {
    this.setState({ searchTerm: e.target.value });
  };

  AddPerson = () => {
    const newPerson = {
      id: this.id,
      name: this.state.name,
      age: this.state.age,
      telephone: this.state.telephone,
    };
    this.people.push(newPerson);
    this.setState({ people: this.people });
    this.id++;
  };

  RemoveElement = (id) => {
    let newPeople = this.people.filter((person) => person.id !== id);
    this.people = newPeople;
    this.setState({ people: this.people });
  };

  ClearItems = () => {
    this.people = [];
    this.setState({ people: this.people });
  };

  render() {
    return (

          <input
            type="text"
            placeholder="Name"
            className="fname"
            onChange={this.getName}
          />
          <input
            type="number"
            placeholder="Age"
            className="age"
            onChange={this.getAge}
          />
          <input
            type="tel"
            placeholder="Telephone Number"
            className="telephone"
            onChange={this.getTelephone}
          />

          <button className="add" onClick={this.AddPerson}>
            Add
          </button>

          {(this.people || []).map((person) => (

                {person.name + ", " + person.age}
                {person.telephone}

              <button
                className="delete"
                onClick={() => this.RemoveElement(person.id)}
              >
                Remove
              </button>

          ))}

        <button className="clear" onClick={this.ClearItems}>
          Clear
        </button>

    );
  }
}

Don't add extra properties to the component for things that belong in state.  Delete this.people because you already have this.state.people.  Move i into state.
Of course if you do that you cannot use mutating operations like ++ and .push() so you need to make a few changes.  When you are setting a state based on a previous state it is a best practice to use an update function to get the current state which ensures that you always have the most recent value.
You can do this:
this.setState({ people: this.state.people.concat(newPerson) });

But it's better to do this:
this.setState(prevState => ({ people: prevState.people.concat(newPerson) }));

The common code convention in JavaScript is to use camelCase for functions and class methods.  So use addPerson, removeElement, etc.
You want to be able to clear the inputs when you add a contact.  In order to do that, your <input> elements need to be controlled components which get their value from state.
Revised Code:
import React, { Component } from "react";

export default function App() {
  return (

React Contact Manager

      <Content />

  );
}

class Content extends Component {
  state = {
    people: [],
    name: "",
    age: "",
    telephone: "",
    searchTerm: "",
    id: 0,
  };

  getName = (e) => {
    this.setState({ name: e.target.value });
  };
  getAge = (e) => {
    this.setState({ age: e.target.value });
  };
  getTelephone = (e) => {
    this.setState({ telephone: e.target.value });
  };
  getSearchTerm = (e) => {
    this.setState({ searchTerm: e.target.value });
  };

  addPerson = () => {
    this.setState(state => ({
      // clear inputs
      name: "",
      age: "",
      telephone: "",
      // increment id
      id: state.id + 1,
      // add person
      people: state.people.concat({
        id: state.id,
        name: state.name,
        age: state.age,
        telephone: state.telephone,
      })
    }));
  };

  removeElement = (id) => {
    this.setState( state => ({
      people: state.people.filter((person) => person.id !== id)
    }));
  };

  clearItems = () => {
    this.setState({ people: [] });
  };

  render() {
    return (

          <input
            type="text"
            placeholder="Name"
            className="fname"
            value={this.state.name}
            onChange={this.getName}
          />
          <input
            type="number"
            placeholder="Age"
            className="age"
            value={this.state.age}
            onChange={this.getAge}
          />
          <input
            type="tel"
            placeholder="Telephone Number"
            className="telephone"
            value={this.state.telephone}
            onChange={this.getTelephone}
          />

          <button className="add" onClick={this.addPerson}>
            Add
          </button>

          {(this.state.people || []).map((person) => (

                {person.name + ", " + person.age}
                {person.telephone}

              <button
                className="delete"
                onClick={() => this.removeElement(person.id)}
              >
                Remove
              </button>

          ))}

        <button className="clear" onClick={this.clearItems}>
          Clear
        </button>

    );
  }
}


Predict the next number in any sequence Yesterday, I came up with a simple method to predict the next value in a sequence.

The method works like this:

Start with a sequence, say 1,4,9,16,25,36, call it Δ0.
Now, Δ1 is the difference between every adjacent element in Δ0. (The first element is left unchanged). For our chosen sequence, this is 1,3,5,7,9,11.
In general, Δn is the difference between every adjacent element in Δn-1.

┌──────────────────┐
│Δ0: 1,4,9,16,25,36│
│Δ1: 1,3,5,7,9,11  │
│Δ2: 1,2,2,2,2,2   │
│Δ3: 1,2,0,0,0,0   │
│Δ4: 1,1,0,0,0,0   │
│Δ5: 1,0,-1,0,0,0  │
│Δ6: 1,-1,-1,1,0,0 │
│...               │
└──────────────────┘

Now we pick first Δn where the sum of the absolute of each value is less than than sum of the absolute values of Δn+1 . In this case it is Δ5: 1,0,-1,0,0,0. Now we duplicate the last value. (1,0,-1,0,0,0,0). Now we repeatedly take the running sums of this list n times, successfully undoing all the "difference between every adjacent element" function, but because we have an extra element, we will have an extra element in this new sequence. In our chosen sequence, this new sequence is 1,4,9,16,25,36,49.

Another example would be the sequence 2,5,3,9,6,2,3(unlike the previous sequence, this one doesn't follow a clear pattern). The sum of the absolutes of this sequence is 30. Δ1 of this sequence is 2,3,-2,6,-3,-4,1. The sum of the absolutes this time is 21. We continue, Δ2 is 2,1,-5,8,-9,-1,5. The sum of the absolutes is 31. Now, we can see that Δ1 is the first with a smaller absolute sum than the next Δ in the series. Now, we duplicate the last value of Δ1, giving 2,3,-2,6,-3,-4,1,1. Since this is Δ1, we take the running sums 1 time giving 2,5,3,9,6,2,3,4 as a final result.

Here's my code.

def runningSums(lst):
    res = [lst[0]]
    for elem in lst[1:]:
        res.append(res[-1] + elem)
    return res
def antiRunningSums(lst):
    res = [lst[0]]
    for i in range(1,len(lst)):
        res.append(lst[i] - lst[i-1])
    return res
def predict(lst):
    deriv = 0
    while True:
        nxt = antiRunningSums(lst)
        if sum(map(abs, nxt)) > sum(map(abs, lst)):
            break
        lst = nxt
        deriv += 1
    lst.append(lst[-1])
    for i in range(deriv):
        lst = runningSums(lst)
    return lst

# Example call. Correctly gives back [1,4,9,16,25,36,49].
print predict([1,4,9,16,25,36])

antiRunningSums

You should use a pairwise helper to raise abstraction:

def antiRunningSums(lst):
    return [b - a for a, b in pairwise(lst)]

pairwise can easily be found in the itertools documentation.

runningSums

I think that referencing the list while you are building it is overcomplicated, I would use a variable to be incremented:

def runningSums(lst):
    s = 0
    for addend in lst:
        s += addend
        yield s

To obtain a list call list on the result of this function.


Reading a controll character in a loop in Python I have a code snippet below and because of habit from other programming languages I wanted to use a do-while loop instead. But Python offers only the construct which I show as a second code snippet instead of a do-while*loop.
What is the best pythonic way to code this?

ch = getch()
while ch != "q":
  do_something()
  ch = getch()

while True:
  ch = getch()
  do_something()
  if ch == "q":
    break

Another way would be using a generator expression. This has the effect of separating the 'control logic' and the 'action logic' (and are extremely useful!).

def getchar_until_quit():
    while True:
        ch = getch()
        if ch != "q":
            yield ch
        else:
            raise StopIteration

for ch in getchar_until_quit():
    do_something()
do_something() # to process the 'q' at the end


Improving my Tic Tac Toe Solution in Scala I'm relatively new to Scala and made a small TicTacToe in a functional Style.
Any improvement options would be highly appreciated.
There are some things which I am unsure of if they are made in an optimal way.

Error Handling in the readMove Function
If i should incorporate types like (type Player = Char, type Field = Int) and if they would benefit the code
The printBoard function looks unclean and I can't yet figure out how to best change it.
board(nextMove) = nextPlayer(board) seems to break the style of immutable values.

This is already an improvement from my first version

import scala.annotation.tailrec
import scala.io.StdIn.readLine

object TicTacToeOld {
  val startBoard: Array[Char] = Array('1', '2', '3', '4', '5', '6', '7', '8', '9')
  val patterns: Set[Set[Int]] = Set(
    Set(0, 1, 2),
    Set(3, 4, 5),
    Set(6, 7, 8),
    Set(0, 3, 6),
    Set(1, 4, 7),
    Set(2, 5, 8),
    Set(0, 4, 8),
    Set(2, 4, 6)
  )

  def main(args: Array[String]): Unit = {
    startGame()
  }

  def startGame(): Unit ={
    println("Welcome to TicTacToe!")
    println("To play, enter the number on the board where you want to play")
    printBoard(startBoard)
    nextTurn(startBoard)
  }

  @tailrec
  private def nextTurn(board: Array[Char]): Unit = {
    val nextMove = readMove(board)
    board(nextMove) = nextPlayer(board)
    printBoard(board)
    if (!isWon(board)) {
      nextTurn(board)
    }
  }

  @tailrec
  private def readMove(board: Array[Char]): Int ={
    try {
      val input = readLine("Input next Turn: ").toInt-1
      if(input<0 || input>8 || !board(input).toString.matches("[1-9]")) {
        throw new Exception
      }
      input
    } catch {
      case _: Exception => readMove(board)
    }
  }

  private def nextPlayer(board: Array[Char]): Char = {
    val remainingTurns = board.count(_.toString.matches("[1-9]"))
    if(remainingTurns%2 == 0) 'x' else 'o'
  }

  private def printBoard(board: Array[Char]): Unit = {
    print(
      0 to 2 map { r =>
        0 to 2 map { c =>
          board(c + r*3)
        } mkString "|"
      } mkString ("__________\n", "\n------\n", "\n")
    )
    println("Next Player is " + nextPlayer(board))
  }

  private def isWon(board: Array[Char]): Boolean = {
    patterns.foreach(pattern=>{
      if(pattern.forall(board(_) == board(pattern.head))) {
        print("Winner is " + board(pattern.head))
        return true
      }
    })
    false
  }
}

you can remove main method and use extends App. It's safe 3 line of code.
replace Array('1', '2', '3', '4', '5', '6', '7', '8', '9') on ('1' to '9').toArray
I prefer always use type annotation if it is not clear. E.g.: val nextMove: Int = readMove(board)
use Intellij autoformat (ctrl + alt + L)
it is good idea to say user what is wrong with his input before prompt new one
[Char].toString.matches("[1-9]") may be replaced on [Char].isDigit (PS: may be it is not correct, because 0 is also digit)
don't use return keyword, especially in foreach, map and so on. It is not what you want, at least in this cases. foreach + forall + return should be replaced on contains + forall or something.
usually list map f used only for non-chained calls, but not for list map m filter filter collect g, because it is unclear for reader. UPDATE: also this syntax used for symbol-named functions, like +, ::, ! (in akka).
you can use special type for positive digit, for example, case class PosDigit(v: Int) {require(v >= 0 && v <= 9, s"invalide positive digit: $v")}
there is no essential reason to pass board as argument. Commonly if argument is passed, it is not changed. In your code it is not.

UPDATE for 10. In functional programming the clear way is to pass immutable collection to function and return new one if you need. It is programming without side effects. In OOP there is way to use mutable collection of class (or class's object). Scala both OOP and FP language.
Sorry for my English.


Javascript bar graph representation using CSS This js was written strictly for a visualization of a graph redesign included in a stats report within our game systems. It all works properly, but I know there has to be a much neater way to organize this. I have some ideas, but I don't quite know how to approach it. Any guidance would be greatly appreciated.

    var barContainer = document.getElementById('bar-graph');
    var barCount = 15;

    for (index = 0; index < barCount; ++index) {
        var liBar = document.createElement('li');
        liBar.innerHTML = '

';
        barContainer.appendChild(liBar);
    }

    //this is for visualization. Sets height of each bar, then adds a margin to the top of each based on height, in order to keep each bar flushed with the bottom of the graph.
    var list = document.getElementsByClassName("bar");
    var percentGroup = new Array();

    for (index = 0; index < list.length; ++index) {
        var bar = list[index];
        var originalHeight = $(bar).height();
        $(bar).css( "height", originalHeight - (index * 10) );
        var barHeight = $(bar).height();
        var percentage = barHeight / originalHeight;
        percentGroup.push(percentage);
        var containerHeight = $('.graph').height();
        var marginTop = containerHeight - barHeight;
        $(bar).css('margin-top', marginTop);
        var graph = $('#bar-graph');
        var graphWidth = graph.width();
        var barWidth = $(bar).width();
        var marginRight = ( graphWidth - (barWidth * barCount) ) / ( barCount - 1 );
        $(".graph ul li").css('margin-right', marginRight);
        $(".graph ul li:nth-last-child(2)").css('margin-right', (marginRight - 1) );
    }

    //this function takes the decimal value percentages generated above, turns them into integers, and multiplies them by 100. It also creates the containers that house the percentages
    var val = document.getElementById('valContainer');

    for (index = 0; index < percentGroup.length; ++index) {
        var bar = percentGroup[index];
        bar = Math.round(bar * 100);
        var li = document.createElement('li');
        li.innerHTML = bar + '%';
        val.appendChild(li);
        $(val).css('width', (graphWidth + 4) );
        $(li).css('margin-right', marginRight);
    }

Too many temporary variables

Try to reduce the number of local temporary variables as they make the code muddy and unnecessary complicated and difficult to change.

Example, these four lines could probably be reduced to a single line:

var barHeight = $(bar).height();
...
var containerHeight = $('.graph').height();
var marginTop = containerHeight - barHeight;
$(bar).css('margin-top', marginTop);

Could be:

$(bar).css('margin-top', $('.graph').height() - $(bar).height());

Delegate tasks to functions

It is not a bad thing to sometimes delegate to functions for clearity at the exepence of minor performance hit:

var graph = $('#bar-graph');
var graphWidth = graph.width();
var barWidth = $(bar).width();
var marginRight = ( graphWidth - (barWidth * barCount) ) / ( barCount - 1 );
$(".graph ul li").css('margin-right', marginRight);
$(".graph ul li:nth-last-child(2)").css('margin-right', (marginRight - 1) );

Could become:

$(".graph ul li").css('margin-right', getRightMargin(bar, barCount));
$(".graph ul li:nth-last-child(2)").css('margin-right', (getRightMargin(bar, barCount) - 1) );
...
function getRightMargin(bar, barCount) {
    var graphWidth = $('#bar-graph').width();
    var barWidth = $(bar).width();
    return ( graphWidth - (barWidth * barCount) ) / ( barCount - 1 );
}

This way the margin calculation will be done twice, but the performance implication in this case is negligible, the code however will be less noisy and more approachable.


Compare two columns and fill in a third column I want to find out the records with same group from Column AS in Column AT and add missing invoices in Column AU considering all the values from the group in Column as below table:

Jan|Feb|Mar|ColumnAS|ColumnAT|ColumnAU

$30|blank|blank|AAA|AAA|Feb

blank|blank|blank|AAA|blank|blank

blank|blank|$35|AAA|blank|blank

blank|blank|blank|AAB|AAB|Jan,Mar

blank|$20|blank|AAB|blank|blank

blank|blank|blank|AAB|blank|blank

I have created 2 loops one (j) which runs through all values from Column AS and other (k) through AT. Temp2 is used to add unique values to column AT after comparing values from temp1

Code:

Sub MissingMonth()
Dim wSheet As Worksheet
Dim LastRow As Long
Dim temp1 As String
Dim temp2 As String
Dim j As Long, k As Long

Set wSheet = ThisWorkbook.Sheets("Source Data")
     wSheet.Select

        LastRow = ThisWorkbook.Sheets("Source Data").Cells(Rows.Count, "A").End(xlUp).Row

        temp2 = vbNullString
        For j = 2 To LastRow

            If Not (wSheet.Range("AS" & j) = temp2) Or temp2 = vbNullString Then
                temp1 = wSheet.Range("AS" & j)
                wSheet.Range("AU" & j) = "Jan Feb Mar Apr May Jun Jul Aug Sep Oct Nov Dec"
                For k = 2 To LastRow
                  temp2 = temp1
                  wSheet.Range("AT" & j) = temp2

                    If wSheet.Range("AS" & k) = temp2 Then
                        Call CheckMissingMonth(k, temp2, "I", j, "Jan ")
                        Call CheckMissingMonth(k, temp2, "J", j, "Feb ")
                        Call CheckMissingMonth(k, temp2, "K", j, "Mar ")
                        Call CheckMissingMonth(k, temp2, "L", j, "Apr ")
                        Call CheckMissingMonth(k, temp2, "M", j, "May ")
                        Call CheckMissingMonth(k, temp2, "N", j, "Jun ")
                        Call CheckMissingMonth(k, temp2, "O", j, "Jul ")
                        Call CheckMissingMonth(k, temp2, "P", j, "Aug ")
                        Call CheckMissingMonth(k, temp2, "Q", j, "Sep ")
                        Call CheckMissingMonth(k, temp2, "R", j, "Oct ")
                        Call CheckMissingMonth(k, temp2, "S", j, "Nov ")
                        Call CheckMissingMonth(k, temp2, "T", j, "Dec")

                    End If
                 Next k
             End If
          Next j
 Set wSheet = Nothing
End Sub

  Sub CheckMissingMonth(k As Long, TempMonth As String, ColumnName As String, j As Long, Replacevalue As String)
     If Not IsEmpty(ThisWorkbook.Sheets("Source Data").Cells(k, ColumnName)) Then
        ThisWorkbook.Sheets("Source Data").Range("AU" & j).Replace What:=Replacevalue, Replacement:="", LookAt:=xlPart, MatchCase:=False
   End If
End Sub

This is how I understand the requirements

Before

After

Column A is required (implied by how last row is determined)
Light red = group 1 (AAA); dark red = missing months for group 1
Light blue = group 2 (AAB); dark blue = missing months for group 2
If current record is in the same as group as previous don't update columns AT and AU

About the posted code

I found the logic a bit convoluted and hard follow (or maintain)

Indentation is inconsistent
Naming convention doesn't provide many clues for intent, examples:

MissingMonth() should be FindMissingMonths() or ShowMissingMonth()
temp1 and temp2 don't imply anything helpful, yet temp2 is critical
Usual loop counters are i, j, k (nesting hierarchy) - probably i was used at some point, then removed
LastRow (local variable names) should start with a lower case letter

temp1 is not needed
wSheet is assigned but in the LastRow assignment wSheet is not used:

LastRow = ThisWorkbook.Sheets("Source Data").Cells(Rows.Count, "A").End(xlUp).Row

should be

LastRow = wSheet.Cells(wSheet.Rows.Count, "A").End(xlUp).Row
in sub CheckMissingMonth(), parameter TempMonth is not used
there are too many interactions with the sheet, especially when calling CheckMissingMonth() and they cause slowness

This is how I would improve it for compactness and maintainability (and a bit for performance):

Version 1

Option Explicit

Public Sub findMissingMonths()
    Const JAN = 9   'start column
    Dim lastRow As Long, i As Long, j As Long, k As Long, miss As String, rng As Range
    Dim prevAS As String, mths As String, mthArr As Variant, lastMonth As Byte

    With ThisWorkbook.Sheets("Source Data")
        Set rng = .Range(.Cells(1, JAN), .Cells(1, JAN + 11)) 'get all months from headers
        mths = Join(WorksheetFunction.Transpose(WorksheetFunction.Transpose(rng)))
        mthArr = Split(mths)    '"Jan Feb Mar Apr May Jun Jul Aug Sep Oct Nov Dec"

        lastMonth = UBound(mthArr)
        lastRow = .Cells(.Rows.Count, 1).End(xlUp).Row

        For i = 2 To lastRow                'check colAS (continue only if new group)
            If .Range("AS" & i).Value2 <> prevAS Or Len(prevAS) = 0 Then
                miss = mths                 'working string to remove the month from
                For j = 2 To lastRow        'check colAT
                    prevAS = .Range("AS" & i).Value2    'remember previous val in colAS
                    .Range("AT" & i).Value2 = prevAS
                    For k = 0 To lastMonth  'del month from full str as needed
                        If .Range("AS" & j).Value2 = prevAS Then
                          If Len(.Cells(j, JAN + k).Value2) > 0 Then
                            miss = Replace(Replace(miss, mthArr(k), ""), "  ", " ")
                          End If
                        End If
                    Next k
                Next j
                .Range("AU" & i).Value2 = Trim(miss)    'commit to cell
            End If
        Next i
    End With
End Sub

.

To optimize for performance I would eliminate all range interactions except for moving all data to memory and, at the end, placing it back on the sheet. This requires a bit more effort and code but the benefit is exponential

Here is another version optimized for speed, and this is the flow at high level:

move all data to array (variant)
create a nested dictionary that will keep track of each month value for each group:
AAA

Jan = Present (boolean)
Feb = Missing
Mar = Present
...

AAB

Jan = Missing
Feb = Missing
Mar = Present
...

first loop:

read all groups from col AS (also in memory as a smaller variant array)
initialize all values to False

second loop:

read all months' data and update all dictionary values to True as needed
update col AT and AU as needed (from dictionaries to array)

write all array data back to the sheet

Example (including all helper functions in the same module):

Version 2

Option Explicit

Private Const MONTHS As String = "Jan Feb Mar Apr May Jun Jul Aug Sep Oct Nov Dec"
Public Sub showMissingMonths()      'in VBE set reference to "Microsoft Scripting Runtime"
    Const COL_I     As Long = 9     'Month columns I to T (9 to 20)
    Const COL_AS    As Long = 45    'Work columns AS to AU (45 to 47)
    Dim ws As Worksheet, arr As Variant, missing As String
    Dim i As Long, j As Long, lastRow As Long, unique As Dictionary
    Set ws = ThisWorkbook.Sheets("Source Data")
    arr = ws.UsedRange              'copy the used range to array
    lastRow = UBound(arr)           'first dimmension of the 2D array
    Set unique = getUnique(ws.UsedRange.Columns(COL_AS).Offset(1).Resize(lastRow - 1))
    If unique.Count > 0 Then
        For i = 2 To lastRow        'by row, determine if each month contains a value
            For j = 0 To 11
                If Not unique(arr(i, COL_AS))(arr(1, COL_I + j)) Then
                    unique(arr(i, COL_AS))(arr(1, COL_I + j)) = Len(arr(i, COL_I + j)) > 0
                End If
            Next
        Next
        For i = 2 To lastRow        'update colAU with missing month, only if new group
            If i = 2 Or arr(i - 1, COL_AS) <> arr(i, COL_AS) Then
              missing = vbNullString
              For j = 0 To 11
                If Not unique(arr(i, COL_AS))(arr(1, COL_I + j)) Then
                  missing = missing & arr(1, COL_I + j) & ", "
                End If
              Next
              arr(i, COL_AS + 1) = arr(i, COL_AS)
              If Len(missing) > 0 Then arr(i, COL_AS + 2) = Left(missing, Len(missing) - 2)
            End If
        Next
    End If
    ws.UsedRange = arr              'commit back to range
End Sub

Private Function getUnique(ByRef rng As Range) As Dictionary
    Dim arr As Variant, i As Long, ub As Long, d As Dictionary, val As String
    arr = rng
    ub = UBound(arr)
    getNewDict d        'each group value contains a dictionary with all months
    If ub > 0 Then
        For i = 1 To ub
            val = arr(i, 1)
            If Len(val) > 0 Then If Not d.Exists(val) Then Set d(val) = getMonthDict
        Next
    End If
    Set getUnique = d   'returned dictionary can be empty
End Function

Private Sub getNewDict(ByRef d As Dictionary, Optional ByVal ignoreCase As Boolean = False)
    If Not d Is Nothing Then Set d = Nothing
    Set d = New Dictionary
    d.CompareMode = IIf(ignoreCase, vbTextCompare, vbBinaryCompare)
End Sub

Private Function getMonthDict(Optional ByVal ignoreCase As Boolean = False) As Dictionary
    Dim arr() As String, m As Variant, d As Dictionary
    getNewDict d
    arr = Split(MONTHS)
    For Each m In arr
        d(m) = False
    Next
    Set getMonthDict = d
End Function

PS. In a working module I'd declare all variables on separate lines, use longer and more meaningful names, and add more vertical white-spaces

PS1. Performance measurements:

Tests duration

1,000 records

As posted:29.672 sec
Version 1:53.320 sec
Version 2:0.047 sec

10.000 records

As posted:60.45  min (3,627.328 sec)
Version 1:109.30  min (6,558.547 sec)
Version 2:0.297 sec

100.000 records

Version 2:3.094 sec
Version 2:3.242 sec
Version 2:3.234 sec

1 Million records

Version 2:32.234 sec
Version 2:32.164 sec
Version 2:32.336 sec


Using a radix tree to find case-insensitive matches in large files I'm using a radix tree library to find case-insensitive matches from several relatively large files (~24 files each on the order ~150MB).

My current algorithm requires me to read the chars of an entire file zipped with their index because finding the offset of match in the file is a requirement.

Currently the processing is quite slow because I am doing redundant work on each new character. However, the alternative is to create subtries, which is memory intensive.

My (not well-commented) code is below... If anything is unclear, I'm happy to comment it more thoroughly.

import com.rklaehn.radixtree.RadixTree

import scala.io.Source

import java.io.File

object RadixTreeReader {
  def fromFile(filename: String): RadixTree[String, Boolean] =
    RadixTreeReader.fromLines(Source.fromFile(filename).getLines().toSeq)

  def fromLines(lines: Seq[String]): RadixTree[String, Boolean] = {
    // we mark a target as a terminal by making the associated value true
    // all substrings will not have a true value, so only exact matches
    // are true
    val pairs = lines.map(line => (line.toLowerCase, true))
    RadixTree[String, Boolean](pairs: _*)
  }
}

final case class Match(filename: String, offset: Int, target: String) {
  def hexOffset: String = offset.toHexString
}

class FileProcessor(private val filename: String, private val trie: RadixTree[String, Boolean]) {

  private val charAndIndex: Iterator[(Char, Int)] = Source.fromFile(filename).zipWithIndex

  private def isTarget(t: RadixTree[String, Boolean], prefix: String): Boolean =
    t.getOrDefault(prefix.toLowerCase, false)

  private def isEmptyWithPrefix(t: RadixTree[String, Boolean], prefix: String): Boolean = {
    val subtrieKeys = trie.subtreeWithPrefix(prefix).keys
    subtrieKeys.isEmpty || (subtrieKeys.size == 1 && subtrieKeys.head.isEmpty)
  }

  // this is the bottleneck -- all chars in sb were in the trie, but
  // we are rechecking them each time.
  // alternative is to pass a a trie into this, costing memory
  private def processCurrent(matches: Seq[Match], sb: StringBuffer, c: Char, i: Int): (Seq[Match], StringBuffer) = {
    sb.append(c)
    val s = sb.toString.toLowerCase
    (isTarget(trie, s), isEmptyWithPrefix(trie, s)) match {
      case (true, true) => (matches :+ Match(filename, i - s.length, s), new StringBuffer)
      case (true, false) => (matches :+ Match(filename, i - s.length, s), sb)
      case (false, true) => (matches, new StringBuffer)
      case _ => (matches, sb)
    }
  }

  def findMatches: Seq[Match] = {
    charAndIndex.foldLeft(Seq.empty[Match], new StringBuffer) { case ((acc, sb), (char, idx)) =>
      processCurrent(acc, sb, char, idx)
    }._1
  }
}

class DirectoryProcessor(dir: String, targetFile: String) {

  private class DirectoryReader(dir: String) {
    val files: Seq[File] = new File(dir) match {
      case d if d.exists && d.isDirectory => d.listFiles.filter(_.isFile)
      case _ => Nil
    }
  }

  private val files = new DirectoryReader(dir).files

  private val trie: RadixTree[String, Boolean] = RadixTreeReader.fromFile(targetFile)

  // Processing a file is read-only, so we can do this in parallel
  def processDirectory: collection.parallel.ParSeq[Match] = files.par.map(f => new FileProcessor(f.getPath, trie))
    .flatMap(_.findMatches)
}

Clearly I could remove unnecessary work by passing a trie (or subtrie) to FileProcessor#processCurrent where I'll traverse based on the current character, but I believe this would make the memory usage rather intractable for large enough tries.

Are there any easy optimizations that I've missed?

time complexity

There is one troubling call in processCurrent that jumps out at me - the downcasing. I get the sense that sb becomes "big", so the nested inner loop for the linear scan seems like trouble. It's the sort of thing that can easily turn e.g. a linear algorithm into a quadratic one. I understand that case matters for correctness, but just humor me for a moment. Imagine  an alternate implementation that calls toLowerCase at file read time, destroying valuable information but allowing processCurrent to happen quicker. With such results in hand as a valuable hint, could you coax the current implementation to report results faster? As a simpler alternative, could we replace all comparisons in the current implementation with case-insensitive comparisons? Maybe downcase data when handing it to the radix tree library. The point is to avoid repeatedly downcasing a given character many thousands of times - we want to do localized work on our current focus rather than doing (ignored) global work.

memory complexity

There are a few large memory consumers such as storing Match hexOffsets, and storing charAndIndex tuples rather than generating them. Their cost is "just" linear with filesize, so maybe it's an acceptable tradeoff in the name of clarity, but you might think about what the code would look like if you deleted a large ancillary object (or made it smaller). Perhaps code that depended on the large object could get by within a smaller footprint? You're memory constrained and you want parallelism, so reducing memory by 2x would let you keep twice as many cores busy.

varying file sizes, and stragglers

The parallel mapping that invokes FileProcessor is very nice, it is simple and clear. But you complain about memory pressure, and I think your input files are of diverse sizes. This leads to two suggestions: consider viewing files as a series of fixed-length subproblems to work on, and consider throwing lots of tasks into a queue that a fixed number of worker bees draw from. Then you can size it so there's about N workers for N cores, each using about 1 / Nth of memory (either 1 / Nth of heap or 1 / Nth of cache, whichever matter more to your elapsed times). If the current code encounters a mix of files where the largest is, say, double the size of the smallest, then it's likely to be a straggler that impacts elapsed time while most of memory and most of cores are idle.

style

Instead of trie, it would have been helpful to me if you had written private val targetTrie, parallel with targetFile which is a nice informative identifier. It would be very helpful to write down pre- & post-conditions that processCurrent maintains.


SICP - exercise 2.20 - same-parity Exercise 2.20.
The procedures +, *, and list take arbitrary numbers of arguments. One way to define such procedures is to use define with dotted-tail notation. In a procedure
definition, a parameter list that has a dot before the last parameter name indicates that, when the procedure is called, the initial parameters (if any)
will have as values the initial arguments, as usual, but the final parameter's value will be a list of any remaining arguments. For instance, given the
definition

(define (f x y . z) )

the procedure f can be called with two or more arguments. If we evaluate

(f 1 2 3 4 5 6)

then in the body of f, x will be 1, y will be 2, and z will be the list (3 4 5 6). Given the definition

(define (g . w) )

the procedure g can be called with zero or more arguments. If we evaluate

(g 1 2 3 4 5 6)

then in the body of g, w will be the list (1 2 3 4 5 6).

Use this notation to write a procedure same-parity that takes one or more integers and returns a list of all the arguments that have the same even-odd
parity as the first argument. For example,

(same-parity 1 2 3 4 5 6 7)
(1 3 5 7)

(same-parity 2 3 4 5 6 7)
(2 4 6)

source
Please review my code.
I have implemented a helper procedure, called predmap (Note: I am not aware of (filter) before writing this exercise). It's identical to map, but the function argument is a predicate. The item will be added in the new list if the predicate is true.
(define (predmap pred lst)
    (if (null? lst) '()
        (let ((carst (car lst))
            (next (predmap pred (cdr lst))))
            (if (pred carst)
                (cons carst next)
                next))))

Here is the same-parity procedure:
(define (same-parity . lst)
    (if (null? lst) '()
        (cons
            (car lst)
            (predmap (if (even? (car lst)) even? (lambda (x) (not (even? x))))
            (cdr lst)))))

In the second argument to cons, I put the if statement in the function argument directly to avoid repeating the call to predmap in the code. But it seems to look complicated now.
How can I make this code better and faster? Is there a better solution? Perhaps one where we would not need procedures like predmap?

Disclaimer: my Lisp is extremely rusty.

same-parity must have at least one argument, so the proper declaration would be (same-parity arg . rest).
A more natural predicate for determining same parity of x,y is (even? (- x y))
A more natural helper is (same-parity-helper arg lst). Notice the absence of dot: it can recurse.
Putting it all together,

    (define (same-parity-helper arg lst)
        (if (null? lst) '()
            (cons
                (if (even? (- arg (car lst))
                    (car lst))
                    '())
                (same-parity-helper arg (cdr lst)))))

    (define (same-parity arg . rest)
        (same-parity-helper arg rest))


Simple Tax Calculator I've been learning Java for a very short period of time but with a couple of pointers, I've been able to put this simple VAT calculator together.  If anyone could offer any ideas for improvement that'd be great!

import java.text.DecimalFormat;
import java.util.Scanner;

public class TaxCalc {

    public static void main(String[] args) {

        System.out.println("Welcome to the simple VAT Calculator.");
        System.out.println("To Calculate VAT Payable and the ex-VAT Cost type the total cost below:");
        System.out.println("----------");
        Scanner cost = new Scanner (System.in);
        double totalCost = cost.nextDouble();

        cost.close();

        System.out.println("----------");
        double exVat;
        exVat = (totalCost * 0.8);
            DecimalFormat df = new DecimalFormat("0.00");
            String exVatStr = df.format(exVat);
            exVat = Double.valueOf(exVatStr);

        double vat;
        vat = totalCost - exVat;
            DecimalFormat df2 = new DecimalFormat("0.00");
            String vatStr = df2.format(vat);
            vat = Double.valueOf(vatStr);

        System.out.println("The VAT payable is £" + vatStr);
        System.out.println("The ex-VAT Cost is £" + exVatStr);

    }
}

I second @Caridorc's recommendation. In general anytime you find repetitive code it's a sign to extract the common parts into a routine.

Use escape characters

Additionally, rather than subsequent calls to println use escape characters and concatenate the designated strings. You also want to use an escape to specify the Unicode for the pound symbol to ensure it works across platforms.

With these suggestions your two primary calls would become:

System.out.println(
    "Welcome to the simple VAT Calculator"
    + "\nTo Calculate VAT Payable and the ex-VAT Cost type the total cost below:"
    + "\n----------"
);

and

System.out.println(
    "The VAT payable is " + vatStr
    + "\n" + "The ex-VAT Cost is \u00A3" + exVatStr
);

\u00A3 is the Java Unicode specification for the pound symbol.

Declare and instantiate in one step

For exVat and vat you declare the type, and then set the value on another line. Might as well just combine steps since there's no reason to separate them in this case. I'm sure you understand this since you've done so for your Scanner and totalCost variable.

Make a habit of using try-with-resources

On topic of your scanner, good job closing it. It's something oft missed even with non-beginners. For future reference, it's ideal to use try-with-resources to ensure that is handled, and is also an example of where you may want to separate declaration and instantiation.

 double totalCost;
 try (Scanner cost = new Scanner(System.in)) {
    totalCost = cost.nextDouble();
 }

When formatting is the goal

If formatting regardless of input is your goal, you should use String's static format method and/or rather than calling print use printf

The way to use it is to add in a reference to what you're calling in the string, and the references themselves afterwards, in order.

e.g. for your purposes,

System.out.printf(
  "----------%nThe VAT payable is %10.2f%nThe ex-VAT Cost is %10.2f\u00A3", vat, exVat
);

Here the % is used for the references that follow, the .2 specifies the number of decimal places, and the 10 reserves up to 10 spaces for alignment. Read more about this here.

One benefit of using printf directly is that you don't need to use the previous routine and your entire program becomes:

import java.util.Scanner;

public class Calculator {
  public static void main(String[] args) {
    System.out.println(
      "Welcome to the simple VAT Calculator"
      + "\nTo Calculate VAT Payable and the ex-VAT Cost type the total cost below:"
      + "\n----------"
    );

    double totalCost;

    try (Scanner cost = new Scanner(System.in)) {
      totalCost = cost.nextDouble();
    }

    double exVat = (totalCost * 0.8);
    double vat = totalCost - exVat;

    System.out.printf(
      "----------%nThe VAT payable is %10.2f%nThe ex-VAT Cost is %10.2f\u00A3", vat, exVat
    );
  }
}


Changing URLs into HTML links This method changes all URLs in the string into an HTML link. The goal is to not mutate the string passed in, which I believe is a good practice.

The new_string code in this method does not seem idiomatic.

def autolink_string string
  return    if string.nil?
  return '' if string.empty?

  regex = /(\S+\.(com|net|org|edu|gov)(\/\S+)?)/
  groups = string.scan regex
  matches = groups.map &:first
  new_string = string.clone

  matches.each do |match|
    new_string.gsub! match, "#{ match }"
  end

  new_string
end

How can I refactor to remove the new_string? Any other suggestions?

You can use gsub! and block directly with a clone.

def autolink_string str
  unless str.empty?
    regex = /(\S+\.(com|net|org|edu|gov)(\/\S+)?)/
    str.clone.gsub!(regex) do |match|
      "#{ match }"
    end || str # Return str if no match
  end
end

In console:

> str = "this is a test abc.com"
> result = autolink_string str
=> "this is a test abc.com"
> str
=> "this is a test abc.com"
> result
=> "this is a test abc.com"

> autolink_string "no url here"
=> "no url here"

> autolink_string ""
=> ""

> autolink_string "this is a test abc.com and test twitter.com"
=> "this is a test abc.com and test twitter.com"


Python - The Collatz Sequence Can I get a review of my code for the the Collatz Sequence from Chapter three of Automate the Boring Stuff with Python?

  The Collatz Sequence

  Write a function named collatz() that has one parameter named number.
  If number is even, then collatz() should print number // 2 and return
  this value. If number is odd, then collatz() should print and return 3
  * number + 1.

  Then write a program that lets the user type in an integer and that
  keeps calling collatz() on that number until the function returns the
  value 1. (Amazingly enough, this sequence actually works for any
  integer—sooner or later, using this sequence, you’ll arrive at 1! Even
  mathematicians aren’t sure why. Your program is exploring what’s
  called the Collatz sequence, sometimes called “the simplest impossible
  math problem.”)

  Remember to convert the return value from input() to an integer with
  the int() function; otherwise, it will be a string value.

  Hint: An integer number is even if number % 2 == 0, and it’s odd if
  number % 2 == 1.

  The output of this program could look something like this:

  Enter number:
3
10
5
16
8
4
2
1

  Input Validation

  Add try and except statements to the previous project to detect
  whether the user types in a noninteger string. Normally, the int()
  function will raise a ValueError error if it is passed a noninteger
  string, as in int('puppy'). In the except clause, print a message to
  the user saying they must enter an integer.

I am mainly wondering if there is a cleaner way to write my solution.

def collatz(num):
    while num > 1:
        if num % 2 == 0:
            print(num//2)
            num = num //2
        elif num % 2 ==1:
            print(3*num+1)
            num = 3*num+1
        else:
            print(num)

def getNum():
    global num
    num = (input("> "))
    try:
        num = int(num)
    except ValueError:
        print('plese enter a number')
        getNum()
getNum()
collatz(num)

Prompt

The most obvious bad practice here is the use of a global variable.  Instead of setting num as a side-effect, your function should return the result.

getNum() is not such a good name for the function.  PEP 8, the official style guide for Python, says that function names should be lower_case_with_underscores.  Furthermore, "get" implies that the function is retrieving a piece of data that is already stored somewhere, which is not the case here.  Finally, "Num" should be more specific.

The use of recursion is not appropriate.  If you want a loop, write a loop.

def ask_integer():
    """
    Return an integer entered by the user (repeatedly prompting if
    the input is not a valid integer).
    """
    while True:
        try:
            return int(input("> "))
        except ValueError:
            print("Please enter an integer")

num = ask_integer()

collatz function

Strictly speaking, you didn't follow the instructions.  Your solution isn't wrong or bad — you just didn't implement the collatz function according to the specification that was given, which says that you should print and return one single number.

def collatz(num):
    """
    Given a number, print and return its successor in the Collatz sequence.
    """
    next = num // 2 if num % 2 == 0 else 3 * num + 1
    print(next)
    return next

num = ask_integer()
while num > 1:
    num = collatz(num)


Rendering content conditional on the authentication state of the user During a code review the following approach of delivering content conditionally dependent upon the state of the user was rejected as adverse to performance due to its multiple use of RenderAction, and instead the approach of RenderPartial was preferred. In order to take this approach it would be necessary to include conditional logic in the view, thus violating separation of concern.

What would you advise as the optimum approach for conditionally including a partial view without including logic in the view?

Header.cshtml

@Model HeaderModel

Some markup

@Html.RenderAction("AuthenticatedUserPanel", Model.SubModel)
@Html.RenderAction("UnauthenticatedUserPanel", Model.SubModel)

Controller.cs

Controller(IContext context)
{
     _context = context;
}

public ActionResult AuthenticatedUserPanel(Model model)
{
    if(_context.IsAuthenticated())
    {
        return View("AuthenticatedUserPanel.cshtml", model)
    }

    return EmptyContentResult();
}

public ActionResult UnauthenticatedUserPanel(Model model)
{
    if(!_context.IsAuthenticated())
    {
        return View("UnauthenticatedUserPanel.cshtml", model)
    }

    return EmptyContentResult();
}

I could possibly include the name of the target view to render in my model e.g. So that my logic for determining which view to use is contained within my ModelAdapter, but this approach feels temperamental and more error prone?

Header.cshtml

@Model HeaderModel

Some markup

@Html.RenderAction(Model.TheViewToRender, Model.SubModel)

One way you could handle this, because it's server side rendering, is perform whatever checks you wish to perform in the controller, and then route to a ViewModel that has the relevant included pages. That results in having more code to maintain, however, it enables you to have partial view rendering as well.

Edit:

I thought I would expound on this idea some. It appears to be what you are doing with the AuthenticatedUserPanel and UnauthenticatedUserPanel.

I don't see a problem with this approach. At the end of the day, you have to put this logic somewhere. Some purists may so no logic in the view. That's fine. My approach, in this situation, is to create all the necessary components, and render a different view (or ViewModel) based on some user context. The benefit to this approach is that the user can only see UI elements for actions they are able to perform, or only view data they have permission to view. It also enables you to (easily) customize the action links for subsequent navigation. The challenge I usually have with this approach is designing a view that is relevant for each type of user.

In closing, I don't see any problem with putting some of this logic in the view, but I also tend to build smaller sites. This is largely a matter of taste, and it's important to do it the same way across the project. In trying to design the way that you will handle this sort of scenario, I would consult other areas of the project that are doing it today. If there are none, then it's usually a good time to discuss with other members of the team how you will handle this case, because you will undoubtedly encounter it again.

I hope my post was helpful, and wish you the best with the project :)


Copy a similar list to another My code works fine and I am just wondering is there a more efficient way to copy a similar list to another and ignore the properties which are not present.

private const BindingFlags flags = BindingFlags.Public | BindingFlags.Instance | BindingFlags.GetProperty | BindingFlags.SetProperty;

public static List<T> MergeListData<T>(List<object> collection)
{
    if (collection == null)
        throw new ArgumentNullException("Collection", "The Collection that you are copying to cannot be null");

    List<T> result = new List<T>();

    for (int x = 0; x < collection.Count; x++)
    {
        var target = (T)Activator.CreateInstance(typeof(T));
        var propInfos = collection[x].GetType().GetProperties(flags);
        for (int i = 0; i < propInfos.Length; i++)
        {
            try
            {
                PropertyInfo _propinfo = target.GetType().GetProperty(propInfos[i].Name, flags);

                if (_propinfo != null)
                    _propinfo.SetValue(target, Nullable.GetUnderlyingType(propInfos[i].PropertyType) ?? propInfos[i].GetValue(collection[x]));

            }
            catch (ArgumentException aex) { if (!string.IsNullOrEmpty(aex.Message)) continue; }
            catch (Exception ex) { if (!string.IsNullOrEmpty(ex.Message)) return default(List<T>); }
        }
        result.Add(target);
    }
    return result;
}

And the usage is:

public class Name1
{
    public int Number   { get; set; }
    public string First { get; set; }
    public string Last  { get; set; }
}

public class Name2
{
    public string First { get; set; }
    public string Last  { get; set; }
    public decimal Wages { get; set; }
    public bool Active { get; set; }
}

 var List1 = new List<Name1> {  new Name1{  First = "Billy", Number = 1, Last = "Harte"}, new Name1 { First = "Jimmy", Number = 1, Last = "Dunne" } };

 List<Name2> bs = MergeListData<Name2>(List1.Select(x => x as object).ToList());

Small things

Your code styling is generally good, but a few possible improvements:

You can add a second generic type parameter to your method to avoid the need to cast to object. i.e. public static List<TIn,TOut> MergeListData<TOut>(List<TIn> collection)
You can probably use a more general collection than List<object> for your input collection. If you use IEnumerable<object> (changing your for for a foreach, which you should probably do anyway), then you can avoid the cast without having to do the previous bullet point, since IEnumarable<T> is covariant
Those exception handlers probably shouldn't be crammed onto a single line. If it were just the catch or the if, it'd be okay, but combining the two seems like too much
_propinfo shouldn't have the underscore, this is usually used for class-level fields.

The general approach

More of an issue is the overall approach of using reflection here. Reflection has three major problems:

It isn't safe. For example, here you could pass a generic type that doesn't have a default constructor and fail on the Activator.CreateInstance. This could be fixed with a generic type constraint, but that limits the useability of the method.
It's hard to read and maintain. The previous bullet point is one potential issue I noticed. Are there others? Who knows, reflective code is really ugly, and verifying all the edge cases by eye is tough!
It's slow. Whether this matters depends on context, but this method is pretty general-purpose, low-level, and operates on collections, which means that decent performance is at least desireable.

So, can we avoid it? Well, when you call the method, you already know what your two types are. Code with that knowledge should also know how convert between the two. So why not delete all that code and just write:

var bs = List1.Select(x => new Name2 { First = x.First, Last = x.Last }).ToList();

What you lose in automagicness you more than make up for in clarity, conciseness, verifiability, and performance. You can of course use a method somewhere instead of a lambda, to avoid spreading the mapping info all over the place.

If you really need this functionality (e.g. because there are lots of types, most of them can map to each other, and you don't feel like writing n^2 mapping methods)... well, that seems like an unusual situation, and I'd start by taking a step back and asking if this indicates a broader architectural problem. But if you've satisfied yourself that it's sensible, then I'd suggest using a library like AutoMapper rather than trying to roll this yourself. That way you don't have to concern yourself with how the magic works, and you get something much more mature and fully-featured (and, probably, performant).


Reading new data from a constantly updating file in Java I have a log file which is constantly updated with new lines of data. I need to get new added data in java as soon as it's written. For now my solution is:

public static void readNonStop(String filename, boolean goToEnd, FileReadCallback readCallback) {
    if(readCallback == null) {
        return;
    }
    try {
        BufferedReader br = new BufferedReader(new FileReader(filename));
        try {
            String line = br.readLine();
            int lineNumber = 0;

            if(goToEnd) {
                while(br.readLine() != null) {}
            }

            while (true) {
                if(line != null) {
                    readCallback.onRead(lineNumber++, line);
                } else {
                    Thread.sleep(1);
                }
                line = br.readLine();
            }
        } finally {
            br.close();
        }
    } catch (Exception e) {
        e.printStackTrace();
    }
}

But I have a feeling that there should be a better way. I don't like the idea of a constant running loop with a "sleep" inside and would prefer some sort of an event driven approach.

If I rely on FileSystem events to reopen the file each time it is modified, it introduces a delay.

What is the correct way of doing it for this situation?

You can use the Java WatchService as described here. You said that WatchService is too slow for you. There are other people who have had that problem and resolved it https://stackoverflow.com/questions/9588737/is-java-7-watchservice-slow-for-anyone-elsein this thread.

On a side note, consider using ScheduledThreadPoolExecutor rather than Thread.sleep. The former will recreate threads if they are killed by an exception, has the potential to reuse threads in a pool, and has other potential advantages.