IP Scanning program Python 3 This is the code I made to scan IP addresses. I am new to python and just want to learn how I can optimize it. I know the subprocess is slower than others to IP scanning but if there is a way to make it faster I would like to know how. :)

#!/usr/bin/env python
# coding:utf-8
import subprocess
import datetime
hostname = input("Entrez la gateway de cette manière(example: 192.168.1)")
# Ask for gateway
while True:
    file_name = input("Comment voulez-vous appelez votre dossier?Sans espace ni   caratère spéciaux(example:file_name)")
    # Ask how the user want to name the file where the result of the scan will be

    if " " in file_name:
        print("Réecrire le nom sans espace")
    # check for spaces in the name file(impossible to create name files with spaces or special characters)
    else:
        break
with open(str(file_name) + ".txt", "w")as i:
    i.write("Start time" + str(datetime.datetime.now()))
    # print the start time of the scan process
    for scan in range(1, 255):
        i.write("-" * 100)

        ip_address = str(hostname) + "." + str(scan)
        ping_response = subprocess.Popen(["ping", ip_address, "-n", '1'],   stdout=subprocess.PIPE).stdout.read()
        #Ping the ip address

        ping_response = ping_response.decode()
        # (la réponse du ping est encoder , cette commande la decode)

        print("-"*100)
        print(ping_response)
        i.write(str(ping_response))
        if ip_address == hostname + "." + "254":

            i.write("End time" + str(datetime.datetime.now()))
            # print the end time of the scan process

            i.close()
            quit()

Use the argparse module for getting user input
Validate user input
Use f"{string}" or "{}".format(string) instead of manually concatting
No need to do i.close() as the with automatically closes the file
Use a if __name__ == '__main__' guard
Add functions for re usability

Revised Code

import subprocess
import datetime
import re

import argparse

def write_result(filename, ping):
    with open(filename, "w") as f:
        f.write(f"Start time {datetime.datetime.now()}")
        for result in ping:
            f.write(result)
        f.write(f"End time {datetime.datetime.now()}")

def ping_subnet(subnet):
    for addr in range(1, 255):
        yield subprocess.Popen(["ping", f"{subnet}.{addr}", "-n", "1"], stdout=subprocess.PIPE) \
                        .stdout.read()                                                          \
                        .decode()

def main(subnet, filename):
    write_result(filename, ping_subnet(subnet))

def parse_arguments():
    parser = argparse.ArgumentParser(usage='%(prog)s [options] <subnet>',
                                     description='ip checker',
                                     epilog="python ipscanner.py 192.168.1 -f somefile.txt")
    parser.add_argument('subnet', type=str, help='the subnet you want to ping')
    parser.add_argument('-f', '--filename', type=str, help='The filename')
    args = parser.parse_args()

    if not re.match(r"(\d{1,3}\.\d{1,3}\.\d{1,3})", args.subnet) \
       or any(a not in range(1, 255) for a in map(int, args.subnet.split("."))):
        parser.error("This is not a valid subnet")

    if " " in args.filename:
        parser.error("There cannot be whitespaces in the filename")

    return args.subnet, args.filename

if __name__ == '__main__':
    main(*parse_arguments())


Calling a function once a threshold is met I'm currently writing an API for our codebase that allows us to use an Arduino Leonardo for our tests. This code is uploaded to the Arduino, and we read through the analog for work.
As stated in the docstring, this function calls a passed function once a threshold has been met. However, I need there to be a delay between function calls, as signals read from the analog can fluctuate and cause the callback function to be called more than expected. So, I devised a solution that requires the signals to depreciate 1/10 of the threshold value in order for callback to be executed again. The secondFactor is if the user wants to define their own limit of when they want the function to be called again. For example:
NeuroBoard board;
board.setTriggerOnEvenlope(700, Serial.println("Reached!");

This executes the statement once 700 is reached. Then once 630 is reached (700 - (700 x .10)), it set a boolean flag so that once 700 is reached again, it will be called. For another example:
NeuroBoard board;
board.setTriggerOnEnvelope(700, Serial.println("Reached!"), 500);

Same as above, however the boolean flag will only be set once the signals reach 500.
I hope I've explained everything so that it's easily understandable. If anything is unclear, please leave a comment and I'll address it! Below is the code:
NeuroBoard.hpp (code omitted for brevity)
class NeuroBoard {

    private:
        bool thresholdMet = false;

    public:
        /**
         * Calls the passed function when the envelope value is greater
         * than the passed threshold.
         *
         * @param threshold Threshold for envelope value.
         * @param callback Function to call when threshold is reached.
         * @param secondFactor Optional parameter for the second threshold the data must pass.
         *
         * @return void.
        **/
        void setTriggerOnEnvelope(const unsigned int& threshold, void callback(), const unsigned int& secondFactor=0);
}

NeuroBoard.cpp (code omitted for brevity)
The ISR is basically a timer that runs on the Arduino. I've configured it so every second I get about ~50,000 readings from the analog per second. The envelopeValue is the highest reading recorded, then subtracted by one every time a reading isn't equal to or greater than it.
int envelopeValue;

ISR (TIMER1_COMPA_vect) {
    int reading = analogRead(channel);
    envelopeValue = (reading >= envelopeValue) ? reading : envelopeValue - 1;
}

void NeuroBoard::setTriggerOnEnvelope(const unsigned int& threshold, void callback(), const unsigned int& secondFactor=0) {

    // TODO: Call callback when passed threshold is met by envelope value.

    const int THRESHOLD_SCALE_FACTOR = threshold / 10;
    int secondThreshold = secondFactor;

    if (secondFactor == 0) {
        secondThreshold = threshold - THRESHOLD_SCALE_FACTOR;
    }

    if (envelopeValue >= threshold) {
        if (!this->thresholdMet) {
            this->thresholdMet = true;
            callback();
        }
    } else {
        if (envelopeValue <= secondThreshold) {
            this->thresholdMet = false;
        }
    }

}

Note: Because this code is for an Arduino, I cannot use any standard library functions or imports. However, because this section of our code is pure c++, I don't think it will be an issue. But please consider that if you decide to write an answer. Thanks!

Pass small trivial types by value.
void f(unsigned ui);  // GOOD
void f(const unsigned& ui);  // WORSE

C and C++ don't do parameters of function type; they do parameters of function pointer type.
void f(void callback());     // BAD
void f(void (*callback)());  // GOOD

See Linus Torvalds' take on "parameters of array type", and then apply the same attitude to "parameters of function type." Don't write them. Write what you mean, instead.

Prefer signed int over unsigned int, unless you're doing bit-twiddling. Especially in your case, where the user might legitimately pass in a secondThreshold of zero or negative. See "The unsigned for value range antipattern" (2018-03-13).

Default function arguments are the devil. Prefer overloading:
void setTriggerOnEnvelope(int threshold, void (*cb)(), int cooldown);

void setTriggerOnEnvelope(int threshold, void (*cb)()) {
    return setTriggerOnEnvelope(threshold, cb, threshold - threshold/10);
}

Notice how I just shortened your original code by about 7 lines, because now you don't need all those helper variables (secondFactor, THRESHOLD_SCALE_FACTOR) and you don't need any mutation.

In fact, you should consider giving those two functions different names. Right now, it is much too easy for a careless programmer or refactorer to write
board.setTriggerOnEnvelope(700, [](){ Serial.println("Reached!"); });

when they actually meant
board.setTriggerOnEnvelope(700, [](){ Serial.println("Reached!"); }, 350);

If the two-argument version were named setTriggerOnEnvelopeWithDefaultCooldown, it'd be a lot harder for the programmer to run into a bug by forgetting the right number of arguments. See "Inheritance is for sharing an interface (and so is overloading)" (2020-10-09).

// TODO: Call callback when passed threshold is met by envelope value.

This TODO comment seems "done", doesn't it? Anyway, you should certainly not have any TODO comments left in code that you're posting to the CodeReview Stack Exchange, because we explicitly don't review non-working code.


Getting data from two tables This query gets data, but for 116819 rows I need to wait 2:19 minutes. Getting the same data from the table takes 2-3 seconds, but I need some rows from the view (that are matching my data from table) in my query. How can I make it faster? The database that I work on is created properly.

select aa.*, b.quantity, b.price, case when b.itemNr is null then 'status' else 'status1' end as Status
    from(
        Select a.*, b.quantity, b.price, b.itemNr
        From table a
        left join view b
        on a.itemNR = b.itemNr and a.itemLine=b.itemLine) aa
    where aa.itemType = 'a type'
    group by ....
    order by ...

If I remove group by and order by, the query takes 2:10 minutes, so I reduce it only by 9 seconds.

Actually I have resolved my own problem. My problem was that the query was taking too long to display data and this problem came from my join to a view! After I found the tables that were creating the view I extracted the one were I found my needed columns and used it in my join.
Probably the problem in joining views is with indexes. I need to read more about indexing a view but I suspect that this was my problem.


Handle stream data with Dataflow I have a websocket connection with 3-rd party API. The API returns a lot of data, in a peak hours it returns hundreds messages per second. I need to process the data for two purposes: save everything in a DB and send some data to RabbitMq.

The idea is the following:
I want to save data to DB when batch size is 1000 or by timeout which is equal to 3 seconds.

I want to publish data to RabbitMQ by 1 second timeout. It's a kind of throttling, because there are a lot of data. Moreover, I need to select the last record for the specific ticker, I have done it in the ActionBlock, f.e: we have the following records in the batch:

  {"Ticker": "MSFT", "DateTime": '2019-05-14T10:00:00:100'}
  {"Ticker": "MSFT", "DateTime": '2019-05-14T10:00:00:150'}
  {"Ticker": "AAPL", "DateTime": '2019-05-14T10:00:00:300'}

I need to publish the last for the specific ticker only, so after filtering there will be 2 records that I am going to publish:

  {"Ticker": "MSFT", "DateTime": '2019-05-14T10:00:00:150'}
  {"Ticker": "AAPL", "DateTime": '2019-05-14T10:00:00:300'}

Full code:

public class StreamMessagePipeline < T > where T: StreamingMessage {
    private readonly BatchBlock < T > _saveBatchBlock;
    private readonly BatchBlock < T > _publishBatchBlock;

    public StreamMessagePipeline() {
        _saveBatchBlock = new BatchBlock < T > (1000);
        _publishBatchBlock = new BatchBlock < T > (500);

        SetupSaveBatchPipeline();
        SetupPublishBatchPipeline();
    }

    private void SetupSaveBatchPipeline() {
        var saveBatchTimeOut = TimeSpan.FromSeconds(3);
        var saveBatchTimer = new Timer(saveBatchTimeOut.TotalMilliseconds);

        saveBatchTimer.Elapsed += (s, e) = >_saveBatchBlock.TriggerBatch();

        var actionBlockSave = new ActionBlock < IEnumerable < T >> (x = >{
            //Reset the timeout since we got a batch
            saveBatchTimer.Stop();
            saveBatchTimer.Start();

            Console.WriteLine($ "Save to DB : {x.Count()}");
        });

        _saveBatchBlock.LinkTo(actionBlockSave, new DataflowLinkOptions {
            PropagateCompletion = true
        });
    }

    private void SetupPublishBatchPipeline() {
        var publishBatchTimeOut = TimeSpan.FromSeconds(1);
        var publishBatchTimer = new Timer(publishBatchTimeOut.TotalMilliseconds);

        publishBatchTimer.Elapsed += (s, e) = >_publishBatchBlock.TriggerBatch();

        var actionBlockPublic = new ActionBlock < IEnumerable < T >> (x = >{
            var res = x.GroupBy(d => d.Ticker).Select(d = >d.OrderByDescending(s =>s.DateTime).FirstOrDefault()).ToList();

            Console.WriteLine($ "Publish data to somewhere : {res.Count()}");
            //Reset the timeout since we got a batch
            publishBatchTimer.Stop();
            publishBatchTimer.Start();

        });

        _publishBatchBlock.LinkTo(actionBlockPublic, new DataflowLinkOptions {
            PropagateCompletion = true
        });
    }

    public async Task Handle(T record) {
        await _saveBatchBlock.SendAsync(record);
        await _publishBatchBlock.SendAsync(record);
    }

}

General comments:  I would move the timeout and batch sizes into private fields.  I don't know what version of c# you are using but you could create a class or if you have the nice tuple support you can just create it like so

private readonly (TimeSpan timeout, int buffer) _saveBatchSettings = (TimeSpan.FromSeconds(3), 1000);
private readonly (TimeSpan timeout, int buffer) _publishBatchSettings = (TimeSpan.FromSeconds(1), 500);

It's nice to have these outside the main program because typically with batching you have to tweak the timeouts and size to find the sweet spot and this make it simpler to find what you need to change.

Also you have no way to complete the source.  I would recommend making the class IDisposable and in the dispose mark the source as complete.

TPL DataFlow comments:   Why not use a BroadcastBlock that is tied to the BatchBlocks then it's one send and not two.

Move this

var res = x.GroupBy(d => d.Ticker).Select(d = >d.OrderByDescending(s =>s.DateTime).FirstOrDefault()).ToList();

Into a TransformBlock and make the ActionBlock just contain the code to publish the result.

For ActionBlock, this is just my preference, I make a method that the action block calls to instead of inline lambda it.  That code is typically the processing code and it's easier to maintain and read if it's in it's own method and outside the pipeline setup.

ReactiveExtensions:  If you are willing to add ReactiveExtensions to your project you can mix and Rx and TPL DataFlow Blocks.  Rx has a built in method for your buffering called Buffer.  The DataFlow blocks has an AsObservable and AsObserver to switch to and from Rx and TPL. This would be an example for using Rx with TPL

public class StreamMessagePipeline<T> : IDisposable
    where T : StreamingMessage
{
    private BroadcastBlock<T> _source;

    private readonly (TimeSpan timeout, int buffer) _saveBatchSettings = (TimeSpan.FromSeconds(3), 1000);
    private readonly (TimeSpan timeout, int buffer) _publishBatchSettings = (TimeSpan.FromSeconds(1), 500);

    public StreamMessagePipeline()
    {
        _source = new BroadcastBlock<T>(x => x);
        SetupSaveBatchPipeline();
        SetupPublishBatchPipeline();
    }

    public async Task Handle(T record)
    {
        await _source.SendAsync(record);
    }

    private void SetupSaveBatchPipeline()
    {
        var actionBlockSave = new ActionBlock<IList<T>>(SaveBatch);

        // Instead of action block you could stay in Rx and just use Subscribe
        _source.AsObservable()
            .Buffer(_saveBatchSettings.timeout, _saveBatchSettings.buffer)
            .Where(x => x.Count > 0) // unlike TriggerBatch Buffer will send out an empty list
            .Subscribe(actionBlockSave.AsObserver());
    }

    private void SetupPublishBatchPipeline()
    {

        var transformBlock = new TransformBlock<IList<T>, IList<T>>(x =>
        {
            return x.GroupBy(d => d.Ticker)
                .Select(d => d.OrderByDescending(s => s.DateTime).FirstOrDefault()).ToList();
        });

        var actionBlockPublish = new ActionBlock<IList<T>>(PublishBatch);

        transformBlock.LinkTo(actionBlockPublish, new DataflowLinkOptions()
        {
             PropagateCompletion = true,
        });

        _source.AsObservable()
            .Buffer(_publishBatchSettings.timeout, _publishBatchSettings.buffer)
            .Where(x => x.Count > 0) // unlike TriggerBatch Buffer will send out an empty list
            .Subscribe(transformBlock.AsObserver());
    }

    private void SaveBatch(IList<T> messages)
    {
        // Save the batch
    }

    private void PublishBatch(IList<T> messages)
    {
        // publish the batch
    }

    #region IDisposable Support
    private bool disposedValue = false; // To detect redundant calls

    protected virtual void Dispose(bool disposing)
    {
        if (!disposedValue)
        {
            if (disposing)
            {
                _source.Complete();
            }
            disposedValue = true;
        }
    }

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

}

Update If you don't want to use Rx you can still make a custom block to that has the same functionality.

public static class TimerBatchBlock
{
    public static IPropagatorBlock<T, T[]> Create<T>(int batchSize, TimeSpan timeSpan, GroupingDataflowBlockOptions options = null)
    {
        var batchBlock = new BatchBlock<T>(batchSize, options ?? new GroupingDataflowBlockOptions());
        var broadCastBlock = new BroadcastBlock<T[]>(x => x);
        var bufferBlock = new BufferBlock<T[]>();

        // timer setup (System.Threading.Timer)
        var timer = new Timer(x => ((BatchBlock<T>)x).TriggerBatch(), batchBlock, timeSpan, timeSpan);
        var resetTimerBlock = new ActionBlock<T[]>(_ => timer.Change(timeSpan, timeSpan)); // reset timer each time buffer outputs
        resetTimerBlock.Completion.ContinueWith(_ => timer.Dispose());

        // link everything up
        var linkOptions = new DataflowLinkOptions()
        {
            PropagateCompletion = true,
        };
        broadCastBlock.LinkTo(resetTimerBlock, linkOptions);
        broadCastBlock.LinkTo(bufferBlock, linkOptions);
        batchBlock.LinkTo(broadCastBlock, linkOptions);

        return DataflowBlock.Encapsulate(batchBlock, bufferBlock);
    }
}

You would link BroadCastBlock -> TimerBatchBlock -> ActionBlock


Reading numbers from three text files in C++ This code is supposed to read some text files. What should I do in order to optimize my code. I was confused about writing a user defined function so that I can pass any text to it and it stores it in vectors. I have different types of vectors as you see below.

#include "stdafx.h"
#include <iostream>
#include <vector>
#include <fstream>
using namespace std;

int main()
{
    unsigned short int num1;
    float num2, num3;
    vector<unsigned short int>index;
    vector<float>lenght;
    vector<float>coordinate;
    ifstream InFile1;
    ifstream InFile2;
    ifstream InFile3;
    InFile1.open("E:\\Programming Languages\\HelloC++\\Geodetic\\Index.txt");
    InFile2.open("E:\\Programming Languages\\HelloC++\\Geodetic\\Lenght.txt");
    InFile3.open("E:\\Programming Languages\\HelloC++\\Geodetic\\Coordinate.txt");
    if (!InFile1 || !InFile2 || !InFile3){
        cerr << "Sorry Couldn't Open The File\n.";
    }
    while (InFile1 >> num1) {
        index.push_back(num1);
    }
    while (InFile2 >> num2) {
        lenght.push_back(num2);
    }
    while (InFile3 >> num3){
        coordinate.push_back(num3);
    }
    for (size_t i = 0; i < coordinate.size(); i++) {
        cout << coordinate[i]<<endl;
    }
    return 0;
}

I'm going to be cheeky here and make a recommendation that's currently beyond what I suspect your current C++ level is, because I feel like this is a great problem to introduce templates:

Consider splitting the file reading in its own function, so that you don't have to reimplement the file loading logic three times. In this case, this will necessarily be a function template, since you can load different type of values.

Here's what it would look like:

template<typename T>
std::vector<T> read_num_file(const char* file_name) {
  // open ifstream and error check here...

  std::vector<T> result;
  T val;
  while(InFile >> val) {
    result.push_back(val);
  }
  return result;
}

int main() {
  std::vector<unsigned short> index = read_num_file<unsigned short>("Index.txt");

  ...
}


Use optional to check Null pointer exception I currently have something like this:

Price price = new Price();
ActualValue actualValue = new ActualValue();
actualValue.setValue(price.getPreviousPrice().getRegion().getValue());

I want to make sure when calling getRegion() and getValue(), no NPE is thrown, trying to make it write in one line so I thought about using Optional

Currently what I have is this:

Optional.of(price)
            .flatMap(d -> Optional.ofNullable(d.getPreviousPrice())
            .flatMap(p -> Optional.ofNullable(p.getRegion())
                    .flatMap(m -> Optional.ofNullable(m.getValue()))))
            .ifPresent(v -> actualValue.setValue(v));

Looks ugly, how can I improve?

Optional.map produces a cleaner code. It's similar to flatMap except the function inside doesn't need to know about Optional. Also passing method references makes things a little bit shorter.

Optional.of(price)
        .map(Price::getPreviousPrice)
        .map(Price::getRegion)
        .map(Region::getValue)
        .ifPresent(ActualValue::setValue);


Check if there's exactly one match in a vector The challenge is to iterate over the vector only once and return the value within the range of the zero. However, if there are no matches or more than 1 match, deliver an error. The vector is unsorted and has no guarantee on it's size or the range of data.
More technically, the number of data accesses into the vector can't exceed N + 1 (access to iterators doesn't count)

I did this with a simple loop with 2 nested checks, and was wondering if there was a stdlib based or a better algorithmic solution that I'm not able to find.

Maybe ranges can help, but I'm limited to c++17 here, though the only c++17 feature I'm using is std::optional for error handling.

#include <optional>
#include <vector>

std::optional<int> get_unambiguous(const std::vector<int> &data,
                                    const int range,
                                    const int zero = 0)
{
    std::optional<int> found;
    for (const auto& item: data)
    {
        if (std::abs(item - zero) < range) {
            if (found) {
                return {};
            }
            found = item;
        }
    }
    return found;
}

The standard library find functions return the .end() iterator for the container if they fail to find anything. We could follow the same convention, instead of using std::optional.

Rather than nesting the checks, we could split it into two separate loops, and use  std::find_if from the <algorithm> header, something like this:

auto const is_in_range = [&] (int item) { return std::abs(item - zero) < range; };

auto const first = std::find_if(data.begin(), data.end(), is_in_range);

if (first == data.end())
    return data.end(); // found nothing

auto const second = std::find_if(std::next(first), data.end(), is_in_range);

if (second != data.end())
    return data.end(); // found a second entry

return first;


Uploading a file from the browser This allows users to upload an file from the browser into my Rails app using the paperclip gem. Once the file is uploaded it gets saved into the filesystem. When the user then goes in and the "show" method or the "edit" method is evoked the image is shown to the user. This is fine for image files but for .csv and .txt files I don't want to show the preview in the browser. This code is clunky and I know there is a better way to do this.

<% if @user.image? %>
<%=filetype = @user.image.url %>

<%if filetype.include? ".jpeg" %>
    isJpeg
    <%= image_tag @user.image.url %>

    <%= link_to @user.image.url, @user.image.url  %>
<% end %>
<%if filetype.include? ".gif" %>
    isGif
    <%= image_tag @user.image.url %>

    <%= link_to @user.image.url, @user.image.url  %>
<% end %>
<%if filetype.include? ".png" %>
    isPNG
    <%= image_tag @user.image.url %>

    <%= link_to @user.image.url, @user.image.url  %>
<% end %>
<%if filetype.include? ".jpg" %>
    isJPG
    <%= image_tag @user.image.url %>

    <%= link_to @user.image.url, @user.image.url  %>
<% end %>
<%if filetype.include? ".csv" %>
    isCSV

Your file was a csv file and has no preview

    <%= link_to @user.image.url, @user.image.url  %>
<% end %>

<%= image_tag @user.image.url %>

<%= link_to @user.image.url, @user.image.url  %>
<% end %>

tokland is right (on both counts), you should push all that logic into a helper. You can also add a bit of OpenStruct into the mix to make the helper nicer:

# in app/helpers/application_helper.rb or another helper
def user_image_info(user)
    info = OpenStruct.new(:has_image? => false)
    return info if(!user.image?)

    # There might be better ways to do this but I don't know paperclip.
    u = user.image.url
    %w[jpeg gif png jpg csv].find do |ext|
        # A small abuse of `find` but reasonable in this case.
        info.is = "is#{ext.upcase}" if(u.include?(".#{ext}"))
    end
    if(info.is == 'isCSV')
        info.preview_link = '
Your file was a csv file and has no preview
'.html_safe
    else
        info.preview_link = (image_tag(user.image.url) + '
').html_safe
    end
    info
end

Then in your ERB, you could do something like this:

<% info = user_image_info(@user) %>
<% if info.has_image? %>
    <%= info.is %>
    <%= info.preview_link %>
    <%= link_to @user.image.url, @user.image.url  %>
<% end %>


Sum of Square Numbers I was trying to solve a task on leetcode, where I need to find if a positive integer is the sum of squares of two other integers. My solution is below, but it fails due to exceeding time limit.

var judgeSquareSum = function(c) {
    let output = false;
    for(let i = c; i >= 0; i--) {
      for(let y = 0; y <= i; y++) {
        let sum = i*i + y*y;
        if (sum == c)
                output = true;
      }
    }

    return output;
};

How can I make this so that is efficient, and without two iterations?

You restrict the scope of the variables to the nearest enclosing scope with let,
which is good.

The use of whitespace is not consistent: for( vs if (.

The variable names seem a be arbitrary: Why i and y, and not
a and b as in the LeetCode problem description?

The first you can do to speed things up is to "early return":

var judgeSquareSum = function(c) {
    for(let i = c; i >= 0; i--) {
        for(let y = 0; y <= i; y++) {
            let sum = i*i + y*y;
            if (sum == c)
                return true;
        }
    }
    return false;
};

There is no need to continue with the loops if a decomposition into a sum of
two squares has been found.

Now note that one integer in the decomposition determines the other. If we have
a test candidate \$ a \$  for  \$ c = a^2 + b^2 \$ then it suffices
to check if \$ \sqrt{c - a^2} \$  is an integer. Also we can assume that \$ a \le b \$,
which restricts the range of \$ a \$:

var judgeSquareSum = function(c) {
    let aMax = Math.sqrt(c/2);
    for (let a = 0; a <= aMax; a++) {
        let b = Math.sqrt(c - a * a);
        if (b === Math.round(b)) {
            return true;
        }
    }
    return false;
};

Only one loop instead of two nested loops now!

Further improvement is possible with the help of mathematics. As stated in
Sum of two squares theorem,

  An integer greater than one can be written as a sum of two squares if and only if
   its prime decomposition contains no prime congruent to 3 (mod 4) raised to an odd power.

So we need to determine if in the prime factorization
$$
  c = p_1^{e_1} p_2^{e_2} \cdots p_n^{e_n}
$$
there is a term \$ p_j^{e_j} \$ such that \$ p_i = 3 \bmod 4 \$
and \$ e_j \$ is odd.

This is more code, but very efficient for for large numbers, because

the range of prime candidates \$ p \$ can be bounded by \$ \sqrt c \$, and
once a prime factor is found, \$ c \$ can be divided by that factor, thus
reducing the remaining number of iterations.

Here is a possible implementation, with more explaining comments inline:

var judgeSquareSum = function(c) {
    // Handle 0, 1, and 2 directly:
    if (c <= 2) {
        return true
    }
    // Remove even factors:
    while (c % 2 === 0) {
        c /= 2;
    }
    // Find odd prime factors:
    for (let p = 3; p <= Math.sqrt(c); p += 2) {
        if (c % p === 0) {
            // Determine exponent e of p in c:
            let e = 0;
            while (c % p === 0) {
                e += 1;
                c /= p; // Remove this prime factor
            }
            // Is it a prime congruent 4 (mod 3), raised to an odd power?
            if (p % 4 === 3 && e % 2 === 1) {
                return false;
            }
        }
    }
    // If c > 1 at this point then it is another prime factor:
    if (c % 4 === 3) {
        return false;
    }
    return true;
};


How can I enhance my linkedlist implementation in java? I recently implemented Linked List on my own. I did not use generics as I am still learning. How can I improve it:

package datastructures;

class LinkedList
{
    Node head = null;
    Node tail = null;
    int size = 0;

    public void add(int data)
    {
        Node n = new Node();
        n.setElement(data);
        if(head == null){head = n; tail = n;}
        else
        {
            tail.setNext(n);
            tail = n;
        }
        size++;
    }

    public void size()
    {
        System.out.println("Size of the list is " + size);
    }

    public void addFront(int data)
    {
        Node n = new Node();
        n.setElement(data);

        if(head == null)    {   head = n; tail = n;   }
        else
        {
            n.setNext(head);
            head = n;
        }
        size++;
    }

    public void addLast(int data)
    {
        Node n = new Node();
        n.setElement(data);

        if(head == null){head = n; tail = n;}
        else
        {
            tail.setNext(n);
            tail = n;
        }
        size++;
    }

    public void removeFirst()
    {
        if(size==0){return ;}
        else
        {
            Node n = head.getNext();
            head = n;
        }
        size--;
    }

    public void removeLast()
    {
        if(size==0){return ;}
        else
        {
            Node tmp , prev = null;
            tmp = head;

        while(tmp != null)
        {
            if(tmp.getNext() == tail){prev = tmp;}
            tmp = tmp.getNext();
        }
            tail = prev;
            tail.setNext(null);
        }
        size--;
    }

    public void printList()
    {
        Node cNode = head;
        while(cNode != null)
        {
            System.out.println(cNode.getElement()+" ");
            cNode = cNode.getNext();
        }
        System.out.println("");
    }

    public static void main(String args[])
    {
        LinkedList l = new LinkedList();
        l.add(1);
        l.add(2);
        l.add(3);
        l.add(4);
        l.add(5);
        l.add(6);
        l.add(7);
        l.add(8);
        l.addLast(9);
        l.addFront(0);
        l.printList();

        System.out.println("Head Node is "+l.head.getElement());
        System.out.println("Tail Node is "+l.tail.getElement());
        l.size();

        System.out.println("\\\\\\\\\\\\\\\\Before removing the first element////////");
        System.out.println("");

        System.out.println("\\\\\\\\\\\\\\\\After removing the first element and removing the last element////////");
        System.out.println("");

        l.removeFirst();
        l.removeLast();
        l.printList();
        System.out.println("Head Node is "+l.head.getElement());
        System.out.println("Tail Node is "+l.tail.getElement());
        l.size();
    }

}
class Node
{
    int data;
    Node next;

    public void setElement(int data)
    {
        this.data = data;
    }

    public int getElement()
    {
        return data;
    }

    public Node getNext()
    {
            return next;
    }

    public void setNext(Node n)
    {
        this.next = n;
    }

}

1 Coding conventions

In Java, you should keep the opening curly brace on the same row as the token it relates to. Instead of writing

public void add(int data)
{
    ...
}

...
else
{
    ...
}

you should write

public void add(int data) {
    ...
}

...
else {
    ...
}

What comes to other conventions, you should have a single space between if-keyword and the opening parenthesis of the condition. Instead of

if(foo(bar)) ...

you should write

if (foo(bar)) ...

Also, what comes to the if keyword, you should have another single space between the closing parenthesis of the condition and the opening curly brace. So, instead of

if (foo(bar)){
    ...
}

you should write

if (foo(bar)) {
    ...
}

Now, I suggest you do not write more than one statement on a single line. Instead of

{head = n; tail = n;}

it would be nicer to write

... {
    head = n;
    tail = n;
}

2 Field initializers

The following is superfluous:

Node head = null;
Node tail = null;
int size = 0;

By default, Java initializes the object fields to null and integer fields to 0. Also, they must be declared private, since otherwise whatever is in your package, it may tamper with the fields. So, you should write

private Node head;
private Node tail;
private int size;

3 class Node

I think it would be a better idea to declare Node as a private static inner class of LinkedList (see Summa summarum for details). Also, I would rename data to datum since it is the singular form of the word. Also, datum/data may be declared final since it is not modified. Moreover, you could declare both data and next as private and provide appropriate getters and setters.

4 printList

The conventional way to do what you do in printList is to override the Object.toString method returning a string representing the contents of your list.

5 remove*

Your list would be more useful, if your remove* methods returned the datum removed from the list.

6 size()

Printing the current size of the list to standard output is not a good idea. Instead, just return the size and let the caller do whatever (s)he wants with it (for example, print it to console).

Summa summarum

Putting all pieces together, I had this implementation in mind:

package datastructures;

import java.util.NoSuchElementException;

public class LinkedList {

    private Node head;
    private Node tail;
    private int size;

    private static final class Node {

        private final int datum;
        private Node next;

        Node(final int datum) {
            this.datum = datum;
        }

        int getDatum() {
            return datum;
        }

        Node getNext() {
            return next;
        }

        void setNext(Node n) {
            this.next = n;
        }

    }

    public void add(final int datum) {
        Node node = new Node(datum);

        if (head == null) {
            head = node;
        } else {
            tail.setNext(node);
        }

        tail = node;
        size++;
    }

    public int size() {
        return size;
    }

    public void addFirst(int datum) {
        Node node = new Node(datum);

        if (head == null) {
            tail = node;
        } else {
            node.setNext(head);
        }

        head = node;
        size++;
    }

    public void addLast(int datum) {
        add(datum);
    }

    public int removeFirst() {
        if (size == 0) {
            throw new NoSuchElementException("Removing from an empty list.");
        }

        final Node firstNode = head;
        head = head.getNext();
        size--;
        return firstNode.getDatum();
    }

    public int removeLast() {
        if (size == 0) {
            throw new NoSuchElementException("Removing from an empty list.");
        }

        final Node lastNode = tail;
        Node previous = null;
        Node current = head;

        while (current.getNext() != null) {
            previous = current;
            current = current.getNext();
        }

        tail = previous;

        if (tail != null) {
            tail.setNext(null);
        }

        size--;
        return lastNode.getDatum();
    }

    @Override
    public String toString() {
        if (size == 0) {
            return "[]";
        }

        final StringBuilder sb = new StringBuilder("[");
        sb.append(head.getDatum());
        Node node = head.getNext();

        while (node != null) {
            sb.append(", ").append(node.getDatum());
            node = node.getNext();
        }

        return sb.append("]").toString();
    }

    public static void main(String args[]) {
        LinkedList l = new LinkedList();
        l.add(1);
        l.add(2);
        l.add(3);
        l.add(4);
        l.add(5);
        l.add(6);
        l.add(7);
        l.add(8);
        l.addLast(9);
        l.addFirst(0);
        System.out.println(l);

        System.out.println("Head Node is " + l.head.getDatum());
        System.out.println("Tail Node is " + l.tail.getDatum());
        l.size();

        System.out.println("\\\\\\\\\\\\\\\\Before removing the first element////////");
        System.out.println("");

        System.out.println("\\\\\\\\\\\\\\\\After removing the first element and removing the last element////////");
        System.out.println("");

        l.removeFirst();
        l.removeLast();
        System.out.println(l);
        System.out.println("Head Node is " + l.head.getDatum());
        System.out.println("Tail Node is " + l.tail.getDatum());
        l.size();
    }
}

Hope that helps.


CSS and JavaScript DropDown menu Here is my first attempt to create an simple drop down menu with CSS and Java Script.
This is the HTML code.

    <button>Menu 1</button>

        Option 1
        Option 2

    <button>Menu 2</button>

        Option 1

    <button>Menu 3</button>

        Option 1
        Option 2
        Option 3

CSS

div.dropdown {
    display: inline-block;
}
div.dropdown button {
    background-color:#fff;
    border:0px;
    color:#424242;
    cursor:pointer;
    padding:5px 0 5px 0;
}
div.menu1, div.menu2 {
    margin:0 0 0 20px;
}
div.menu1 button {
    border-bottom:2px solid #00BCD4;
}
div.menu2 button {
    border-bottom:2px solid #4CAF50;
}
div.menu3 button {
    border-bottom:2px solid #9FA8DA;
}
div.dropdown-content {
    display: none;
}
div.dropdown-content-active {
    margin-top:5px;
    display: block;
    background-color: #fff;
    border:1px solid #EEEEEE;
    box-shadow: 0px 0px 2px 1px #dcdcdc;
    position:absolute;
    border-radius:3px;
    z-index: 10;
}
div.dropdown-content a {
    display: block;
    width:100%;
    text-decoration: none;
    color:#3d3d3d;
    padding:4px;
    box-sizing: border-box;
    font-size:0.9em;
}
div.dropdown-content a:hover {
    background-color: #f0f0f0;
}

JavsScript

document.addEventListener('DOMContentLoaded', function() {
    [].forEach.call(document.querySelectorAll('div.dropdown button'), function(dropdown) {
        dropdown.addEventListener('click', function() {

            if(getComputedStyle(this.nextSibling).display === 'none') {

                [].forEach.call(document.querySelectorAll('div.dropdown div.dropdown-content-active'), function(dropdown_active) {
                    dropdown_active.classList.remove('dropdown-content-active');
                });

                this.nextSibling.classList.add('dropdown-content-active');
            } else {
                this.nextSibling.classList.remove('dropdown-content-active');
           }

        });
    });
});

The JavaScript is really simple but I am not sure that this is the best way I can achieve this. Would appreciate your comments.

Debugging:

When running your code in Firefox, I get the following error:

  TypeError: Argument 1 of Window.getComputedStyle does not implement
  interface Element.

This is because button.nextSibling() is a text node. Use nextElementSibling() instead.

Suggestions:

I recommend attaching an "active" class to the dropdown itself instead of its content. The semantics are clearer and it provides you with more flexibility. E.g. you could style an active toggle button differently.
Also, instead of querying the computed display style (slow!) of the button in order to infer whether a dropdown is active or inactive via

if (getComputedStyle(this.nextSibling).display === 'none') {
  ...
}

you could simply check for the dropdown's "active" class via

if (dropdown.classList.contains('.active') {
  ...
}

which is faster and agnostic to the specific CSS properties of an "active" dropdown button.
Additionally, instead of relying on a fixed order of dropdown toggle button and dropdown content by querying for the dropdown button first and then getting the content via nextSibling() or nextElementSibling(), I recommend querying for the dropdown first and then separately querying its children for a dropdown-toggle and the dropdown-content.
Finally, I recommend simplifying the deeply nested structure of your code by moving the reusable deeply nested code into  a toggleDropdown function.

Style:

While some people consider the following idiomatic:

[].forEach.call(document.querySelectorAll('...'), function(element) {
  ...
});

I prefer using the simpler for ... of iteration:

for (let element of document.querySelectorAll('...')) {
  ...
}

Assigning the query results to a variable with a self-documenting name might improve readability:

let elements = document.querySelectorAll('...');
for (let element of elements) {
  ...
}

Enhancements:

You might want to listen to the "blur" event and deactivate the dropdown when its focus is transferred to an outside element.

function toggleDropdown(dropdown) {
  if (dropdown.classList.contains('active')) {
    dropdown.classList.remove('active');
  } else {
    let activeDropdowns = document.querySelectorAll('.dropdown.active');
    for (let activeDropdown of activeDropdowns) {
      activeDropdown.classList.remove('active');
    }
    dropdown.classList.add('active');
  }
}

let dropdowns = document.querySelectorAll('.dropdown');
for (let dropdown of dropdowns) {

  // Toggle when dropdown toggle clicked:
  let toggle = dropdown.querySelector('.dropdown-toggle');
  toggle.addEventListener('click', function() {
    toggleDropdown(dropdown);
  });

  // Deactivate when focus leaves dropdown:
  dropdown.addEventListener('blur', function(event) {
    if (!this.contains(event.relatedTarget)) {
      this.classList.remove('active')
    }
  }, true);
}
.dropdown {
  display: inline-block;
}

.dropdown .dropdown-toggle {
  background-color: #fff;
  border: 0;
  color: #424242;
  cursor: pointer;
  padding: 5px 0 5px 0;
}

.dropdown .dropdown-content {
  display: none;
}

.dropdown.active .dropdown-content {
  margin-top: 5px;
  display: block;
  background-color: #fff;
  border: 1px solid #eee;
  box-shadow: 0 0 2px 1px #dcdcdc;
  position: absolute;
  border-radius: 3px;
  z-index: 10;
}

.dropdown .dropdown-content a {
  display: block;
  width: 100%;
  text-decoration: none;
  color: #3d3d3d;
  padding: 4px;
  box-sizing: border-box;
  font-size: 0.9em;
}

.dropdown .dropdown-content a:hover {
  background-color: #f0f0f0;
}

.dropdown.menu1,
.dropdown.menu2 {
  margin: 0 0 0 20px;
}

.dropdown.menu1 .dropdown-toggle {
  border-bottom: 2px solid #00BCD4;
}

.dropdown.menu2 .dropdown-toggle {
  border-bottom: 2px solid #4CAF50;
}

.dropdown.menu3 .dropdown-toggle {
  border-bottom: 2px solid #9FA8DA;
}

    <button class="dropdown-toggle">Menu 1</button>

        Option 1
        Option 2

    <button class="dropdown-toggle">Menu 2</button>

        Option 1

    <button class="dropdown-toggle">Menu 3</button>

        Option 1
        Option 2
        Option 3

Edit:

You could move the declaration of both event listeners out of the loop body. However, the dropdown toggle's 'click' event listener needs access to the dropdown element it belongs to. So you would still need a closure or alternatively bind the event listener's this to the dropdown:

function onDropdownBlur(event) {
  if (!this.contains(event.relatedTarget)) {
    this.classList.remove('active')
  }
}

function onDropdownToggle(event) {
  toggleDropdown(this);
}

let dropdowns = document.querySelectorAll('.dropdown');
for (let dropdown of dropdowns) {

  // Toggle when dropdown toggle clicked:
  let toggle = dropdown.querySelector('.dropdown-toggle');
  toggle.addEventListener('click', onDropdownToggle.bind(dropdown));

  // Deactivate when focus leaves dropdown:
  dropdown.addEventListener('blur', onDropdownBlur, true);
}


Hacker rank Jesse and Cookies I am trying to solve a problem on Hacker Rank and the question is as follows:

  Jesse loves cookies. He wants the sweetness of all his cookies to be greater than value \$K\$. To do this, Jesse repeatedly mixes two cookies with the least sweetness. He creates a special combined cookie with:

sweetness =(1× Least sweet cookie + 2× 2nd least sweet cookie).

  He repeats this procedure until all the cookies in his collection have a sweetness \$\ge K\$.
  You are given Jesse's cookies. Print the number of operations required to give the cookies a sweetness \$\ge K\$. Print \$−1\$ if this isn't possible.

  Input Format

  The first line consists of integers \$N\$, the number of cookies and \$K\$, the minimum required sweetness, separated by a space.
  The next line contains \$N\$ integers describing the array \$A\$ where \$Ai\$ is the sweetness of the \$i\$th cookie in Jesse's collection.

  Constraints

  \$1\le N\le 10^6\$
    \$0\le K\le 10^9\$
    \$0\le Ai\le 10^6\$

  Output Format

  Output the number of operations that are needed to increase the cookie's sweetness \$\ge  k\$.
  Output \$−1\$ if this isn't possible.

For this I have written code in Java like this:

    public class Solution {
        public int getMinStepsToGetK(long k,LinkedList<Integer> newList){
            Collections.sort(newList);
            int count=0;
            while(newList.getFirst()<k){
                if(newList.size()>=2){
                count++;
                int tempFirst = newList.removeFirst();
                int tempSecond = newList.removeFirst();
                newList.add(tempFirst+(tempSecond*2));
                Collections.sort(newList);
                }
                else{
                    return -1;
                }
            }
            return count;
        }
        public static void main(String[] args) {
            /* Enter your code here. Read input from STDIN. Print output to STDOUT. Your class should be named Solution. */
            Solution newObj = new Solution();
            Scanner scanObj = new Scanner(System.in);
            int numOfCookies = scanObj.nextInt();
            long minSweetness = scanObj.nextLong();
            LinkedList<Integer> newList = new LinkedList<Integer>();
            for(int i=0;i<numOfCookies;i++){
                newList.add(scanObj.nextInt());
            }
            System.out.println(newObj.getMinStepsToGetK(minSweetness,newList));
        }
    }

For the above code I am getting half of my test cases right and the other half is giving time out exception (taking more than 4 seconds to give output). My question is basically: where can I improve the performance of the code? Is there any other approach which I should go about?

Your algorithm can be summarized as follows:

  Fetch the list of cookies and sort in ascending order.
  Initialize a counter to zero
  If the smallest cookie is less than K,  then:

  (a) Increment the counter and combine this cookie with the next smallest cookie (or return -1 if there are fewer than 2 cookies left)
  (b) Remove the two smallest cookies from the list and add the new cookie to the list
  (c) Sort the list in ascending order again

  Otherwise, exit with the value of the counter
  Go back to step 3

Your code is taking a long time to run because you are sorting the entire list at step 3(c). This is unnecessary; since the list is already sorted (apart from the new value being added), you can just do a binary search in \$\mathcal{O}(\log(n))\$ time to find the correct position in which to insert the combined cookie. This is going to be much faster than sorting, which typically takes \$\mathcal{O}(n\log(n))\$ time.

An even better approach would be to use a min-heap data structure, which will keep track of the smallest element in a set in the most efficient way possible.

Addendum:

I converted your code to use a PriorityQueue data structure (Java's equivalent to a min-heap). I also created some test data using the following code:

perl -e '$n=100000;$h=$n/2;print "$n $h\n";for $i(0..$n){$r = int(rand()*$n); print "$r ";};print "\n";' > testdata.txt

Your original code took 1 minute to process 100,000 items. With a priority queue, this went down to 0.7 seconds. Here's my code:

import java.util.*;

public class Solution2 {

  private static int getMinStepsToGetK(long k,PriorityQueue<Integer> newQueue){
    int count=0;
    while(newQueue.peek()<k) {
      if(newQueue.size()>=2) {
        count++;
        int tempFirst = newQueue.poll();
        int tempSecond = newQueue.poll();
        newQueue.offer(tempFirst+(tempSecond*2));
      }
      else {
        return -1;
      }
    }
    return count;
  }

  public static void main(String[] args) {
    Scanner scanObj = new Scanner(System.in);
    int numOfCookies = scanObj.nextInt();
    long minSweetness = scanObj.nextLong();
    PriorityQueue<Integer> newQueue = new PriorityQueue<Integer>();
    for(int i=0;i<numOfCookies;i++) {
      newQueue.offer(scanObj.nextInt());
    }
    System.out.println(getMinStepsToGetK(minSweetness,newQueue));
  }

}

Note: There's no need to create a new Solution object in order to access the getMinStepsToGetK() member function. Since it isn't needed externally, I declared it as a private static function.


CtCi 2.6: Implement a function to check if a linked list is a palindrome I would appreciate feedback on the linked list implementation and usage below for question 2.6 from Cracking the coding interview book.

struct Node {
    char value;
    shared_ptr<Node> next;
    Node(char v) : value(v) {};
};

shared_ptr<Node> head;

void createSampleList(string str)
{
    auto ptr = head;
    for (const char ch : str)
    {
        if(!head) {
            head = make_shared<Node>(ch);
            ptr = head;
        }
        else {
            ptr->next = make_shared<Node>(ch);
            ptr = ptr->next;
        }
    }
}

bool isPalindrome() {

    auto slow = head;
    auto fast = head;
    stack<char> tempStack;

    while (fast != nullptr && fast->next != nullptr)
    {
        tempStack.push(slow->value);
        slow = slow->next;
        fast = fast->next->next;
    }

    if(fast != nullptr)
        slow = slow->next;

    while (slow != nullptr)
    {
       char reverseVal = tempStack.top();
        tempStack.pop();
        if(reverseVal != slow->value)
            return false;
        slow = slow->next;
    }

    return true;
}

Don't use using namespace std;. See any other C++ question for the reasons.

The declaration createSampleList(string str) means that when you pass a string to that function, the whole string will be copied. That's unnecessary. Declaring the function as createSampleList(const string &str) is much more efficient.

The variable name tempStack can be improved. The word temp should not appear here at all, since it doesn't explain anything about the code.

I was first confused on why you called tempStack.pop() two times in a row. But then I saw that it was top and pop. There's nothing you can do against that, since the C++ API just doesn't provide the method that combines these two. Maybe you could move the tempStack.pop() below the if sentence.

The char reverseVal line is not indented properly. Let your IDE fix the indentation.

After each if, there should be a space. Only for function calls the space should be omitted.

A general rule is that you should not use global variables. The global variable in this case is head. It is better to pass such variables as parameters. This corresponds to natural language speaking, where you would ask Is the word asdfdsa a palindrome? instead of Is the word written at the wall over there a palindrome?. This makes your code much more flexible.

The rest of the code looks great for learning algorithms. If you want to use that code in practice, it should be much shorter. Something like this:

#include <cassert>
#include <string>

bool is_palindrome(const std::string &str)
{
    std::string rev(str.rbegin(), str.rend());
    return str == rev;
}

int main()
{
    assert(is_palindrome(""));
    assert(is_palindrome("a"));
    assert(is_palindrome("aa"));
    assert(!is_palindrome("ab"));
    assert(!is_palindrome("aaabcaaa"));
}


Worded Mastermind game in Python Just completed a /r/dailyprogrammer challenge. Challenge #238 to be precise.

A Github repository with the wordbank.txt is available here.

import random

difficulty = {1: [4, 5, 6], 2: [7, 8],
              3: [9, 10, 11], 4: [12, 13],
              5: [14, 15]
              }
#Keys are the various word lengths for each difficulty (1-5).
acceptedWord = []
#Start a blank list, used in getWords()

def getWords():
    while True:
        try:
            level = int(input("What difficulty would you like? (1,2,3,4,5) "))
            break
        except (ValueError, KeyError):
            print("That is not a number")
    #Take a numerical input from the user, attempt to parse it, if it fails, carry on the loop.
    num = random.choice(difficulty[level])
    #num (used to store word lengths) is a random choice from the available lengths for the selected difficulty.
    with open('wordbank.txt', 'r') as file:
        #Open the file containing word-bank
        for x in map(str.upper, map(str.strip, file.readlines())):
            #loop over the stripped down words
            if len(x) == num:
                acceptedWord.append(x)
                 #If length of word is equal to random choice from suitable lengths, add it to a list.

    #Random index from total number of items in list.
    answers  = random.sample(acceptedWord, num)
    #picks a selection of the available words. Amount of words = word length.
    trueWord = answers[random.randint(0, len(answers)-1)]
    #trueWord = answer. Random item of the shortlisted avaliable words
    print('\n'.join(answers))
    #Prints hints.
    game(trueWord)

def game(trueWord):
    for x in range(0,5):
        #The user has 5 guesses
        print("Guesses Left: ", 5-x)
        userInput = input("Guess: ").upper()
        #All guesses/inputs are parsed to uppercase.
        userList = list(userInput)
        #List of the letters the user inputed
        trueList = list(trueWord)
        #List of letter of
        if userInput == trueWord:
            print("Well done, you guessed correctly")
            getWords()
            #If the user enters the correct word, quit the program
        correctGuess = 0
        for item in list(userList):
            #for each letter the user inputed
            if item == trueList[userList.index(item)]:
                #if the letter is in the same position as the answer
                correctGuess += 1
                #increment the correct amount of letters by 1
        print(correctGuess, "out of ", len(trueList), "correct.")
    print("Bad luck! The answer was: ", trueWord)
    getWords()

getWords()

Some obvious changes that I'd make:

rename all the variables / methods such that they follow the snake_case convention
remove all the obvious comments and rather add a docstrings into each function instead
add only one space before and after any operator (except if we're passing some arguments into a function / class)
after , you should have a space
the acceptedWord list should be defined within the getWords() function (the same goes for the difficulty dictionary). (or better yet, remove its declaration completely and use list comprehension instead)
you should add if __name__ == '__main__'
user_list is already a list so you don't have to specify the exact same thing here: for item in list(user_list).
use string formatting when you print something: this: print(correct_guess, "out of ", len(true_list), "correct.") will become this: print("{} out of {}".format(correct_guess, len(true_list)))
when you use with to open a file in read mode, you can omit r as that's passed by default.

Reviewed code:
import random

def get_words():
    """Docstring goes here"""
    difficulty = {1: [4, 5, 6], 2: [7, 8], 3: [9, 10, 11], 4: [12, 13], 5: [14, 15]}
    while True:
        try:
            level = int(input("What difficulty would you like? (1,2,3,4,5) "))
            break
        except (ValueError, KeyError):
            print("That is not a number")

    num = random.choice(difficulty[level])

    with open('wordbank.txt') as file:
        accepted_word = [x for x in map(str.upper, map(str.strip, file.readlines())) if len(x) == num]

    answers = random.sample(accepted_word, num)
    true_word = answers[random.randint(0, len(answers) - 1)]
    print('\n'.join(answers))
    game(true_word)

def game(true_word):
    """Docstring goes here"""
    for x in range(0, 5):
        print("Guesses Left: {}".format(5 - x))
        user_input = input("Guess: ").upper()

        user_list, true_list = list(user_input),  list(true_word)
        if user_input == true_word:
            print("Well done, you guessed correctly")
            get_words()
        correct_guess = 0
        for item in user_list:
            if item == true_list[user_list.index(item)]:
                correct_guess += 1
        print("{} out of {}".format(correct_guess, len(true_list)))
    print("Bad luck! The answer was: {}".format(true_word))
    get_words()

if __name__ == '__main__':
    get_words()

More, I'd split the get_words() methods into two methods: get_user_level() and get_words():
def get_user_level():
    """Docstring goes here"""
    difficulty = {1: [4, 5, 6], 2: [7, 8], 3: [9, 10, 11], 4: [12, 13], 5: [14, 15]}
    while True:
        try:
            level = int(input("What difficulty would you like? (1,2,3,4,5) "))
            break
        except (ValueError, KeyError):
            print("That is not a number")

    return random.choice(difficulty[level])

def get_words():
    """Docstring goes here"""
    num = get_user_level()

    with open('wordbank.txt') as file:
        accepted_word = [x for x in map(str.upper, map(str.strip, file.readlines())) if len(x) == num]

    answers = random.sample(accepted_word, num)
    true_word = answers[random.randint(0, len(answers) - 1)]
    print('\n'.join(answers))
    game(true_word)


Removing C++ template arguments from a C++ function name I'm working on some code that deals with mangled C++ function names. I'm displaying these to the user, so to help with readability of some of the really long C++ template class/function names, I'm removing some of the template arguments based on how deeply nested the template argument is.

The code below works fine, but it seems fairly clunky. It feels like I'm "writing C in any language". I'm wondering if I can improve it, shorten it, and make it more pythonic.

def removeTemplateArguments(str, depthToRemove):
    """ Remove template arguments deeper than depthToRemove
        so removeTemplateArguments("foo<int>()", 1) gives foo<>()
           removeTemplateArguments("foo< bar<int> >()", 1) gives foo< >()
           removeTemplateArguments("foo< bar<int> >()", 2) gives foo< bar<> >()"""
    if depthToRemove <= 0:
        return str
    currDepth = 0
    res = ""
    for c in str:
        if currDepth < depthToRemove:
            res += c
        if c == "<":
            currDepth += 1
        elif c == ">":
            currDepth -= 1
            if currDepth < depthToRemove:
                res += ">"
    return res

For that kind of parsing itertools.groupby is the right tool.
With groupby, as I used it down below, every group is either: <,> or an actual piece of code (like foo). So the only thing left to do is to increment/decrement the depth level and decide if the group should be appended or not to the final result list.
from itertools import groupby

def remove_arguments(template, depth):
    res = []
    curr_depth = 0
    for k,g in groupby(template, lambda x: x in ['<', '>']):
        text = ''.join(g)    # rebuild the group as a string
        if text == '<':
            curr_depth += 1

        # it's important to put this part in the middle
        if curr_depth < depth:
            res.append(text)
        elif k and curr_depth == depth:   # add the inner <>
            res.append(text)

        if text == '>':
            curr_depth -= 1

    return ''.join(res)   # rebuild the complete string

It's important to put the depth-level check in between the increment and decrement part, because it has to be decided if the </> are in or out the current depth level.
Output examples:
>>> remove_arguments('foo<int>()', 1)
foo<>()
>>> remove_arguments('foo< bar<int> >()', 1)
foo<>()
>>> remove_arguments('foo< bar<int> >()', 2)
foo< bar<> >()
>>> remove_arguments('foo< bar >()', 2)
foo< bar >()

Also, a couple of quick style notes:

Don't use str as variable name or you'll shadow the builitin str.
Don't use CamelCase for functions/variable names (look at PEP8).

Inspired by lvc comment, I've grouped here possible lambdas/functions to be used in groupby:
groupby(template, lambda x: x in ['<', '>'])    # most obvious one
groupby(template, lambda x: x in '<>')   # equivalent to the one above
groupby(template, '<>'.__contains__)    # this is ugly ugly
groupby(template, '<>'.count)    # not obvious, but looks sweet

Update
To handle cases like: foo<bar<int>>(), you'll need a better groupby key function. To be specific, a key function that return the current depth-level for a given charachter.
Like this one:
def get_level(ch, level=[0]):
    current = level[0]
    if ch == '<':
        level[0] += 1
    if ch == '>':
        level[0] -= 1
        current = level[0]

    return current

That take advantage of the mutable argument level to perform some kind of memoization.
Observe that remove_arguments will now be more simple:
def remove_arguments(template, depth):
    res = []
    for k,g in groupby(template, get_level):
        if k < depth:
            res.append(''.join(g))
    return ''.join(res)


Flatten binary tree to linked list I'm trying to flatten a binary tree into a linked list.

I have a working, correct solution:

# Definition for a binary tree node.
  class TreeNode:
      def __init__(self, x):
          self.val = x
          self.left = None
          self.right = None

def insert(root, node):
    if root.right is None:
        root.right = node
        return None
    else:
        insert(root.right, node)
class Solution:
    def flatten(self, root: TreeNode) -> None:
        """
        Do not return anything, modify root in-place instead.
        """
        if root is None:
            return None
        if root.left is None and root.right is None:
            return root
        left = self.flatten(root.left)
        right = self.flatten(root.right)
        root.left = None
        if left:
            root.right = left
            insert(root.right, right)
        else:
            root.right = right
        return root

I'm not certain, but I think the time complexity is \$O(n^2)\$. How do I make it run faster? Specifically, how do I modify it so that it runs in optimal time complexity (linear time)?

Encapsulating the wrong things

insert and flatten should be methods on TreeNode. Solution doesn't particularly need to exist as a class. Those two methods can assume that self is the root.

Return types

As @Carcigenicate suggested: what do you return from flatten? Probably the return type hint should be changed to TreeNode, and you should modify your comment to describe the circumstances under which None will or won't be returned.


Scraping the names of a whole category with a three-liner code I've written a script in python using BeautifulSoup to parse the name of different coffee shops spreading across 51 pages in yellowpage website. I'm thrilled to see that it works perfectly. Except for importing libraries, I used three lines of code to do this. I think this time I've done this errorlessly.

Here is what I've tried with:

import requests
from bs4 import BeautifulSoup

for i in range(1, 52):
    for title in BeautifulSoup(requests.get("https://www.yellowpages.com/search?search_terms=coffee&geo_location_terms=Los+Angeles%2C+CA&page={0}".format(i)).text, "lxml").findAll("h2",{"class":"n"},"a"):
        print(title.text)

I would add more lines, but improve on performance:

parse only the relevant parts of the page with SoupStrainer
re-use the same session when making requests

Improved version:

import requests
from bs4 import BeautifulSoup, SoupStrainer

URL_TEMPLATE = "https://www.yellowpages.com/search?search_terms=coffee&geo_location_terms=Los+Angeles%2C+CA&page={0}"

parse_only = SoupStrainer("h2", class_="n")

with requests.Session() as session:
    for page_number in range(1, 52):
        response = session.get(URL_TEMPLATE.format(page_number))

        soup = BeautifulSoup(response.content, "lxml", parse_only=parse_only)
        for title in soup.select("h2.n"):
            print(title.get_text())

Also note a variable name change - page_number would be more descriptive than i.


Optimizing quicksort I created simple quicksort algorithm using iterators. I'm happy with implementation but not with its speed. Profiler tells me that the slowest operation here is std::iter_swap. I don't really know how to improve this code. Note that I am not asking for different kind of quicksort algorithm but about improving this one.

#include <iterator>
#include <utility>
#include <functional>

template <typename RandomIt, typename Compare = std::less_equal<>>
void Sort::QuickSort(RandomIt first, RandomIt last, Compare compare = Compare())
{
    if (std::distance(first, last) <= 1) return;
    RandomIt pivot = Partition(first, last, compare);
    QuickSort(first, pivot, compare);
    QuickSort(std::next(pivot, 1), last, compare);
}

template <typename RandomIt, typename Compare>
RandomIt Sort::Partition(RandomIt first, RandomIt last, Compare compare)
{
    RandomIt pivot = MedianOf3(first, last, compare);
    RandomIt i = first;
    for (RandomIt j = first; j != pivot; ++j)
        if (compare(*j, *pivot)) {
            std::iter_swap(i, j);
            ++i;
        }
    std::iter_swap(i, pivot);
    return i;
}

template <typename RandomIt, typename Compare>
RandomIt Sort::MedianOf3(RandomIt first, RandomIt last, Compare compare)
{
    auto collectionSize = std::distance(first, last);
    RandomIt middle = std::next(first, collectionSize / 2);
    RandomIt targetPivot = std::prev(last, 1);
    if (compare(*middle, *first))
        std::iter_swap(middle, first);
    if (compare(*targetPivot, *first))
        std::iter_swap(targetPivot, first);
    if (compare(*middle, *targetPivot))
        std::iter_swap(middle, targetPivot);
    return targetPivot;
}

For testing I used vectors of 100 000 random elements. This code average sorting time is about 1626 ms on my machine and std::sort on the same set of data performs 10 times better, around 163 ms. I know that std::sort uses introsort but still quicksort shouldn't be 10 times slower.

To get this code to compile I had to add the Sort namespace definition:

namespace Sort
{
    template <typename RandomIt, typename Compare = std::less_equal<>>
    void QuickSort(RandomIt first, RandomIt last, Compare compare = Compare());

    template <typename RandomIt, typename Compare>
    RandomIt Partition(RandomIt first, RandomIt last, Compare compare);

    template <typename RandomIt, typename Compare>
    RandomIt MedianOf3(RandomIt first, RandomIt last, Compare compare);
}

And I had to remove the default argument from the redeclaration of Sort::QuickSort:

template <typename RandomIt, typename Compare = std::less_equal<>>
void Sort::QuickSort(RandomIt first, RandomIt last, Compare compare)

Review requests should have issues like that already accounted for.

The performance of the Partition() function can be improved by doing fewer swaps.  The current loop swaps when *j < *pivot, without considering whether *i is less or greater than *pivot.  Instead, we can use two iterators, and advance them towards each other, swapping only when the ascending iterator points to an item greater than pivot and the descending iterator points to one smaller than pivot.  The tricky bit is to avoid the iterators passing each other without terminating.


Looking at the hill of indents seems like this can be improved I've spent some time on this as an answer elsewhere, and did my best to optimize it. But looking at the hill of indents seems like this can be improved. I've tried to implement an any() statement to replace the for break part, without any luck. Does anyone have any suggestions?
Groups = [[['NM1', 'OP', '1', 'SMITH', 'JOHN', 'PAUL', 'MR', 'JR'],
           ['ABC', '1L', '690553677'],
           ['DIR', '348', 'D8', '20200601'],
           ['DIR', '349', 'D8', '20200630']],
          [['NM1', 'OP', '1', 'IMA', 'MEAN', 'TURD', 'MR', 'SR'],
           ['ABC', '1L', '690545645'],
           ['ABC', '0F', '001938383',''],
           ['DIR', '348', 'D8', '20200601']]]

def ids(a, b):
    l = []
    for group in Groups:
        for lst in group:
            if lst[0] == a and lst[1] == b:
                if lst[2] == 'D8':
                    l.append(lst[3])
                else:
                    l.append(lst[2])
                break
        else:
            l.append(None)
    return l

current_id = ids('ABC', '1L')
prior_id = ids('ABC', '0F')
start_date = ids('DIR', '348')
end_date = ids('DIR', '349')

print(current_id)
print(prior_id)
print(start_date)
print(end_date)

Output:
['690553677', '690545645']
[None, '001938383']
['20200601', '20200601']
['20200630', None]

So basically, I have this Groups list, in that list are 2 nested lists. Assuming the output lists are [x, y], the first nested list in Groups is x, and the second is y.
You see my function has two parameters, a and b. x and y are determined when their corresponding nested list has a list that has a as the first index, and b as the second index.
If the nested list doesn't have a list that meets that requirement, the x or y will be None.

UPDATE: Is there any way to flatten the series of indents? At least a few?

As we do not know the task given we cannot fully review. While the code itself looks reasonable there is some smell.
date
To me it is very smelly, that a function named id() returns a date
start_date = ids('DIR', '348')

This is also requiring a switch inside your function, so most probably there are two functions intermingled that should not be.
data structure
list() is a bad structure to search in. While this may be the data format you read or get passed, you most probably should convert it to a structure allowing direct lookup (converting once). The description of the current structure is missing, also we do not know if there are other functions accessing the data.


Performance

It should be possible to accomplish this task in seconds rather than minutes, with the right data structure.  This is your main mistake:

paid = list(set(t))

The problem is, for a list with n items, it takes O(n) time to check whether the list contains a particular item.  It's particularly bad if the vast majority of the entries that you are seeking do not appear in the list — then it really does need to look at every single member of the list before concluding that an item is not in the list.

On the other hand, testing whether a particular element is a member of a set is O(1) — extremely efficient!

Miscellaneous

Please avoid cryptic filenames such as x and t.  Calling the file handle f is OK, since f is a typical name for a file handle, but since you have two files in this program, more distinctive names would help readability.  I would also avoid renaming the patent data file to f.txt, which is too cryptic.

It's a good habit to always call open() in the context of a with block, so that the file handle will be automatically closed for sure.  The 'r' mode is implied, and can be omitted.

Suggested solution

with open('data.txt') as paid_file:
    paid = set(line.rstrip() for line in paid_file)

with open('MaintFeeEvents_20160711.txt') as patent_events_file:
    for line in patent_events_file:
        if line.split(' ', 1)[0] in paid:
            print(line, end='')


Removing key/value pairs in list of dicts I have a list of dicts that all have the same keys. If a key's value is None in all dicts then I want to remove them (A solution that creates a new dict is fine as well).

I'm concerned about my Big O complexity of 2n2 (correct me if I'm wrong). Are there better solutions to what I have below?

dicts = [{'a': 1, 'b': None, 'c': 4},
         {'a': 2, 'b': None, 'c': 3},
         {'a': None, 'b': None, 'c': 3}]

expected = [{'a': 1, 'c': 4}, {'a': 2, 'c': 3}, {'a': None, 'c': 3}]

keys = dicts[0].keys()
keep_keys = []

for key in keys:
    vals = []
    for d in dicts:
        if d[key] is not None:
            vals.append(d[key])

    if len(vals) != 0:
        keep_keys.append(key)

for d in dicts:
    for k in d.keys():
        if k not in keep_keys:
            del d[k]

print dicts == expected

Any solution will have to read the values associated to each key of each dictionary; so you won't be able to drop under \$\mathcal{O}(n\times{}m)\$ where \$m\$ is the length of each dictionary. This is pretty much what you are doing, but the if k not in keep_keys call slows things a bit as it is \$\mathcal{O}(m)\$ when it could be \$\mathcal{O}(1)\$ by using a set or a dictionary.

If you change the keep_keys list into a set you simplify the logic a bit: as soon as you find a key whose value is not None you can add it into the set.

dicts = [{'a': 1, 'b': None, 'c': 4}, {'a': 2, 'b': None, 'c': 3}, {'a': None, 'b': None, 'c': 3}]
expected = [{'a': 1, 'c': 4}, {'a': 2, 'c': 3}, {'a': None, 'c': 3}]

keep_keys = set()

for d in dicts:
    for key, value in d.items():
        if value is not None:
            keep_keys.add(key)

remove_keys = set(d) - keep_keys

for d in dicts:
    for k in remove_keys:
        del d[k]

print dicts == expected

This code, as your original one, assume that there is at least one item in dicts; otherwise set(d) will generate an exception as the variable d is not defined yet.

But this code mixes the actual logic with some tests. You should wrap it in a function to ease reusability and put the testing code under an if __name__ == '__main__': clause:

def filter_nones(dictionaries):
    if not dictionaries:
        return

    keep_keys = set()

    for dict_ in dictionaries:
        for key, value in dict_.iteritems():
            if value is not None:
                keep_keys.add(key)

    remove_keys = set(dict_) - keep_keys

    for dict_ in dictionaries:
        for key in remove_keys:
            del dict_[key]

if __name__ == '__main__':
    dicts = [
            {'a': 1, 'b': None, 'c': 4},
            {'a': 2, 'b': None, 'c': 3},
            {'a': None, 'b': None, 'c': 3},
    ]
    expected = [
            {'a': 1, 'c': 4},
            {'a': 2, 'c': 3},
            {'a': None, 'c': 3},
    ]

    filter_nones(dicts)
    print dicts == expected


Simple OOP currency converter - follow-up This is a follow up question on this post
This is what I changed on my previous code:

I use Exchanger and PairCurrency class instead of Bank

I renamed addRate and addCommission to setRate and setCommission respectively

Exchanger object contains multiple PairCurrency objects which can be added to Exchanger by addPairCurrency() method.

This is my code and tests:
import pytest

class DuplicatePairError(Exception):
    pass

class Exchanger:
    def __init__(self):
        self.pair_currencies = []
        self.commission = 0.015

    def addPairCurrency(self, pair_object):
        if pair_object in self.pair_currencies:
            raise DuplicatePairError('Cannot add duplicate pair of currency')

        self.pair_currencies.append(pair_object)

    @property
    def last_pair(self):
        return self.pair_currencies[-1]

    def getRate(self, pair):
        for pair_obj in self.pair_currencies:
            if pair_obj.name == pair:
                return pair_obj.rate

    def convert(self, src_dest, amount):
        return amount / self.getRate(src_dest) * (1-self.commission)

    def editRate(self,new_object):
        for pair_obj in self.pair_currencies:
            if pair_obj.name == new_object.name:
                 pair_obj.rate = new_object.rate

class PairCurrency:

    def __init__(self, name):
        self.name = name

    def setRate(self, rate):
        self.rate = rate

    def editRate(self, new_rate):
        self.rate = new_rate

    def __eq__(self, object):
        return self.name == object

def test_canInitializePair_CurrenciesListInExhangeClass():
    exchanger = Exchanger()

    assert exchanger.pair_currencies == []

def test_canSetRateinPairCurrencyClass():
    USD_GBP = PairCurrency('USDGBP')
    USD_GBP.setRate(2)

    assert USD_GBP.name == 'USDGBP'
    assert USD_GBP.rate == 2

def test_canSetCommisionInExchanger():
    exchanger = Exchanger()

    assert exchanger.commission == 0.015

@pytest.fixture()
def USD_GBP():
    USD_GBP = PairCurrency('USDGBP')
    USD_GBP.setRate(2)
    return USD_GBP

@pytest.fixture()
def GBP_USD():
    GBP_USD = PairCurrency('GBPUSD')
    GBP_USD.setRate(0.5)
    return GBP_USD

@pytest.fixture()
def exchanger(USD_GBP,GBP_USD):

    exchanger = Exchanger()
    exchanger.addPairCurrency(USD_GBP)
    exchanger.addPairCurrency(GBP_USD)
    return exchanger

def test_canAddPairCurrencyinExchangerClass(exchanger, USD_GBP, GBP_USD):

    assert USD_GBP == exchanger.pair_currencies[-2]
    assert GBP_USD == exchanger.last_pair

def test_cannotAddDuplicatePair(exchanger, USD_GBP):
    USD_GBP2 = PairCurrency('USDGBP')
    USD_GBP2.setRate(5)

    exchanger = Exchanger()
    exchanger.addPairCurrency(USD_GBP)
    with pytest.raises(DuplicatePairError) as excinfo:
        exchanger.addPairCurrency(USD_GBP2)
    assert str(excinfo.value) == 'Cannot add duplicate pair of currency'

def test_canEditRateInPairCurrencyClass(USD_GBP):

    USD_GBP.editRate(3)
    assert USD_GBP.rate == 3

    USD_GBP.editRate(4)
    assert USD_GBP.rate == 4

def test_canEditRateinExhangerClass(exchanger):
    USD_GBP = PairCurrency('USDGBP')
    USD_GBP.setRate(5)
    exchanger.editRate(USD_GBP)
    assert exchanger.getRate('USDGBP') == 5

def test_canConvertInExchangerClass(exchanger, USD_GBP, GBP_USD):

    assert exchanger.convert(src_dest = 'USDGBP', amount = 100) == 49.25
    assert exchanger.convert(src_dest='GBPUSD', amount=100) == 197

I have few questions:

How good is my OOP design?

Is my test list fine?

Should I remove editRate() in PairCurrency and just keep editRate() in Exchanger?

Do my classes violate the SOLID Principle ?

What I have to fix to improve my code and OOP design ?

Set?

You have logic preventing duplicate currency pairs. The easier (and more performant) thing to do is to simply represent pair_currencies as a set instead of a list. When you add, it will automatically discard duplicates.

snake_case

The standard is to name methods like your last_pair - i.e., get_rate.

Lookups

You have a loop in getRate and editRate to try and find the correct currency. Instead, you should be using a dictionary, which does not require a loop.

Remove duplicate methods

editRate and setRate do the exact same thing, so delete the former.

Equality test is wrong

Your __eq__ method should be comparing to an object instance, not a string. As such, you should be comparing self.name with object.name.


"import translate" in a way that works with Python 2 and 3 I'm trying to make my script cross-Python (Python 2 and 3 compatible),
and to solve an import problem I did this:

__init__.py file

import sys

if (sys.version_info[0] < 3):
    from core import translate
else:
    from .core import translate

Is it the good way to do it?

No that's not the best way to import in both Python2 and Python3, if you have to support Python 2.5.0a1 and above.
This is as you can use:

from __future__ import absolute_import
from .core import translate

As documented in the __future__ module.


C# Messy Function - Refactoring Just started to clean up an old code base.
I'm interested in your perspective and ways that you would change the following function.

    private void MarkSelectedRow()
    {
        Object obj = Bankkonto1.Value;
        if ((obj != null) && (myBankkonten.Contains(obj.ToString())))
        {
            foreach (UltraGridRow myRow in Bankkonto1.Rows)
            {
                if (myRow.Cells[STR_Id].Text != obj.ToString())
                    continue;

                myRow.Selected = true;
                try
                {
                    hV_OPPanel1.SetBankListe(myBankkonten, Bankkonto1.SelectedRow.Index);
                }
                catch
                {}
                return;
            }
            if (Bankkonto1.Rows.Count > 0)
                Bankkonto1.Rows[0].Selected = true;
        }
        else
            SelectDefaultBank();

        try
        {
            Bankkonto2.Rows[Bankkonto1.SelectedRow.Index].Selected = true;
            BankkontoS.Rows[Bankkonto1.SelectedRow.Index].Selected = true;
            AutoBank.Rows[Bankkonto1.SelectedRow.Index].Selected = true;
            hV_OPPanel1.SetBankListe(myBankkonten, Bankkonto1.SelectedRow.Index);
        }
        catch
        {}
    }

    public void SetBankListe(BankAccountList bankliste, int SelectedIndex)
    {

        if (OPBankkonto.DataSource != bankliste)
            OPBankkonto.SetDataBinding(bankliste, "");

        OPBankkonto.ValueMember = "Id";
        OPBankkonto.DisplayMember = "DisplayText";
        OPBankkonto.DataBind();
        OPBankkonto.Rows[SelectedIndex].Selected = true;
    }

Thanks,
Alex

I would consider inverting the first if to make it simpler and quicker to be aware of the else.

I strongly suggest always leaving something inside a catch - if you have nothing else to put there at least leave a comment.

I would change

if (Bankkonto1.Rows.Count > 0)

to, using System.Linq; :

if (Bankkonto1.Rows.Any())

I would save reused values in scoped variables:

string objValue = obj.ToString();
...
int selectedRow = Bankkonto1.SelectedRow.Index;

I would consider refactoring the foreach

        foreach (UltraGridRow myRow in Bankkonto1.Rows)
        {
            if (myRow.Cells[STR_Id].Text != obj.ToString())
                continue;

into a Linq select:

        var row = Bankkonto1.Rows.FirstOrdefault( r => r.Cells[STR_Id].Text == objValue);
        if(row != null) {


Split mp3 of album into individual tracks Recently, I've been transferring some of my old vinyl records to MP3 files.  One of the things I did to make some progress was to automate things, so I initially set things up so that it would record each album to a single MP3 file.  Now I've gotten to the step where I want to separate those out into individual tracks, so I wrote this program to do that automatically.  It takes an input file in this format:

infile=~/vinyl/UK.mp3
artist=UK
genre=Rock
album=UK

1 - 00:00 - In the Dead Of Night
2 - 05:38 - By the Light Of Day
3 - 10:10 - Presto Vivace and Reprise
4 - 13:18 - Thirty Years
5 - 21:30 - Alaska
6 - 26:04 - Time to Kill
7 - 31:02 - Nevermore
8 - 39:20 - Mental Medication

It then takes the input file in this metadata file and splits it into named tracks adding the album, artist and genre into each MP3 file using ffmpeg.  Here's the script:

songsplit.py

#!/usr/bin/python3
import subprocess
import sys

class Track(object):
    def __init__(self, tracknum, starttime, name):
        self.tracknum = int(tracknum)
        self.starttime = starttime
        self.endtime = starttime
        self.name = name

    def setEnd(self, endtime):
        self.endtime = endtime

    def setargs(self, infile, metadata):
        args = ['ffmpeg', '-i', infile, '-ss', self.starttime]
        if self.endtime != self.starttime:
            args.extend(['-to', self.endtime])
        for key in metadata.keys():
            args.extend(['-metadata', "{}={}".format(key, metadata[key])])
        args.extend(['-metadata', "{}={}".format("track", self.tracknum)])
        args.extend(['-metadata', "{}={}".format("title", self.name)])
        args.append("{:02d} {}.mp3".format(self.tracknum, self.name))
        return args

    def __repr__(self):
        return "{}, {}, {}, {}".format(self.tracknum, self.starttime,
                                       self.endtime, self.name)

def extract(file):
    metadata = {}
    tracks = []
    lines = [line.strip("\n") for line in open(file)]
    for line in lines:
        if line.find('=') > 0:
            m = line.split(sep='=', maxsplit=1)
            if m[0] == 'infile':
                infile = m[1]
            else:
                metadata[m[0]] = m[1]
        else:
            m = line.split(sep=" - ", maxsplit=2)
            if len(m) == 3:
                m = [x.strip() for x in m]
                tk = Track(m[0], m[1], m[2])
                tracknum = int(tk.tracknum)
                if tracknum > 1:
                    tracks[tracknum-2].setEnd(tk.starttime)
                tracks.append(tk)
    for track in tracks:
        subprocess.run(track.setargs(infile, metadata))

if __name__ == "__main__":
    if len(sys.argv) < 2:
        print("Usage: {} metafile".format(sys.argv[0]))
    else:
        extract(sys.argv[1])

If the metadata file shown above is named UK.meta, then this would be invoked as songsplit.py UK.meta and the result would be a directory of MP3 files like this:

01 In the Dead Of Night.mp3
02 By the Light Of Day.mp3
03 Presto Vivace and Reprise.mp3
04 Thirty Years.mp3
05 Alaska.mp3
06 Time to Kill.mp3
07 Nevermore.mp3
08 Mental Medication.mp3

Some particular questions I have are about the use of subprocess.run() and what error handling I should add.  So far, I haven't encountered any problems.

I'll consider answering your last question (at least at the moment) about subprocess.run().

There's a similar method you can use for this kind of situations named subprocess.check_call(). The difference between the two is that call() already gives you the return code —
 not as an attribute, just as the return value of the function. The reason you might want to use check_call() is that it takes care of checking that return code for you, raising a CalledProcessError.

Anyway, either one will raise an OSError for the program not being found.

So, a first solution to your problem might look like this:

try:
    subprocess.check_call(track.setargs(infile, metadata))
except subprocess.CalledProcessError:
    pass  # handle errors in the called executable
except OSError:
    pass  # executable not found

Now, I don't necessary like this solution because sincerely I don't know about those specific exception cases, or check_call() / run() (which apparently they both block). What I learned from my previous projects is that .run() basically merges all the old functions — call(), check_call(), check_output() — into one friendlier function.

You should know that subprocess will raise an exception, OSError, when a command is not found. So, if you catch OSError you can handle the non-existent command, and if you check the result code you can find out whether the command succeeded or not.

As a personal advice, If you have no good way to handle it, let it raise. If you want to continue the execution even if this fails, log it somewhere:

try:
    subprocess.run(track.setargs(infile, metadata))
except OSError as e:
    logger.WARNING("Error: {}".format(e))

Depending on what track.setargs() raises, you can change the Error type.

It's worth mentioning that you might want to use except Exception which will cover any runtime exceptions. With except Exception you can remove the OSError except since Exception will cover those as well. This way of exception handling is kind of hand wavey (and it's best avoided) but will keep your program running.

Other aspects:

you're reading the entire file into memory before processing it.  If the file is very large, that will be slow; and if it's very large, it'll fail. Consider reading the file line by line.
I wouldn't bother writing a setEnd method. Calling it is no simpler than just saying tk.endtime = 3.
you can test if a string has '=' with:   if '=' in line:

PEP8

methods are usually snake_cased so if you'll keep your setEnd(self, endtime) it should be renamed to set_end(self, endtime)

Overall, nice job! You did pretty well! Keep going the good work.


Checking whether a string ends with one of a list of suffixes I want to check if some line ends with any substring defined in some list. Is there a better way than following one?

endsA = ['foo', 'bar', 'fur', 'beer']
endsB = ['door', ... ]

def checkEnds(line, ends):
    for end in ends:
        if line.endswith(end):
            return True

....

if checkEnds(line, endsA):
    do_something()

Yes, there is! Generator comprehensions and the any function to the rescue.

def checkEnds(line, ends):
    return any(line.endswith(end) for end in ends)

Just like the original code, this only checks as many line.endswith(end) as necessary, returning True on the first True result.


Determining whether a number is positive, negative or zero I am a beginner in Python and am currently learning about making beautiful code.  I've started with a small program.  Can you please review my code?

def inputAndRes():
    try:
        a = raw_input("Enter no. :- ")
        n = int(a)
    except Exception:
        print "-----------------------------------"
        print "Please Enter No. " , "( The Number you entered is not a number )"
        inputAndRes()
    else:
        if(n>0):
            print "Number is Positive"
        if(n<0):
            print "Number is Negative"
        if(n==0):
            print "Number is ZERO"
        wantRestart()

def checkYN(checkResult):
    if (checkResult == 'Y'):
            inputAndRes()
    if (checkResult == 'N' ):
            print (" !!! Program Quit !!!")
    else:
        print ("Invalid Option Make sure that you type (Y / N)")
        print ("Continue..")
        wantRestart()

def wantRestart():
    print "\nWant to Restart Program, if Yes then Enter 'Y' and if No enter 'N' :- ",
    try:
        checkResult = raw_input(" ")
    except Exception as e:
        print ("Sorry Error Occured: ",e)
    else:
        checkYN(checkResult)
#...Start of Program...
inputAndRes()

Flow control

The conditions here are mutually exclusive:

if(n>0):
    print "Number is Positive"
if(n<0):
    print "Number is Negative"
if(n==0):
    print "Number is ZERO"

It would be better to rewrite with elif and else:

if n > 0:
    print "Number is Positive"
elif n < 0:
    print "Number is Negative"
else:
    print "Number is ZERO"

Same thing here:

if (checkResult == 'Y'):
        inputAndRes()
if (checkResult == 'N' ):
        print (" !!! Program Quit !!!")
else:
    print ("Invalid Option Make sure that you type (Y / N)")
    print ("Continue..")
    wantRestart()

Should be:

if checkResult == 'Y':
    inputAndRes()
elif checkResult == 'N' :
    print (" !!! Program Quit !!!")
else:
    print ("Invalid Option Make sure that you type (Y / N)")
    print ("Continue..")
    wantRestart()

Finally, instead of raw_input(" "), it would be simpler to write as raw_input().

Writing style

There is an official coding style guide for Python called PEP8. Give it a good read and try to follow it. Your most notable coding style violations:

Spacing around operators: prefer n > 0 instead of n>0

Also, avoid unnecessary parenthesis in if statements, for example if n > 0: is better than if (n > 0):

Function naming should be snake_case instead of camelCase
Put 2 empty lines in front of a function definition

Misc

Instead of running inputAndRestart unconditionally, the common practice is this:

if __name__ == '__main__':
    inputAndRestart()

The advantage of this is that you will be able to import your module without running anything, because the if will only be true when you run the script directly.

Usability

The program wants me to enter Y to restart and N to exit, but I'm kinda lazy to press the shift. It would be great if I could use just y and n. Or even q and Q to exit. An easy way to do that:

if result in 'yY':
    input_and_restart()
elif result in 'nNqQ':
    print (" !!! Program Quit !!!")