How to deal with method which throws 7 exceptions I'm re factoring some code and have come across a method which is throwing 7 exceptions, the method length is approx 20 lines. The method caller wraps this method in a try catch, and just catches the generic Exception. Is there a better way to handle this ? I don't think I should handle each possible exception thrown individually ?

try {
setter();
}
catch(Exception e){
e.printStackTrace();
}

    private void setter() throws MalformedURLException, IOException, ParserConfigurationException, SAXException, TransformerFactoryConfigurationError, TransformerException, XPathExpressionException
    {
        .....
    }

This depends on what you want to do in case of an exception. If your handling is just printing the stack trace your solution is OK. But you might want to react in a different way depending on the problem.

Just an example:

an IO problem could mean that you have to retry (in case on network)
a malformed URL might mean that you have to generate an error message and ask the user to re-enter the data
and so on

If you do not try to recover from the error, I don't know if it makes sense to catch the exception, print the stack trace and then continue. It might be the same as letting the application crash (and print the exception anyway).


How to reduce this archive detection function and make it supports wildcards The function below is called to determine if a given file is the archive of another file. I'm also looking for a way to supports wildcards. For example if the original log file is serverw3c.log and we type serverw3c*.log, it returns true for the following:

serverw3c.log.2011-02-04
serverw3c.log
serverw3c.log.20110204_120132
serverw3c_20110204.log

The code:

/// <summary>
/// Check if a given file is an archive of an original file.
/// The check is performed on the names of the files only.
/// </summary>
/// <param name="originalFile">The original file.</param>
/// <param name="archivedFile">The file which is supposed to be an archive of the original one.</param>
/// <returns>True if the file is an archive, False otherwise.</returns>
public bool IsArchive(string originalFile, string archivedFile)
{
    // We assume that an archived file has the name of the original
    // concatenated with a timestamp '.YYYYMMDD_HHMMSS'
    Regex exp = new Regex(string.Concat(originalFile, ".", "[0-9][0-9][0-9][0-9][0-1][0-9][0-9][0-9]_[0-2][0-9][0-5][0-9][0-5][0-9]"));
    if (exp.IsMatch(archivedFile))
        return true;

    exp = new Regex(string.Concat(Path.GetFileNameWithoutExtension(originalFile), "_", "[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]", Path.GetExtension(".log")));
    if (exp.IsMatch(archivedFile))
        return true;

    exp = new Regex(string.Concat(Path.GetFileNameWithoutExtension(originalFile), "_", "[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]", "_", "[0-9][0-9][0-9]", Path.GetExtension(".log")));
    if (exp.IsMatch(archivedFile))
        return true;

    exp = new Regex(string.Concat(originalFile, ".", "[0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]"));
    if (exp.IsMatch(archivedFile))
        return true;

    return false;
}

Your regex can be shortened using {x}, which repeat a pattern x times. So your third regex would become:

exp = new Regex(string.Concat(Path.GetFileNameWithoutExtension(originalFile),
                              "_[0-9]{8}_[0-9]{3}",
                              Path.GetExtension(".log")));

As far as wildcards, if you use * it will be entered into the regex. It looks from MSDN that it won't be removed from the file name when you call GetFileNameWithoutExtension(). However, I was unable to test this as I don't have Visual Studio on my computer. It worked as a wildcard on my regex tests.


My first build.gradle for a 3D space graphics program I'm writing a 3D space graphics program and I just changed the build system to using gradle. This is my first gradle project. It works but I'm sure there is room for improvement.

apply plugin: 'java'
apply plugin: 'application'
apply plugin: 'eclipse'
apply plugin: 'idea'

mainClassName = 'spaceworld.UFOSpaceWorld'

repositories {
    jcenter()
}

ext.jmeVersion = "[3.1,)"

project(":assets") {
    apply plugin: "java"

    buildDir = rootProject.file("build/assets")

    sourceSets {
        main {
            resources {
                srcDir '.'
            }
        }
    }
}
repositories {
    mavenCentral()
    maven {
        url "https://mvnrepository.com/artifact/cz.advel.jbullet/jbullet"

    }
    maven {
        url 'http://nifty-gui.sourceforge.net/nifty-maven-repo'
    }

}

dependencies {

    compile "org.jmonkeyengine:jme3-core:$jmeVersion"
    compile "org.jmonkeyengine:jme3-desktop:$jmeVersion"
    compile "org.jmonkeyengine:jme3-lwjgl:$jmeVersion"
    compile "org.jmonkeyengine:jme3-blender:$jmeVersion"
    compile "org.jmonkeyengine:jme3-bullet:$jmeVersion"
    compile "org.jmonkeyengine:jme3-plugins:$jmeVersion"
    compile "org.jmonkeyengine:jme3-networking:$jmeVersion"
    compile 'lessvoid:nifty:1.4.1'
    compile 'lessvoid:nifty-default-controls:1.4.1'
    compile 'lessvoid:nifty-style-black:1.4.1'
    compile group: "cz.advel.jbullet", name: "jbullet", version: "20101010"
    compile files('libs/cai-nmgen-0.2.0.jar')
    compile files('libs/ext.bundle.opengl.jmonkey-1.1.3.jar')
    runtime project(':assets')
}

task wrapper(type: Wrapper) {
}

task createDirs << {

    def pkg = 'spaceworld'
    def dirs = [
            file("./src/main/java/$pkg"),
            file("./src/main/resources"),
            file("./assets/Interface"),
            file("./assets/MatDefs"),
            file("./assets/Materials"),
            file("./assets/Effects"),
            file("./assets/Textures"),
            file("./assets/Models"),
            file("./assets/Scenes"),
            file("./assets/Shaders"),
            file("./assets/Sounds"),
            file("./assets/Textures"),
    ]

    dirs.each {
        if (!it.exists()) {
            println "Creating " + it
            it.mkdirs()
        }
        if (it.listFiles().length == 0) {
            def stub = new File(it, 'removeme.txt')
            println "Creating stub file to allow git checkin, file:$stub"
            stub.text = "Remove me when there are files here."
        }
    }
}

The part where you create directories with a single file removeme.txt doesn't belong in a build.gradle file of an Android project. This appears to be a one time setup for your project, specifically for using Git as the version control system. After a single execution followed by a commit, this code is pointless, it doesn't need to be executed again, so I suggest to put it in a separate helper script file.

It's a very minor thing, but to keep "empty"directories in Git, a common name for the place holder file seems to be .gitkeep


Generating and sending new password to user Generate new password and send to user, if user is registered.

Two TextBox on page, txtUserName & txtEamil.
User can provide either UserName or Email to get new password.
If UserName is provided then find User on username, if exists then
generate new password and Email.
If user provided email then check if any user exists with provided
email then generate new password and Email.

protected void btnSubmit_Click(object sender, EventArgs e)
{
    string userName = txtUserName.Text.Trim();
    string email = txtEmail.Text.Trim();

    //
    if (string.IsNullOrEmpty(userName) && string.IsNullOrEmpty(email))
    {
        lblMessage.Text = "Please provide at least either of the one, User Name or Email.";
    }
    else
    {
        SendNewPasswordToUser(userName, email);
    }
}

private void SendNewPasswordToUser(string userName, string email)
{
    if (!string.IsNullOrEmpty(userName))
    {
        MembershipUser mu = Membership.GetUser(userName);
        if (mu != null)
        {
            string password = mu.ResetPassword();
            EmailPassword(password, mu.Email);
        }
    }
    else if (!string.IsNullOrEmpty(email))
    {
        userName = Membership.GetUserNameByEmail(email);
        if (!string.IsNullOrEmpty(userName))
        {
            MembershipUser mu = Membership.GetUser(userName);
            string password = mu.ResetPassword();
            EmailPassword(password, mu.Email);
        }
    }
}

private void EmailPassword(string password, string toEmail)
{
    string mailBody = string.Format("Your new password is {0}", password);
    Mail.SendMail("admin@abc.com", toEmail, "New Password", mailBody);
}

Is there a better approach to avoid so many ifs?

Before reviewing the code, the concept itself has some flaws:

It is possible to reset the password of another user by entering their email address or username.
The password is provided directly in the email which is bad practise (not only is this in clear text but it will be retained in the user's inbox which is not a good place to store passwords).

Entering the email or username should display the same message to the website user regardless of whether the user exists or not. This would stop an attacker from enumerating usernames (i.e. finding whether a particular username or email address is registered on your system). In the email itself include either the details of a password reset link or an account not found message as appropriate for that username/email - this way only the email address owner can see whether they have an account on your system.

The password reset link should be include a time limited reset token that will allow the user to change their password to one of their choosing and it should not then email this out anywhere. Make sure the token is securely generated and does not use a predictable sequence (i.e. a cryptographically secure sequence and not one generated using a Random).


Templated string formatting This function formats a string by replacing every occurrence of '{n}' with the corresponding argument that was passed to the function where n is an index that starts at 0. The same index can be used multiple times and the order is irrelevant. If n is out of bounds or cannot be converted to an integer number I just add '{n}' to the result. The function is intended to be used e.g with config files that can be changed by the user where it would be unsafe to use sprintf. The use of std::ostringstream allows me to pass any type that has an overloaded << operator. I was considering using iterators in the loops but I'm not sure whether that would gain anything.

#pragma once

#include <string>
#include <vector>
#include <sstream>
#include <string_view>

namespace util::str{
    inline void to_string_vector(std::vector<std::string>&){} //Dummy used to end recursion

    template<typename T, typename ... Args>
    void to_string_vector(std::vector<std::string>& result, const T& t, Args&& ...args){
        result.push_back((std::ostringstream{} << t).str());
        to_string_vector(result, args...);
    }

    template<typename ... Args>
    std::string format(std::string_view fmt, Args&& ...args){
        std::string result;
        std::vector<std::string> values;

        result.reserve(fmt.size());
        to_string_vector(values, args...);

        for(std::size_t first = 0; first < fmt.size(); ++first){
            if(fmt[first] == '{'){
                for(std::size_t second = first + 1; second < fmt.size(); ++second){
                    if(fmt[second] == '}'){
                        try{
                            std::size_t index = std::stoi(std::string{fmt.substr(first + 1, second - first - 1)});

                            if(index < values.size())
                                result += values[index];
                            else
                                result += fmt.substr(first, second - first + 1);
                        }catch(...){
                            result += fmt.substr(first, second - first + 1);
                        }

                        first = second;

                        break;
                    }else if(fmt[second] == '{'){
                        result += fmt.substr(first, second - first);
                        first = second - 1;

                        break;
                    }
                }
            }else{
                result.push_back(fmt[first]);
            }
        }

        return result;
    }
}

As I already said in the comments, some white space would be good to add, especially around { and }. Then I mentioned that you could possibly avoid creating the vector of strings, but well, that's not that easy:

template<class... T> struct pack {
    static_assert(sizeof...(T) == 0, "Unmatched argument data");
    static constexpr size_t count = 0;

    template<class Action>
    bool exec(size_t pos, Action&& action = Action()) {
        return false;
    }
};
template<class T> struct pack<T> {
    static constexpr size_t count = 1;
    template<size_t I = 0> using type = std::enable_if_t<I == 0, T>;
    T value;
    pack() = default;
    template<class V>
    pack(V&& value) : value(std::forward<V>(value)) {}

    template<class Action>
    bool exec(size_t pos, Action&& action = Action()) {
        if (pos > 0) return false;
        action(value);
        return true;
    }
};
template<class T, class... Next> struct pack<T, Next...> {
    static constexpr size_t count = 1 + sizeof...(Next);
    template<size_t I = 0> using type = std::conditional_t<I == 0,
        T, typename pack<Next...>::template type<I == 0 ? 0 : I - 1>>;

    T value;
    pack<Next...> next;

    pack() = default;
    template<class First, class... Args>
    pack(First&& value, Args&&... args)
        : value(std::forward<First>(value))
        , next(std::forward<Args>(args)...) {}

    template<class Action>
    bool exec(size_t pos, Action&& action = Action()) {
        if (pos >= count) return false;
        if (pos == 0) action(value);
        else next.exec(pos - 1, std::forward<Action>(action));
        return true;
    }
};
template<class... Args>
inline auto make_pack(Args&&... args) {
    return pack<std::decay_t<Args>...>(std::forward<Args>(args)...);
}
template<class... Args>
std::string format(std::string_view fmt, Args&& ...args) {
    auto pack = make_pack(std::forward<Args>(args)...);
    std::ostringstream result;
    for (std::size_t at = 0; at < fmt.size();) {
        char c = fmt[at++];
        if (c == '{') {
            for (std::size_t start = at; at < fmt.size();) {
                c = fmt[at++];
                if (c == '}') {
                    try {
                        std::size_t index = std::stoi(std::string{ fmt.substr(start, at - 1 - start) });

                        if (index < pack.count)
                        {
                            pack.exec(index, [&result](auto it) { result << it; });
                            break;
                        }
                    }
                    catch (...) {
                    }
                    result << fmt.substr(start - 1, at - start + 1);
                    break;
                }
                if (c == '{') {
                    result << fmt.substr(start - 1, at - start + 1);
                    break;
                }
            }
        }
        else {
            result.put(c);
        }
    }
    return result.str();
}

I have also changed the indexing a bit, not that big of a difference.
The pack is in no way perfect (reduced version of my data pack) and pack.exec(index, [&result](auto it) { result << it; }); is not easy for the compiler to optimize, but the machine code I have observed was not bad (two arguments, release build).


Reading separate components of 8.3 filename into proper filename in C I need to read an archive file format which stores filenames as 8 characters for the name (null-terminated if less than 8 characters, not otherwise), immediately followed by 3 characters for the extension (i.e. without a dot). So basically, I need to read the components and add a dot between the name and extension (name.ext).

Is this the way to go, or is there a better/more common way of doing it?

#include <stdio.h>  // (f)printf, fopen, fseek, ftell, fread, fclose
#include <stdlib.h> // calloc, free

void read_filename(FILE *arc, char *filename)
{
    // read file name
    if(8 != fread(&filename[0], 1, 8, arc))
    {
        fprintf(stderr, "ERROR: Failed reading file name at 0x%08X\n", ftell(arc));
        exit(EXIT_FAILURE);
    }

    // find the "real" end of the name and append a dot
    int c;
    for(c=0; c < 8 && filename[c] != '\0'; c++);
    filename[c] = '.';

    // read extension into the offset of the dot + 1
    if(3 != fread(&filename[c+1], 1, 3, arc))
    {
        fprintf(stderr, "ERROR: Failed reading file extension at 0x%08X\n", ftell(arc));
        exit(EXIT_FAILURE);
    }
}

int main(int argc, char *argv[])
{
    if (argc != 2) return 1;

    // open the archive
    FILE *arc = fopen(argv[1], "rb");
    if(arc==NULL)
    {
        fprintf(stderr, "ERROR: Could not open file.\n");
        exit(EXIT_FAILURE);
    }

    // seek to 0x28, which is the offset of the first filename
    fseek(arc, 0x28, SEEK_SET);

    // allocate cleared memory for the file name
    char *filename = (char *)calloc(12, 1);

    // read the file name into the memory block allocated
    read_filename(arc, filename);

    printf("First filename: %s...", filename);

    free(filename);
    fclose(arc);

    return 0;
}

An example of a filename might look like this (hex dump):

0x00000028   46 49 4C 45 4E 41 4D 00 65 78 74 00   FILENAM ext

the space in the example above is a null byte.

or it can look like this:

0x00000028   46 49 4C 45 4E 41 4D 45 65 78 74 00   FILENAMEext

in this example there is no null terminator for the name -- as mentioned before, the extension always immediately follows the first 8 bytes.

Here are some things that may help you improve your code.

Avoid "tricky" loops

There is nothing technically wrong with this line:

for(c=0; c < 8 && filename[c] != '\0'; c++);

However, because one doesn't usually write a for loop with no body, it would probably be useful for your code to call attention to the fact that there's nothing happening within the loop.  You could write it like this:

for(c=0; c < 8 && filename[c] != '\0'; c++) {
    // do nothing
}

Or another way:

for(c=0; c < 8 && filename[c] != '\0'; c++)
    continue;

All will likely compile to the exact same object code.

Don't mix I/O with data operations

Generally speaking having a printf within a data manipulation function such as read_filename is not a good idea.  It makes it harder to reuse the function.  It's OK for diagnostics and for troubleshooting, but it would be better to omit it from the final version.  Similarly, it would be better if the fread would read the entire 8.3 filename into a memory buffer and then pass the memory buffer to code that would do the parsing.  That cleanly separates I/O from data manipulation and is probably more efficient as well.

Return an error code

The read_filename function can detect errors, but summarily exits the entire program rather than returning an error code that would allow the calling function to decide how to handle the error. A more robust design that might allow the code to be reused would return an error code.

Eliminate "magic numbers"

Things like if (argc != 2) are OK, because programmers reading this will know what that code is doing.  However, when you write if(8 != fread(&filename[0], 1, 8, arc)) it's not at all obvious whether 8 is the correct value, or exactly what it represents.  Better would be to use a named const value there such as const size_t FILENAMESIZE = 8;

Be careful with assumptions about argument size

In the fprintf functions, the parameter should be 0x%08lX rather than 0x%08X because ftell() returns a long.

Try this

Here's one way to do this, incorporating all of these suggestions:

// max size of 8.3 filename
const size_t FILENAMELEN = 11u;
const size_t EXTOFFSET = 8u;
const size_t EXTLEN = 3u;

char *read_filename(FILE *arc)
{
    char *filename = malloc(FILENAMELEN+2);
    if (filename == NULL) {
        return filename;
    }
    filename[FILENAMELEN] = '\0';
    if (FILENAMELEN != fread(filename, 1u, FILENAMELEN, arc)) {
        free(filename);
        return NULL;
    }
    return filename;
}

char *parse_filename(char *filename)
{
    if (filename == NULL) {
        return filename;
    }
    // insert the '.'
    memmove(&filename[EXTOFFSET+1], &filename[EXTOFFSET], EXTLEN);
    filename[EXTOFFSET] = '.';
    size_t i;
    for (i=0; filename[i] && i < EXTOFFSET; ++i) {
    }
    if (i < EXTOFFSET) {
        memmove(&filename[i], &filename[EXTOFFSET], EXTLEN+2);
    }
    return filename;
}


Invert n bits beginning at position p of an unsigned integer In an attempt to create a function that returns x with the n bits that begin at position p inverted, leaving the others unchanged - following the exercise in K&R - I've come up with the following piece of code.
Still, I find it unclear where to start counting, i.e. the result would be different if starting from the right. I am starting from the left - bit one is the first bit from the left.
unsigned invert(unsigned x, unsigned p, unsigned n)
{
  return x ^ ((~0U >> (p - 1 + (sizeof(int) * CHAR_BIT - p + 1 - n))) << (sizeof(int) * CHAR_BIT - p + 1 - n));
}

Which I'd then simplify to:
unsigned invert(unsigned x, unsigned p, unsigned n)
{
  return x ^ ((~0U >> (sizeof(int) * CHAR_BIT - n)) << (sizeof(int) * CHAR_BIT - p + 1 - n));
}

I'm not performing any boundary checks.
Is this good practice? Is there a cleaner solution?

When you find it "unclear where to start counting" you have encountered one of the greatest problems in software development: extracting useful specifications from your stakeholders!  In these situations, when we can't just ask, then we need to choose an interpretation that seems most useful and reasonable, and (this is the important bit), be very clear that we had to make a choice, and what we chose.
In this case, I would probably make a somewhat more specific name, and also add an explanatory comment:
/*
 * Returns x with n bits inverted starting at bit p
 * (counting from the left)
 * E.g. invert_bits(0x0000, 4, 2) == 0x0c00 for 16-bit unsigned
 */
unsigned invert_bits(unsigned x, unsigned p, unsigned n)

That example in the comment would normally become one of the tests I use to verify the function.
I find it odd that we're working with unsigned yet we use sizeof (int) to calculate the width.  Although we know that int and unsigned int have the same size, it's clearer to be consistent.  Actually, I would just use sizeof x, which then makes it easier to re-use the code - e.g. if we write a version for unsigned long.
I think the arithmetic could be simplified if we consider that a mask with the leftmost p bits unset is ~0u >> p.  Using invert_bits(0x0000, 4, 2) again, as in the comment, we can visualise what we're doing:
      0000111111111111   mask_p = ~0u >> p;
      0000001111111111   mask_n = mask_p >> n;
      0000110000000000   mask_p ^ mask_n

As a completed function, we can reuse a single mask variable for this:
unsigned invert_bits(unsigned x, unsigned p, unsigned n)
{
    unsigned mask = ~0u >> p;
    mask ^= mask >> n;
    return x ^ mask;
}

The nice thing about this is that it adapts automatically to the type's size, without needing any sizeof at all in the computation.

In case I've provided too much spoiler here, I provide some follow-up exercises:

Write the same function, but with the assumption that you count bits from the rightmost (least significant) bit.
Write a version for unsigned long.
Write versions that unconditionally set and reset the specified group of bits.
Write a main() that tests whether the above functions work as advertise.  Remember to return EXIT_SUCCESS or EXIT_FAILURE as appropriate.

Which code can be shared in the answers to these questions?


Euler Project problem #3 Problem: What is the largest prime factor of the number 600851475143?

Hi, I'm really new with Python. Can anyone give me some feedback for this code? it seem to produce the correct answer

a=600851475143
x=2
while x<(a/x):
    if a%x==0: a=a/x
    else: x+=1

print a

It produces the right answer in this particular case, but it wouldn't produce the right answer for all inputs. Consider a=4. The first time you hit the while guard, x is 2 and a/x is 2, so it skips the loop and prints 4.

Is there any reason for using an op= construct for x+=1 but not for a=a/x?

I'm not a Pythonista, but I'm pretty sure it's preferred style to put the if and else blocks on a new line even if they're only one line long:

    if a%x==0:
        a/=x
    else:
        x+=1


Calling a Class Method on a Module I have a module (in file dialect.rb) defined as such:

require 'dialect/generators/elements'

module Dialect
  def self.included(caller)
    caller.extend Dialect::Generator::Element
  end

  def self.version
    "Dialect v#{Dialect::VERSION}"
  end
end

Then I have the file dialect/generators/elements.rb, which looks like this:

module Dialect
  module Generator
    module Element

      puts Dialect.version

    end
  end
end

If I run my app, I get:

/lib/dialect/generators/elements.rb:7:in `<module:Element>':
undefined method `version' for Dialect:Module (NoMethodError)

My question/problem is: I didn't understand why the Element module could not find the version method here.

How I call Dialect is like this:

require 'dialect'

class PageTest
  include Dialect
end

So you can see Dialect is mixed-in to an existing class. It's when this class is instantiated that I get the error above.

When I try a simple IRB session doing what appears to be this same logic, this all seems to work:

irb(main):001:0> module Dialect
irb(main):002:1>   def self.version
irb(main):003:2>     puts "Version number"
irb(main):004:2>   end
irb(main):005:1> end
=> nil
irb(main):006:0> module Dialect
irb(main):007:1>   module Generator
irb(main):008:2>     module Element
irb(main):009:3>       puts Dialect.version
irb(main):010:3>     end
irb(main):011:2>   end
irb(main):012:1> end
Version number

Here I get the "Version number" text back, which tells me (I think?) that Dialect::Generator::Element can call the method version on Dialect.

The issue ended up being corrected by simply moving my require statement to the end, like this:

module Dialect
  def self.included(caller)
    caller.extend Dialect::Generator::Element
  end

  def self.version
    "Dialect v#{Dialect::VERSION}"
  end
end

require 'dialect/generators/elements'

Having the require statement at the end solves the problem I was having.

The question then becomes: is this a good way to do this? I feel like making my logic depend on where the require statement goes seems like a bad idea.

The require line in the first file means that ruby runs the second file before the first file, which is before the version method is declared...

Doing it the other way around will work, since only when  included is called is the module Element is required.

dialect/dialect.rb:

module Dialect
  def self.included(caller)
    caller.extend Dialect::Generator::Element
  end

  def self.version
    "Dialect v#{Dialect::VERSION}"
  end
end

dialect/generators/elements.rb:

require 'dialect/dialect'

module Dialect
  module Generator
    module Element

      puts Dialect.version

    end
  end
end

Alternatively, you could also write a file declaring only the version:

dialect/version.rb:

module Dialect
  def self.version
    "Dialect v#{Dialect::VERSION}"
  end
end

Edit

To answer your question - this is not a good way to do this. The problem you encountered with the require hints to circular dependency - Dialect calls Element, which uses Dialect on its creation.

The correct resolution is to break this dependency by using my version.rb suggestion above, so the code will look like this:

dialect/version.rb:

module Dialect
  def self.version
    "Dialect v#{Dialect::VERSION}"
  end
end

dialect/generators/elements.rb:

require 'dialect/version'

module Dialect
  module Generator
    module Element

      puts Dialect.version

    end
  end
end

dialect/dialect.rb:

require 'dialect/version'
require 'dialect/generators/elements'

module Dialect
  def self.included(caller)
    caller.extend Dialect::Generator::Element
  end
end


N rotations of an array, either left or right, and then printing the result Disclaimer, I'm new to OOP, but have experience in haskell and javascript.

Essentially, the challenge was to create a program which would shift an array a given amount of times to the left and then print the result.

I've tried to make this more extendable by allowing the option to shift to the right.

My code is

#include <iostream>
#include <vector>

using namespace std;

vector<int> rotate(vector<int> myVector, int direction, int count);
vector<int> rotate(vector<int> myVector, int direction);
void print(vector<int> rotatedVector);

void print(vector<int> rotatedVector){
    for(int i = 0; i < rotatedVector.size(); i++){
        cout << rotatedVector[i] << endl;
    }
}

vector<int> rotate(vector<int> myVector, int direction, int count){

    while(count > 0){
        myVector = rotate( myVector,  direction); // should I create a new variable, or just overwrite myVector each time?
        --count;
    }

    return myVector;
}

vector<int> rotate(vector<int> myVector, int direction){

    const int length = myVector.size();

    vector<int> rotatedVector(length);

    for(int i = 0; i < myVector.size(); i++){
        rotatedVector.at(i) = myVector.at((length+i-direction)%length);
    }

    return rotatedVector;
}

int main(){

    // Array to rotate
    int arr[5] = {1,2,3,4,5};

    // Times to rotate
    const int count = 2;

    // Creating a vector object out of array
    // Should I have worked with the array given?
    int n = sizeof(arr) / sizeof(arr[0]);
    vector<int> myVector(arr, arr + n);

    enum direction  {left = -1, right = 1}; // should this be global?

    myVector = rotate(myVector, left, count);

    print(myVector);

    // Could potentially turn vector back into array?

    return 0 ;
}

My design questions are...

Is this extendable enough, how could it be made more so?
Should the enums be made global?
Should I have used multiple
functions instead of using enums to make the rotation functions
generic? (e.g a right rotation function, and a left one)
Should I have kept with the array, instead of creating a vector from it?
Was overloading the function necessary?

The most important thing I would like to clarify from this review, is the concept of "state" in these types of programming puzzles (done in C++/OOP)

The line which overwrites the vector is...

myVector = rotate( myVector,  direction);

Is it better form to perhaps implement something like...

myVector.rotate(direction);

and thus action on the same object each time? (and if so why)

When would I use one over the other?

In my head, I feel as though it would be best to make the vector immutable and keep creating a new one each time, but am unsure and slightly confused about which one to use.

There are two basic ways to solve this type of problem.  One is having to do the rotation in place, where you can't allocate additional storage for a copy of the array.  This uses less memory but takes longer, since the rotations have to go by small increments (usually 1, since that is easiest to implement).  The other way is to do the rotation into a copy.  This has the advantage of being faster but can take additional memory.  Your implementation is a hybrid of both.

Don't write using namespace std;.

You're passing all your vectors by value, which will result in copies being made.  Since you're making a new vector to store the rotated vector in, you can pass them by const &.  For example,

std::vector<int> rotate(const std::vector<int> &myVector, int direction);

print can take advantage of range based for loops:

void print(const vector<int> &rotatedVector) {
    for (int v: rotatedVector){
        std::cout << v << '\n';
    }
}

This also avoids using endl, since that will do a flush which can greatly slow the rate output is displayed.

Your two parameter rotate function can rotate by more than one element at a time.  You can combine the two rotate functions into one, with an offset of -direction * count instead of -direction.  (With appropriate code to precalculate that value and a check so that the result is not larger than -length to avoid having a negative number on the left of %.)


A consumable byte array, in Swift For decoding binary data (in my case, delivered by a Bluetooth device), I've written this struct:

public struct ConsumableByteArray {
    private let bytes: [UInt8]
    private var idx = 0

    enum Error: Swift.Error {
        case notEnoughBytes
    }

    init(data: Data) {
        bytes = [UInt8](data)
    }

    init(bytes: [UInt8]) {
        self.bytes = bytes
    }

    mutating func consume() throws -> UInt8 {
        guard idx < bytes.count else {
            throw Error.notEnoughBytes
        }
        defer { idx += 1}
        return bytes[idx]
    }

    mutating func consume() throws -> UInt16 {
        guard idx+1 < bytes.count else {
            throw Error.notEnoughBytes
        }
        defer { idx += 2 }
        return UInt16(bytes[idx+1]) << 8 + UInt16(bytes[idx])
    }

    mutating func consume() throws -> Int16 {
        guard idx+1 < bytes.count else {
            throw Error.notEnoughBytes
        }
        defer { idx += 2 }
        return Int16(bytes[idx+1]) << 8 + Int16(bytes[idx])
    }

    mutating func consume() throws -> UInt32 {
        guard idx+3 < bytes.count else {
            throw Error.notEnoughBytes
        }
        defer { idx += 4 }
        // Swift compiler insists on splitting this expression up
        let b3 = UInt32(bytes[idx+3]) << 24
        let b2 = UInt32(bytes[idx+2]) << 16
        let b1 = UInt32(bytes[idx+1]) << 8
        let b0 = UInt32(bytes[idx+0]) << 0

        return b3 + b2 + b1 + b0
    }
}

Given some data buffer, likely containing ints of varying widths packed together, it allows those fields to be read out:

let buffer = ConsumableByteArray(data: someData)
let header:UInt8 = try buffer.consume()
let word1:UInt16 = try buffer.consume()
let word2:UInt16 = try buffer.consume()
let crc32:UInt32 = try buffer.consume()

Values in the early data may alter the structure of the later data (e.g. whether a feature is supported or not), hence the need for the flexibility to extract data progressively.

Would you write the implementation any differently, or change the API?

Instead of implementing consume() for each integer type separately,
you can implement a single generic method:

mutating func consume<T: FixedWidthInteger & UnsignedInteger>() throws -> T {
    let size = MemoryLayout<T>.size
    guard idx + size <= bytes.count else {
        throw Error.notEnoughBytes
    }
    defer { idx += size }
    return bytes[idx..<idx + size].enumerated().reduce(0) {
        $0 + T($1.element) << (8 * $1.offset)
    }
}

which can be used for all unsigned integer types UInt, UInt8, ..., UInt64, e.g.

let header: UInt8 = try buffer.consume()

and for the signed integer types via the bitPattern: initializer, e.g.

let word = try! Int16(bitPattern: buffer.consume())

Another option is to copy the bytes into a value of the desired type
instead of bit shifting and adding:

mutating func consume<T: FixedWidthInteger>() throws -> T {
    let size = MemoryLayout<T>.size
    guard idx + size <= bytes.count else {
        throw Error.notEnoughBytes
    }
    var value: T = 0
    bytes.withUnsafeBytes { _ = memcpy(&value, $0.baseAddress! + idx, size) }
    idx += size
    return T(littleEndian: value)
}

Instead of letting the compiler infer the return type from the context,
one can alternatively pass it as a parameter:

mutating func consume<T: FixedWidthInteger>(_: T.Type) throws -> T { ... }

which is then – for example – called as

let crc32 = try buffer.consume(UInt32.self)

I would probably call the method get() or read() instead of consume().

Defining a local enum Error type which conforms to the (global)
Error protocol is possible, but might be confusing to the reader.
I would use a different name for the concrete error type, for example:

enum ReadError: Error {
    case notEnoughBytes
}

Now let's have a look how an error would be reported.
The caller does not know the actual error type, so a typical calling
sequence is:

do {
    let someData = Data(bytes: [1])
    var buffer = ConsumableByteArray(data: someData)
    let crc32: UInt32 = try buffer.consume()
    print(crc32)
} catch {
    print(error.localizedDescription)
}

This produces the output:

The operation couldn’t be completed. (MyProg.ConsumableByteArray.ReadError error 0.)

This can be improved by adopting the LocalizedError protocol
(see for example How to provide a localized description with an Error type in Swift? on
Stack Overflow):

enum ReadError: Error, LocalizedError {
    case notEnoughBytes

    public var errorDescription: String? {
        switch self {
        case .notEnoughBytes:
            return "Not enough bytes in buffer"
        }
    }
}

Now the error output of the above program becomes

Not enough bytes in buffer

You can even store additional information about the error in associated
values:

enum ReadError: Error, LocalizedError {
    case notEnoughBytes(available: Int, needed: Int)

    public var errorDescription: String? {
        switch self {
        case .notEnoughBytes(let available, let needed):
            return "Not enough bytes in buffer (available: \(available), needed: \(needed))"
        }
    }
}

Then by throwing

 throw ReadError.notEnoughBytes(available: bytes.count - idx, needed: size)

an error message like

Not enough bytes in buffer (available: 1, needed: 4)

is produced.

Finally note that there is a ByteBuffer type as part of the
SwiftNIO framework,
which can do all this and more. Even if you decide not to use it,
having a look at its documentation and interface might be instructive.


Searching a list of an int arrays into another list of int arrays We are given two list on integer arrays, and the objective is to retrieve another list which contains the duplicates in both lists.

The code developed is the following:

public List<int[]> GetDuplicates(List<int[]> pInputList1, List<int[]> pInputList2)
{
    var outputList = new List<int[]>();

    pInputList2.ForEach(x =>
    {
        outputList.AddRange(pInputList1.Select(j => j).Where(y =>
        {
            for (int i = 0; i < y.Length; i++)
                if (y[i] != x[i])
                    return false;
            return true;
        }));
    });

    return outputList;
}

The code works as intended what I am concerned about is optimization, how to improve this code in both, execution time and stability.

What I am doing in this code is compare each element from one list with ALL of the elements of the other list (I see a problem here, but don't know how to approach it well, the foreach seems not to be the best option here).

The original code:

public List<int[]> GetDuplicates(List<int[]> pInputList1, List<int[]> pInputList2)
{
    var outputList = new List<int[]>();

    pInputList2.ForEach(x =>
    {
        outputList.AddRange(pInputList1.Select(j => j).Where(y =>
        {
            for (int i = 0; i < y.Length; i++)
                if (y[i] != x[i])
                    return false;
            return true;
        }));
    });

    return outputList;
}

I'd start with a few readability simplifications first:

Select(j => j) is a no-op and can be removed
The usage of ForEach is hurting readability (and performance, due to passing delegates around) a little bit
Moving the pInputList1.Where(...) outside of the AddRange also makes it clearer what's going on.

public List<int[]> GetDuplicates(List<int[]> pInputList1, List<int[]> pInputList2)
{
    var outputList = new List<int[]>();

    foreach (var x in pInputList2)
    {
        var duplicates = pInputList1.Where(y =>
        {
            for (int i = 0; i < y.Length; i++)
                if (y[i] != x[i])
                    return false;
            return true;
        });

        outputList.AddRange(duplicates);
    }

    return outputList;
}

Next, as mentioned in the comments, the intention is to filter the second list according to whether an identical array appears in the first list. The method and its arguments (and derived local variables) should be renamed to reflect that. I'd also swap the parameters over, because pInputList2 is the important part of the method, while pInputList1 is the set against which pInputList2 is checked.

public List<int[]> FilterByWhitelist(List<int[]> testArrays, List<int[]> whiteListedArrays)
{
    var outputList = new List<int[]>();

    foreach (var testArray in testArrays)
    {
        var duplicates = whiteListedArrays.Where(whiteListedArray =>
        {
            for (int i = 0; i < whiteListedArray.Length; i++)
                if (whiteListedArray[i] != testArray[i])
                    return false;
            return true;
        });

        outputList.AddRange(duplicates);
    }

    return outputList;
}

Using the same point in the comments: we don't need to find every matching array in whiteListedArrays - we only need to know whether there is one. As such, we can use Any instead of Where:

public List<int[]> FilterByWhitelist(List<int[]> testArrays, List<int[]> whiteListedArrays)
{
    var outputList = new List<int[]>();

    foreach (var testArray in testArrays)
    {
        var isWhiteListed = whiteListedArrays.Any(whiteListedArray =>
        {
            for (int i = 0; i < whiteListedArray.Length; i++)
                if (whiteListedArray[i] != testArray[i])
                    return false;
            return true;
        });

        if (isWhiteListed)
        {
            outputList.Add(testArray);
        }
    }

    return outputList;
}

Another bug in this implementation is: what if one of your testArrays is longer than one of the whiteListedArrays? You're accessing x[i] while only ensuring that i is less than the length of y. You'll probably get an ArgumentOutOfRangeException from this. Because two arrays can't be equal if their lengths don't match, this should be checked first.

On the other hand, Linq has a nice built-in method for checking whether two sequences are equal:

public List<int[]> FilterByWhitelist(List<int[]> testArrays, List<int[]> whiteListedArrays)
{
    var outputList = new List<int[]>();

    foreach (var testArray in testArrays)
    {
        var isWhiteListed = whiteListedArrays.Any(
            whiteListedArray => testArray.SequenceEqual(whiteListedArray));

        if (isWhiteListed)
        {
            outputList.Add(testArray);
        }
    }

    return outputList;
}

This method now iterates once over its main argument, and returns a subsequence of that argument. This sounds like an ideal scenario for an extension method on IEnumerable.

public static IEnumerable<int[]> FilterByWhitelist(this IEnumerable<int[]> testArrays, List<int[]> whiteListedArrays)
{
    foreach (var testArray in testArrays)
    {
        var isWhiteListed = whiteListedArrays.Any(
            whiteListedArray => testArray.SequenceEqual(whiteListedArray));

        if (isWhiteListed)
        {
            yield return testArray;
        }
    }
}

This would be called like:

var originalList = new List<int[]> { ... };
var whiteList = new List<int[]> { ... };

var filteredList = originalList.FilterByWhitelist(whiteList).ToList();

One final performance improvement I can see is pre-grouping the whiteListedArrays by length, and only testing for sequence equality where the lengths are already known to be equal:

public static IEnumerable<int[]> FilterByWhitelist(this IEnumerable<int[]> testArrays, List<int[]> whiteListedArrays)
{
    var lengthGroupedWhiteListedArrays = whiteListedArrays
        .GroupBy(arr => arr.Length)
        .ToDictionary(group => group.Key, group => group.ToList());

    foreach (var testArray in testArrays)
    {
        List<int[]> lengthMatchedWhiteListedArrays;

        if (!lengthGroupedWhiteListedArrays.TryGetValue(testArray.Length, out lengthMatchedWhiteListedArrays))
        {
            continue;
        }

        var isWhiteListed = lengthMatchedWhiteListedArrays.Any(
            whiteListedArray => testArray.SequenceEqual(whiteListedArray));

        if (isWhiteListed)
        {
            yield return testArray;
        }
    }
}

At this point, we're only iterating over whiteListedArrays once, so that too can become an IEnumerable:

public static IEnumerable<int[]> FilterByWhitelist(this IEnumerable<int[]> testArrays, IEnumerable<int[]> whiteListedArrays)
{
    var lengthGroupedWhiteListedArrays = whiteListedArrays
        .GroupBy(arr => arr.Length)
        .ToDictionary(group => group.Key, group => group.ToList());

    foreach (var testArray in testArrays)
    {
        List<int[]> lengthMatchedWhiteListedArrays;

        if (!lengthGroupedWhiteListedArrays.TryGetValue(testArray.Length, out lengthMatchedWhiteListedArrays))
        {
            continue;
        }

        var isWhiteListed = lengthMatchedWhiteListedArrays.Any(
            whiteListedArray => testArray.SequenceEqual(whiteListedArray));

        if (isWhiteListed)
        {
            yield return testArray;
        }
    }
}

We could, at this point, extract the array-specific code into a class that encapsulates the whitelist:

public class Whitelist
{
    private readonly IReadOnlyDictionary<int, List<int[]>> _lengthGroupedWhiteListedArrays;

    public Whitelist(IEnumerable<int[]> whitelistedArrays)
    {
        _lengthGroupedWhiteListedArrays= whitelistedArrays
            .GroupBy(arr => arr.Length)
            .ToDictionary(group => group.Key, group => group.ToList());
    }

    public bool Contains(int[] item)
    {
        List<int[]> correctLengthFilterArrays;

        if (!_lengthGroupedWhiteListedArrays.TryGetValue(item.Length, out correctLengthFilterArrays))
        {
            return false;
        }

        return correctLengthFilterArrays.Any(filterArray => item.SequenceEqual(filterArray));
    }
}

...

public static IEnumerable<int[]> FilterByWhitelist(this IEnumerable<int[]> testArrays, Whitelist whitelist)
{
    foreach (var testArray in testArrays)
    {
        if (whitelist.Contains(testArray))
        {
            yield return testArray;
        }
    }
}

Although by doing this, FilterByWhitelist has more or less become Linq's Where:

var originalList = new List<int[]> { ... };
var whiteList = new Whitelist(new List<int[]> { ... });

var filteredList = originalList
    .Where(arr => whiteList.Contains(arr))
    .ToList();

This way, the pre-grouping of array lengths can be used in many places.


Converting enum values to strings in C++ Question 1: functionxxx_as_string() is used below.  How else could it be more elegantly named?

Question 2: Is the static char* array method adopted below the only solution?  Best solution?  Any suggestions?

Generally, I have this issue where my list of enums will be from 3 - say 50 items, mostly less than 20 items and they are fairly static.

   #include <iostream>

   enum thing_type
   {
      DTypeAnimal,
      DTypeMineral,
      DTypeVegetable,
      DTypeUnknown
   };

   class thing {
   public:
      thing(thing_type type) : type_(type) {}
      const thing_type get_state() const { return type_; }
      const char* get_state_as_string() const {
         static const char* ttype[] = {
               "Animal",
               "Mineral",
               "Vegetable",
               "Unknown"
         };
         return ttype[type_];
      }

   private:
      thing_type type_;
   };

   int main() {
      thing th(DTypeMineral);
      std::cout << "this thing is a " << th.get_state_as_string() << std::endl;

      return 0;
   }

I am preferring to remove all the printing stuff from the class interface and use the operator<< overloading idea in 200_success answer like this:

#include <iostream>

enum thing_type
{
   DTypeUnknown,
   DTypeAnimal,
   DTypeMineral,
   DTypeVegetable
};

const char* type2string(thing_type ttype) {
   static const char* thtype[] = {
      "Unknown",
      "Animal",
      "Mineral",
      "Vegetable"
   };
   return ttype < sizeof(thtype) ? thtype[ttype] : thtype[0];
}

std::ostream& operator<<(std::ostream& os, const thing_type type) {
   os << type2string(type);
   return os;
}

class thing {
public:
   thing(thing_type type) : type_(type) {}
   const thing_type get_type() const { return type_; }

private:
   thing_type type_;
};

std::ostream& operator<<(std::ostream& os, const thing& th) {
   os << "This is a " << type2string(th.get_type());
   return os;
}

int main() {
   thing th(DTypeMineral);
   std::cout << th << std::endl;

   return 0;
}

Question 1: functionxxx_as_string() is used below. How else could it be more elegantly named?

How about:

std::ostream& operator<<(std::ostream& str, thing const& data);

  Question 2: Is the static char* array method adopted below the only solution? Best solution? Any suggestions?

You will need a static char array (or equivalent (like a switch)) somewhere as there is no built in way to convert enum values (which are integers) to a string.


Auto change page title while bookmarking a page Here is what I'm trying:

Please review the code:

function quicklyChangePageTitle() {
  var currentTitle = document.title; // remember original title
  document.title = "temp title"; // change to the temporary title

  setTimeout(function() { // revert back to original title
    document.title = currentTitle;
  }, 1);

}
document.addEventListener("keydown", function(event) {
  if (event.ctrlKey && event.keyCode == 68) { // Ctrl + D
    quicklyChangePageTitle();
  }
});

That seems like a very fragile thing to do.  If it works at all, you would still only succeed on browsers where the keyboard shortcut is ControlD.  It probably won't work on a Mac or on a mobile device.  It will probably break if the user interface is in a non-English locale with different keyboard shortcuts.  It would also have no effect if the user initiates the bookmark creation using the menu rather than a keyboard shortcut.

You should probably step back and consider why such a hack is necessary in the first place.  Ideally, you should design the page titles such that they already make reasonable bookmark titles.


Optimizing VBA Select Code I'm trying to reword the code below to avoid using the Select and Selection terms.  I tried to Range("S"...).Copy instead of .Select but that didn't work for some reason.  Does anyone know a way I can reword this to avoid Select and Selection statements and why my change .Select to .Copy wouldn't have worked?

The code below looks in Column A for the identifiers Tango and Alpha.  Then it uses those identifiers to figure out the range between rows to select in Column S and copies and pastes them as values.  I can confirm the code does run as is.

Sub copy_paste()
        Dim wb As Workbook
        Dim ws As Worksheet
        Dim FoundCell1 As Range
        Dim FoundCell2 As Range
        Set wb = ActiveWorkbook
        Set ws = ActiveSheet

        Const WHAT_TO_FIND1 As String = "Tango"
        Set FoundCell1 = ws.Range("A:A").Find(What:=WHAT_TO_FIND1)

        Const WHAT_TO_FIND2 As String = "Alpha"
        Set FoundCell2 = ws.Range("A:A").Find(What:=WHAT_TO_FIND2)

        Range("S" & FoundCell1.Row + 1 & ":S" & FoundCell2.Row - 1).Select
        Selection.Copy
        Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks _
         :=False, Transpose:=False

End sub

To start with:  You are currently not using the wb Workbook object after setting it.  Is it needed in other code, or can it be removed?

If you are just trying to 'flatten' the range to get rid of any formula, you can just use <Range>.Value = <Range>.Value, instead of Copy / PasteSpecial

Range("S" & FoundCell1.Row + 1 & ":S" & FoundCell2.Row - 1).Select
'Selection.Copy
'Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks _
     :=False, Transpose:=False
Selection.Value = Selection.Value

You can also use a With Statment to replace the requirement for a Select or Selection.

'Range("S" & FoundCell1.Row + 1 & ":S" & FoundCell2.Row - 1).Select
With w.Range("S" & FoundCell1.Row + 1 & ":S" & FoundCell2.Row - 1)
    .Value = .Value
End With
'Selection.Copy
'Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks _
     :=False, Transpose:=False

And, this is personal preference, but you could use .Offset, .EntireRow and .Column to define the range instead of building a text string address:

With ws.Range(FoundCell1.Offset(1, 0), FoundCell2.Offset(-1, 0)).EntireRow.Columns("S")
    .Value = .Value
End With

(This only requires "Column S" to be defined once, in case you need to change it later - the other option would be to have it in a Const at the start of the Sub)

As yet-another-alternative, you could use WorksheetFunction.Match to retrieve the Rows directly (TangoRow = WorksheetFunction.Match(WHAT_TO_FIND1, ws.Columns("A"), 0) would give the same result as FoundCell1.Row)


Program based on a physics concept I am a beginner and I wanted to make programs that would help me while I learnt Java. I made a program to help me in my assignments. Any suggetions?

Main method

public class MirrorFormula {

public static void main(String[] args) {
        Scanner keyboardInput = new Scanner(System.in);
        double u =0;
        double v =0;
        double f =0;

        System.out.println("Hello! This is a calculator based on the Mirror Formula. \n");
        System.out.println("The mirror formula is a relation between the object distance, the image distance \n and the focal length.");
        System.out.println("The formula is : 1/u + 1/v = 1/f.  (u=distance of object, v=distance of image, f=focal length.)");
        System.out.println("It is applicable for both convex mirrors and concave mirrors.");
        System.out.println();
        System.out.println("This calculator can calculate :");
        System.out.println();
        System.out.println("1. Distance of the object from the pole of the spherical mirror.");
        System.out.println("2. Distance of the image from the pole of the spherical mirror.");
        System.out.println("3. Focal length");
        System.out.println("[If you have 2 of the values.]");
        System.out.println("________________________________________________________________________________________________________________________________");
        System.out.println("What value would you like to find out? ");
        System.out.println("Enter: \n 1. for Distance of object \n 2. for distance of image \n 3. for Focal length \n (Please note that you don't need to enter units. The ans. is in whatever unit you enter the values)" );

        int userChoice = keyboardInput.nextInt();

        System.out.println("________________________________________________________________________________________________________________________________");

        switch (userChoice) {

        case 1 :
            System.out.println("Please enter the distance of the image: ");
            v = keyboardInput.nextDouble();
            System.out.println("Please enter the focal length: ");
            f = keyboardInput.nextDouble();
            u = findU(v, f);
            break;

        case 2 :
            System.out.println("Please enter the distance of the object: ");
            u = keyboardInput.nextDouble();
            System.out.println("Please enter the focal length: ");
            f = keyboardInput.nextDouble();
            v = findV(u, f);
            break;

        case 3 :
            System.out.println("Please enter the distance of the object: ");
            u = keyboardInput.nextDouble();
            System.out.println("Please enter the distance of the image: ");
            v = keyboardInput.nextDouble();
            f = findF(u, v);
            break;

        }
    System.out.println("___________________________________________________");
    System.out.println(" Distance of the object from the pole is  | " + u + "  |");
    System.out.println(" Distance of the image from the pole is   | " + v + "  |");
    System.out.println(" The focal length                         | " + f + "  |");
    System.out.println("___________________________________________________");
}

Other methods

These calculate the ans. based on the input:

public static double findU(double v, double f) {

    double oneUpon;

    oneUpon = 1/f - 1/v;
    return 1/oneUpon;

}

public static double findV(double u, double f){

    double oneUpon;

    oneUpon = 1/f - 1/u;
    return 1/oneUpon;
}

public static double findF(double u, double v){

    double oneUpon = 1/u + 1/v;
    return 1/oneUpon;
}

}

First, the findX functions don't need to initialise the variable oneUpon inside. It's also unclear, anyway.

public static double findU(double v, double f) {

    double oneUpon;

    oneUpon = 1/f - 1/v;
    return 1/oneUpon;

}

public static double findV(double u, double f){

    double oneUpon;

    oneUpon = 1/f - 1/u;
    return 1/oneUpon;
}

public static double findF(double u, double v){

    double oneUpon = 1/u + 1/v;
    return 1/oneUpon;
}

Just return the variable directly.

I'd also like to point out that findU and findV are identical in function, just named differently.

Consider the following instead:

public static double findUV(double uv, double f) {
    return 1/(1/f - 1/uv);
}

public static double findF(double u, double v){
    return 1/(1/u + 1/v);
}

    System.out.println("Hello! This is a calculator based on the Mirror Formula. \n");
    System.out.println("The mirror formula is a relation between the object distance, the image distance \n and the focal length.");
    System.out.println("The formula is : 1/u + 1/v = 1/f.  (u=distance of object, v=distance of image, f=focal length.)");
    System.out.println("It is applicable for both convex mirrors and concave mirrors.");
    System.out.println();
    System.out.println("This calculator can calculate :");
    System.out.println();
    System.out.println("1. Distance of the object from the pole of the spherical mirror.");
    System.out.println("2. Distance of the image from the pole of the spherical mirror.");
    System.out.println("3. Focal length");
    System.out.println("[If you have 2 of the values.]");

This is not how you print large blocks. Separate your strings with line separators like /n. Keep the text as a large multi-line string block, and print that out instead. It will take less maintenance and less System.out.println calls.

Feel free to take the user input as a string/letter instead of an integer, makes everything much clearer. On a related note, an exit function would be nice too.

    switch (userChoice) {

    case 1 :


Generate set of random numbers and remove lowest I'm learning some Python with my kids, and we're making a program to generate D & D characters from user input.

One portion of the code is to generate stats, which are a set of six numbers where each number is the sum of three random values from 1-6. We're total softies, though, so for each value, we're generating four values from 1-6 and tossing out the lowest. We're also doing this seven times, and throwing out the lowest of the seven, to end up with a list of six values.

import random

stats = []

for i in range(7):
    rolls = []

    for j in range(4):
        roll = random.randint(1,6)
        rolls.append(roll)

    stat = sum(rolls) - min(rolls)
    stats.append(stat)

stats.remove(min(stats))

print stats

It works, but I feel like there might be more elegant solutions/more Pythonic approaches. Any suggestions?

This seems like a nice thing to do with your kids!

Python has these things called comprehensions.
For example a list comprehension for the first 5 multiples of 2 could be either:

[i * 2 for i in range(1, 6)]
[i for i in range(11) if i % 2 == 0]

The latter one is a bit harder to understand, and you don't need to know it. So I'll give you an example of what the first one would be if it were not a comprehension.

list_ = []
for i in range(1, 6):
    list_.append(2 * i)

You could change rolls to be a list comprehension, and remove some varibles:

stats = []
for _ in range(7):
    rolls = [random.randint(1, 6) for _ in range(4)]
    stats.append(sum(rolls) - min(rolls))
stats.remove(min(stats))
print stats

There's a way you could do this all in two comprehensions.
But you are not allowed to define variables in comprehensions.
So you would need to use sorted and a slice to get the rolls.
I personally think this is harder to understand, and could be a challenge to teach your kids.

roll = sum(sorted(random.randint(1, 6) for _ in range(4))[1:])

However if you think it's nice, you could use it:

stats = [
    sum(sorted(random.randint(1, 6) for _ in range(4))[1:])
    for _ in range(7)
]
stats.remove(min(stats))
print stats

It's a lot shorter than your original, mostly due to not defining variables like roll.
But could be harder to understand.


Hackerrank Inherited Code I'm back at it again with an Hackerrank challenge-Inherited Code . I just learnt exception and I thought it will be a great idea to get a second opinion on my implemented BadLengthException struct .

  You inherited a piece of code that performs username validation for
  your company's website. The existing function works reasonably well,
  but it throws an exception when the username is too short. Upon
  review, you realize that nobody ever defined the exception.

  The inherited code is provided for you in the locked section of your
  editor. Complete the code so that, when an exception is thrown, it
  prints Too short: N (where \$N\$  is the length of the given
  username).

  Input Format

  The first line contains an integer, \$T\$ , the number of test cases.
  Each of the \$T\$ subsequent lines describes a test case as a single
  username string, \$ U\$ .

  Constraints

  \$ 1 \leq T \leq 1000\$
  \$ 1 \leq |U| \leq 100\$
  The username consists only of uppercase and lowercase letters.

  Output Format

  You are not responsible for directly printing anything to stdout. If
  your code is correct, the locked stub code in your editor will print
  either Valid (if the username is valid), Invalid (if the username
  is invalid), or Too short: N (where \$N\$ is the length of the
  too-short username) on a new line for each test case.

  Sample Input

3
Peter
Me
Arxwwz

  Sample Output

Valid
Too short: 2
Invalid

  Explanation

  Username Me is too short because it only contains  characters, so your
  exception prints Too short: 2 .  All other validation is handled by
  the locked code in your editor.

#include <iostream>
#include <string>
#include <sstream>
#include <exception>
using namespace std;

/* Define the exception here */
struct BadLengthException : public exception {
    int number;
    public :
        BadLengthException(int n){
            number = n;
        }

   const char * what () const throw () {
       std::stringstream ss;
       ss << number;
       return ss.str(). c_str();

   }
};

bool checkUsername(string username) {
    bool isValid = true;
    int n = username.length();
    if(n < 5) {
        throw BadLengthException(n);
    }
    for(int i = 0; i < n-1; i++) {
        if(username[i] == 'w' && username[i+1] == 'w') {
            isValid = false;
        }
    }
    return isValid;
}

int main() {
    int T; cin >> T;
    while(T--) {
        string username;
        cin >> username;
        try {
            bool isValid = checkUsername(username);
            if(isValid) {
                cout << "Valid" << '\n';
            } else {
                cout << "Invalid" << '\n';
            }
        } catch (BadLengthException e) {
            cout << "Too short: " << e.what() << '\n';
        }
    }
    return 0;
}

Final Thoughts

I thought it was a great idea to used a struct as opposed class as a struct is for small grouping
Just like the previous question, I have no access to change  the headers,  main() and checkUsername() so I couldn't remove the using namespace std

This isn't really aimed at the core of your code and is more a reflection of the code challenge you're solving.

BadLengthException isn't a great name for the exception being represented.  It doesn't tell the caller that the name was too short (the exception could be used to represent a length that was too long).  The exception as you've implemented it has no why, so the exception itself doesn't contain sufficient information to tell the caller that it was too short / too long.  With that in mind, I would have been tempted to actually implement a TooShortException and then create typedef so that the supplied code still worked:

typedef TooShortException BadLengthException;


Compare last modification time with specfied time I'm writing a function in Python that compares the modification time of a file in Linux (using os.stat) with a specific time and compare the dates. If the modification date is older than the date I specified, an event should happen.

import os
import time
import datetime

# File Modification time in seconds since epoch
file_mod_time = round(os.stat(file).st_mtime)

# Time in seconds since epoch for time, in which logfile can be unmodified.
t = datetime.datetime.today() - datetime.timedelta(minutes=30)
should_time = round(time.mktime(t.timetuple()))

# Time in minutes since last modification of file
last_time = round((int(time.time()) - file_mod_time) / 60, 2)

if (file_mod_time - should_time) < args.time:
    print "CRITICAL:", file, "last modified", last_time, "minutes. Threshold set to 30 minutes"
else:
    print "OK. Command completed successfully", last_time, "minutes ago."

It is working but I have the feeling that this could be largely improved, because there are so many time conversions. Can you give me a hint on how I can make it better?

I think you were actually on the right path. I really do not concur with the "represent dates with floats" approaches: they force you to add many comments that (1) describe in what units your variables are (seconds, microseconds, whatever) and (2) indicate the reference time (epoch time, etc.). Instead, I recommend to manipulate dates directly: (1) the code is readily understandable, and (2) this allows one to do simple prints of the variables and get obviously meaningful results (useful for debugging and maintaining the code, etc.).

Here is a simple implementation:

import os
from datetime import datetime, timedelta

file_mod_time = datetime.fromtimestamp(os.stat(file_name).st_mtime)  # This is a datetime.datetime object!
now = datetime.today()
max_delay = timedelta(minutes=args.time)

if now-file_mod_time > max_delay:
    print "CRITICAL: {} last modified on {}. Threshold set to {} minutes.".format(file_name, file_mod_time, max_delay.seconds/60)
else:
    print "OK. Command completed successfully {} minutes ago.".format((now-file_mod_time).seconds/60)

(This is not exactly what your original code does, but I'm not sure whether you really intended to use both a delay of 30 minutes and args.time, from the text of the question: I merged them. The code can be easily adapted to your case, were it slightly different.)

This is much more legible. There is also really no need to add many comments about what is being done, as everything is explicit (today(), timedelta(), .seconds,…). No need to do much calculations either, or to round things (int(), round()). You also get a neat date+time printing for free (the printed file_mod_time).

More generally, I would advise to always have variables be of the type of object that they represent. In your case, Python offers a nice date+time class, so I would not never suggest to downgrade dates and time intervals to pure numbers, as their meaning is not obvious (units? time corresponding to number 0?).


Sort a linked-list with only three distinct values I am trying to solve this problem. The problem is to sort a linked list containing only 0s, 1s and 2s by changing their links.

  Sort a linked list of 0s, 1s and 2s by changing links Given a linked
  list of 0s, 1s and 2s, sort it.

  Examples:

  Input  : 2->1->2->1->1->2->0->1->0 Output : 0->0->1->1->1->1->2->2->2

  Input : 2->1->0 Output : 0->1->2

Here is my code:

#include <stdio.h>
#include <stdlib.h>

typedef struct node* Node;
struct node
{
  int data;
  struct node* next;
};

void addnode(Node* head, int data)
{
  Node temp = (Node)malloc(sizeof(struct node));
  temp->data = data;
  temp->next = NULL;
  if( *head == NULL )
  {
      *head = temp;
      return;
  }
  Node p =(Node)malloc(sizeof(struct node));
  p = *head;
  while( p->next != NULL )
  {
    p = p->next;
  }
  p->next = temp;
}

void display(Node head)
{
  Node p =(Node)malloc(sizeof(struct node));
  p = head;
  while( p != NULL )
  {
    printf("%d ", p->data);
    p = p->next;

  }
}

void zeroonetwo(Node* head)
{
  if( *head == NULL )
  {
    return;
  }
  Node zero = (Node)malloc(sizeof(struct node));
  Node one = (Node)malloc(sizeof(struct node));
  Node two = (Node)malloc(sizeof(struct node));
  zero = NULL;
  one = NULL;
  two = NULL;
  Node p = (Node)malloc(sizeof(struct node));
  p = *head;

  Node temp1 = (Node)malloc(sizeof(struct node));
  Node temp2 = (Node)malloc(sizeof(struct node));
  Node temp3 = (Node)malloc(sizeof(struct node));
  temp1 = NULL;
  temp2 = NULL;
  temp3 = NULL;

  while( p != NULL  )
  {
    if( p->data == 0 )
    {
      if( temp1 == NULL )
      {
          temp1 = p;
          zero = temp1;
      }
      else
      {
          temp1->next = p;
          temp1 = temp1->next;
      }
    }

    else if( p->data == 1  )
    {
      if( temp2 == NULL )
      {
          temp2 = p;
          one = temp2;
      }
      else
      {
          temp2->next = p;
          temp2 = temp2->next;
      }

    }
    else
    {
        if( temp3 == NULL )
        {
            temp3 = p;
            two = temp3;
        }
        else
        {
            temp3->next = p;
            temp3 = temp3->next;
        }
    }

    p = p->next;
  }

  if( zero != NULL && one != NULL && two != NULL )
  {
    *head = zero;
    temp1->next = one;
    temp2->next = two;
    temp3->next = NULL;

  }
  else if( zero == NULL && one != NULL && two != NULL )
  {
    *head = one;
    temp2->next = two;
    temp3->next = NULL;
  }
  else if( zero == NULL && one == NULL && two != NULL )
  {
    *head = two;
    temp3->next = NULL;
  }
  else if( zero != NULL && one == NULL && two != NULL )
  {
    *head = zero;
    temp1->next = two;
    temp3->next = NULL;
  }
  else if( zero != NULL && one == NULL && two == NULL )
  {
    *head = zero;
    temp1->next = NULL;
  }
  else if( zero == NULL && one != NULL && two == NULL )
  {
    *head = one;
    temp2->next = NULL;
  }
  else
  {
    *head = zero;
    temp1->next = one;
    temp2->next = NULL;
  }

}

int main()
{
  int N;
  scanf("%d", &N);
  Node head = (Node)malloc(sizeof(struct node));
  head = NULL;
  for(int i = 0; i < N; i++)
  {
    int data;
    scanf("%d", &data);
    addnode(&head, data);
  }
  zeroonetwo(&head);
  display(head);
}

But I believe the code can be shortened. The 8 checks to check NULL for zero, one and two seem redundant. I would really appreciate if you someone could provide me suggestions and point any bugs if any exist.

Algorithm

The algorithm linked in the post is a peculiar implementation of counting sort. I'm not sure why splicing and chaining is used, but I believe blunt counting into an array of three elements is much more effective. Your algorithm would be written in generic and comprehensive manner, as ints are always copy constructible.

Code

As mentioned in the comments, this is ANSI C. I've actually tried it on ideone and it worked perfectly. I also established that your code works correctly for small inputs (haven't tested others). I'm not sure if reimplementing linked list is part of the task, but I'll suggest you standard library alternatives for the code you've written:

Reading:

Not everybody might agree with me, but I like this usage:

unsigned int size{};
std::cin >> size;
std::list<int> values;
std::copy_n(std::istream_iterator<int>{std::cin}, size,
            std::back_inserter(values));

As you can see, everything is handled for us. Starting from looping size times, to push_backing items in. It is better to write declarative code that is also efficient. It is not always possible, but this case is easy enough to be handled by declarative code.

printing:

Here is what people usually prefer:

for (int value : values)
    std::cout << value << ' ';
std::cout << '\n';

But I prefer:

std::copy(values.begin(), values.end(),
          std::ostream_iterator<int>(std::cout, " "));
std::cout << '\n';

It is much harder to say which one is more expressive, thus this is more a matter of taste.

sorting:

C++ algorithms on containers are usually written using iterators. Do note though that sometimes iterators get in the way, like when you'll try to implement heap sort (e.g. heavy use of indices is needed). In this case, iterators are pretty good.

Lets start from the signature:

using iterator = std::list<int>::iterator;

void terse_sort(iterator start, iterator end)

Most of the range algorithms are declared like that: start and end of the range. Do note that end is one past the end. Here is an example: if your range is {1, 0, 1, 2}, start will point to leftmost 1, and end will point to "uncertainty" after 2. With that out of the way, lets get to the most interesting part of the algorithm:

counting:

As name of the algorithm implies, use counting:

unsigned int occurence_count[3] = {0};

auto count_pos = start;
while (count_pos != end)
    ++occurence_count[*count_pos++];

Thw while loop is the same as (p != NULL) in the code. For every index 0, 1, 2, occurence_count will tell how many of those occured in the list. Now, lets put the values in the correct places:

for (unsigned int value = 0; value < 3; ++value)
    for (unsigned int i = 0; i < occurence_count[value]; ++i)
        *start++ = value;

As it can be observed, for index 0, it is gonna write 0 from the start, occurence_count[0] times. The same for indices 1 and 2.

The algorithm mentioned in the linked post

I actually thought about it a little. The thing is that it doesn't perform any swaps! So, if blunt counting sort performs copies (e.g. doesn't regard cases where key!=value), the mentioned algorithm preserves relative order and regards keys not being the same as value. I found it really peculiar, thus decided to implement somewhat more generic version:

#include <map>

template <template <typename ...> typename List, typename ValueType, typename ... Rest>
void splice_sort(List<ValueType, Rest...>& list)
{
    std::map<ValueType, List<ValueType>> locomotives;
    while (list.size() != 0)
    {
        auto locomotive_end = locomotives[list.front()].cend();
        locomotives[list.front()].splice(locomotive_end, list, list.begin());
    }

    //no elements left in list,
    //put it back in the correct order
    for (auto&& locomotive : locomotives)
        list.splice(list.cend(), locomotive.second);
}

Since the algorithm is designed for types that are heavy to swap, they are probably not ints, but std::arrays or some other weird swappable type.

A bit of trivia.

Why locomotive?

By Marcus Wong Wongm [GFDL (http://www.gnu.org/copyleft/fdl.html) or CC BY-SA 4.0 (https://creativecommons.org/licenses/by-sa/4.0)], from Wikimedia Commons

In countries of post-Soviet era, the trains led by locomotives are commonplace, especially just after the collapse of Soviet union. When reading about the algorithm, I recalled my travels on those when I was a kid. The algorithms behaves like merge of trains: trains with lower value are appended to ones which have next bigger value, and so on until all trains are merged into one. Also the splice sort preserves relative order and regards keys not being values, like people in wagons are not their priorities, and vice versa.

Full code to play with

#include <map>

template <template <typename ...> typename List, typename ValueType, typename ... Rest>
void splice_sort(List<ValueType, Rest...>& list)
{
    std::map<ValueType, List<ValueType>> locomotives;
    while (list.size() != 0)
    {
        auto locomotive_end = locomotives[list.front()].cend();
        locomotives[list.front()].splice(locomotive_end, list, list.begin());
    }

    //no elements left in list,
    //put it back in the correct order
    for (auto&& locomotive : locomotives)
        list.splice(list.cend(), locomotive.second);
}

#include <list>
using iterator = std::list<int>::iterator;

void terse_sort(iterator start, iterator end)
{
    unsigned int occurence_count[3] = { 0 };

    auto count_pos = start;
    while (count_pos != end)
        ++occurence_count[*count_pos++];

    for (unsigned int value = 0; value < 3; ++value)
        for (unsigned int i = 0; i < occurence_count[value]; ++i)
            *start++ = value;
}

#include <iostream>
#include <algorithm>
#include <iterator>
#include <sstream>

void terse_sort_demo(std::istream& in)
{
    unsigned int size{};
    in >> size;
    std::list<int> values;
    std::copy_n(std::istream_iterator<int>{in}, size, std::back_inserter(values));

    std::copy(values.begin(), values.end(),
              std::ostream_iterator<int>(std::cout, " "));
    std::cout << '\n';

    terse_sort(values.begin(), values.end());

    std::copy(values.begin(), values.end(), std::ostream_iterator<int>(std::cout, " "));
    std::cout << '\n';
}

void splice_sort_demo(std::istream& in)
{
    unsigned int size{};
    in >> size;
    std::list<int> values;
    std::copy_n(std::istream_iterator<int>{in}, size, std::back_inserter(values));

    std::copy(values.begin(), values.end(),
        std::ostream_iterator<int>(std::cout, " "));
    std::cout << '\n';

    splice_sort(values);

    std::copy(values.begin(), values.end(), std::ostream_iterator<int>(std::cout, " "));
    std::cout << '\n';
}

int main()
{
    const std::string input = "6\n"
        "2 0 0 2 1 1\n";
    std::istringstream input_stream(input);

    std::cout << "splice sort:\n";
    splice_sort_demo(input_stream);
    std::cout << "\n\n";

    input_stream.str(input);
    std::cout << "raw counting sort:\n";
    terse_sort_demo(input_stream);
}

Demo on Wandbox.


Multiple server FTP download class I've written this a short time ago. Do you see anywhere I can improve my logic? This is my first python script.

#!/usr/bin/python
# -*- coding: utf-8 -*-
import os
from ftplib import FTP  # ftplib comes with the py install

class Ftp:

    def __init__(self, hostname, username, password, port=21):

        print '\nConnecting to %s on port %d' % (hostname, port)

        ftp = FTP()

        try:
            ftp.connect(hostname, port)
        except:
            raise Exception('\n\nConnection failed: %s:%d' % (hostname, port))

        try:
            ftp.login(user=username, passwd=password)
            self.start = ftp.pwd()

            print 'Login Successful: %s\n' % username
        except:  # ftplib.error_perm
            raise Exception('\n\nAuth: \n%s@%d\n\n' % (hostname, username))

        self.ftp = ftp
        self.hostname = hostname

    def end(self):
        try:
            self.ftp.quit()
        except:
            pass
        print '\n\nConnection to %s has been closed\n' % self.hostname

    def ls(self):
        return self.ftp.nlst()

    def get(self, file):
        return self.ftp.retrbinary(
            'RETR ' + file, open('%s/%s' % (self.hostname, file), 'wb').write)

    def cwd(self, file):
        return self.ftp.cwd(file)

    def pwd(self):
        return self.ftp.pwd()

    def download(self):
        print 'Downloading Files:'
        self._dl()
        print '\nAll Files Downloaded'
        self.end()

    def _dl(self, changedir='/'):

        if changedir is not '/':
            self.cwd(changedir)
        current = self.pwd()
        folders = []
        files = self.ls()

        for file in files:
            try:
                self.cwd(file)
                if current is not '/':
                    path = '%s%s/%s' % (host, current, file)
                else:
                    path = '%s%s%s' % (host, current, file)
                if not os.path.exists(path):
                    os.makedirs(path)
                    if path != '%s/.' % host:
                        folders.append(path)

                self.cwd(current)
            except:
                if current is not '/':
                    self.get('%s/%s' % (current, file))
                else:
                    self.get('%s' % file)
                print '    %s' % file
        try:
            for fold in folders:
                print '  /%s' % fold
                self._dl(fold.replace(host, ''))
        except:
            pass

Usage:

hosts = ['domain01.com', 'domain02.com']
user = 'username'
password = 'secr3t'

for host in hosts:
    Ftp(host, user, password).download()

I tried to make it as PEP8 compliant as it can be. It is a short file and straightforward. I need better commenting.

Overall - looks good and readable. Just a few comments.

I didn't quite get it but looks like you are relying on exception to be thrown to get a signal and download file - this is bad and expensive.
Is there a way to check if this is file and download instead of trying to treat it as directory - cwd - and catch exception.
Is this what you are trying to do?

In general it is bad to organize program workflow around exceptions - they are for exceptional situations - not for routine work.

Use os.path.join to concatenate paths - link
Use some constant like ROOT_PATH instead of hard-coding '/'
Use logging module for debug/error logging - this is industry standard
Don't swallow exceptions - log them, use extended except:

except Exception as e:
Minor - use "{0}{1}/{2}".format(host, current, file) instead of % notation as this is sometimes wonky and format is just better and industry standard.
Small comment - you are hard-coding logic on where to download files on local disk inside Ftp class which is working but not the best - just add optional parameter to be able to override root directory where to download data to.


JavaScript percentage calculation functions I am building a calculator and I'm aware of what these functions do.

What I basically want feedback on is: Do I need to comment this or is there sufficient information to ascertain its purpose:

function percentOfValue(percentage, of) {
    return (percentage / 100) * of;
}

function whatPercentOfValue(small, large) {
    return (small / large) * 100
}

Code level comments usually lose their context if not properly maintained. Using self descriptive variable names is a good choice. Appreciate your efforts in keeping the code highly readable.

First method, is trying to get the Percentage value, I would update the method definition to getPercentOfValue(from, percentage).
Second method, is trying to calculate percent rate, I would update the method definition to getPercentRate(newValue, originalValue) to follow the terminology in formula.


Resume code of a SQL selecting function in Java I am new to Java so I started to learn by doing. Right now I am developing a store management software. I'm trying to do it the right and clean way, so I'm looking for some reviews of my code. The code is trying to resume the connection and the selecting from a database, so you will found a demonstration in the main method:

public class DBObject {
    Connection c=null;
    Statement stmt = null;
    ResultSet rs=null;

    public DBObject(Connection c,Statement stmt,ResultSet rs ){
        this.c=c;
        this.stmt=stmt;
        this.rs=rs;
    }
    public DBObject(){

    }
    //excute select query (don't forget to close all of ResultSet,Statment,Connection objects)
    public static DBObject executeSelectQuery(String query,String DBname){
        Connection c = null;
        Statement stmt = null;
        DBObject dbo=null;
        try {
              Class.forName("org.sqlite.JDBC");
              c = DriverManager.getConnection("jdbc:sqlite:"+DBname);
              c.setAutoCommit(false);
              System.out.println("Opened database successfully");
              stmt = c.createStatement();
              ResultSet rs = stmt.executeQuery(query);
              dbo=new DBObject(c,stmt,rs);
            } catch ( Exception e ) {
              System.err.println( e.getClass().getName() + ": " + e.getMessage() );
              System.exit(0);
            }
        return dbo;
    }
    public static void main(String[] args) throws SQLException{
        String q="select *from compte";
        DBObject dbo=null;
        String id=null,password=null,nom=null,prenom=null,adresse=null,numtelephone=null;
        dbo=executeSelectQuery(q, "lib.db");
        while(dbo.rs.next()){
            id=dbo.rs.getString("compte_id");
            password=dbo.rs.getString("password");
            nom=dbo.rs.getString("nom");
            prenom=dbo.rs.getString("prenom");
            adresse=dbo.rs.getString("adresse");
            numtelephone=dbo.rs.getString("numtelephone");
        }
        System.out.println(id+" "+password+" "+nom+" "+" "+prenom+" "+adresse+" "+numtelephone);
    }
}

Pool your connections

To set up a DB connection is usually very expensive, relative to the other tasks your application does:
The steps which are necessary to get a connection go right through:

your application
the JDBC Driver
your application's JVM
the network stack on the application machine
the network
the network stack on the DB machine
the DB's authentication mechanism, checking roles and grants etc.

.. the acquired connection is then passed back through the same channels

If you do this for every Statement you would like to run on the DB, your application will spend most of its time waiting for connections to be established (and torn down after the Statement).

I would strongly suggest to read up on JDBC connection pooling.
Which pooling library you would like to use depends on your project setup:
Some Application Servers have a JDBC Connection Pool bundled with them.

Two stand alone JDBC connection pools are C3P0 and apache common's DBCP:
Example with C3P0
Example with DBCP

But more importantly:

Try not to re-invent the wheel

Your code is fine as a proof-of-concept / a worked out example of how a JDBC connection works. (Yes, it can be improved, as jrtapsell and J_H show, but as a proof-of-concept it's OK).

The underlying question is more interesting:
Should you write something yourself, which has many excellent solutions already?

Let's have a look at what you do:

You establish a DB Connection (which should be pooled, see above):

  Connection c = DriverManager.getConnection("jdbc:sqlite:"+DBname);

You use this DB Connection to run a query against the DB

  ResultSet rs = c.createStatement().executeQuery(query);

You create a Java Object / Java Objects from the results of the queries (in your main method)

   while(dbo.rs.next()){
     id=dbo.rs.getString("compte_id");
     password=dbo.rs.getString("password");
     ..
     numtelephone=dbo.rs.getString("numtelephone");
   }

These are requirements of almost all applications, which have a multitude of well established, stable and convenient solutions.

I suggest, you read up on O/R Mapping (Wikipedia, hibernate) and a couple of its implementations (e.g. Hibernate getting started, JPA, Spring Data)

Of course, which tools and libraries you end up using in your application is up to you, but to my experience, it's better to have someone else make and fix an error than to have to make all errors yourself :-)


Iterating an Array Object & Assigning the Attributes Values to those that Match the Condition Is there any approaches to reduce the below coding snippet in Angular 2+ with few lines of code by making use of ECMA Script or by some other ways
this.testCenterAccomodations.forEach((x: { utilityCode: string; isChecked: boolean; isSelected: boolean; }) => {
    if (x.utilityCode === 'PSDQRA') {
        if (this.voiceOverLanguageChecked) {
            x.isChecked = true;
            x.isSelected = true;
        } else {
            x.isChecked = false;
            x.isSelected = false;
        }
    }
});

Looping an Array of objects, we are trying to find the object with the column named utilityCode having PSDQRA value and setting those particular object's properties of isChecked & isSelected to be true  iff, this.voiceOverLanguageChecked boolean value is true, otherwise set those properties to false.
The above snippets is simpler enough to read & understand. But is it possible for someone to achieve the same piece of logic with few lines of code by making use of map, filter, reduce, lambda and rest..

Hmm i would in a first refactoring change it to this:
this.testCenterAccomodations.forEach((x: { utilityCode: string; isChecked: boolean; isSelected: boolean; }) => {
  if (x.utilityCode != 'PSDQRA') {
    return;
  };
  x.isChecked = this.voiceOverLanguageChecked;
  x.isSelected = this.voiceOverLanguageChecked;
});

The next changes do not have the goal of "shortening" the code. But to make it more readable and easier to change. I would love to hear your feedback about my thoughts.
I would create a small (perhaps local) interface, because { utilityCode: string; isChecked: boolean; isSelected: boolean; } seems to be a recurring thing.
interface UtilityCodeSelection {
  utilityCode: string;
  isChecked: boolean;
  isSelected: boolean;
}

Which will result in
this.testCenterAccomodations.forEach((x: UtilityCodeSelection ) => {
  if (x.utilityCode != 'PSDQRA') {
    return;
  };
  x.isChecked = this.voiceOverLanguageChecked;
  x.isSelected = this.voiceOverLanguageChecked;
});

Then i would replace the magic variable "PSDQRA" by a constant (or by an enum if you have multiple utility codes we are coding against). Then we could use that constant at all necessary places and would be able to change it centraly. ALSO we can easily find out where our code is refering to this specific value.
interface UtilityCodeSelection {
  utilityCode: string;
  isChecked: boolean;
  isSelected: boolean;
}

export enum UtilityCodes {
    PSDQRA = 'PSDQRA', // Or if PSDQRA has in reality a better name, use that for the enum
    ...
}

this.testCenterAccomodations.forEach((x: UtilityCodeSelection ) => {
  if (x.utilityCode != UtilityCodes.PSDQRA) {
    return;
  };
  x.isChecked = this.voiceOverLanguageChecked;
  x.isSelected = this.voiceOverLanguageChecked;
});

And in some cases i would even extract some parts into its own method
this.testCenterAccomodations.forEach((x: UtilityCodeSelection ) => this.setUtilityCodeSelection(x, UtilityCode.PSDQRA, this.voiceOverLanguageChecked));

 private setUtilityCodeSelection(code: UtilityCodeSelection, forCode: UtilityCode, setTo: boolean):void{
    if (code.utilityCode != forCode) {
      return;
    };
    code.isChecked = setTo;
    code.isSelected = setTo;
}

This has some drawbacks. I personaly do not like implict changes (here we change the given code) and prefere to change explicitly (make a copy, change the copy and return the copy). Also the extra method is only useful if there are more similar checks. In general its a bad idea to try to generalize things, if there is (at least now) no need for it.
Therefor the chance is high that in your example, i would not move the code in its own method..
As always... Ask 3 developers about coding style and you will get 4 answers. :-)
And they all will be right in some way.
warm regards


Taking the first few items of a list until a target sum is exceeded I'm a rank beginner in Python, and I am working my way through various relevant OpenCourseware modules. In response to the prompt

  Write a procedure that takes a list of numbers, nums, and a limit,
  limit, and returns a list which is the shortest preﬁx of nums the sum
  of whose values is greater than limit. Use for. Try to avoid using
  explicit indexing into the list.

I wrote the following simple procedure

def numberlist(nums,limit):
    sum=0
    i=0
    for i in nums:
        sum=sum+i
        if sum>limit:
            return i
        else:
            print i

It gets the job done, but the division of labor between if and else seems inelegant, as does the use of both return and print. Is there a better way to structure this basic loop?

So, a couple things:

The problem statement says nothing about printing data, so you can omit the print statement, and thus the entire else: clause, entirely.
The problem statement says to return a list, and you're just returning the last item in that list, not the entire list.

Here's a short but inefficient way to do it:

def numberlist(nums, limit):
    i = 0
    while sum(nums[:i]) < limit:
        i += 1
    return nums[:i]

or a more efficient but longer way:

def numberlist(nums, limit):
    prefix = []
    sum = 0
    for num in nums:
        sum += num
        prefix.append(num)
        if sum > limit:
            return prefix


Different path for grid move Given a m * n grids, and one is allowed to move up or right, find the different number of paths between two grid points.

My major idea is, if move r steps right, u steps up, we can find (1) solutions for r-1 steps right and u steps up, then combine with one final right step (2) solutions for r steps right and u-1 steps up, then combine with one final up step.

I use a dynamic programming matrix dp to track number of path. For example, dp[i][j] means if we move i steps right and j steps up.

Any advice on code bugs, smarter ideas in terms of algorithm time complexity or code style advice is appreciated.

def move_right_up_count(rights, ups):
    dp = [[0] * (ups+1) for _ in range(1+rights)]
    for i in range(1,rights+1):
        dp[i][0] = 1
    for j in range(1, ups+1):
        dp[0][j] = 1
    for i in range(1, rights+1):
        for j in range(1, ups+1):
            dp[i][j] = dp[i-1][j] + dp[i][j-1]
    return dp[-1][-1]

if __name__ == "__main__":
    print move_right_up_count(2,3)

PEP 8

PEP 8 complains about this line:

for i in range(1,rights+1):

You should have a single space after the comma and before rights+1:

for i in range(1, rights+1):

Straight to business

Taking on the advice of Raziman T. V.: you are allowed to move \$u\$ steps up and \$r\$ steps right. Now denote a move upward by u and the move to the right by r. The total number of moves is \$u + r\$. Next, every valid path is a string of length \$u + r\$ containing \$u\$ characters u and \$r\$ characters r; clearly, any such string denotes a valid path.

Now, you can permute a string of length \$u + r\$ in \$(u + r)!\$ ways. However, since all us (respectively, rs) are equal, you divide \$(u + r)!\$ by \$u!r!\$ since there is \$u!\$ ways to permute the subsequence of \$u\$ characters u and each of them are equal; same for characters r.

The above leads to

$$\frac{(u + r)!}{u!r!},$$

and, finally, to the code

def factorial(n):
    if n == 0:
        return 1
    return n * factorial(n - 1)

def count_paths(r, u):
    return factorial(r + u) / factorial(r) / factorial(u)

Edit

You can optimize the above a little bit:

def count_paths_v2(r, u):
    min_ru, max_ru = min(r, u), max(r, u)
    value = 1
    for i in range(max_ru + 1, r + u + 1):
        value *= i
    return value / factorial(min_ru)

count_paths performs \$(u + r) + u + r\$ steps, and count_paths_v2 only \$\min(u, r) + (r + u) - \max(r, u).\$

Cleaning a bit

What comes to your version, you can write it a little bit more succintly:

def move_right_up_count(rights, ups):
    grid = [[1 for _ in range(rights + 1)] for _ in range(ups + 1)]
    for y in range(1, ups + 1):
        for x in range(1, rights + 1):
            grid[y][x] = grid[y - 1][x] + grid[y][x - 1]
    return grid[-1][-1]

Hope that helps.