Parsing XML commands and parameters I'm C# programmer and started Python recently for a new project.

This is a part of my project, and I want your opinion about my code and if it's clean or dirty, too long, has duplicate code, or it's good and standard.

(cmd= command ... at the end of file is just for class testing)

import xml.etree.ElementTree as ET

#TODO: processing attributes should be case insencitive

class command:
    def __init__ (self,htmlcommand):
        self.preview=False
        self.members=[]
        self.__parse__(htmlcommand)

    def __parse__(self,htmlcommand):
        root = ET.fromstring(htmlcommand)
        self.service=root.attrib['service']
        self.name=root.attrib['name']
        self.source=root.attrib['source']
        self.preview=bool(root.attrib['preview'])
        self.procedurename=root.attrib['procedurename']

        mems=root.findall('member')
        if(len(mems)==0):
            raise Exception('command has no member')
        for m in mems:
            self.members.append(member(m))

class member:
    def __init__ (self,member):
        self.params=[]
        self.__parse__(member)

    def __parse__(self,member):
        self.name=member.attrib['name']
        self.method=member.attrib['method']
        try:
            self.order=member.attrib['order']
        except:
            pass
        try:
            self.sort=member.attrib['sort']
        except:
            pass
        self.__parseparams__(member)

    def __parseparams__(self,member):
        params=member.findall('./params/param')
        for p in params:
            self.params.append(param(p))

    #def __getparams(member):

class param:
    def __init__(self,param):
        self.name=param.attrib['name']
        self.value=param.attrib['value']

cmd= command("""<basis core="external.ws.ws" service="bimepasargad" content="33,444" name="Internalissuance" source="cmsDbService" procedurename="sbserviceprocedure2" preview="true">
            <member name="bimepasargadInternalIssuance" method="internalissuance" sort="random">
                <params>
                    <param name="cms.query.token" value="111"/>
                    <param name="duration" value="2222"/>
                </params>
            </member>
            <member name="2" method="2" sort="random">
                <params>
                    <param name="cms.query.token" value="3"/>
                </params>
            </member>
        </basis> """)

Multiple things to fix:

variable naming - class names should be in camel-case and start with a capital letter (reference); when there are multiple words in a variable, you should put underscore between them, e.g. in your case procedurename would become procedure_name
missing spaces - there should be spaces around operators, like around the = here:

self.name = param.attrib['name']

incorrect private method naming - when you put double underscores around a method name (this is called "dunder"), you are implying that it is a "magic method" which may confuse future readers of your code (which can also be you); while, I think you meant to create a private method. In other words, naming the method __parse, not __parse__
if(len(mems)==0): can be simplified as if not mems:
bare except clause should be avoided, either replace with handling a more specific error type:

try:
    self.order=member.attrib['order']
except:
    pass

with:

try:
    self.order = member.attrib['order']
except KeyError:
    pass

Or, follow the @LaurentLAPORTE's advice and use the .get() method to set the self.order to None if the "order" attribute does not exist:

self.order = member.attrib.get('order')


Add Number to First Number on Each Line of File then Write to Another File - Python Python program adds a value to the first number in a fixed width file of variable length. Normally one line header. Before user deletes original file and renames new file they need to check it.

How can the error handling be improved?

# Add number to first number on each line of file then write to another file
#
# Initialise
infile = 'egtxt.prn'
hdrln = 1
llen = 0
fw = 7  # Field width of first number on line
add = 80.0  # Add value to first number on line

print ('Processing {}, first number is {} characters wide, adding {} ...'.format(infile, fw, add))

ifile = open(infile, 'r')
tfile = open(infile+'.out', 'w')
# Skip header
line = ifile.readline()
tfile.write (line)

while True:
    line = ifile.readline()
    if not line:
        break
    llen = len(line) - fw
    tfile.write ('{:{sfw}.{sprec}f}{}'.format(float(line[:fw])+add, line[-llen:], sfw=str(fw), sprec='2'))

# Finalise
ifile.close()
tfile.close()
print (' done.')

Sample egtxt.prn follows:

ID      Name               Address                         Comment
   2.88 Barnett, Irene A.  P.O. Box 829, 4484 Aliquet Ave  viverra. Maecenas iaculis
  30.58 Irwin, Honorato X. Ap #446-1219 Nisi. Road         dui
3428.21 Talley, Bree S.    862-3586 Duis Ave               leo. Morbi
 176.12 Hensley, Kai T.    Ap #842-3806 Class Street       eu

Constants uppercase

A very widespread convention is that constants should be uppercase, so please conform to the standard for readability.

Unused variables

hdrln = 1
llen = 0

The first one is never used, the second one is never used with the original value, so I suggest removing that declaration.

with and iterable interface for files

with is a commonly used to keyword to simplify working with files to take care of automatic closing, also files are iterable so you can just do: for line in file that should simplify your code.

Isolate logic in functions

tfile.write ('{:{sfw}.{sprec}f}{}'.format(float(line[:fw])+add, line[-llen:], sfw=str(fw), sprec='2'))

That should be in its function along with documentation because it is the main part of the program, the logic should be highlighted and explained not hidden inside the boilerplate.


Observer pattern using a set of observers and delegated properties The idea is to implement the observer pattern, with a separate notification method for each observed property of the observed object. Let me start with a simplified use case:

class Point {
    val observers: ObserverSet<PointObserver>()
    val x by observers.observe(0f, PointObserver::xChanged)
    val y by observers.observe(0f, PointObserver::yChanged)
}

interface PointObserver {
    fun xChanged(old: Float, new: Float)
    fun yChanged(old: Float, new: Float)
}

class PointView(point: Point) {
    init {
        point.observers.add(object : PointObserver {
            override fun xChanged(old: Float, new: Float) { ... }
            override fun yChanged(old: Float, new: Float) { ... }
        })
    }
}

I'm new to Kotlin, so I'd like some feedback on the following implementation:

import kotlin.properties.ReadWriteProperty
import kotlin.reflect.KProperty

/**
 * A set of observers for a particular object, to easily implement the Observer Pattern without needing inheritance.
 *
 * This can be made a public property of the object. Call [observe] to create delegated properties for each property
 * that observers need to be notified about.
 *
 * @param O The type of the observers, typically an interface.
 */
class ObserverSet<O> {

    private val observers = mutableSetOf<O>()
    private val properties = mutableListOf<ObservedProperty<*>>()

    /**
     * Sets whether an observer is present in the set. If the observer was actually added to the set, its notification
     * methods will be called with the current value of the property.
     *
     * @param observer The observer.
     * @param present If true, the observer will be added to the set if not already there. If false, the observer will
     *   be removed from the set if it's there.
     */
    fun toggle(observer: O, present: Boolean) {
        if (present) {
            val wasPresent = observer in observers
            observers.add(observer)
            if (!wasPresent) {
                init(observer)
            }
        } else {
            observers.remove(observer)
        }
    }

    /**
     * Creates a delegate property with a callback that notifies each observer.
     *
     * @param V The type of the property.
     * @param initialValue The initial value of the property.
     * @param onChanged Function on the [O] interface that will be called with the old and new value after the
     *   property has been changed.
     */
    fun <V> observe(initialValue: V, onChanged: O.(old: V, new: V) -> Unit): ReadWriteProperty<Any?, V> {
        val property = ObservedProperty(initialValue, onChanged)
        properties.add(property)
        return property
    }

    private fun init(observer: O) {
        properties.forEach { property ->
            property.initObserver(observer)
        }
    }

    private inner class ObservedProperty<V>(initialValue: V, private val onChanged: O.(old: V, new: V) -> Unit) :
            ReadWriteProperty<Any?, V> {

        private var value = initialValue

        override fun getValue(thisRef: Any?, property: KProperty<*>): V {
            return value
        }

        override fun setValue(thisRef: Any?, property: KProperty<*>, value: V) {
            val oldValue = this.value
            this.value = value
            observers.forEach { observer ->
                observer.onChanged(oldValue, value)
            }
        }

        fun initObserver(observer: O) {
            observer.onChanged(value, value)
        }
    }
}

(The toggle method is the only one I need right now; regular add and remove could of course be implemented as well.)

I'm mainly interested in efficiency considerations, ways to make this more general, and following Kotlin best practices.

You don't need a wasPresent variable. MutableSet.add returns true if the element was added to the set:

if (observers.add(observer)) {
    init(observer)
}

Sometimes in Kotlin you don't have to declare temporary variables so that you can do something with a return value before returning it. e.g.:

return ObservedProperty(initialValue, onChanged).apply { properties.add(this) }

Unfortunately this can sometimes look a bit strange using this, etc. but in Kotlin 1.1 you can use bound callable references:

return ObservedProperty(initialValue, onChanged).apply(properties::add)

You might want to take a look at Delegates.observable - stdlib - Kotlin Programming Language which you may be able to reuse to improve and reduce your code.


JavaScript function to convert an array to an array of strings (formatted) Input

categories - an array of categories

[
    { id: 1, title: 'Smartphones' },
    { id: 2, title: 'Laptops' }
]

products - an array of products, linked to categories

[
    { id: 1, title: 'iPhone X', category_id: 1, price_min: 1200, price_max: 1700 },
    { id: 2, title: 'MacBook Pro Retina 13', category_id: 2, price_min: 1500, price_max: 4500 },
    { id: 3, title: 'Samsung Galaxy S8', category_id: 1, price_min: 0, price_max: 0 }
]

Output

[
    "*Smartphones*\niPhone X\n ($1200 - $1700)\nSamsung Galaxy S8 (free)",
    "*Laptops*\nMacBook Pro Retina 13 ($1500 - $4500)"
]

Current implementation

function printPriceList (categories, products) {
    return categories.map(category => {
        const pcontent = products
            .filter(product => product.category_id === category.id)
            .map(product => {
                const samePrice = product.price_min === product.price_max
                const price = samePrice ? product.price_min : `${product.price_min} - ${product.price_max}`

                if (price === 0) {
                    return `${product.title} (free)`
                }

                return `${product.title} ($${price})`
            })
        return [`*${category.title}*`].concat(pcontent)
    }).map(cat => cat.join('\n'))
}

I'd like to find out a way to refactor it to be more readable, easy to understand and efficient.

What can we use?

all the modern JS features, like a spread operator (...smth)
lodash

First impressions:

too much nesting, too many nested returns
inconsistent style, inconsistent naming

You already recognized the potential for refactoring. The simplest possible technique for refactoring is renaming variables:

Your code currently uses camelCase (printPriceList, samePrice), underscores (category_id, price_min) and something else (pcontent). For me, it doesn't matter that much which style you chose as long as you use it consistently and as long as you use descriptive names.

Other stylistic suggestions:

It is a habit of mine to split complex chains such as...

const result = elements
    .filter(...)
    .map(...);

...into separate assignments with self-documenting names:

const blabla = elements.filter(...);
const result = blabla.map(...);

I prefer to terminate statements with semicolons instead of having to think about the rules of JavaScript's automatic semicolon insertion.
Instead of the 'print' prefix which signals some kind of active output e.g. to a stream, you could use e.g. 'format'.

The most helpful refactoring is to extract code fragments that can be grouped together into their own functions:

Printing a product price:

function print_product_price({price_min, price_max}) {
    if (price_max === 0) {
        return 'free';
    } else if (price_min === price_max) {
        return `$${price_min}`;
    } else {
        return `$${price_min} - $${price_max}`
    }
}

Printing a product:

function print_product(product) {
    const price = print_product_price(product);
    return `${product.title} (${price})`;
}

Printing a category:

function print_category(category, products) {
    const category_products = products.filter(product => product.category_id === category.id);

    const category_rows = [
        `*${category.title}*`,
        ...category_products.map(print_product)
    ];

    return category_rows.join('\n');
}

Using Lodash, the above .filter expression could be written as _.filter(products, { 'category_id': category.id }) - suggested by @Gerrit0.
And finally printing the price list:

function print_price_list(categories, products) {
    return categories.map(category => print_category(category, products));
}

As you can see, due to extracting those functions, no secondary or deeper nesting is needed.


Star rating: Update stars on click and don't lose the actual vote on re-hover I'm trying to make a star based rating system.

First of all, I don't like to make it all with JS and only innerHTML the rated stars in a given div. I want to make it more like a plugin with the ability to make anything votable and you just have to pass the classes.

My biggest problem, however, is that I'm setting classes on hover and removing them on mouse leave. That's just fine until the user clicks and the classes are set. Think you can imagine what happens if the user hovers again: The classes would get removed.

But still I want the user to be able to change his vote. I managed it with two different divs where one is hidden. I absolutely don't feel comfortable with this solution:

With jQuery I'm enabling .votable on hover. On click I'm setting the actual vote to .voted and on mouse leave I'm hiding .votable and showing .voted.

$('.voted').hover(
    function() {
        $('.voted').addClass('hide');
        $('.votable').removeClass('hide');
    }
);

$('.votable').on('mouseleave',function(){
    $('.voted').removeClass('hide');
    $('.votable').addClass('hide');
});

$('.votable > i').hover(
    function() {
        $(this).prevAll().andSelf().removeClass('fa-star-o').addClass('fa-star');
        $(this).nextAll().removeClass('fa-star').addClass('fa-star-o');
    },
    function() {
        $(this).prevAll().andSelf().removeClass('fa-star').addClass('fa-star-o');
    }
);

$('.votable > i').click(function(e) {
    var vote_type = $(this).attr('data-vote-type');
    var el = $('.voted > i[data-vote-type="' + vote_type + '"]');

    $(el).prevAll().andSelf().removeClass('fa-star-o').addClass('fa-star');
    $(el).nextAll().removeClass('fa-star').addClass('fa-star-o');
});

You can see the code working at my fiddle.

What would be a better way of achieving this?

About your HTML:

Consider using the meter element to display the rating:

<meter min="0" max="5" value="3">★★★☆☆</meter>

(However, I don’t know how current user agents present this, i.e., if they add some special styling.)
In any case, you should not use the i element for displaying an icon via CSS. Use span instead if you need an empty element.
If you don’t want to use a form for voting, consider using WAI-ARIA to make voting accessible.


Unit Testing for Spring Controller This is my first attempt to do unit test for one of my controllers.

@RunWith(MockitoJUnitRunner.class)
public class AddressControllerUnitTests {

    private MockMvc mockMvc;

    @Mock
    SubmissionService submissionService;

    @Mock
    MessageSource messageSource;

    @InjectMocks
    AddressController addressController;

    @Before
    public void setup() {
        mockMvc = MockMvcBuilders.standaloneSetup(addressController).build();
    }

    @Test
    public void submitSuccess() throws Exception {
        when(submissionService.isEmailAvailable("email@email.com",0)).thenReturn(true);
        when(messageSource.getMessage("form.submitted.success", null, Locale.ENGLISH)).thenReturn("Form has been submitted successfully");
        this.mockMvc
                .perform(post("/address")
                        .param("address", "my address")
                        .param("phone", "08123456789")
                        .param("email", "email@email.com")
                        .param("userId", "14"))
                .andExpect(status().isFound())
                .andExpect(flash().attribute("message", "Form has been submitted successfully"))
                .andExpect(redirectedUrl("/address"));
    }
}

Do I really need to perform expect on the flashAttribute ? Or Should I just remove it completely ? It looks funny that I have to set

when(messageSource.getMessage("form.submitted.success", null, Locale.ENGLISH)).thenReturn("Form has been submitted successfully");

and then

.andExpect(flash().attribute("message", "Form has been submitted successfully"))

You've written 40 lines of code to test 2-3 assignment statements and possibly one if statement. You've also tighly coupled your test to the implementation. This does not seem like a good tradeoff to me.

Read this before you continue with the rest of the review.

Your test is very much in the solitary category and I think that's a problem.

Get rid of the service mocks. Use the real implementations of services and instead inject test doubles at the repository level. Then don't write any tests specifically for your services, unless you consider their implementation particulary tricky (and therefore need more direct feedback while implementing them).

In the article I linked Fowler did not really go into the benefits of sociable unit tests so let me expand upon that. A solitary test does only support refactorings inside the tested class. In my experience most problems with code do not come from the implementation of a single class but rather from poor choices when it comes to what constitutes a class and from the dependencies between classes. A sociable test allows you to perform much larger refactorings.

Note that a solitary test will not only not aid you in performing a larger refactoring, but it will actually get in your way as you will have to rewrite the test.

Also note that replacing remote services with test doubles is fine not only because those services are slow and introduce non-determinism, but also because refactorings across remote services are very rare (and probably shouldn't be called refactorings anymore).

Finally, I acknowledge that in this small piece of code this may not seem as a major issue. But in a large project this is the difference between an unmaintanable pile of shit and something I'd happily work on.


Validating user access to a function I was reviewing the following code block and noticed there was validation check at the top.  The 3rd line in the function checks if the invoice belongs to the logged in customer.

I am wondering what the correct way of doing this might be.  Some options that come to me head are:

GetInvoice should take an overload and also take in the loggedInCustomerId.  The GetInvoice method would then throw an exception if it was an invalid call. (Would the business logic be the best place for this)
Filter attribute should be created (i think this is overkill)

What would the best approach to something like this be?

    public ActionResult ViewInvoice(Guid invnum)
    {
        int loggerInCustomerId = GetTheLoggedInCustomerId();
        Invoice invoice = _invoiceLogic.GetInvoice(invnum);

        if (invoice.CustomerId != loggerInCustomerId)
        {
            //Invalid Action
            return RedirectToAction("Index", "MyInvoices");
        }
        //do other stuff as normal
    }

public ActionResult ViewInvoice(Guid invnum)

Regardless that one could figure out that invnum means "invoice number" this name isn't good. First it isn't a number but a globally unique identifier and second using abbreviations for variable naming shouldn't be done because it reduces readability.

-> how about renaiming this to invoiceId ?

int loggerInCustomerId = GetTheLoggedInCustomerId();

The variable name is misspelled you should change it to loggedInCustomerId

The "validation" part wether a invoice belongs to the current loggedin customer should be done by querying the database (if one is used) to get the invoice which matches the invoiceId and the loggedInCustomerId. An overloaded GetInvoice(GUID, int) could be used in the business layer (will be _invoiceLogic) which shouldn't throw an exception but return either null or some Invoice.Empty constant. Throwing an exception doesn't buy you much but will slow down the application because you will also need to handle it.

Maybe a method like the Dictioary<T>.TryGetValue() would be a good fit so one could write something along this lines

public ActionResult ViewInvoice(Guid invoiceId)
{
    int loggedInCustomerId = GetTheLoggedInCustomerId();
    Invoice invoice;

    if (!_invoiceLogic.TryGetInvoice(invoiceId, loggedInCustomerId, out invoice)
    {
        return RedirectToAction("Index", "MyInvoices");
    }
    //do other stuff as normal
}

A version without accessing the data layer could then look like

public bool TryGetInvoice(Guid invoiceId, int customerId, out Invoice invoice)
{
    invoice = GetInvoice(invoiceId);
    if (invoice.CustomerId == customerId)
    {
        return true;
    }

    invoice = null;
    return false;
}

A version with a fictive data layer (because I don't know if you use one) could look like this

public bool TryGetInvoice(Guid invoiceId, int customerId, out Invoice invoice)
{
    invoice = _dataLayer.GetInvoice(invoiceId, customerId);

    return invoice != null;
}

If you use a TryGetXXX method you need to make sure that such a method never throws an exception because it is expected that it doesn't.


Random walk in Python + turtle I decided to implement a random walk.

The rules

The rules are easy, an object is moved in the direction dictated by random or pseudo-random numbers.
If you want to read more about it, see the Wikipedia Page.

A simple example

Let's say we decide:

1 = "Go right"
2 = "Go down"
3 = "Go left"
4 = "Go up"

We then generate the random or pseudo-random sequence: 3,1,4,1.
And we convert it to directions:

Left
Right
Up
Right

we obtain the following:

A more complex example

Runnning my code with:

step_size = 15
step_number = 1000

Generated the following walk:

Running the code with the exact same parameters may yield completely different results because pseudo-random numbers are used.

I think my code is well written and fully Pep8 compliant, two things bother me

I define many go_<direction> methods to go to absolute directions, I could have done something like:

turn_of_random_deegres()
turtle.forward(step)

But it looked cleaner to me, albeit a little longer, to define absolute moving.

The go_<direction> methods change between logo and standard edition. (Control-Find setheading in this doc-page)

The code

import turtle
import random

def go_right(step):
     turtle.setheading(0)
     turtle.forward(step)

def go_up(step):
     turtle.setheading(90)
     turtle.forward(step)

def go_left(step):
     turtle.setheading(180)
     turtle.forward(step)

def go_down(step):
     turtle.setheading(270)
     turtle.forward(step)

def make_random_walk(step_size, step_number):
    move_dict = {1: go_up,
                 2: go_right,
                 3: go_left,
                 4: go_down
                 }
    for _ in range(step_number):
        move_in_a_direction = move_dict[random.randint(1, 4)]
        move_in_a_direction(step_size)

if __name__ == "__main__":
    turtle.hideturtle()
    turtle.speed("fastest")
    make_random_walk(15, 1000)

Your code looks nice and seems to be fully PEP 8 compliant indeed. The fact that you have 1 blank line between functions except in one place where you have 2 puzzles me a bit but that's not a huge issue.

You can actually make your code much easier. Here are a individual steps, you'll find the final code at the end.

Notice that you call turtle.forward(step) in each go_somewhere function. You might as well remove this and put it once and for all after your call to move_in_a_direction(step_size). Now, the go_somewhere function doesn't need to be given a step anymore.
Notice that the different function go_somewhere are just a call to setheading with a custom parameter. You could transform move_dict to map numbers to angles and call turtle.setheading(move_dict[random.randint(1, 4)).
Notice that your map is just converting 1 into 0, 2 into 90, 3 into 180, 4 into 270. This can be substitued with a simple operation : remove 1 then multiply by 90. You now have turtle.setheading((random.randint(1, 4) - 1) * 90).
Notice that generating an integer in [0, 1, 2, 3] makes things even easier as you don't need to remove 1.

Here's the corresponding code :

def make_random_walk(step_size, step_number):
    for _ in range(step_number):
        turtle.setheading(90 * random.randint(0, 3))
        turtle.forward(step_size)


React component to render price change I've written this Card component which is composed of thumbnail image, name, symbol, total_volume, current_price, and price_change_percentage_24h.

I want to refactor my renderPriceChange function since it's using the same pattern.
const renderPriceChange = (price) => {
  if (price > 0) {
    return (
      <Text style={styles.rise}>
        <AntDesign name='caretup' color='#03AE9D' size={10} />{' '}
        {percentageFormat(item.price_change_percentage_24h)}%
      </Text>
    );
  } else {
    return (
      <Text style={styles.drop}>
        <AntDesign name='caretdown' color='#fb2c33' size={10} />{' '}
        {percentageFormat(item.price_change_percentage_24h)}%
      </Text>
    );
  }
};

How to make it look cleaner with smart and less code?

It is equivalent to:
const renderPriceChange = (price) => {
  return (
    <Text style={price > 0 ? styles.rise : styles.drop}>
      <AntDesign
        name={price > 0 ? 'caretup' : 'caretdown'}
        color={price > 0 ? '#03AE9D' : '#fb2c33'}
        size={10}
      />{' '}
      {percentageFormat(item.price_change_percentage_24h)}%
    </Text>
  )
}

You can inline the if...else by using JavaScript's ternary operator a ? b : c


Palindrome Checker in Java My objective is to take a string as input and determine if it is a palindrome. I'm wondering how I could improve this code (efficiency, bugs, etc). I'm relatively new to Java so I'm expecting that my code will contain some errors.

import java.util.Scanner;

    /**
     * Created by Adam on 6/5/2015.
     */
    public class Reversal {
        public static void main(String args[]) {
            String word = "";
            String reversed = "";
            Scanner in = new Scanner(System.in);
            System.out.println("Please enter the string: ");
            word = in.nextLine();
            int length = word.length();
            for(int i = length; i > 0; i--) {
                reversed += word.charAt(i-1);
            }
            if(removeSpace(word).compareTo(removeSpace(reversed)) == 0) {
                System.out.println("The string " + word + " is a palindrome");
            }
            else {
                System.out.println("The string " + word + " is not a palindrome");
            }
        }

        private static String removeSpace(String aword) {
         String buffer = "";
            for(int i = 0; i < aword.length(); i++)   {
             if(!(String.valueOf(aword.charAt(i)).compareTo(" ") == 0)) {
                 buffer += aword.charAt(i);
             }
         }
            return buffer;
        }
    }

Firstly, this is something I see a distressing amount in beginner Java programmers:

removeSpace(word).compareTo(removeSpace(reversed)) == 0

There's a method for checking if two Strings are equal. It's called equals. It'd be used like this:

removeSpace(word).equals(removeSpace(reversed))

If you wanna ignore case, you could use equalsIgnoreCase:

removeSpace(word).equalsIgnoreCase(removeSpace(reversed))

Secondly, a formatting tip: Except in method declarations or calls, always put a space before ( and a space after ). So, for example:

if(...) {

becomes

if (...) {

In your removeSpaces method:

In general, when find that you're doing it a lot, avoid using String += String. Instead, initialize a StringBuilder and append to it.
String#charAt() returns a char, which can be compared with ==; I'm not sure why you first turn it into a String, then use compareTo (which, again, should be equals, since you were comparing Strings for equality)

With those issues fixed, this is what the method looks like:

private static String removeSpace(String aword) {
     StringBuilder buffer = new StringBuilder();
        for (int i = 0; i < aword.length(); i++)   {
         if (aword.charAt(i) != ' ')) {
             buffer.append(aword.charAt(i));
         }
     }
     return buffer.toString();
}

(Note that I changed what would have been !(... == ...) to ... != ... and that ' ' uses single quotes.)

The whole function could be replaced with word.replaceAll(" ", ""), but that's no fun, so I'm gonna keep using yours through here. Besides, it's a good way to practice to write your own methods -- just remember, when you're writing production code, to do a thorough search of the docs first. If there's a version in the standard API, a whole gaggle of people have been optimizing it to high heaven, and it might even be implemented in native code. That alone would make it a bazillion times faster than anything you could write in pure Java.

You use removeSpaces in the comparison, which somewhat obfuscates what really happens: The String is checked for palindromity (word coined?) ignoring the spaces. It's clearer to move those method calls to when you get it (plus, this removes an extra function call, which is more efficient :D):

word = removeSpaces(in.nextLine());
// ...
if(word.equals(reversed)) { // Note that I'm not using compareTo here
// etc.

Thirdly, I'd recommend pulling the code that reverses your String into another method that takes a String argument and returns a String (and applying my earlier advice about StringBuilder):

public static String reverse(String word) {
    StringBuilder reversed = new StringBuilder(word.length);
    for(int i = word.length(); i > 0; i--) {
        reversed.append(word.charAt(i - 1));
    }
}

In that snippet above, I also removed length, since it's ultimately useless. You're only using it once, so there's no point in declaring a variable, and it's actually clearer to be able to directly see what String's length we're talking about -- you don't need to go back in the code to see what it means

With all this, your main becomes a bit clearer:

public static void main(String args[]) {
    String word = "";
    String reversed = "";
    Scanner in = new Scanner(System.in);
    System.out.println("Please enter the string: ");
    word = removeSpaces(in.nextLine());
    reversed = reverse(word);
    if (word.equals(reversed)) {
        System.out.println("The string " + word + " is a palindrome");
    }
    else {
        System.out.println("The string " + word + " is not a palindrome");
    }
}

We could clean it up a little more by moving your word and reversed declarations to where the variables are first assigned, rather than their own lines (which, incidentally, don't need the = "" because they aren't used before you assign them later):

public static void main(String args[]) {
    Scanner in = new Scanner(System.in);
    System.out.println("Please enter the string: ");
    String word = removeSpace(in.nextLine());
    String reversed = reverse(word);
    if (word.equals(reversed)) {
        System.out.println("The string " + word + " is a palindrome");
    }
    else {
        System.out.println("The string " + word + " is not a palindrome");
    }
}

This means that your whole class, all together, looks something like this:

public class Reversal {
    public static String reverse(String word) {
        StringBuilder reversed = new StringBuilder(word.length);
        for (int i = word.length(); i > 0; i--) {
            reversed.append(word.charAt(i - 1));
        }
    }

    private static String removeSpace(String aword) {
         StringBuilder buffer = new StringBuilder();
            for (int i = 0; i < aword.length(); i++)   {
             if(aword.charAt(i) != ' ')) {
                 buffer.append(aword.charAt(i));
             }
         }
         return buffer.toString();
    }

    public static void main(String args[]) {
        Scanner in = new Scanner(System.in);
        System.out.println("Please enter the string: ");
        String word = removeSpace(in.nextLine());
        String reversed = reverse(word);
        if (word.equals(reversed)) {
            System.out.println("The string " + word + " is a palindrome");
        }
        else {
            System.out.println("The string " + word + " is not a palindrome");
        }
    }
}

If you want to get rid of your custom method to remove spaces (which I would recommend doing if you ever had to remove spaces in production code):

public class Reversal {
    public static String reverse(String word) {
        StringBuilder reversed = new StringBuilder(word.length);
        for (int i = word.length(); i > 0; i--) {
            reversed.append(word.charAt(i - 1));
        }
    }

    private static String removeSpace(String aword) {
         StringBuilder buffer = new StringBuilder();
            for (int i = 0; i < aword.length(); i++)   {
             if(aword.charAt(i) != ' ')) {
                 buffer.append(aword.charAt(i));
             }
         }
         return buffer.toString();
    }

    public static void main(String args[]) {
        Scanner in = new Scanner(System.in);
        System.out.println("Please enter the string: ");
        String word = in.nextLine().replaceAll(" ", "");
        String reversed = reverse(word);
        if (word.equals(reversed)) {
            System.out.println("The string " + word + " is a palindrome");
        }
        else {
            System.out.println("The string " + word + " is not a palindrome");
        }
    }
}

And, finally, an utterly unreadable version of your code that does the same thing, which you should never use but was annoyingly fun to write so I thought I'd provide it anyway:

public static void main(String args[]) {
    String l;
    System.out.println("The string "+(l=new Scanner(System.in).nextLine())+" is"+(l.replaceAll(" ", "").equals(new StringBuilder(l).reverse().toString().replaceAll(" ", ""))?" ":" not ")+"a palindrome");
}


Library of news articles I have a library of news articles. Each story is unique by id.

 public String getStory(String id) throws Exception {
        Connection conn = MysqlDb.getConn(dbName);

        try {
            for(Publisher p : Publishers) {
                PreparedStatement prpStmt = conn.prepareStatement("SELECT story FROM " + p + " WHERE id='" + id + "'");
                ResultSet rs = prpStmt.executeQuery();

                while (rs.next()) {
                    String story = rs.getString(1);
                    rs.close();
                    return story;
                }
                rs.close();
            }
        } finally {
            conn.close();
        }
        return null;
    }

Come what may (like an exception, or something), I want to make sure to release the JDBC resources used by the Connection and ResultSet objects. Does finally always execute in Java?

Closing the database connection in a finally block makes sense, right? In the code I've looked at, I've not seen finally really used for anything.
Maybe making sure that the ResultSet closes is not absolutely necessary? But, the close() method must exist for some reason... But, in my method above, the scope is too narrow to be closed by the finally block. What to do? Create another finally with adequate scope for the ResultSet?

The best way to close a Connection is... not doing it yourself,
but using try-with-resources, which is the best practice as of Java 7:

public String getStory(String id) throws Exception {
    try (Connection conn = MysqlDb.getConn(dbName)) {
        for (Publisher p : Publishers) {
            PreparedStatement prpStmt = conn.prepareStatement("SELECT story FROM " + p + " WHERE id='" + id + "'");
            ResultSet rs = prpStmt.executeQuery();

            while (rs.next()) {
                String story = rs.getString(1);
                rs.close();
                return story;
            }
            rs.close();
        }
    }
    return null;
}

There are a couple of other issues in your code:

You're not using the prepared statement properly and to its full advantage. Don't create a query string as a concatenation of variables used in the WHERE clause. Those values should be set via the prepared statement. In its current form, your query may be vulnerable to SQL injection attacks.
The inner while loop is not actually looping. You should replace the while with an if.
You should use try-with-resources for the prepared statement and the result set too
"Publishers" looks like a collection, a variable, but by the naming conventions of Java only class names should start with a capital letter. So this should be renamed to publishers.

With the above suggestions applied, the code becomes:

public String getStory(String id) throws Exception {
    try (Connection conn = MysqlDb.getConn(dbName)) {
        for (Publisher p : publishers) {
            String sql = "SELECT story FROM " + p + " WHERE id = ?";
            try (PreparedStatement prpStmt = conn.prepareStatement(sql)) {
                prpStmt.setString(1, id);
                try (ResultSet rs = prpStmt.executeQuery()) {
                    if (rs.next()) {
                        return rs.getString(1);
                    }
                }
            }
        }
    }
    return null;
}


Guess-the-number game by a Python beginner For some background I'm pretty much a beginner and attempting to learn Python currently. So I decided after a few days of lessons I'd look for some beginner projects to test my knowledge. A website suggested I do a 'Guess the Number' game and basically suggested I follow this guideline.

Random function
Variables
Integers
Input/Output
Print
While loops
If/Else statements

In short I read the documentation on random and decided to give it a shot. Now for the actual question. Is this a good way of doing this or can I simplify it?

I've pretty much been working at this for longer than I would like to admit (approximately 90 minutes), because I'd get stumped and then rewrite what I was thinking.

# guess the number game
import random

print "Welcome to guess the number!"
print "You have 10 tries to guess the correct number!"
print "The range of numbers will be from 0-25"

# variables to store winning number, user guess and number of tries
number = random.randint(0, 25)
guess = raw_input("Please enter a number (0-25): ")
tries = 0

# check to see if the user guessed the right number
if int(guess) == number:
    print "Congratulations you\'ve won!"

# noticed that you could input invalid numbers and it counted as a guess so this is how i solved that
while int(guess) > 25 or int(guess) < 0:
    guess = raw_input("Please enter a VALID guess: ")
else:
    # my attempt at making the game loop
    while tries < 10 and int(guess) != number:
        guess = raw_input("Please guess again: ")
        tries = tries + 1
        # i noticed if i guessed the right answer out of the loop it would just exit so i duplicated here to prevent it
        if int(guess) == number:
            print "Congratulations you\'ve won!"
        # implemented the lose mechanic
        elif tries == 10:
            print "You've Lost!"
        # same with the correct answer issue i had so i put it in the loop as well
        elif int(guess) > 25 or int(guess) < 0:
            while int(guess) > 25 or int(guess) < 0:
                guess = raw_input("Please enter a VALID guess: ")
                # this is here because I didn't want to take tries away for invalid guesses
                tries = tries

So the game for me works as expected. You can win, lose, guess invalid numbers (I haven't tried letters but I won't get into that yet). Just not sure if this is the most efficient I can get. But if it's good enough for a beginner, I'll take it.

This is a good first stab at a guess the number game.

Here's a few things:

You should be learning/using Python 3 instead of Python 2. So far the only difference for you will be raw_input becomes input and print "foo" becomes print("foo").
The line tries = tries doesn't do anything meaningful. You don't need it
You should put all of this inside a function called main and then at the bottom you run it with this (tests if this script is being run standalone):

if __name__ == '__main__':
    main()

You do int(guess) a lot. This is something that can fail (if someone types abc for example). You should do it once and check for failure.

try:
    guess = int(guess)
except ValueError:``
    print('Guess must be between 0 and 25')
    # ask for another guess

It's a good idea to comment. That's a fantastic habit to get into. However, you run the risk of over-commenting. And that is more distracting than having too few. As a rule, don't explain what, explain why. More concretely, "check to see if a user guessed the right number" is obvious from the if guess == secret_number:. Finding a balance is a skill, but if you work on it and read good quality open source code, you'll pick it up.
When you do your range check, you can do it in a much more pythonic way. Instead of checking guess < 0 or guess > 25, you can do if not 0 <= guess <= 25
tries = tries + 1 can be tries += 1
You don't need to escape ' inside a " (so "Congratulations you\'ve won!" can be "Congratulations you've won!")

The overarching issue you have though is most of your logic is duplicated in several different places. This becomes a problem if you want to, say, change the number range from 0-25 to 0-50. You'd need to change that in 6 places. What happens if you miss one? Then your game will break in weird ways.

What's the solution? Look to pull out duplicate logic like this into smaller, manageable chunks. In this case, it's helpful to identify the steps of your game.

Generate a secret number
Collect a guess from the user
If the user guessed the secret or more than 10 attempts have been made, end, else go back to 2

One easy thing you can pull out is "collect a guess from the user." You can do this in a function that gets the number, converts it to an int (handling the exception), and checking it is within range.

LOW, HIGH = 0, 25

def get_guess():
    while True:
        try:
            guess = int(input(f'Guess a number between {LOW}-{HIGH}: '))
        except ValueError:
            print('Please enter a number')
        else:
            if LOW <= guess <= HIGH:
                return guess

            print(f'Number must be between {LOW} and {HIGH}')

Now you can use get_guess() when you need to guess the guess from the user without having to add any extra control structures (like wrapping everything in a while) or duplicate any logic.

Now, when using get_guess() you can concern yourself with fewer facets of the game. Namely, checking the number of attempts and if the guess was correct:

from random import randint

MAX_ATTEMPTS = 10

def main():
    secret = randint(LOW, HIGH)

    for _ in range(MAX_ATTEMPTS):
        guess = get_guess()

        if guess == secret:
            print('You win!')
            break
    else:
        print('Too many attempts; you lose.')


How can I speed up my shell sort? I'm trying to implement a bunch of sorting algorithms in JavaScript, and I can't figure out why my shell sort is so slow. It's 6x slower than my merge sort, and only slightly faster than my insertion sort. I've seen another implementation online, but I'm more focused on making it clear and readable (as I have a blog for noobs) and the faster implementation is too concise for my purposes. Any thoughts on how I can keep the general plan but get it moving faster? I tried using Marcin Ciura's gap sequence [701, 301, 132, 57, 23, 10, 4, 1], but it was slightly slower. I'm not sure what's the biggest factor slowing down my sort.

  var shellSort = function( list ) {
    var gapSize = Math.floor( list.length / 2 );

    while( gapSize > 0 ) {
      _shellInsertionSort( list, gapSize );
      gapSize = Math.floor( gapSize / 2 );
    }

    return list;
  };

  function _shellInsertionSort( list, gapSize ) {
    var temp, i, j;

    for( i = gapSize; i < list.length; i += gapSize ) {
      j = i;
      while( j > 0 && list[ j - gapSize ] > list[j] ) {
        temp = list[j];
        list[j] = list[ j - gapSize ];
        list[ j - gapSize ] = temp;
        j -= gapSize;
      }
    }
  };

My tests:

  var testSpeed = function( testSize, rounds ) {
    var testArrays = [],
        algorithms = Array.prototype.slice.call( arguments, 2 ),
        results = [],
        i, j;

    for( i = 0; i < rounds; i++ ) {
      testArrays[i] = [];
      for( j = 0; j < testSize; j++ ) {
        testArrays[i].push( Math.ceil( Math.random() * testSize ));
      }
    }

    for( i = 0; i < algorithms.length; i++ ) {
      for( j = 0; j < rounds; j++ ) {
        if( !results[i] ) {
          results[i] = [];
        }
        results[i].push( testAlgorithm( algorithms[i], testArrays[j] ));
      }
    }

    return results;
  };

  var testAlgorithm = function( algorithm, set ) {
    var clone = set.slice(),
        start = new Date().getTime(),
        end;

    algorithm( clone );

    end = new Date().getTime();

    return end - start;
  };

@David Harkness sent my search in the right direction. As it is, the insertion sort portion is only going through one round for each gap, rather than walking up each round, so it's not actually going for more than one round, and most of the sorting isn't done until the gap is 1. When I start fixing problem, it just started getting more convoluted, so I ditched my original approach and followed the pseudocode on wikidepia. Now, it's quite snappy:

var shellSort = function( list ) {
    var gap = Math.floor( list.length / 2 );

    while( gap > 0 ) {
      for( i = gap; i < list.length; i++ ) {
        temp = list[i];
        for( j = i; j >= gap && list[j - gap] > temp; j -= gap ) {
          list[j] = list[j - gap];
        }
        list[j] = temp;
      }

      gap = Math.floor( gap / 2 );
    }

    return list;
  };


Implement StringBuilder I was asked this in an interview yesterday. I could add whatever methods, members and constructors I wanted but I couldn't change the signature of the provided toString and append methods.

I implemented something like this:

import java.util.ArrayList;
import java.util.List;

public class StringBuilder {

  private List<Character> characterBuffer;

  public StringBuilder() {
    characterBuffer = new ArrayList<Character>();
  }

  public void append(String anotherString) {
    if(anotherString != null) {
      char[] charArray = anotherString.toCharArray();
      for(int i=0;i<charArray.length; i++) {
        characterBuffer.add(charArray[i]);
      }
    }
  }

  public String toString() {
    char[] charArray = new char[characterBuffer.size()];
    for(int i=0; i<charArray.length; i++) {
      charArray[i] = characterBuffer.get(i);
    }
    return new String(charArray);
  }

}

When asked if there was another approach I might have taken, I suggested using a primitive char array buffer that can grow if there is an overflow. Not liking to leave things unfinished, I implemented this solution when I got home:

import java.util.Arrays;

public class StringBuilder {

  private char[] characterBuffer;
  private int curIndex;

  public StringBuilder() {
    characterBuffer = new char[5];
    curIndex = 0;
  }

  public void append(String anotherString) {
    if(anotherString != null) {
      char[] charArray = anotherString.toCharArray();
      int charArrayLength = charArray.length;
      if(overflow(charArrayLength)) {
        characterBuffer = Arrays.copyOf(characterBuffer, newLength(charArrayLength));
      }
      for(int i=0;i<charArray.length; i++) {
        characterBuffer[curIndex++] = charArray[i];
      }
    }
  }

  private int newLength(int arrayLength) {
    return (characterBuffer.length + arrayLength) + 10;
  }

  private boolean overflow(int arrayLength) {
    return (arrayLength + curIndex) > characterBuffer.length;
  }

  public String toString() {
    return new String(characterBuffer, 0, curIndex);
  }

}

I'd like both approaches reviewed.

I'm concentration on performance as it's the sole reason for the existence of StringBuilder (otherwise a String would do).

private List<Character> characterBuffer;

This is obviously very inefficient. List is fine, but converting every char in  Character surely is not. The memory consumption is bad, too, namely 4 or 8 bytes (depending on using compressed oops) for the reference and 0 or 8 bytes for the object (0 if it's a cached Character, i.e., ASCII). So you need 4 to 16 bytes per char instead of 2.

public StringBuilder() {
    characterBuffer = new ArrayList<Character>();
}

This should be done in the initializer.

if(anotherString != null) {

There should be a space after if. Silently ignoring null is strange (not that appending "null" would be any better).

char[] charArray = anotherString.toCharArray();

This is needlessly inefficient as you're creating a throw-away charArray while you can use anotherString.charAt instead.

An interesting approach would be to collect the given strings instead. If your string builder gets fed mostly with strings, collecting them as they come could be better than flattening them into a single char[]. While this would make some operations (e.g., delete) pretty complicated, it could be pretty efficient for the most important operations.

  I suggested using a primitive char array buffer that can grow if there is an overflow.

That's the standard approach.

private int curIndex;

Let's call it length as it's what length() returns.

public StringBuilder() {
    characterBuffer = new char[5];
    curIndex = 0;
}

The former belongs to the initializer, the latter belongs nowhere (it's the default).

if(overflow(charArrayLength)) {

A method checking for overflow is fine, but a method handling it would be better.

private int newLength(int arrayLength) {
    return (characterBuffer.length + arrayLength) + 10;
}

No! You'll be resizing too often. Imagine appending "0123456789A" in a loop. Every single append will copy the whole buffer, more and more characters... complexity \$O(n^2)\$.

What you need is an exponential growth. Something like

2 * characterBuffer.length + arrayLength + 10;

or

characterBuffer.length + (characterBuffer.length >> 1) + arrayLength + 10;

if you want a growth factor of 1.5.

private boolean overflow(int arrayLength) {
    return (arrayLength + curIndex) > characterBuffer.length;
}

Needless parens. I guess, it should read >=.


Transforming lists of products to boolean vectors Task

Inputs

Lists of products stored in a CSV file, where each line represents the shopping list of a single client:

Outputs

A CSV file:

Header: products names sorted by number of appearances in the input file.
For each line k we output boolean vectors where each of its elements indicates whether the i-th product was present on the line k in the input file or not.

Approach 1

from collections import defaultdict

with open('bigtest.csv', 'r') as inp:
    index = defaultdict(lambda: set())
    for line_number, line_str in enumerate(inp):
        products = line_str[:-1].split(',')
        for product in products:
            index[product].add(line_number)

product_names = index.iterkeys()
sorting_key = lambda name: len(index[name])

header = sorted(product_names, key=sorting_key, reverse=True)

def generate_table():
    for line in range(line_number + 1):
        yield (1 if line in index[product] else 0 for product in header)

with open('formal0.csv', 'w') as out:
    print >> out, ','.join(header)
    for vector in generate_table():
        print >> out, ','.join(str(val) for val in vector)

Approach 2

More declarative

from collections import defaultdict

with open('bigtest.csv', 'r') as inp:
    data = (line[:-1].split(',') for line in inp)

    index = defaultdict(lambda: set())
    for line_number, products in enumerate(data):
        for product in products:
            index[product].add(line_number)

header = sorted(
    index.iterkeys(),
    key=lambda p: len(index[p]),
    reverse=True
)

table = ((1 if line in index[product] else 0 for product in header)
         for line in range(line_number + 1))

with open('formal0.csv', 'w') as out:
    print >> out, ','.join(header) + '\n' + '\n'.join(','.join(map(str, vector))
                                                      for vector in table)

Concerns

Readability
Clean way to make nested generators?
Am I abusing .join()?
How much declarative style is too much?

1. Review

Just reviewing approach 2.

Even if you have a good reason to be using Python 2.7, it is worth noting that the Python developers plan to end support for this version in 2020 and so it is a good idea to get into the habit of writing code that will be easy to port to Python 3 should you have to take that step.

In this case it would be easy for you to start the program with

from __future__ import print_function

and then use the print function instead of the print statement. (But we'll see later that we don't even need the print function since we can use the csv module instead.)
If a script has hard-coded filenames like 'bigtest.csv' then you have to edit it every time you want to run it on files with different names. The script would be more reusable if it took the filenames from the command line.
It looks as though the input is supposed to be in CSV format. But the input is parsed like this:

data = (line[:-1].split(',') for line in inp)
for line_number, products in enumerate(data):

This doesn't take into account the full details of the CSV format: in particular, fields can be quoted, and quoted fields can contain commas. Python has a built-in csv module that handles the CSV format. This allows you to write the input loop like this:

reader = csv.reader(inp)
for line_number, products in enumerate(reader):

Instead of lambda:set(), you can just write set! Computer scientists call this η-conversion.
The variable name index does not clearly indicate what it contains. The value of this variable is a mapping from a product name to the set of line numbers on which that product appears, so I would call it something like product_lines.
The expression

1 if line in index[product] else 0

can be written more simply as

int(line in index[product])

since False converts to int as 0 and True as 1.
Using the csv module as discussed above, the output loop can be written like this:

writerow = csv.writer(out).writerow
writerow(header)
for line in range(line_number + 1):
    writerow([int(line in index[product]) for product in header])

The rewritten code still looks up index[product] for every product on every row. These lookups can be avoided by constructing the header list so that it contains the sets of line numbers as well as the product names:

header = sorted(index.items(), key=lambda p:len(p[1]), reverse=True)

and then:

writerow = csv.writer(out).writerow
writerow([product for product, _ in header])
for line in range(line_number + 1):
    writerow([int(line in lines) for _, lines in header])

2. Revised code

from collections import defaultdict
import csv
import sys

def main(in_filename='bigtest.csv', out_filename='formal0.csv'):
    """Read lists of products from the CSV file in_filename, convert them
    to Boolean vectors of products, and write the results to out_filename.

    """
    # Mapping from product to set of line numbers on which it appears.
    product_lines = defaultdict(set)

    with open(in_filename) as in_file:
        reader = csv.reader(in_file)
        for line_number, products in enumerate(reader):
            for product in products:
                product_lines[product].add(line_number)

    # List of pairs (products, lines) in reverse order by len(lines)
    header = sorted(product_lines.items(), key=lambda p:len(p[1]), reverse=True)

    with open(out_filename, 'w') as out_file:
        writerow = csv.writer(out_file).writerow
        writerow([product for product, _ in header])
        for line in range(line_number + 1):
            writerow([int(line in lines) for _, lines in header])

if __name__ == '__main__':
    main(*sys.argv[1:])


Finding subsequences of vector from a vector I'm trying to find subsequences of vector from a vector.

For example, if I want to find {2, 3} from {1, 2, 3, 1, 2, 3, 4}, the result should be {1, 4}.

Here's what I wrote so far.

vector<int> v1 = {1, 2, 3, 1, 2, 3, 4}; //source vector
vector<int> v2 = {2, 3}; //the subsequence to find
vector<size_t> founds; //positions found

for (size_t i=0; i<v1.size(); ++i) {

    auto it = search(v1.begin()+i, v1.end(), v2.begin(), v2.end());

    if (it != v1.end()) {

        size_t pos = distance(v1.begin(), it);
        founds.push_back(pos);
        i += pos;
    }
}
for (size_t i=0; i<founds.size(); ++i) {

    cout << founds[i] << endl; //prints 1, 4
}

While this seems to work, I would appreciate any advice to make the code simpler and efficient.

EDIT : I updated the code based on @user1118321's advice.

Here's an updated code.

vector<int> source = {5, 8, 1, 2, 1, 2, 1, 2, 1}; //source vector
vector<int> toSearch = {1, 2, 1}; //the subsequence to find
vector<size_t> results; //positions found

const auto srcBegin = source.begin();
const auto srcEnd = source.end();

for (auto nextIndex = srcBegin; nextIndex < srcEnd; nextIndex++)
{
    const auto it = search(nextIndex, srcEnd, toSearch.begin(), toSearch.end());

    if (it != srcEnd) {

        const size_t pos = distance(srcBegin, it);
        results.push_back(pos);
        nextIndex = srcBegin + pos;
    }
}
for (size_t i=0; i<results.size(); ++i) {

    cout << results[i] << endl; //prints 2, 4, 6
}

And if I want to ignore the overlapping copies of a subsequence, I can change nextIndex = srcBegin + pos; to nextIndex = srcBegin + pos + toSearch.size(); Then it prints 2, 6

This is a really straightforward and simple solution that's easy to understand! I don't know that you could make it much simpler than it is. But here are some things I would change:
Variable Names
I think the naming could be improved. The names v1 and v2 don't exactly tell us what they're for. I recommend something like source or toSearch for v1 and subsequence or searchFor for v2. founds is also a really weird sounding name. I would name it results or subsequenceIndexes or something a little more descriptive.
Loops
You have 2 for loops that use old-school indexing. It would be better to use an iterator here since you need to pass one to search() anyway. Something like this:
for(auto nextIndex = v1.begin(); nextIndex < v1.end(); nextIndex++)
{
    auto it = search(nextIndex, v1.end(), v2.begin(), v2.end());
    // ... etc.
}

Likewise for the second for loop.
Errors
Also, is the code in the if correct? It seems wrong to me. It looks like you're trying to skip over the contents of the subsequence you just found and start at the element one past the end of the subsequence. There are 2 problems here. The first is that you're skipping too much. For example, if you subsequence is found at position 7, but the subsequence is only 3 characters long, you're adding 7 to the current position, so you're missing 4 characters that could contain another copy of the subsequence.
But beyond that, there's also the issue of overlapping copies of a subsequence. Let's say the sequence to search for is "1, 2, 1" and the string to search is "5, 8, 1, 2, 1, 2, 1, 2, 1". How many copies of the subsequence are there? If the answer is 2, then you were on the right track. But if the answer is 3, then your solution will miss one. (I'm seeing 3 at index 2, 4, and 6, but you could argue that you only want the ones at indexes 2 and 6.)


Count all X-shaped bits from 8 integers I am given 8 positive 32-bit integer numbers. The task is to write program to count all X-bits.

X-bits are groups of 9 bits (3 rows x 3 columns) forming the letter "X". task is to count all X-bits and print their count on the console. Valid X-bits consist of 3 numbers where their corresponding bit indexes are exactly {"101", "010", "101"}. All valid X-bits can be part of multiple X-bits (with overlapping).

Expected input and output:

Bellow is my solution, for any of the inputs it prints out the correct value, and it passes every test in our judge system, which makes me think that it's a valid solution to the problem.
The idea of my approach is simple, the array is treated as if it was a 2d array and each group of 9 bits across 3 ints of the array is compared to the required pattern.

import java.util.Scanner;

public class Main {
    public static void main(String[] args) {
        Scanner stdin = new Scanner(System.in);
        int xCount = 0;
        int nums[] = new int[8];
        for(int i = 0; i < nums.length; i++) {
            nums[i] = Integer.parseInt(stdin.nextLine());
        }
        for (int i = 0; i < nums.length - 2; i++) { // nums.length - 2 because in validX(...) we have index (passed as i) + 2
            for (int j = 0; j < 29; j++) {              // 31 (index of the leftmost bit) - 2 because we have startingPosition + 2
                if(validX(nums, i, j) == true) {
                    xCount++;
                }
            }
        }
        System.out.println(xCount);
    }

    public static int get_bit(int num, int pos) {
        return ((num >> pos) & 1);
    }
    public static boolean validX(int nums[], int index, int startingPosition) {
        if(get_bit(nums[index], startingPosition) == 1 && get_bit(nums[index], startingPosition + 1) == 0
        && get_bit(nums[index], startingPosition + 2) == 1 &&
                get_bit(nums[index + 1], startingPosition) == 0 && get_bit(nums[index + 1], startingPosition + 1) == 1
                && get_bit(nums[index + 1], startingPosition + 2) == 0 &&
        get_bit(nums[index + 2], startingPosition) == 1 && get_bit(nums[index + 2], startingPosition + 1) == 0 &&
        get_bit(nums[index + 2], startingPosition + 2) == 1) {
            return true;
        }
        return false;
    }
    /*
     * in actuality what this function is doing is getting each individual bit and comparing it to the pattern
     * we need, which is:
     * {"101",
     *  "010",
     *  "101"}
     */
}

The issue I have with this code is that it's ugly. Truth be told I'm slightly ashamed of my validX(...) method, but I can't think of a quicker way or one that would take even less memory. The idea of my approach is simple, the array is treated as if it was a 2d array and each group of 9 bits is compared to the required pattern.

And just to get a few things out of the way, nobody assigned me this problem, I'm doing it to get better in Java. Whatever code I receive (if any at all) won't be submitted anywhere. Quite frankly I'm interested to know if there's a more elegant, readable solution which avoids hardcoded values. The reason I'm not happy with the default solution is because I don't think 0.2 s allowed runtime and 16 MB allowed memory is adequate for a problem like this.

I think the main issue is that you took a bit-by-bit approach to a problem that seems to be designed to elicit approaches that take advantage of bitwise operations, which made the solution less elegant(?) than intended. There is a lot of code to extract individual bits and testing them, and an extra loop to go over the startingPosition, and we can do away with that. I don't expect a performance problem with your solution though, 0.2 seconds is a ton of time (running literally a billion instructions in that time is not unreasonable) and barely any memory is allocated.

As you already discovered, the question of "is there an X here" can be answered by ANDing
 together the bits that make up the X and also ANDing that with the complements of the bits at the positions that must be zero. Rather than working with booleans, we could work with entire 32bit masks at once, and get an entire 32bit mask as a result, which will then indicate the positions where an X is found. The Xs that are present can be counted using Integer.bitCount.

Forming that mask is really the same idea as the code you already wrote, but instead of adding offsets to the startingPosition there will be some bit-shifts. For example:

private static int calculateXMask(int top, int middle, int bottom) {
    return (top >>> 1) & ~top & (top << 1) &
        (~middle >>> 1) & middle & (~middle << 1) &
        (bottom >>> 1) & ~bottom & (bottom << 1);
}

Which can then be applied to every "window" of 3 successive integers in num, with the results bitCounted and added up.


Base converting with strings from bases 1 to 36 to bases 1 to 36 I'm writing a fastest-code challenge over at PPCG relating to base conversion of bases 1 to 36, and as a part of the process for writing the competition, I decided to write my own program for it. This is the code I wish to be reviewed:

public class BaseConv {

    // Disallow instantiation
    private BaseConv() {
    }

    // Base setup
    private static final String BASEINFO = "0123456789abcdefghijklmnopqrstuvwxyz";

    /**
     * Converts a string as though it was an integer in base "fromBase" to base
     * "toBase".
     *
     * @param toConvert
     *            The string to convert bases from.
     * @param startbase
     *            The base to convert from. Must be less than 37 and greater
     *            than 1.
     * @param endbase
     *            The base to convert to. Must be less than 37 and greater than
     *            1.
     * @return converted The converted integer as type String.
     */
    public static String convBase(String toConvert, int fromBase, int toBase) throws NumberFormatException {
        if (toBase > 36 | fromBase > 36 | toBase < 2 | fromBase < 2) {
            throw new NumberFormatException("Bases specified must be less than 36 and greater than 2.");
        }
        String converted = "", thisBase = BASEINFO.substring(0, fromBase);
        char[] argument = toConvert.toCharArray(), baseinfochar = BASEINFO.toCharArray();
        int sum = 0, i = argument.length, multi = 1, baseind;
        for (; i > 0; multi *= fromBase) {
            baseind = thisBase.indexOf(argument[--i]);
                    if (baseind == -1)
                        throw new NumberFormatException("Character " + argument[i] + " is not a valid number in base " + toBase);
                    else
                        sum += multi * baseind;
        }
        multi = (int) (Math.log(sum) / Math.log(toBase));
        for (i = multi, multi = 1; i > 0; i--)
            multi *= toBase;
        for (; multi >= 1; sum %= multi, multi /= toBase) {
            converted += baseinfochar[sum / multi];
        }
        return converted;
    }
}

I wish to make sure that this code, with its current algorithm, is in the fastest setup possible, as well as being "proper" code (I plan on using this for more formal stuff later).

The current algorithm goes something like this:

The code checks to make sure that it's not attempting to exceed the bounds for possible base conversions.
The code creates its own String and char[] variables from the first Base String, limiting the string to what the input fromBase base is (needn't change the char array size, since we use that specifically for output and it will never go over the specified output base).
The code generates a sum of the numbers by multiplying according to their respective powers of the base.
Calculates the log base toBase of the sum in order to find the largest power that it will be finding (to get rid of fluff).
It then divides the sum by the respective power of the sought base, finding the indexes of the 'BASEINFO' (as a char array for the slightest speed increase) that align with them, and enacting a modulo on the sum to reduce its size.

Note:  The PPCG challenge explicitly forbids the use of built-in base conversion methods.

I don't see anything particularly wrong with your code, except that you could benefit from using a StringBuilder to create the String to return, instead of appending to new Strings each time in

for (; multi >= 1; sum %= multi, multi /= toBase) {
    converted += baseinfochar[sum / multi];
}

But, in your case, you need to support only 32-bit signed integer. So it would be way easier to use built-in methods to make that conversion:

public static String convBase(String toConvert, int fromBase, int toBase) {
    return Integer.toString(Integer.parseInt(toConvert, fromBase), toBase);
}

This parses the given String from the given base and converts it back into a String with the target base. Integer.parseInt(s, radix) works for all bases between 2 and 36, like your requirement. It doesn't throw an exception on an invalid base; it defaults to 10, but you could add a simple check for that. Similarly Integer.toString(i, radix) works for base 2 to 36, defaulting to base 10 in case of invalid base.

I ran a JMH benchmark of your current code versus that simple solution on random integers between 0 and 100,000, 1,000,000 and 10,000,000. The benchmark simply converts the number from base 10 to 8; you could make it accross all bases but the result would be pretty much the same. Here are the results:

Benchmark               (length)  Mode  Cnt    Score    Error  Units
StreamTest.convBase       100000  avgt   30  139,514 ± 11,276  ns/op
StreamTest.convBase      1000000  avgt   30  151,537 ±  6,123  ns/op
StreamTest.convBase     10000000  avgt   30  176,489 ±  8,768  ns/op
StreamTest.convBaseInt    100000  avgt   30   51,728 ±  3,473  ns/op
StreamTest.convBaseInt   1000000  avgt   30   54,398 ±  2,329  ns/op
StreamTest.convBaseInt  10000000  avgt   30   64,694 ±  6,240  ns/op

They show that the built-in solution is actually faster than your current code, about 3 times faster. I also ran it with making the StringBuilder change mentioned above but the results were practically the same.

Here's the benchmark code:

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

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

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

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

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

        private String value;

        @Setup(Level.Iteration)
        public void setUp() {
            value = String.valueOf(ThreadLocalRandom.current().nextInt(length));
        }

    }

    private static final String BASEINFO = "0123456789abcdefghijklmnopqrstuvwxyz";

    public static String convBase(String toConvert, int fromBase, int toBase) throws NumberFormatException {
        if (toBase > 36 | fromBase > 36 | toBase < 2 | fromBase < 2) {
            throw new NumberFormatException("Bases specified must be less than 36 and greater than 2.");
        }
        String converted = "", thisBase = BASEINFO.substring(0, fromBase);
        char[] argument = toConvert.toCharArray(), baseinfochar = BASEINFO.toCharArray();
        int sum = 0, i = argument.length, multi = 1, baseind;
        for (; i > 0; multi *= fromBase) {
            baseind = thisBase.indexOf(argument[--i]);
                    if (baseind == -1)
                        throw new NumberFormatException("Character " + argument[i] + " is not a valid number in base " + toBase);
                    else
                        sum += multi * baseind;
        }
        multi = (int) (Math.log(sum) / Math.log(toBase));
        for (i = multi, multi = 1; i > 0; i--)
            multi *= toBase;
        for (; multi >= 1; sum %= multi, multi /= toBase) {
            converted += baseinfochar[sum / multi];
        }
        return converted;
    }

    public static String convBaseInt(String toConvert, int fromBase, int toBase) {
        return Integer.toString(Integer.parseInt(toConvert, fromBase), toBase);
    }

    @Benchmark
    public String convBase(StringContainer container) {
        return convBase(container.value, 10, 8);
    }

    @Benchmark
    public String convBaseInt(StringContainer container) {
        return convBaseInt(container.value, 10, 8);
    }

}


Suggestions for beginner Rust code I just stated learning Rust (again...), here is a short bit of code I wrote:

#![allow(non_snake_case)]

/**
 * takes a 2D Vec and returns a 90° rotated version
 */
pub fn createRotated90(arr: &Vec<Vec<bool>>) -> Vec<Vec<bool>> {
    let newWidth = arr[0].len();
    let newHeight = arr.len();
    let mut newRetArr = createNewFilled2DVec(newWidth as u8, newHeight as u8, false);

    forEachXY(
        newHeight as u8,
        newWidth as u8,
        &mut |x: u8, y: u8| newRetArr[y as usize][newHeight - x as usize - 1] = arr[x as usize][y as usize]
    );

    newRetArr
}

pub fn createNewFilled2DVec<T: std::clone::Clone>(width: u8, height: u8, filler: T) -> Vec<Vec<T>> {
    vec![vec![filler; height as usize]; width as usize]
}

pub fn forEachXY<F: FnMut(u8, u8)>(width: u8, height: u8, mut func: F) {
    let mut x: u8 = 0;
    let mut y: u8 = 0;

    while x < width {
        while y < height {
            func(x, y);

            y += 1;
        }

        x += 1;
    }
}

I am hoping for some general suggestion on how to improve my "Rusting".
Some points I noticed:

a lot of as usize
It seems Rust doesn't want me to use "normal" for loops (see forEachXY)
I am using Vec's instead of arrays, even though they don't change in size

Correctness

In

let mut x: u8 = 0;
let mut y: u8 = 0;

while x < width {
    while y < height {
        println!("{}, {}", x, y);
        func(x, y);

        y += 1;
    }

    x += 1;
}

y increments up to height, but then never gets reset. That means that the inner loop only runs once. To fix this, move the initialization of y inside the loop, or use a "normal loop" (see below).

Formatting and convention

Right now, it looks like you're trying to write Rust in a different language. Whenever possible, use the conventions suggested by the Rust compiler and the linter clippy (run cargo clippy). In particular, using #![allow(non_snake_case)] to set your own convention is discouraged.

A few other things:

Use /// for documentation. /* */ comments are fairly rare and /// comments will automatically be included in the documentation produced by cargo doc.
Keep lines fairly short. It's OK to go over the traditional limit of 80 characters sometimes, but whenever possible, break up lines for readability. For example, with a function signature or call, the arguments can be placed on separate lines. cargo fmt will do this automatically to the best of its ability.
Vec<T> is itself a pointer (and length and capacity), so &Vec<T> has no advantage over &[T]. In fact, due to automatic deref coercion, using the latter in a function signature allows that function to take either kind of argument. See this question for more.

Use for loops.

When iterating over a collection, for loops are usually the best way. There's no need to keep track of mutating variables. If an index is needed, the enumerate method in the Iterator trait can be used.

Here's how I'd rewrite your loop.

for (x, col) in arr.iter().enumerate() {
    for (y, &elem) in col.iter().enumerate() {
        new_arr[y][new_height - x - 1] = elem
    }
}

Add tests

Before I even touched your code, I made sure to add at least one test case. That alone caught the bug that I mentioned above. Adding more was very helpful when figuring out how the width and height worked (see the first item in Miscellaneous below). Just as a template, you can use

#[cfg(test)]
mod tests {
    use super::*;

    fn my_test() {
        // Use things like `assert!` and `assert_eq!` to verify the output of your functions
        // ...
    }
}

Tests can be run with cargo test.

Miscellaneous

This may just be personal preference, but when I see a 2-dimensional array like

vec![
    vec![1, 2, 3],
    vec![4, 5, 6],
]

I think of this has having a width of 3 and a height of 2. That is, the first row is [1, 2, 3] and the second row is [4, 5, 6]. Indexing at (x, y) is arr[y][x] (first access yth row, then xth column). From what I can tell, you've reversed this. For example, vec![vec![filler; height as usize]; width as usize] would have a height of width and a width of height in my convention.

Note that rotating the vector switches the role of height and width, so new_width is the height of the original and new_height is the width of the original.

The u8/usize problems were largely solved by rewriting the loop, but in general, I'd suggest just sticking with one type. There's no reason for create_new_filled_2d_vec or for_each_xy to take u8 arguments. That just leads to unnecessary conversions. Since the length of a vector is always usize, it makes sense to just use usize everywhere.

If you want to use arrays, you'll eventually be able to use const generics to accomplish this. For now, they'll need to be dynamically allocated as Box<[T]>. You can produce a Box<[T]> from a Vec<T> using the as_boxed_slice method. Since the only difference between this and Vec<T> is the capacity field, Box<[T]> is fairly rare in practice. The benefit isn't very large.


I was wondering if there was a better way to improve my XNA joystick code? (Vb. Net) I'm on a high school robotics team and I'm trying to code a Xbox 360 controller so that it'll controller the motors with variable speed. So far it works alright, but where I'm quite the novice coder I'd like to have more experienced eyes peer at my code and see if they can find a better way, or more effective way to do the same thing. As of now, as the code shows, when the sticks are moved it causes the value (-1 to 1) to be multiplied by 100 and sent to the motor controller as a value for its speed. A timer is also attached to continuously update the values to have the most accuracy possible. So far the only issue I've encountered is a minor dead zone at each perfect diagonal. I've pondered just coding in the diagonals just like the other directions but I've had little luck. Any pointers would be appreciated! Here's the current code:

Private Sub JoySticks()
    'Left Joystick drive motor controls.
    Dim LY As Integer
    Dim LX As Integer
    LY = GamePad.GetState(PlayerIndex.One).ThumbSticks.Left.Y * 100
    LX = GamePad.GetState(PlayerIndex.One).ThumbSticks.Left.X * 100
    HScrollLX.Value = LX
    VScrollLY.Value = LY
    If LY > 0 Then
        MC1.Velocity(0) = LY
        MC1.Velocity(1) = LY
    ElseIf LY < 0 Then
        MC1.Velocity(0) = LY
        MC1.Velocity(1) = LY
    ElseIf LX > 0 Then
        MC1.Velocity(0) = LX
        MC1.Velocity(1) = LX * -1
    ElseIf LX < 0 Then
        MC1.Velocity(0) = LX
        MC1.Velocity(1) = LX * -1
    ElseIf LX Or LY = 0 Then
        MC1.Velocity(0) = 0
        MC1.Velocity(1) = 0
    End If
End Sub

Private Sub TimerJoysticks_Tick(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles TimerJoysticks.Tick
    JoySticks()
End Sub

Just on the if statements.  Could you do something like:

    Dim LY As Integer = GamePad.GetState(PlayerIndex.One).ThumbSticks.Left.Y * 100
    Dim LX As Integer = GamePad.GetState(PlayerIndex.One).ThumbSticks.Left.X * 100

    HScrollLX.Value = LX
    VScrollLY.Value = LY

    SetVelocity(LY);
    SetVelocity(LX,-1);

    // Private method to set the Velocity values
    private sub SetVelocity(position, factor = 1)
        dim v as integer

        if position = 0 then
            MC1.Velocity(0) = 0
            MC1.Velocity(1) = 0
        else
            MC1.Velocity(0) = position
            MC1.Velocity(1) = position * factor
        end if
    end sub


Setter error checking I have a class called Duration which has the attributes hours, mins, and secs. These attributes have to stay within acceptable ranges, so I implemented the following in their setters:

public void setHours(int newHours){
    if (newHours >= 0){
        hours = newHours;
    }
    else{
         throw new RuntimeException("Invalid argument values");
    }
}

and so on for the other attributes.

Is there a better design pattern I could be using here? Is this an appropriate use of the RuntineException?

Input validation is a critical aspect of any reliable program. Your code is functional, but there are a few things to note.

First up, the RuntimeException is not a good error type to return, use an IllegalArgumentException instead (because it is an illegal argument). Note that IllegalArgumentException extends RuntimeException so it does not need to be explicitly checked/thrown.

Next, you should validate inputs before using the values - using a "Guard Clause". While your code technically does that, the logic looks like the validation is done after the hours are updated.

Finally, error messages should give the user/programmer an indication of what's needed to fix the problem. Your message should be more useful.

So, all things considered, I would do the code as:

private void setHours(int newHours) {
    if (newHours < 0){
         throw new IllegalArgumentException("Input hours must be positive value: " + newHours);
    }
    hours = newHours;
}


designing application using polymorphism I have created a polymorphic system, but I don't know whether this is the correct way of doing it. Am I abusing Polymorphism here? Here is the code:

class WriteObj
{
    public string Obj1 { get; set; }
    public string Obj2 { get; set; }
    public string Obj3 { get; set; }
}

The above code is a data object that I am passing around in the methods, since I would be using a List of these objects.

abstract class BaseWriter
{
    public abstract void Write(List<WriteObj> writeObjList);
}

class ConsoleWriter : BaseWriter
{
    public override void Write(List<WriteObj> writeObjList)
    {
        for (int i = 0; i < writeObjList.Count; i++)
        {
            Console.Writeline("I am in Console Writer, parameter: " + writeObjList[i].Obj1);
        }

    }
}

class FileWriter : BaseWriter
{
    public override void Write(List<WriteObj> writeObjList)
    {
        for (int i = 0; i < writeObjList.Count; i++)
        {
            Console.Writeline("I write in file, parameter: " + writeObjList[i].Obj1);
        }
    }
}

class DatabaseWriter : BaseWriter
{
    public override void Write(List<WriteObj> writeObjList)
    {
        for (int i = 0; i < writeObjList.Count; i++)
        {
            Console.Writeline("I write in database, parameter: " + writeObjList[i].Obj2);
        }
    }
}

In my main method I call them like:

    static void main()
    {
        List<WriteObj> col = new List<WriteObj>();
        col.AddRange(new WriteObj[2] { new WriteObj { Obj1 = "this is obj1 iteration 1", Obj2 = "This is obj2 iteration 1" },
            new WriteObj { Obj1 = "this is obj1 iteration 2", Obj2 = "This is obj2 iteration 2" } });

        //some factory will generate these concrete types,
        //but the sake of simplicity I am instantiating it like that.
        BaseWriter a = new ConsoleWriter();
        a.Write(col);

        BaseWriter b = new FileWriter();
        b.Write(col);

        BaseWriter c = new DatabaseWriter();
        c.Write(col);
    }

Is it Ok to pass List of WriteObj in the Write method of the respective concrete implementations?

Update : I have used abstract class because it will be having some methods in it. I haven't mentioned it here for the sake of simplicity.

Yes, I don't see a problem with that.

If you don't have any implementation at all in the base class, consider making it an interface instead:

interface IWriter {
    void Write(List<WriteObj> writeObjList);
}

class ConsoleWriter : IWriter {
  public void Write(List<WriteObj> writeObjList) {
    foreach (WriteObj obj in writeObjList) {
      Console.Writeline("I am in Console Writer, parameter: " + obj.Obj1);
    }
  }
}

etc.


Presenting a boolean as an int for XMLSerialization in C# I'm working with an XML structure that requires booleans to be represented as 1 or 0.

So, suppose the XML output has to look like:

<valid>1</valid>

Several nodes can be represented this way, so I made a struct to handle this. The solution I found works, but lacks oomph. OK, no, it doesn't just lack oomph, it sucks.

(Implemented in .NET 4)

public struct MyCrappyBool : IXmlSerializable
{
    private int _IntValue;
    private bool _BoolValue;

    public int IntValue
    {
        get { return _IntValue; }
        set
        {
            if (value < 0 || value > 1)
                throw new ArgumentOutOfRangeException();
            _IntValue = value;
            _BoolValue = value == 1;
        }
    }
    public bool BoolValue
    {
        get { return _BoolValue; }
        set
        {
            _BoolValue = value;
            _IntValue = value ? 1 : 0;
        }
    }

    public MyCrappyBool(int intValue) : this()
    {
        IntValue = intValue;
    }
    public MyCrappyBool(bool boolValue): this()
    {
        BoolValue = boolValue;
    }

    #region IXmlSerializable Members
    public XmlSchema GetSchema() { return null; }
    public void ReadXml(XmlReader reader)
    {
        IntValue = int.Parse(reader.ReadString());
    }
    public void WriteXml(XmlWriter writer)
    {
        writer.WriteString(IntValue.ToString());
    }
    #endregion
}

This is incredibly tedious, first and foremost because I need to instantiate every time:

MyCrappyBool isValid = new MyCrappyBool(true);

But even then, some nifty implicit overloading can address that:

    public static implicit operator MyCrappyBool(bool boolValue)
    {
        return new MyCrappyBool(boolValue);
    }
    public static implicit operator MyCrappyBool(int intValue)
    {
        return new MyCrappyBool(intValue);
    }

That's still a lot of code for one... crappy boolean.

Later, it can be (XML) serialized as part of any other class:

[XmlElement("valid")]
public MyCrappyBool IsValid { get; set; }
[XmlElement("active")]
public MyCrappyBool IsActive { get; set; }

Is there a simpler way about this I'm not seeing?

Update: Reverse implicit operator overloading!

    public static implicit operator bool(MyCrappyBool myCrappyBool)
    {
        return myCrappyBool.BoolValue;
    }

    public static implicit operator int(MyCrappyBool myCrappyBool)
    {
        return myCrappyBool.IntValue;
    }

This means all my properties can be private.

I'd implement it like this:

public struct Bit : IEquatable<Bit>, IXmlSerializable
{
    private int value;
    public Bit(bool value) { this.value = value ? 1 : 0; }
    public Bit(int value) : this(value != 0) { }

    public bool Equals(Bit other) { return this.value == other.value; }
    public override bool Equals(object obj) { return obj is Bit && this.Equals((Bit)obj); }
    public override int GetHashCode() { return value.GetHashCode(); }
    public override string ToString() { return this.value.ToString(); }

    public static implicit operator int(Bit value)  { return value.value; }
    public static explicit operator bool(Bit value) { return value.value != 0; }

    System.Xml.Schema.XmlSchema IXmlSerializable.GetSchema() { throw new NotImplementedException(); }
    void IXmlSerializable.ReadXml(XmlReader reader)  { this.value = reader.ReadElementContentAsInt(); }
    void IXmlSerializable.WriteXml(XmlWriter writer) { writer.WriteValue(this.value); }
}

It would be better to store your value as an integer as that is the "true" value that you're interested in.  It's value is normalized using C semantics (i.e., false if 0, true otherwise as opposed to true if 1, false otherwise).  That would feel more natural to me but it's up to you.

I'd be careful with the operator overloading.  Don't do it just because you can, do it because it makes sense.  I'd stop at implementing casting operators from the type explicitly (since that's the only way to get at the value), use the constructors otherwise.  And make only one of them an implicit cast, otherwise you will run into ambiguity errors if you use this a lot in your code.

Consider implementing the IEquatable<T> interface and other appropriate overrides.  If you intend to use this in your code, this could be invaluable.

Consider implementing the IXmlSerializable interface explicitly.  If you intend to use this type as you would any other in your code, you probably wouldn't want to see the serialization methods all the time.  Otherwise if you are only using this for serialization, implement it implicitly.


Using Parsec for simple arithmetic language I've been reading Types and Programming Languages and I wanted to try to implement the first language in Haskell to understand it properly

I have barely written any Haskell before and not used Parsec so I would grateful for any feedback on this code

Here are some specific points I am unsure about

Is my main function sensible?
Can the eval function be expressed any better?
I'm unhappy with functionParser and ifParser. How can I code these better. In particular can the ifParser be coded in an applicative style?

If there's anything else you consider odd don't hesitate to mention it

import Control.Monad
import System.Environment
import Text.ParserCombinators.Parsec

data Term = TmTrue
          | TmFalse
          | TmIf Term Term Term
          | TmZero
          | TmSucc Term
          | TmPred Term
          | TmIsZero Term
          | TmError
    deriving Show

main :: IO[()]
main = do
    args <- getArgs
    forM args (\arg -> case parseArith arg of
                Left e -> print e
                Right term -> print $ eval term)

isNumerical :: Term -> Bool
isNumerical term = case term of
    TmZero -> True
    TmSucc subterm -> isNumerical subterm
    TmPred subterm -> isNumerical subterm
    _ -> False

eval :: Term -> Term
eval term = case term of
    TmTrue -> TmTrue
    TmFalse -> TmFalse
    TmZero -> TmZero
    TmIf term1 term2 term3 -> case eval term1 of
        TmTrue -> eval term2
        TmFalse -> eval term3
        _ -> TmError
    TmIsZero subterm -> case eval subterm of
        TmZero -> TmTrue
        t2 | isNumerical t2 -> TmFalse
        _ -> TmError
    TmPred TmZero -> TmZero
    TmPred (TmSucc subterm) -> eval subterm
    TmSucc subterm -> case eval subterm of
        t2 | isNumerical t2 -> TmSucc t2
        _ -> TmError
    _ -> TmError

parseArith :: String -> Either ParseError Term
parseArith input = parse arithParser "Failed to parse arithmetic expression" input

arithParser :: GenParser Char st Term
arithParser = try( ifParser )
          <|> try( succParser )
          <|> try( predParser )
          <|> try( isZeroParser )
          <|> try( trueParser )
          <|> try( falseParser )
          <|> try( zeroParser )

trueParser :: GenParser Char st Term
trueParser = string "true" >> return TmTrue

falseParser :: GenParser Char st Term
falseParser = string "false" >> return TmFalse

zeroParser :: GenParser Char st Term
zeroParser = char '0' >> return TmZero

functionParser :: String -> (Term -> Term) -> GenParser Char st Term
functionParser name funcTerm = do
    string $ name ++ "("
    term <- arithParser
    char ')'
    return $ funcTerm term

succParser :: GenParser Char st Term
succParser = functionParser "succ" TmSucc

predParser :: GenParser Char st Term
predParser = functionParser "pred" TmPred

isZeroParser :: GenParser Char st Term
isZeroParser = functionParser "iszero" TmIsZero

ifParser :: GenParser Char st Term
ifParser = do
    string "if"
    spaces
    term1 <- arithParser
    spaces
    string "then"
    spaces
    term2 <- arithParser
    spaces
    string "else"
    spaces
    term3 <- arithParser
    return $ TmIf term1 term2 term3

Where you used case you could have used pattern matching. This yields more idiomatic code in many cases.

isNumerical :: Term -> Bool
isNumerical TmZero = True
isNumerical (TmSucc subterm) = isNumerical subterm
isNumerical (TmPred subterm) = isNumerical subterm
isNumerical _ = False

eval :: Term -> Term
eval TmTrue  = TmTrue
eval TmFalse = TmFalse
eval TmZero  = TmZero
eval (TmIf term1 term2 term3) = evalIf (eval term1) term2 term3
eval (TmIsZero subterm) = evalIsZero $ eval subterm
eval (TmPred subterm) = evalPred $ eval subterm
eval (TmSucc subterm) = evalSucc $ eval subterm
eval TmError = TmError

evalIf :: Term -> Term -> Term -> Term
evalIf TmTrue a  _ = eval a
evalIf TmFalse _ b = eval b
evalIf _ _ _  = TmError

evalIsZero :: Term -> Term
evalIsZero TmZero = TmTrue
evalIsZero term
    | isNumerical term = TmFalse
    | otherwise        = TmError

evalPred :: Term -> Term
evalPred TmZero = TmZero
evalPred t@(TmSucc subterm) = t
evalPred _ = TmError

evalSucc :: Term -> Term
evalSucc term
    | isNumerical term = TmSucc term
    | otherwise        = TmError

Note the complex terms are farmed off to their own functions. This makes testing easier. Especially as the runtime gets more complex.

You asked about main's type. If you use forM_ you can define it as main :: IO ().

As for Applicative:

functionParser :: String -> (Term -> Term) -> GenParser Char st Term
functionParser name funcTerm = funcTerm <$> (string (name ++ "(")
                                             *> arithParser
                                             <* char ')')
ifParser :: GenParser Char st Term
ifParser = TmIf <$> (string "if" *> spaces *> arithParser)
                <*> (spaces *> string "then" *> spaces *> arithParser)
                <*> (spaces *> string "else" *> spaces *> arithParser)

Also, the last 'try' should not be used. 'try' means to attempt a parse and do not consume the input if it failed. The last parse action is the end of the parse so there is no need to leave the input in the parser.


XMLHttpRequest in the browser The intention is to implement an xmlHttpRequest in plain vanilla js while considering all possible errors and problem situations without crashing in the browser. Result and faults are to be properly communicated back to the caller.

function get() {
    return new Promise(function(resolve, reject){
        var xmlHttpRequest = new XMLHttpRequest();
        xmlHttpRequest.open("GET", "http://localhost/products", true);
        xmlHttpRequest.onreadystatechange = function() {
            if (xmlHttpRequest.readyState === 4) {
                try {
                    var data = JSON.parse(xmlHttpRequest.response);
                } catch (error) { // in case parser error
                    reject(error);
                    return;
                }
                if(xmlHttpRequest.status === 200) {
                    resolve(data);
                } else {
                    reject(data);
                }
            }
        };
        xmlHttpRequest.addEventListener("error", function(error){
            reject(error);
        });
        xmlHttpRequest.send();
    });
}

Here are a few issues with your code

If older browsers aren't a concern, you can just use fetch instead of XHR. It's built-in and uses promises.
HTTP 200 isn't the only "successful" response status. See https://en.wikipedia.org/wiki/List_of_HTTP_status_codes.
Status "0" is a generic network error, not just being offline. It can also be caused by the browser suppressing the request due to some restriction, an extension blocking the request, an insecure connection/invalid certificate.
Send an Accept header with application/json as value. This is because, while the server might support JSON, it might not respond with it by default.
The third argument of xhr.open is by default true. You may omit that third argument.
You can use readystatechange with addEventListener instead of the onreadystatechange property to assign your callback.